gmail service workers crash nightly 20160909030428 [@ mozilla::net::InterceptedChannelContent::ResetInterception ]

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcmanus, Assigned: bkelly)

Tracking

({crash, regression})

50 Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

3 years ago
this morning's nightly 20160909030428

gmail segvs on a null deref. reproducible

https://crash-stats.mozilla.com/report/index/fda71608-473a-45c8-a34b-de4382160909

bkelly says a fix is forthcoming
Priority: -- → P1
Patrick, does backing out bug 1301678 help at all?
Flags: needinfo?(mcmanus)
I mean, does backing out bug 1187335 help?
Crash Signature: [@ mozilla::net::InterceptedChannelContent::ResetInterception]
Severity: normal → critical
Keywords: crash
Summary: gmail service workers crash nightly 20160909030428 → gmail service workers crash nightly 20160909030428 [@ mozilla::net::InterceptedChannelContent::ResetInterception ]
I believe this is what happened:

* Bug 1187335 delayed the destruction of the internal necko channel held in mChannel.
* We did not notice, though, that InterceptedChannel.cpp was using mChannel to indicate "open vs closed" state.
* So we were effectively keeping the InterceptedChannel open when it was really closed.
* This let a duplicate ResetInterception() come along and try to operate on invalid state.

The solution is to just store a separate boolean indicating when the InterceptedChannel has been closed.

Patrick, can you try this in a local build with your profile and see if it fixes the crash?

Its unclear to me what error path is being taken to trigger this, though.  Clearly we don't have test coverage for it.  We should try to land a test in a follow-on bug.
Flags: needinfo?(mcmanus)
Attachment #8789795 - Flags: review?(josh)
Attachment #8789795 - Flags: feedback?(mcmanus)
Comment on attachment 8789795 [details] [diff] [review]
Make InterceptedChannel use a different state variable to indicated the channel is closed. r=jdm

Review of attachment 8789795 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, there is a bad bug in my patch.  Sorry!
Attachment #8789795 - Flags: review?(josh)
Attachment #8789795 - Flags: feedback?(mcmanus)
Comment on attachment 8789799 [details] [diff] [review]
Make InterceptedChannel use a different state variable to indicated the channel is closed. r=jdm

I can't reproduce this on my machine.  Its likely timing dependent.

Patrick tells me he is doing an opt build.  Lets see if it helps on his machine.

Josh, I think we should probably do this change regardless.  So flagging for review now.  Passes tests for me locally.
Attachment #8789799 - Flags: review?(josh)
Attachment #8789799 - Flags: feedback?(mcmanus)
Reporter

Comment 7

3 years ago
Comment on attachment 8789799 [details] [diff] [review]
Make InterceptedChannel use a different state variable to indicated the channel is closed. r=jdm

Review of attachment 8789799 [details] [diff] [review]:
-----------------------------------------------------------------

verified the patch

I could repro the crash against 89dba842cbc46502aaa105e26f7c5610332972cc and the patch fixed it
Attachment #8789799 - Flags: feedback?(mcmanus) → feedback+
Reporter

Comment 8

3 years ago
base in comment 8 was wrong - was really 0f2c669a489af1ebea7512c3f7bb841463c22602
Thanks for testing!
Blocks: 1187335
Keywords: regression
Attachment #8789799 - Flags: review?(josh) → review+
I spoke with Ryan on IRC.  We are going to wait for try to be green and then land on m-c directly.
Status: NEW → ASSIGNED
Nightly updates are off for this, please e-mail r-d when we should turn them on
Flags: needinfo?(bkelly)
Ryan, can you land this?  I'm going to be away from my desk longer than I thought.  Otherwise I'll land it in an hour or two.
Flags: needinfo?(bkelly) → needinfo?(ryanvm)
Landing now, will trigger nightlies on the subsequent push.
Flags: needinfo?(ryanvm)

Comment 14

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/644b3de5d7a1
Make InterceptedChannel use a different state variable to indicated the channel is closed. r=jdm, a=RyanVM
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1301848

Comment 15

3 years ago
Sorry, but could you please explain what kind of gmail service workers this bug is about?

(Nightly 51 x64 in win10 x64)
I use gmail, but, I don't see any service worker registered for gmail in about:serviceworkers (in fact there's no entry in there).
Also, there's no relevant entry in about:debugging#workers either: 
https://i.imgur.com/atUJgcq.jpg
I believe Google was experimenting with a SW on plus.google.com.  I don't have it in all profiles.
Duplicate of this bug: 1301828
You need to log in before you can comment on or make changes to this bug.