Closed Bug 1301678 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Service Workers, defect, P1)

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- unaffected
firefox51 --- fixed

People

(Reporter: mcmanus, Assigned: bkelly)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 1 obsolete file)

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)
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+
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)
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1301848
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.