Closed Bug 966384 Opened 6 years ago Closed 6 years ago

Promises on workers should use correct busy count.

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 + fixed
firefox30 + fixed

People

(Reporter: nsm, Assigned: nsm)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → nsm.nikhil
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-
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 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+
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Attachment #8373595 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.