915233 - Port DOM Promises to Workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

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

Attachment: promise-worker

Even though Promise is in flux right now, I had this patch ready and just want to put it up so nobody has to duplicate effort.

Comment 1

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

Promises interact with the event loop, so this patch depends on a solution for bug 914762.

Comment 2

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

Attachment: DOM Promises on Workers.

Two things I'm concerned about:
1) Using the Maybe<AutoCxPusher> only on the main thread. Will the worker maintain the right context with the code I've written?
2) AsyncErrorReporter currently dispatches to current thread. I don't know where I'm supposed to see this error, don't see it in the JS console. Should it dispatch to the main thread?

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 813336 [details] [diff] [review]
DOM Promises on Workers.

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

LGTM.
But before checked it in, can you ask bz or smaug about this CX question?

::: dom/promise/Promise.cpp
@@ +135,5 @@
>  
> +class WorkerPromiseResolverTask MOZ_FINAL : public WorkerRunnable
> +{
> +public:
> +  WorkerPromiseResolverTask(WorkerPrivate* aWorkerPrivate, Promise* aPromise, JS::Handle<JS::Value> aValue, Promise::PromiseState aState)

indent

@@ +533,5 @@
>  
>    nsCOMPtr<nsPIDOMWindow> win =
>      do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(&mResult.toObject()));
>  
> +  // FIXME(nsm): Send to main thread?

follow up?

@@ +626,5 @@
> +    } else {
> +      JSContext* cx = nsContentUtils::GetDefaultJSContextForThread();
> +      WorkerPrivate* worker = GetWorkerPrivateFromContext(cx);
> +      MOZ_ASSERT(worker);
> +      nsRefPtr<WorkerPromiseResolverTask> task = new WorkerPromiseResolverTask(worker, this, aValue, aState);

indent.

::: dom/promise/Promise.h
@@ +32,2 @@
>    friend class PromiseResolverTask;
> +  friend class WorkerPromiseResolverTask;

sort

::: dom/promise/PromiseCallback.cpp
@@ +159,5 @@
>  {
> +  // AutoCxPusher and co. interact with xpconnect, which crashes on
> +  // workers. On workers we'll get the right context from
> +  // GetDefaultJSContextForThread(), and since there is only one context, we
> +  // don't need to push or pop it from the stack. Is that correct?

This is a good question. Can you ping smaug or bz?

Comment 4

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

Attachment: DOM Promises on Workers.

bz, smaug

Could you take a look at the two comments:
1) AutoCxPusher initialization only in the main thread in dom/promise/PromiseCallback.cpp
2) Necessity of rooting mValue in WorkerPromiseResolverTask.
and tell me if I'm doing the right thing.

Comment 5

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

Comment on attachment 817442 [details] [diff] [review]
DOM Promises on Workers.

Andrea, I changed MaybeReportRejected() to dispatch to main thread, but as I mentioned on IRC, normal JS exceptions thrown in a resolve/reject callback don't seem to have an errorReport. Do you know how to hit the last line? I can't see a test case for it.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Nikhil Marathe [:nsm] from comment #4) 
> Could you take a look at the two comments:
> 1) AutoCxPusher initialization only in the main thread in
> dom/promise/PromiseCallback.cpp
No cx stack on workers, and we do similar thing in CallbackObject.
(I wonder why Promises can't use normal CallbackObjects)



> 2) Necessity of rooting mValue in WorkerPromiseResolverTask.
> and tell me if I'm doing the right thing.
Yes, you need to root the value. Nothing would otherwise guarantee that the value stays alive.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 817442 [details] [diff] [review]
DOM Promises on Workers.


>+'Promise': {
>+    'wantsQI': False
>+},
Based on https://bugzilla.mozilla.org/show_bug.cgi?id=920196 this shouldn't be needed.

>@@ -176,17 +263,17 @@ Promise::EnabledForScope(JSContext* aCx,
>   // certified app.
>   if (PrefEnabled()) {
>     return true;
>   }
Why is reading the pref from non-main-thread ok?

>+  // Should work according to khuey since there is only one global object.
drop the this


>+    } else {
>+      JSContext* cx = nsContentUtils::GetDefaultJSContextForThread();
>+      WorkerPrivate* worker = GetWorkerPrivateFromContext(cx);
Hmm, we don't have a helper method for this? We really need the cx?

>   // Now post an event to do the real reporting async
>-  NS_DispatchToCurrentThread(
>+  NS_DispatchToMainThread(
>     new AsyncErrorReporter(JS_GetObjectRuntime(&mResult.toObject()),
>                            report,
>                            nullptr,
>                            nsContentUtils::GetObjectPrincipal(&mResult.toObject()),
>                            win));
Does this really work? You get some sane principal even on workers?
>+  JSContext *cx = nsContentUtils::GetDefaultJSContextForThread();
* should go with the type


r- because I don't think the pref check is thread safe.

Comment 8

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

Attachment: DOM Promises on Workers.

Olli,

So I moved the pref check to the main thread using the super convenient SyncRunnable class.
It works until we have a story around doing preferences properly (although this might be the proper story).

As for the principals check. The nsContentUtils call isn't doing anything thread-unsafe or something that asserts on non main thread. mResult is also owned by the worker thread, so the compartment and thing will work out. I don't see the problem their. Of course, still need baku to tell me how to trigger this code path.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 818165 [details] [diff] [review]
DOM Promises on Workers.

>+    // FIXME(nsm): Is this required in workers and is the comment still valid?
You need to root JS stuff always if you don't otherwise explicitly keep it alive.

> Promise::EnabledForScope(JSContext* aCx, JSObject* /* unused */)
> {
>   // Enable if the pref is enabled or if we're chrome or if we're a
>   // certified app.
>-  if (PrefEnabled()) {
>+  nsCOMPtr<nsIThread> mainThread;
>+  nsresult rv = NS_GetMainThread(getter_AddRefs(mainThread));
>+  MOZ_ASSERT(NS_SUCCEEDED(rv));
>+
>+  nsRefPtr<PrefEnabledRunnable> r = new PrefEnabledRunnable();
>+  // Force dispatch (third arg) is not passed, which means it will behave like
>+  // a normal function call if running on the main thread.
>+  SyncRunnable::DispatchToThread(mainThread, r);
>+  if (r->Enabled()) {
>     return true;
>   }
So we want to do this runnable stuff only when we're not in the main thread.
And I think it should be ok to do it once per worker, since this can cause rather bad
pause in worker execution.


> Promise::RunResolveTask(JS::Handle<JS::Value> aValue,
>                         PromiseState aState,
>                         PromiseTaskSync aAsynchronous)
> {
>   // If the synchronous flag is unset, queue a task to process our
>   // accept callbacks with value.
>   if (aAsynchronous == AsyncTask) {
>-    nsRefPtr<PromiseResolverTask> task =
>-      new PromiseResolverTask(this, aValue, aState);
>-    NS_DispatchToCurrentThread(task);
>+    if (MOZ_LIKELY(NS_IsMainThread())) {
>+      nsRefPtr<PromiseResolverTask> task =
>+        new PromiseResolverTask(this, aValue, aState);
>+      NS_DispatchToCurrentThread(task);
>+    } else {
>+      JSContext* cx = nsContentUtils::GetDefaultJSContextForThread();
>+      WorkerPrivate* worker = GetWorkerPrivateFromContext(cx);
>+      MOZ_ASSERT(worker);
>+      nsRefPtr<WorkerPromiseResolverTask> task =
>+        new WorkerPromiseResolverTask(worker, this, aValue, aState);
>+      worker->Dispatch(task);
>+    }
Would it be possible to integrate the dispatching code to the *Task class so that the same class could be used in workers and main thread.
We don't want to have very similar PromiseResolverTask and WorkerPromiseResolverTask classes.

Comment 10

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

Attachment: DOM Promises on Workers.

I can't combine the two classes because WorkerRunnable requires a non-null worker in the constructor, which I cannot get on the main thread.
Also it seems a little dangerous to override nsIRunnable::Run() in a subclass when WorkerRunnable uses it for some important processing in the workers case, even though I could guarantee that it is always called on the worker thread. I've compromised by introducing PromiseResolverMixin which at least combines the logic and adds owning thread assertions.

Comment 11

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

Comment on attachment 824310 [details] [diff] [review]
DOM Promises on Workers.

Got that AsyncErrorReporter code path to trigger, crashes on worker, I'll have a new patch up soon.

Comment 12

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

Attachment: DOM Promises on Workers.

bz for MaybeReportRejected()
smaug for the mixin
khuey for GetCurrentThreadWorkerPrivate()

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 824797 [details] [diff] [review]
DOM Promises on Workers.

Ok. The runnable handling will hopefully get better
once event loop in workers has been fixed.

>-  nsCOMPtr<nsPIDOMWindow> win =
>-    do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(&mResult.toObject()));
>+  // Remains null in case of worker.
>+  nsCOMPtr<nsPIDOMWindow> win;
>+  // Remains null in case of a content worker.
>+  nsIPrincipal* principal = nullptr;
>+
>+  if (MOZ_LIKELY(NS_IsMainThread())) {
>+    win =
>+      do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(&mResult.toObject()));
>+    principal = nsContentUtils::GetObjectPrincipal(&mResult.toObject());
>+  } else {
>+    WorkerPrivate* worker = GetCurrentThreadWorkerPrivate();
>+    MOZ_ASSERT(worker);
>+    // AsyncErrorReporter only cares whether principal is system principal
>+    // or not.
>+    // We have to do this workaround because worker->GetPrincipal() is main
>+    // thread only.
>+    if (worker->IsChromeWorker()) {
>+      principal = nsContentUtils::GetSystemPrincipal();
>+    }
>+  }
> 
>   // Now post an event to do the real reporting async
>-  NS_DispatchToCurrentThread(
>+  NS_DispatchToMainThread(
>     new AsyncErrorReporter(JS_GetObjectRuntime(&mResult.toObject()),
>                            report,
>                            nullptr,
>-                           nsContentUtils::GetObjectPrincipal(&mResult.toObject()),
>+                           principal,
>                            win));
Principal handling is a bit scary, but should be ok, even in workers. 
I think better would be to have different ctor for AsyncErrorReporter which
doesn't take nsIPrincipal but just bool to tell chrome or content. 
But using principal should be fine for now.


>-  AutoJSContext cx;
>+  // AutoCxPusher and co. interact with xpconnect, which crashes on
>+  // workers. On workers we'll get the right context from
>+  // GetDefaultJSContextForThread(), and since there is only one context, we
>+  // don't need to push or pop it from the stack. Is that correct?
>+  JSContext *cx = nsContentUtils::GetDefaultJSContextForThread();
* goes with the type.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 824797 [details] [diff] [review]
DOM Promises on Workers.

Using nsContentUtils::GetSystemPrincipal not on the main thread gives me the willies.

If all AsyncErrorReporter cares about is system vs not, should we change the argument it takes to a boolean instead?

Also, have you verified that nsIScriptError is safe to use off-main-thread?  I assume nsIConsoleService is....

Comment 15

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

(In reply to Boris Zbarsky [:bz] from comment #14)
> Comment on attachment 824797 [details] [diff] [review]
> DOM Promises on Workers.
> 
> Using nsContentUtils::GetSystemPrincipal not on the main thread gives me the
> willies.
> 
> If all AsyncErrorReporter cares about is system vs not, should we change the
> argument it takes to a boolean instead?

This can be fixed to take a bool, it only uses the check to decide the category under which to show the error.

> 
> Also, have you verified that nsIScriptError is safe to use off-main-thread? 
> I assume nsIConsoleService is....

I am not using any of this off main thread. it's dispatched to the main thread.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I am not using any of this off main thread. it's dispatched to the main thread.

Ah, perfect; I somehow missed the change there.  ;)

Comment 17

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

Attachment: DOM Promises on Workers.

AsyncErrorReporter boolean arg.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 824937 [details] [diff] [review]
DOM Promises on Workers.

r=me on the MaybeReportRejected bits.

Comment 19

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

Attachment: DOM Promises on Workers.

Dropping r? khuey because he added GetWorkerPrivateForCurrentThread() in another patch.

Comment 20

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

Comment on attachment 827671 [details] [diff] [review]
DOM Promises on Workers.

Promise on workers is hidden behind dom.promise.workers.enabled and checked in RegisterBindings.cpp.

Comment 21

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

You can't check the pref off the main thread.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 827671 [details] [diff] [review]
DOM Promises on Workers.

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

sr=me with that fixed.

::: dom/promise/Promise.cpp
@@ +261,5 @@
> +    const char* pref;
> +    if (mOnWorker) {
> +      pref = "dom.promise.workers.enabled";
> +    } else {
> +      pref = "dom.promise.enabled";

I don't see a reason to use separate prefs. Just use the same one.

::: dom/workers/test/promise_worker.js
@@ +1,1 @@
> +function ok(a, msg) {

Is there no way we can use the same js file to test both main thread and workers? That should make it eaiser to add more tests.

Comment 23

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

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

Comment 24

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

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/a00afeffd0ac for breaking the build: https://tbpl.mozilla.org/php/getParsedLog.php?id=30176750&tree=Mozilla-Inbound

Comment 25

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

Let's try again:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b2316c5a1d69

Comment 26

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f86c5c6d100d for Windows b2g desktop bustage

Comment 27

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

Not just b2g, but all of Windows.  https://hg.mozilla.org/integration/mozilla-inbound/annotate/b2316c5a1d69/dom/promise/Promise.cpp#l257 has the wrong calling convention on Windows ... you need to use the NS_IMETHOD macro.

Comment 28

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

third time sticky?
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8222e9ae0a21

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Backed out under suspicion of making Windows debug xpcshell perma-fail a la bug 845190. Good luck.
https://hg.mozilla.org/integration/mozilla-inbound/rev/95fa48bbbc0a

https://tbpl.mozilla.org/php/getParsedLog.php?id=30232602&tree=Mozilla-Inbound

Comment 30

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

Attachment: DOM Promises on Workers.

The problem was SyncRunnable. The main thread may enter shutdown while the worker thread is blocked on SyncRunnable, after which the worker doesn't quit.

Comment 31

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

Comment on attachment 829580 [details] [diff] [review]
DOM Promises on Workers.

Switched to reading and storing pref in RuntimeService.

bent, review for RuntimeService.{h,cpp} and RegisterBindings.cpp, Promise::EnabledForScope() please.

Comment 32

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

Attachment: DOM Promises on Workers.

Slight fix to ensure that chromeworker and system principal checks run on main thread pref check.

Comment 33

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8333986 [details] [diff] [review]
DOM Promises on Workers.

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

::: dom/workers/RuntimeService.cpp
@@ +200,5 @@
> +// Protected by RuntimeService::mMutex.
> +bool gPromiseEnabled;
> +
> +static int
> +PromiseEnableChanged(const char* aPrefName, void* aClosure)

You're going to hate me, but I think we should do this totally differently. When it was just Dump() I was ok with the mutex, but if we're going to start doing pref checks at random times for production-quality code we don't want all workers to block on each other.

So let's add a per-worker pref cache for the prefs we know we need. For now they're only booleans so that should be pretty tiny. We can add it to WorkerPrivate, then update them using the same basic mechanism as the JSSettings.

I'm happy to chat about this, sorry to make you do more work here :(

Comment 34

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8333986 [details] [diff] [review]
DOM Promises on Workers.

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

Ok, this patch plus the one in bug 936700 should be fine.

::: dom/workers/moz.build
@@ +9,5 @@
>  MODULE = 'dom'
>  
>  # Public stuff.
>  EXPORTS.mozilla.dom += [
> +    'RuntimeService.h',

This is not needed any more I think

Comment 35

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4d80ce96c34

Comment 36

[Phil Ringnalda (:philor)]

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

Comment 37

[Ioana (away)]

Marking in-testsuite+ since this is covered automatically by
dom/workers/test/promise_worker.js
dom/workers/test/test_promise.html

Is there any need for manual QA for this feature?

Comment 38

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

(In reply to Ioana Budnar, QA [:ioana] from comment #37)
> Marking in-testsuite+ since this is covered automatically by
> dom/workers/test/promise_worker.js
> dom/workers/test/test_promise.html
> 
> Is there any need for manual QA for this feature?

The automated tests should be enough, since they match the main thread promise tests.

Comment 39

[Chris Mills [:cmills]]

covered on https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers