Last Comment Bug 747215 - Ensure redirect pathways always call both OnStart/OnStopRequest
: Ensure redirect pathways always call both OnStart/OnStopRequest
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla16
Assigned To: Jason Duell [:jduell] (needinfo me)
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 738484
Blocks: 759043
  Show dependency treegraph
 
Reported: 2012-04-19 15:43 PDT by Jason Duell [:jduell] (needinfo me)
Modified: 2012-06-07 05:49 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make head_channels.js always check for missing OnStartRequest (1.21 KB, patch)
2012-04-19 15:46 PDT, Jason Duell [:jduell] (needinfo me)
cbiesinger: review+
Details | Diff | Splinter Review
Fix bug 255119 so OnStartRequest gets called. (1.96 KB, patch)
2012-05-04 21:56 PDT, Jason Duell [:jduell] (needinfo me)
mcmanus: review+
Details | Diff | Splinter Review

Description Jason Duell [:jduell] (needinfo me) 2012-04-19 15:43:08 PDT
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 [:jduell] (needinfo me) 2012-04-19 15:46:00 PDT
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.
Comment 2 Christian :Biesinger (don't email me, ping me on IRC) 2012-04-19 16:10:52 PDT
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 [:jduell] (needinfo me) 2012-05-03 00:36:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d1d626759c
Comment 4 Ed Morley [:emorley] 2012-05-03 02:51:27 PDT
(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 [:jduell] (needinfo me) 2012-05-04 21:56:23 PDT
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
Comment 6 Jason Duell [:jduell] (needinfo me) 2012-05-07 17:33:53 PDT
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 [:jduell] (needinfo me) 2012-06-06 00:03:27 PDT
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 (⌚ UTC+1/+2) 2012-06-07 05:49:44 PDT
https://hg.mozilla.org/mozilla-central/rev/0b6433ad0453

Note You need to log in before you can comment on or make changes to this bug.