747215 - Ensure redirect pathways always call both OnStart/OnStopRequest

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

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).

Comment 1

[Jason Duell]

Attachment: 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.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 3

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12d1d626759c

Comment 4

[Ed Morley [:emorley]]

(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

Comment 5

[Jason Duell]

Attachment: 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

Comment 6

[Jason Duell]

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.

Comment 7

[Jason Duell]

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

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/0b6433ad0453