966384 - Promises on workers should use correct busy count.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Runnables dispatched from the worker thread to itself should modify the busy count so that the worker doesn't get GCed before the runnable is run.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Promises on workers use correct busy count.

Comment 2

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

Comment on attachment 8368716 [details] [diff] [review]
Promises on workers use correct busy count.

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

I would prefer that we create a subclass for this so that other people can inherit from it if needed.  Maybe WorkerSameThreadRunnable?

::: dom/promise/Promise.cpp
@@ +1112,5 @@
> +        // worker between here and the task running.
> +        // FIXME(nsm): khuey: Should this not be done if worker is closing?
> +        if (!worker->ModifyBusyCountFromWorker(worker->GetJSContext(), true)) {
> +          // FIXME(nsm): What's the right error handling pattern here?
> +        }

We really should make ModifyBusyCountFromWorker never fail and then not handle this.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Promises on workers use correct busy count.

Kyle, while I've asserted that ModifyBusyCountFromWorker should never fail in this particular case , would you prefer just changing the return type of ModifyBusyCountFromWorker to void and asserting the Dispatch() inside it?

Comment 4

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

Comment on attachment 8373595 [details] [diff] [review]
Promises on workers use correct busy count.

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

r=me

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b98808ff6c89

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8373595 [details] [diff] [review]
Promises on workers use correct busy count.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
DOM Promises
User impact if declined: 
Promises on worker threads may not work correctly.

Testing completed (on m-c, etc.): 
Yes

Risk to taking this patch (and alternatives if risky): 
None, we haven't shipped Promises yet.

String or IDL/UUID changes made by this patch:
None

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b98808ff6c89

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fbabfaa4ab77