Closed Bug 1502055 Opened 3 years ago Closed 3 years ago
Http Channel crash with Clear Site Data header if the server returns 304
The crash is because of this assertion: https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/netwerk/protocol/http/nsHttpChannel.cpp#7342 STR: 1. we have a server which sends a 304 + ClearSiteData header for a request already in the necko cache 2. ClearSiteData service suspends the channel during the cleanup - the cleanup doesn't touch the necko cache. ClearSiteData header contains 'storage'. 3. after the cleanup the channel is resumed 4. OnStartRequest crashes.
Assignee: nobody → michal.novotny
Priority: -- → P3
Regression likely from bug 1396395. What happens here is: - in nsHttpChannel::ContinueProcessResponse1 we are suspended (from an on-modify handler), so we schedule nsHttpChannel::ContinueProcessResponse1 be called on resume - this holds the OnStopRequest call from transaction (where we release the transaction) from being fired (transaction is suspended) - ResumeInternal is called, which: - drops the suspend cntr to zero - schedules a runnable to : - call nsHttpChannel::ContinueProcessResponse1 (the mCallOnResume thing), this leads to: - ReadFromCache() it creates the cache pump and asyncRead()'s it OnStartRequest from the cache pump is scheduled - resumes the network transaction OnStopRequest from the transaction pump is scheduled So, in OnStartRequest fired from cache pump we still have mTransactionPump. Not sure about a fix, tho...  https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/netwerk/protocol/http/nsHttpChannel.cpp#9184
Assignee: michal.novotny → nobody
Priority: P3 → P2
(sorry Michal, mid air collision)
Baku, could you please refer the original bug with the test case? I would like to check the patch locally with it. Thanks.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e43bbe5f7e8921103a75cc6ee24688914cb3396 When mCallOnResume handler creates a new pump (in the case of this bug it's the cache pump) it is created suspended - the new mAsyncResumePending flag controls that. Then after we called mCallOnResume, we resume previously existing pumps and only after that any newly created pumps. Note that I lost the bug# with a STR, so not tested for that case.
Attachment #9020608 - Flags: review?(dd.mozilla)
Attachment #9020608 - Flags: review?(dd.mozilla) → review+
Thanks for the review! Now it's pending a test from :baku.
This triggers the crash without your patch.
Attachment #9020736 - Flags: review?(honzab.moz)
Comment on attachment 9020736 [details] [diff] [review] crash.patch Review of attachment 9020736 [details] [diff] [review]: ----------------------------------------------------------------- thanks so much for this! ::: netwerk/test/mochitests/file_1502055.sjs @@ +11,5 @@ > + response.write('<html><body>Hello world!</body></html>'); > + setState('count', '1'); > + return; > + } > + nit: tws
Attachment #9020736 - Flags: review?(honzab.moz) → review+
Backed out 2 changesets (Bug 1502055) for mochitest failures on test_1502055.html Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fe7380322a2bad3b6d67bb398a17efa9dd5819f Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=c5317de7cf018c2e00fc67d0db3f0c45fd6a1483&selectedJob=208399101 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=208399101&repo=mozilla-inbound&lineNumber=3283
baku's test -> ni
Flags: needinfo?(honzab.moz) → needinfo?(amarchesini)
note that the try push of the code changes is fully green.
Actually, it's the fix that doesn't work. See this: https://treeherder.mozilla.org/logviewer.html#?job_id=208398960&repo=mozilla-inbound Assertion failure: mRaceCacheWithNetwork || !(mTransactionPump && mCachePump) || mCachedContentIsPartial (If we have both pumps, the cache content must be partial), at /builds/worker/workspace/build/src/netwerk/protocol/http/nsHttpChannel.cpp:7379 We still crash there.
Flags: needinfo?(amarchesini) → needinfo?(honzab.moz)
(In reply to Andrea Marchesini [:baku] from comment #15) > Actually, it's the fix that doesn't work. See this: > https://treeherder.mozilla.org/logviewer.html#?job_id=208398960&repo=mozilla- > inbound > > Assertion failure: mRaceCacheWithNetwork || !(mTransactionPump && > mCachePump) || mCachedContentIsPartial (If we have both pumps, the cache > content must be partial), at > /builds/worker/workspace/build/src/netwerk/protocol/http/nsHttpChannel.cpp: > 7379 > > We still crash there. Ah! Thanks. I though you ran the test and it DID pass for the original problem. OK, interesting, I will look into this. It's so convenient now when we have an actual test for this :)
So, it turns out that OnStartRequest from the newly created cache pump is already scheduled, regardless the added Suspend (AsyncRead calls AsyncWait on the stream, suspend is actually handled in OnInputStreamReady, which has not been called until the time we call Resume, and when it is called, the counter is already at 0.) Hence, instead of some complicated code changes, I rather simply post again to resume the pumps. I think worth another r? round. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdefcda0405abc63d9c9fbea79ad15b1ba7e9063
This test works fine with patch v2.
Attachment #9020745 - Attachment is obsolete: true
Attachment #9021482 - Flags: review?(dd.mozilla) → review+
Thanks again! :) Andrea, do you want to ask for r on the test so we could land it?
Attachment #9021504 - Flags: review?(honzab.moz)
Comment on attachment 9021504 [details] [diff] [review] test Review of attachment 9021504 [details] [diff] [review]: ----------------------------------------------------------------- Works! Thanks.
Attachment #9021504 - Flags: review?(honzab.moz) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/7517d1d77643 Make nsHttpChannel::ResumeInternal keep order of transaction pump OnStopRequest before cache pump OnStartRequest, r=dragana https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab5e4dd08b7 test for Clear-Site-Data + 304 http status r=mayhemer
You need to log in before you can comment on or make changes to this bug.