1299887 - Crashes writing to Heap<> during Worker thread shutdown

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Attachment: service_worker_talos_crash_case.zip

STR:

1) Download and unzip the attached service_worker_talos_crash_case.zip.
2) cd service_worker
3) python -m SimpleHTTPServer 8000
4) Open a fresh nightly 51
5) Navigate to http://localhost:8000/images.html?images=25
6) Observe that the process running the service worker thread crashes.

The crash will look like:

https://crash-stats.mozilla.com/report/index/f5f8cfa6-acd6-4e7a-8e0f-2c9512160901

This only reproduces on 51.  It may not effect earlier branches since they have the older c++ promises.  The crash appears to be triggered by a Promise destructor running during Worker thread shutdown.

From what I gather on IRC, this means that we are doing something not quite right during worker shutdown.  Terrence, what do you think?

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

[Tracking Requested - why for this release]:

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

FYI, this only happens in opt.  DEBUG builds (even with optimization enabled) does not trigger.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Adding Till in case this is a spidermonkey Promise issue.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make dom/cache FetchHandler cycle collected. r=smaug

Making the dom/cache FetchHandler cycle collected avoids the crash for me.

Its not clear, however, if this is just working around a bigger problem with CC and worker shutdown.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

It seems with spidermonkey promises PromiseNativeHandler objects are now exposed to self-hosted script:

  https://dxr.mozilla.org/mozilla-central/source/dom/promise/Promise.cpp#800

Whose callbacks are themselves cycle collected now:

  https://dxr.mozilla.org/mozilla-central/source/dom/promise/Promise.cpp#405

I wonder if other PromiseNativeHandler implementations need to be cycle collected for this to work in general.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make dom/cache FetchHandler cycle collected. r=bz

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Cycle collect service worker KeepAliveHandler::InternalHandler. r=bz

We have another PromiseNativeHandler object that references a c++ Promise in service worker code.  I haven't seen this trigger the same crash, but we should probably cycle collect it as well.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8787365 [details] [diff] [review]
P2 Cycle collect service worker KeepAliveHandler::InternalHandler. r=bz

I don't see any other PromiseNativeHandler classes that hold a Promise directly.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8787365 [details] [diff] [review]
P2 Cycle collect service worker KeepAliveHandler::InternalHandler. r=bz

Test failures on try.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8787363 [details] [diff] [review]
P1 Make dom/cache FetchHandler cycle collected. r=bz

This is the patch we were discussing last week.  To refresh, we are crashing because:

1) Worker shutdown is triggering a cycle collect
2) The cycle collection is causing the promise holding this PromiseNativeHandler to go away.
3) The PromiseNativeHandler then disposes of its internal mPromise
4) The Promise destruction tries to call back into self-hosted js on the exiting JS runtime triggering badness.

This patch works around the problem by cycle collecting the PromiseNativeHandler as well.  I believe this avoids the issue by dropping the mPromise ref earlier in the CC/GC cycle.  Its kind of a bandaid, though.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Note why KeepAliveHandler::InternalHandler is not cycle collected. r=bz

This is the only other PromiseNativeHandler object I could find that directly holds another mPromise.  We don't want to cycle collect this object, though.  I wrote down the reasons in a comment in this patch.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

>4) The Promise destruction tries to call back into self-hosted js on the exiting JS runtime triggering badness.

It does?  Where is that happening?

What I see from code auditing, and the stack posted earlier in this bug, is that Promise::~Promise only calls mozilla::DropJSObjects(this).  This does eventually end up in CycleCollectedJSRuntime::RemoveJSHolder which tries to do something with the nsScriptObjectTracer.  Is the value it gets out of mJSHolders bogus or something?  Where is the self-hosted JS being called?

> Its kind of a bandaid, though.

Totally seems like one...  Nothing wrong with a band-aid, but it's not quite clear to me what we're _really_ band-aiding over here, and I'd like to understand that.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8788504 [details] [diff] [review]
P2 Note why KeepAliveHandler::InternalHandler is not cycle collected. r=bz

r=me, but seems like this could cause the same crash issues, no?

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> What I see from code auditing, and the stack posted earlier in this bug, is
> that Promise::~Promise only calls mozilla::DropJSObjects(this).  This does
> eventually end up in CycleCollectedJSRuntime::RemoveJSHolder which tries to
> do something with the nsScriptObjectTracer.  Is the value it gets out of
> mJSHolders bogus or something?  Where is the self-hosted JS being called?

Sorry.  Not into self-hosted, but it has to drop js functions that are defined in self-hosted code.  We did not have to do this with the c++ promises in the past.

> Totally seems like one...  Nothing wrong with a band-aid, but it's not quite
> clear to me what we're _really_ band-aiding over here, and I'd like to
> understand that.

I wonder if we are getting a nullptr rt here:

https://dxr.mozilla.org/mozilla-central/source/xpcom/base/HoldDropJSObjects.cpp#39

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Comment on attachment 8788504 [details] [diff] [review]
> P2 Note why KeepAliveHandler::InternalHandler is not cycle collected. r=bz
> 
> r=me, but seems like this could cause the same crash issues, no?

I remember now.  KeepAliveHandler::InternalHandler uses the WorkerHolder mechanism to explicitly break the cycle here if the worker thread is being terminated:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#312
https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#284

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Use a PromiseNativeHandler shim to ensure real PromiseNativeHandlers are released when the Promise settles. r=bz

A better solution we hashed out over IRC.  Basically, we should try to maintain the lifetime PromiseNativeHandlers had in the past.  The spidermonkey promise change inadvertently extended the life of PromiseNativeHandlers until the JS is GC'd.

This patch uses a shim to more aggressively release the PromiseNativeHandler objects.  The shim then lives until the JS is GC'd.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8788594 [details] [diff] [review]
Use a PromiseNativeHandler shim to ensure real PromiseNativeHandlers are released when the Promise settles. r=bz

>+  NS_DECL_ISUPPORTS
>+NS_IMPL_ISUPPORTS0(PromiseNativeHandlerShim);

PromiseNativeHandlerShim needs to implement cycle collection and cc mInner.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1299887_promiseshim.patch

Updated with more circle gathering.

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8788599 [details] [diff] [review]
bug1299887_promiseshim.patch

r=me

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05cd37129db4
Use a PromiseNativeHandler shim to ensure real PromiseNativeHandlers are released when the Promise settles. r=bz

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/05cd37129db4

Comment 24

[Marcia Knous [:marcia]]

Don't think we have to track this for 51 since it is now fixed.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Releasing PromiseNativeHandlers later was introduced in bug 911216 which was released in FF50.  We should probably uplift this.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8788599 [details] [diff] [review]
bug1299887_promiseshim.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 911216
[User impact if declined]: Increased memory footprint and potentially crashes in worker code.  This was revealed in new code in FF51, but the underlying issue exists in FF50 and could occur under certain workloads.
[Describe test coverage new/current, TreeHerder]: No new tests.  Existing tests exercise promises quite a bit.
[Risks and why]: Minimal.
[String/UUID change made/needed]: None.

Comment 27

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

Comment on attachment 8788599 [details] [diff] [review]
bug1299887_promiseshim.patch

Fixes a recent regression, Aurora50+

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=d73d2a4d9be2