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

Status: RESOLVED FIXED

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

Description

[Patrick McManus [:mcmanus]]

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

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Patrick, does backing out bug 1301678 help at all?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I mean, does backing out bug 1187335 help?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make InterceptedChannel use a different state variable to indicated the channel is closed. r=jdm

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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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!

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make InterceptedChannel use a different state variable to indicated the channel is closed. r=jdm

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Patrick McManus [: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

Comment 8

[Patrick McManus [:mcmanus]]

base in comment 8 was wrong - was really 0f2c669a489af1ebea7512c3f7bb841463c22602

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Thanks for testing!

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

I spoke with Ryan on IRC.  We are going to wait for try to be green and then land on m-c directly.

Comment 11

[Justin Wood (:Callek)]

Nightly updates are off for this, please e-mail r-d when we should turn them on

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Landing now, will trigger nightlies on the subsequent push.

Comment 14

[Pulsebot]

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

Comment 15

[Kostas]

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

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

I believe Google was experimenting with a SW on plus.google.com.  I don't have it in all profiles.