The default bug view has changed. See this FAQ.

Ensure redirect pathways always call both OnStart/OnStopRequest

RESOLVED FIXED in mozilla16

Status

()

Core
Networking: HTTP
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jduell, Assigned: jduell)

Tracking

(Blocks: 1 bug)

unspecified
mozilla16
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

While fixing bug 738484 I discovered a pathway where we wind up never calling OnStartRequest (see comment 20).  This needs to be fixed, although it does not actually appear to be causing any problems currently (the pathway renders the appropriate error page for the error code: but any intermediate listeners in the chain could easily get messed up from the missing OnStart).
(Assignee)

Comment 1

5 years ago
Created attachment 616781 [details] [diff] [review]
Make head_channels.js always check for missing OnStartRequest

Biesi: correct me if I've misunderstood the fundamental contract of nsIChannel, but AFAICT omitting the OnStart check here if there's been an error is just wrong.
Attachment #616781 - Flags: review?(cbiesinger)
Comment on attachment 616781 [details] [diff] [review]
Make head_channels.js always check for missing OnStartRequest

Review of attachment 616781 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, you're right. No idea why this code looks the way it does.
Attachment #616781 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d1d626759c
(In reply to Jason Duell (:jduell) from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/12d1d626759c

Sorry, had to backout the push for xpcshell failures on all platforms:
(Note the CPG landing before this is responsible for the OS X opt specific failure, so ignore that one)

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=12d1d626759c
eg https://tbpl.mozilla.org/php/getParsedLog.php?id=11419058&tree=Mozilla-Inbound

{
TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_nojsredir.js | running test ...
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_nojsredir.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>
### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/tmpUl4FRR/runxpcshelltests_leaks.log

TEST-INFO | (xpcshell/head.js) | test 1 pending

TEST-INFO | (xpcshell/head.js) | test 2 pending

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | running event loop

TEST-PASS | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/head_channels.js | [null : 147] 16 == 16

TEST-PASS | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_nojsredir.js | [completeIter : 30] true == true

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/head_channels.js | onStopRequest without onStartRequest event! - See following stack:
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 462
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/head_channels.js :: <TOP_LEVEL> :: line 132

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/head_channels.js | Error in onStopRequest: 2147500036 - See following stack:
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 462
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/head_channels.js :: <TOP_LEVEL> :: line 149
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/074c8fb332a8
(Assignee)

Comment 5

5 years ago
Created attachment 621244 [details] [diff] [review]
Fix bug 255119 so OnStartRequest gets called.

So it turns out that the fix for 255119 never winds up calling OnStartRequest (if a redirect observer vetoed a redirect with NS_ERROR_CORRUPTED_CONTENT, we'd have also skipped OnStart).

This was present in the code before my tweaking for bad location URIs.  It's just been exposed by the onStart check I'm adding in this bug to head_channels.js
Attachment #621244 - Flags: review?(mcmanus)
Attachment #621244 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 6

5 years ago
Comment on attachment 621244 [details] [diff] [review]
Fix bug 255119 so OnStartRequest gets called.

This patch is being replaced by Attachment #621802 [details] [diff] in bug 738484.
Attachment #621244 - Attachment is obsolete: true
Blocks: 759043
(Assignee)

Comment 7

5 years ago
comment 6 is wrong--this patch wasn't replaced by bug 738484.  That was just a prerequisite for this to land.

  https://hg.mozilla.org/integration/mozilla-inbound/rev/0b6433ad0453
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/0b6433ad0453
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.