Closed Bug 1502055 Opened 7 years ago Closed 7 years ago

nsHttpChannel crash with ClearSiteData header if the server returns 304

Categories

(Core :: Networking: HTTP, defect, P2)

Product:
Core
Component:
Networking: HTTP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

(Reporter: baku, Assigned: mayhemer)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(3 files, 3 obsolete files)

Necko log
16.04 KB, application/x-bzip
Details
v2
9.18 KB, patch
dragana
: review+
Details | Diff | Splinter Review
test
3.64 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
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.
Attached file Necko log —
Assignee: nobody → michal.novotny
Priority: -- → P3
Whiteboard: [necko-triaged]
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 [1]: - 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... [1] https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/netwerk/protocol/http/nsHttpChannel.cpp#9184
Assignee: michal.novotny → nobody
Blocks: 1396395
Priority: P3 → P2
(sorry Michal, mid air collision)
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Baku, could you please refer the original bug with the test case? I would like to check the patch locally with it. Thanks.
Flags: needinfo?(amarchesini)
Attached patch v1 (obsolete) — — Splinter Review
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)
Bug 1501695
Flags: needinfo?(amarchesini)
Attachment #9020608 - Flags: review?(dd.mozilla) → review+
Thanks for the review! Now it's pending a test from :baku.
Flags: needinfo?(amarchesini)
Attached patch crash.patch (obsolete) — — Splinter Review
This triggers the crash without your patch.
Flags: needinfo?(amarchesini)
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+
Keywords: checkin-needed
Attached patch test (obsolete) — — Splinter Review
Attachment #9020736 - Attachment is obsolete: true
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 :)
Attached patch v2 — — Splinter Review
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
Attachment #9020608 - Attachment is obsolete: true
Flags: needinfo?(honzab.moz)
Attachment #9021482 - Flags: review?(dd.mozilla)
Attached patch test — — Splinter Review
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?
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
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+
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com: 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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7517d1d77643 https://hg.mozilla.org/mozilla-central/rev/7ab5e4dd08b7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: