If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Assertion failure: mConcurrentCacheAccess, nsHttpChannel.cpp:1235 with Ghostery extension

RESOLVED FIXED in Firefox 54

Status

()

Core
Networking: HTTP
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

53 Branch
mozilla54
Points:
---

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

10 months ago
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]
(Assignee)

Comment 1

10 months ago
Created attachment 8817366 [details] [diff] [review]
v1 (avoid duplicate onstartrequest notify)

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

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29602bb0fbe6d048e0a8aaff5f550142b386d347
Whiteboard: [necko-next] → [necko-active]
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1322346
(Assignee)

Comment 4

10 months ago
(find reviewer after bug 1318759 baked)
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 5

9 months 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 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 months ago
Created attachment 8838027 [details] [diff] [review]
v1 [merged] (avoid duplicate onstartrequest notify)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbb07e0f92c916176b47ba46a164617f84ac346c
Attachment #8817366 - Attachment is obsolete: true
Attachment #8838027 - Flags: review+
(Assignee)

Comment 8

7 months ago
Created attachment 8838028 [details] [diff] [review]
v1 [merged, commit message] (avoid duplicate onstartrequest notify)
Attachment #8838027 - Attachment is obsolete: true
Attachment #8838028 - Flags: review+
(Assignee)

Comment 9

7 months ago
Created attachment 8839624 [details] [diff] [review]
v1 [merged again, commit message] (avoid duplicate onstartrequest notify)
Attachment #8838028 - Attachment is obsolete: true
Attachment #8839624 - Flags: review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 10

7 months 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 months 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 months ago
Attachment #8839624 - Flags: review+
(Assignee)

Comment 13

7 months ago
Created attachment 8839910 [details] [diff] [review]
v1.1 [fixed build issue on And4.2x86] (avoid duplicate onstartrequest notify)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6737c2d791a48d3583ce1dfc0feff1ead795b5ce
Attachment #8839624 - Attachment is obsolete: true
Attachment #8839910 - Flags: review+
(Assignee)

Comment 14

7 months ago
Created attachment 8839996 [details] [diff] [review]
v1.2 [one more build fix]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b578cd9716d30f73444c768238895f36bd93617
Attachment #8839910 - Attachment is obsolete: true
(Assignee)

Comment 15

7 months ago
Passed!
Keywords: checkin-needed

Comment 16

7 months 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 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b77ceef12298
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months 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.