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)

53 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 5 obsolete files)

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
Whiteboard: [necko-next]
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
(find reviewer after bug 1318759 baked)
Flags: needinfo?(honzab.moz)
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 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+
Attachment #8838027 - Attachment is obsolete: true
Attachment #8838028 - Flags: review+
Attachment #8838028 - Attachment is obsolete: true
Attachment #8839624 - Flags: review+
Keywords: checkin-needed
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
(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)
Attachment #8839624 - Flags: review+
Passed!
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/b77ceef12298
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.