Promises on workers should use correct busy count.

RESOLVED FIXED in Firefox 29

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

unspecified
mozilla30
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox29+ fixed, firefox30+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 8368716 [details] [diff] [review]
Promises on workers use correct busy count.
Assignee: nobody → nsm.nikhil
Attachment #8368716 - Flags: feedback?(khuey)
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.
Attachment #8368716 - Flags: feedback?(khuey) → feedback-
Created attachment 8373595 [details] [diff] [review]
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?
Attachment #8373595 - Flags: review?(khuey)
status-firefox29: --- → affected
status-firefox30: --- → affected
tracking-firefox29: --- → ?
tracking-firefox30: --- → ?
Attachment #8368716 - Attachment is obsolete: true
tracking-firefox29: ? → +
tracking-firefox30: ? → +
Comment on attachment 8373595 [details] [diff] [review]
Promises on workers use correct busy count.

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

r=me
Attachment #8373595 - Flags: review?(khuey) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b98808ff6c89
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
Attachment #8373595 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b98808ff6c89
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox30: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Attachment #8373595 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/fbabfaa4ab77
status-firefox29: affected → fixed
Duplicate of this bug: 966170

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.