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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Honza Bambas (:mayhemer)]

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

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: 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.

Comment 2

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29602bb0fbe6d048e0a8aaff5f550142b386d347

Comment 4

[Honza Bambas (:mayhemer)]

(find reviewer after bug 1318759 baked)

Comment 5

[Honza Bambas (:mayhemer)]

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.

Comment 6

[Michal Novotny [:michal]]

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.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1 [merged] (avoid duplicate onstartrequest notify)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbb07e0f92c916176b47ba46a164617f84ac346c

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v1 [merged, commit message] (avoid duplicate onstartrequest notify)

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v1 [merged again, commit message] (avoid duplicate onstartrequest notify)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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

Comment 12

[Honza Bambas (:mayhemer)]

(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.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1.1 [fixed build issue on And4.2x86] (avoid duplicate onstartrequest notify)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6737c2d791a48d3583ce1dfc0feff1ead795b5ce

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: v1.2 [one more build fix]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b578cd9716d30f73444c768238895f36bd93617

Comment 15

[Honza Bambas (:mayhemer)]

Passed!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b77ceef12298