Closed
Bug 1322355
Opened 7 years ago
Closed 7 years ago
Assertion failure: mConcurrentCacheAccess, nsHttpChannel.cpp:1235 with Ghostery extension
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: mayhemer, Assigned: mayhemer)
References
Details
(Whiteboard: [necko-active])
Attachments
(1 file, 5 obsolete files)
8.61 KB,
patch
|
Details | Diff | Splinter Review |
Cause: a vetoed redirect (redirectTo - as described at [1]) leads to double notification. first comes from HttpBaseChannel::DoNotifyListener, when rv = failure, second comes from the regular nsHttpChannel::ContinueProcessNormal -> nsHttpChannel::CallOnStartRequest sequence when we continue processing the channel. Solution: cancel the channel and not call onstart/onstop when there is a pump to resume after the redirect. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1322346#c1
Updated•7 years ago
|
Whiteboard: [necko-next]
Assignee | ||
Comment 1•7 years ago
|
||
This doesn't fix any crash, just the assertion failure and is not critical to land. I want this to land separately from bug 1318759.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29602bb0fbe6d048e0a8aaff5f550142b386d347
Whiteboard: [necko-next] → [necko-active]
Assignee | ||
Comment 4•7 years ago
|
||
(find reviewer after bug 1318759 baked)
Flags: needinfo?(honzab.moz)
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8817366 [details] [diff] [review] v1 (avoid duplicate onstartrequest notify) The actual change is in nsHttpChannel::StartRedirectChannelToURI only. The rest just adds useful LOGs that helped me to identify the issue here. Michal, feel free to bounce back the r? request, I'm not sure who should do this.
Flags: needinfo?(honzab.moz)
Attachment #8817366 -
Flags: review?(michal.novotny)
Comment 6•7 years ago
|
||
Comment on attachment 8817366 [details] [diff] [review] v1 (avoid duplicate onstartrequest notify) Review of attachment 8817366 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but I'm not sure I understand this code enough to reveal a potential problem.
Attachment #8817366 -
Flags: review?(michal.novotny) → review+
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbb07e0f92c916176b47ba46a164617f84ac346c
Attachment #8817366 -
Attachment is obsolete: true
Attachment #8838027 -
Flags: review+
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8838027 -
Attachment is obsolete: true
Attachment #8838028 -
Flags: review+
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8838028 -
Attachment is obsolete: true
Attachment #8839624 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 10•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a93c0f43ccf8 Cancel http:// channel when secure update (redirect) to https:// is vetoed to avoid duplicate OnStartRequest notification + added logs. r=michal
Keywords: checkin-needed
I had to back this out for android build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=79155484&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/bc401cae3a90
Flags: needinfo?(honzab.moz)
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #11) > I had to back this out for android build bustage like > https://treeherder.mozilla.org/logviewer.html#?job_id=79155484&repo=mozilla- > inbound > > https://hg.mozilla.org/integration/mozilla-inbound/rev/bc401cae3a90 Thanks. It needs the new printf-fu apparently.
Flags: needinfo?(honzab.moz)
Assignee | ||
Updated•7 years ago
|
Attachment #8839624 -
Flags: review+
Assignee | ||
Comment 13•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6737c2d791a48d3583ce1dfc0feff1ead795b5ce
Attachment #8839624 -
Attachment is obsolete: true
Attachment #8839910 -
Flags: review+
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b578cd9716d30f73444c768238895f36bd93617
Attachment #8839910 -
Attachment is obsolete: true
Comment 16•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b77ceef12298 Cancel http:// channel when secure update (redirect) to https:// is vetoed to avoid duplicate OnStartRequest notification + added logs. r=michal
Keywords: checkin-needed
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b77ceef12298
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•