1229970 - Crash in `anonymous namespace''::ScriptLoaderRunnable::OnStartRequest while dragging tabs to their own window

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Trammel May]

Attachment: All Tabs Crashed.jpg

Actual Result:
While pulling tabs out into their own window, all tabs crashed. 

Expected Result:
The user should be able to pull tabs out without crashing the tabs. 

Steps to Recreate:
1. Launch firefox and navigate to alexa top sites. 
2. Click on Cambodia
3. Ctrl click all the links and open the link tabs. 
4. Start using the webpages as you would regularly use them. 
5. Start pulling tabs out and pluggin them back in to the main window. 
6. Observe the tabs all crash. 

Note:
Difficulty reproducing.

Comment 1

[Trammel May]

Attachment: Kal El DxDiag.txt

Comment 2

[Trammel May]

Attachment: Kal El Graphics 1.txt

Comment 3

[Trammel May]

Attachment: Kal El Graphics 2.txt

Comment 4

[u279076]

Please provide crash reports from about:crashes.

Comment 5

[Trammel May]

Attachment: Kal El Crashes.txt

Comment 6

[u279076]

(In reply to Trammel May from comment #5)
> Created attachment 8695911 [details]
> Kal El Crashes.txt

Thanks, next time you can just copy & paste the ID from about:crashes.

Looking at this further it appears to be a low-volume DOM crash:
 0 	xul.dll 	`anonymous namespace'::ScriptLoaderRunnable::OnStartRequest(nsIRequest*, nsISupports*) 	dom/workers/ScriptLoader.cpp
1 	xul.dll 	nsStreamListenerTee::OnStartRequest(nsIRequest*, nsISupports*) 	netwerk/base/nsStreamListenerTee.cpp
2 	xul.dll 	mozilla::net::HttpChannelChild::DoOnStartRequest(nsIRequest*, nsISupports*) 	netwerk/protocol/http/HttpChannelChild.cpp
3 	xul.dll 	mozilla::net::HttpChannelChild::OnStartRequest(nsresult const&, mozilla::net::nsHttpResponseHead const&, bool const&, mozilla::net::nsHttpHeaderArray const&, bool const&, bool const&, unsigned int const&, nsCString const&, nsCString const&, mozilla::net::NetAddr const&, mozilla::net::NetAddr const&, unsigned int const&) 	netwerk/protocol/http/HttpChannelChild.cpp
4 	xul.dll 	mozilla::net::HttpChannelChild::RecvOnStartRequest(nsresult const&, mozilla::net::nsHttpResponseHead const&, bool const&, mozilla::net::nsHttpHeaderArray const&, bool const&, bool const&, unsigned int const&, nsCString const&, nsCString const&, mozilla::net::NetAddr const&, mozilla::net::NetAddr const&, short const&, unsigned int const&) 	netwerk/protocol/http/HttpChannelChild.cpp
5 		@0xab7 	
6 	xul.dll 	mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PContentChild.cpp
7 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
8 	xul.dll 	mozilla::ipc::MessageChannel::OnMaybeDequeueOne() 	ipc/glue/MessageChannel.cpp
9 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc
10 	ntdll.dll 	LdrpFreeLoadContext 	
11 		@0xb0417f 	

More reports: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=%60anonymous+namespace%27%27%3A%3AScriptLoaderRunnable%3A%3AOnStartRequest

Comment 7

[Trammel May]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #6)
> (In reply to Trammel May from comment #5)
> > Created attachment 8695911 [details]
> > Kal El Crashes.txt
> 
> Thanks, next time you can just copy & paste the ID from about:crashes.
> 
> Looking at this further it appears to be a low-volume DOM crash:
>  0 	xul.dll 	`anonymous
> namespace'::ScriptLoaderRunnable::OnStartRequest(nsIRequest*, nsISupports*) 
> dom/workers/ScriptLoader.cpp
> 1 	xul.dll 	nsStreamListenerTee::OnStartRequest(nsIRequest*, nsISupports*) 
> netwerk/base/nsStreamListenerTee.cpp
> 2 	xul.dll 	mozilla::net::HttpChannelChild::DoOnStartRequest(nsIRequest*,
> nsISupports*) 	netwerk/protocol/http/HttpChannelChild.cpp
> 3 	xul.dll 	mozilla::net::HttpChannelChild::OnStartRequest(nsresult const&,
> mozilla::net::nsHttpResponseHead const&, bool const&,
> mozilla::net::nsHttpHeaderArray const&, bool const&, bool const&, unsigned
> int const&, nsCString const&, nsCString const&, mozilla::net::NetAddr
> const&, mozilla::net::NetAddr const&, unsigned int const&) 
> netwerk/protocol/http/HttpChannelChild.cpp
> 4 	xul.dll 	mozilla::net::HttpChannelChild::RecvOnStartRequest(nsresult
> const&, mozilla::net::nsHttpResponseHead const&, bool const&,
> mozilla::net::nsHttpHeaderArray const&, bool const&, bool const&, unsigned
> int const&, nsCString const&, nsCString const&, mozilla::net::NetAddr
> const&, mozilla::net::NetAddr const&, short const&, unsigned int const&) 
> netwerk/protocol/http/HttpChannelChild.cpp
> 5 		@0xab7 	
> 6 	xul.dll 	mozilla::dom::PContentChild::OnMessageReceived(IPC::Message
> const&) 	obj-firefox/ipc/ipdl/PContentChild.cpp
> 7 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message
> const&) 	ipc/glue/MessageChannel.cpp
> 8 	xul.dll 	mozilla::ipc::MessageChannel::OnMaybeDequeueOne() 
> ipc/glue/MessageChannel.cpp
> 9 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc
> 10 	ntdll.dll 	LdrpFreeLoadContext 	
> 11 		@0xb0417f 	
> 
> More reports:
> https://crash-stats.mozilla.com/report/
> list?product=Firefox&signature=%60anonymous+namespace%27%27%3A%3AScriptLoader
> Runnable%3A%3AOnStartRequest

ok, will do

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

ScriptLoaderRunnable::OnStartRequest() only seems to execute if its loading and caching a service worker script.  This is currently disabled on beta.  This is confusing to me.

Comment 9

[u279076]

(In reply to Ben Kelly [:bkelly] from comment #8)
> ScriptLoaderRunnable::OnStartRequest() only seems to execute if its loading
> and caching a service worker script.  This is currently disabled on beta. 
> This is confusing to me.

Is it possible for someone to pref this on in Beta?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #9)
> Is it possible for someone to pref this on in Beta?

It is.  I guess it depends on how low volume this really is.  I would expect exceptionally low volume on beta.

Comment 11

[u279076]

I would say volume is "exceptionally" low. Here are the numbers to the last week of data:

Firefox 45.0a1 - 17 crashes
Firefox 44.0a2 - 6 crashes
Firefox 42.0b1 - 5 crashes
Firefox 43.0a1 - 1 crash
Firefox 42.0a2 - 1 crash

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

It appears https://www.khmerload.com is registering a service worker to handle push notifications.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I see both e10s and non-e10s channels in the crashes.  So it affects both configurations.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

I have a theory here, although I have not been able to reproduce.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Abort script loading start requests if a load has been canceled. r=khuey

Trammel, does this patch fix the issue for you?

If you don't want to do your own build, I think you can get a binary from this try once it finishes:

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

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

New try not based on any other bugs:

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

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8696709 [details] [diff] [review]
Abort script loading start requests if a load has been canceled. r=khuey

This is still a bit speculative, but I think its a safe and correct change.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8696709 [details] [diff] [review]
Abort script loading start requests if a load has been canceled. r=khuey

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

Seems reasonable.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/286ad5849931

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

We should consider uplifting this to 44 beta if it fixes the problem.

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/286ad5849931

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

There have been 3 crashes on 44, but none on 45 since this landed.  I think we should uplift it to 44 beta.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8696709 [details] [diff] [review]
Abort script loading start requests if a load has been canceled. r=khuey

Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: Low frequency crash, but it may become higher frequency as more sites start using service workers.
[Describe test coverage new/current, TreeHerder]: Mainly observing crashstats after landing this change.  Its so low frequency we never reproduced it locally.
[Risks and why]: Minimal.  This is a very small change and only affects service workers.
[String/UUID change made/needed]:  None.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Note, I want to land this on 44.  I think that will be beta very soon, thats why I requested the beta flag.  If we can land it sooner on aurora before the merge to beta, that would be even better.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8696709 [details] [diff] [review]
Abort script loading start requests if a load has been canceled. r=khuey

Crash fix, taking it in Beta44.

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm getting conflicts trying to uplift this to beta 44 in ScriptLoader.cpp.
Can we get a rebased patch for this?

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: beta patch

Here is a beta patch.  Ritu asked me to post here instead of pushing it.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6411e85ba1eb

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6411e85ba1eb