1300658 - throttle runnables from Worker threads to the main thread

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Bug 1300118 adapts the xpcom TaskQueue class a bit to be used to prevent main thread runnable flooding.  This bug will use TaskQueue in dom/workers to prevent flooding the main thread from a worker thread.

For example, this patch will keep this worker script from killing main thread responsiveness:

  while(true) { console.log('foo'); }

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Expose a main thread TaskQueue WorkerPrivate. r=baku

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Don't fail test_consoleAndBlobs.html if other console messages are received. r=baku

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make dom/fetch use the worker MainThreadTaskQueue. r=baku

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make service worker APIs use the MainThreadTaskQueue. r=baku

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make service worker APIs use the MainThreadTaskQueue. r=baku

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Fix one place XHR used NS_DispatchToMainThread() directly from a worker. r=baku

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f2f0b81af81
https://treeherder.mozilla.org/#/jobs?repo=try&revision=19fce4757c78

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make service worker APIs use the MainThreadTaskQueue. r=baku

Probably best not to use a WorkerPrivate* right after setting it to nullptr.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Expose a main thread TaskQueue WorkerPrivate. r=baku

Rebase around changes in bug 1300118.  Also, change the API around a little bit to focus on nsIEventTarget instead of TaskQueue.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku

Rebase.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Fix TaskQueue sCurrentThread TLS handling for nsIEventTarget targets. r=jwwang

I removed that other assertion per the last comment.  I also tried to make the patch description text better reflect what its doing.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8788749 [details] [diff] [review]
P3 Fix TaskQueue sCurrentThread TLS handling for nsIEventTarget targets. r=jwwang

Wrong bug...

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8788698 [details] [diff] [review]
P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku

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

::: dom/workers/WorkerPrivate.cpp
@@ -401,5 @@
>      mFinishedWorker->ForgetMainThreadObjects(doomed);
>  
>      RefPtr<MainThreadReleaseRunnable> runnable =
>        new MainThreadReleaseRunnable(doomed, loadGroupToCancel);
> -    if (NS_FAILED(NS_DispatchToMainThread(runnable))) {

I think this super error prone. Can we change NS_DispatchToMainThread so that we don't need to change the way we dispatch runnables to main-thread in workers?

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #17)
> I think this super error prone. Can we change NS_DispatchToMainThread so
> that we don't need to change the way we dispatch runnables to main-thread in
> workers?

Well, I generally think that its better to be explicit about the queue we are dispatching to and not try to do magic in the NS_DispatchToMainThread().  It lets us:

1) Incrementally roll this out to parts of the tree a little bit at a time.
2) Makes it explicit at the callsite which queue or "bucket" you are using.
3) Allows us to introspect the queue or "bucket" and perform back-pressure logic specific to the caller.  For example, an nsTimeout implementation could freeze timers if its queue is backed up, etc.
4) There may be cases where we don't want to use the queue or "bucket".  That's much easier to code and understand later if we are explicitly dispatching to a different nsIEventTarget handle.
5) It might be confusing that you get this magic behavior in NS_DispatchToMainThread(), but not if you do do_GetMainThread()->Dispatch().  Doing this above the xpcom layer for now avoids this kind of confusion.

For workers we could possibly use GetCurrentThreadWorkerPrivate() in NS_DispatchToMainThread(), but I'd rather not do that to start.  I'm not sure that approach will work well in non-worker thread cases and I don't want to set the precedent for it yet.

I'd like to prove this out as a layer above xpcom first and then later pull it into our automatic dispatch routines once its stable.

Anyway, thats my view on it.  What do you think?

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make NS_DispatchToMainThread() use WorkerPrivate::DispatchToMainThread() on worker threads. r=froydnj

This patch builds on P1 and replaces P2 to P6.  It makes NS_DispatchToMainThread() use the WorkerPrivate if we're on a worker thread.

Andrea, is this what you had in mind?

Nathan, what do you think of this?  Would you be ok with a dependency from xpcom to workers here?

Comment 20

[Nathan Froyd [:froydnj]]

Comment on attachment 8789553 [details] [diff] [review]
Make NS_DispatchToMainThread() use WorkerPrivate::DispatchToMainThread() on worker threads. r=froydnj

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

This does not strike me as a good idea.  I'd really like to limit the dependencies that xpcom has on other parts of the code; we have enough dependencies already.  This is also increasing the overhead for every caller of NS_DispatchToMainThread, too.

I don't understand Andrea's objection in comment 17; is the concern that people in worker code have to remember to use ->DispatchToMainThread rather than something else?

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make service worker APIs use the MainThreadTaskQueue. r=baku

I'm able to use GetCurrentThreadWorkerPrivate() in a couple places in service workers with the patch in bug 1301519.

Comment 22

[Andrea Marchesini [:baku]]

> I don't understand Andrea's objection in comment 17; is the concern that
> people in worker code have to remember to use ->DispatchToMainThread rather
> than something else?

Right. that is the point. but avoiding dependences seems to me a good point as well.

Workers have already a custom way to dispatch runnables. With these patches we are introducing a new way to dispatch runnables from workers to the main-thread. If some part of the code forgets to use ->DispatchToMainThread we could end up having runnables scheduled in the wrong order and this could introduce serious bugs.

But I let Nathan decide this as XPCOM owner.

Comment 23

[Andrea Marchesini [:baku]]

Comment on attachment 8788298 [details] [diff] [review]
P4 Make dom/fetch use the worker MainThreadTaskQueue. r=baku

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

::: dom/fetch/Fetch.cpp
@@ +968,5 @@
> +  nsresult rv = NS_OK;
> +  if (mWorkerPrivate) {
> +    rv = mWorkerPrivate->DispatchToMainThread(r.forget());
> +  } else {
> +    rv = NS_DispatchToMainThread(r.forget());

Yeah, that is the thing I would like to avoid.
Maybe we can introduce a new DispatchToMainThread(workerPrivateOrNull, runnable) method?

Comment 24

[Andrea Marchesini [:baku]]

See comment 22 and 23.

Comment 25

[Nathan Froyd [:froydnj]]

(In reply to Andrea Marchesini [:baku] from comment #22)
> > I don't understand Andrea's objection in comment 17; is the concern that
> > people in worker code have to remember to use ->DispatchToMainThread rather
> > than something else?
> 
> Right. that is the point. but avoiding dependences seems to me a good point
> as well.
> 
> Workers have already a custom way to dispatch runnables. With these patches
> we are introducing a new way to dispatch runnables from workers to the
> main-thread. If some part of the code forgets to use ->DispatchToMainThread
> we could end up having runnables scheduled in the wrong order and this could
> introduce serious bugs.

My sense--as an outside observer, not somebody who deals with this on a regular basis--is that workers already have enough special rules about how to operate that adding DispatchToMainThread almost makes things *more* regular, not less.  (We could also put in DEBUG-only assertions in NS_DispatchToMainThread for catching cases when you ought to be using ->DispatchToMainThread.)  Moving towards full-blown document-specific event queues will also bring about more of this "call special methods to dispatch runnables" behavior, so doing it in workers now doesn't seem like a bad thing.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8788698 [details] [diff] [review]
P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku

I said I would reflag this last week in IRC, but forgot.  Of course, feel free to discuss options with Nathan first.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b67fecfa036
P1 Expose a main thread TaskQueue WorkerPrivate. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/623fb54446e0
P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/2453a17bc6c1
P3 Don't fail test_consoleAndBlobs.html if other console messages are received. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d98a586c04d
P4 Make dom/fetch use the worker MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6fb32982e7
P5 Make service worker APIs use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/980659720b86
P6 Fix one place XHR used NS_DispatchToMainThread() directly from a worker. r=baku

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Backed out for bad bug number in commit message:

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56496fad6494
P1 Expose a main thread TaskQueue WorkerPrivate. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e921d61f036
P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9d70b040a2
P3 Don't fail test_consoleAndBlobs.html if other console messages are received. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/39fbc61739ef
P4 Make dom/fetch use the worker MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c74062a27462
P5 Make service worker APIs use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf3a60640cf
P6 Fix one place XHR used NS_DispatchToMainThread() directly from a worker. r=baku

Comment 30

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

Backed out for frequent xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=35719398&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d314f15d679

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Fairly certain this test races removing this file with stopping the use of the file in the os.file chrome worker:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_downloads_search.js#98

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make test_ext_downloads_search.js wait for full window file removal. r=aswan

A better attempt at fixing the race.

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

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make test_ext_downloads_search.js wait for full window file removal. r=aswan

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

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make test_ext_downloads_search.js wait for full window file removal. r=aswan

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

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8790568 [details] [diff] [review]
P7 Make test_ext_downloads_search.js wait for full window file removal. r=aswan

Andrew, this patch fixes a small race in the cleanup of this test.  It calls downloadDir.remove(), but its possible for components in the system or OS to still be holding the file open.  On linux and mac this is fine, but on windows .remove() throws an error if the file is still open.

This patch loops on removing the file until its gone or we give up after 25 times.  This is how we handle the problem in other places in the xpcshell test runner:

https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#475

Comment 37

[Andrew Swan [:aswan]]

Comment on attachment 8790568 [details] [diff] [review]
P7 Make test_ext_downloads_search.js wait for full window file removal. r=aswan

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

Ugh, windows.  I don't really understand the relationship to the bug this is linked to (I assume your changes for that bug made this start failing regularly?  but why?).  There is very similar code to this in a bunch of the other test_ext_downloads_*.js test files, off the top of my head I can't think of a reason why this one is special and it is failing while the others aren't.
But in any case, I suspect you don't want to spend a bunch of time plumbing the depths of these particular tests, the actual fix seems reasonable enough, r+.

::: toolkit/components/extensions/test/xpcshell/head.js
@@ +97,5 @@
> +      } catch (e) {
> +        // ignore
> +      }
> +      if (!dir.exists() || count >= 25) {
> +        return resolve();

If this hasn't succeeded after 25 tries, I think it should reject instead of resolving without actually removing the directory.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Swan [:aswan] from comment #37)
> Ugh, windows.  I don't really understand the relationship to the bug this is
> linked to (I assume your changes for that bug made this start failing
> regularly?  but why?).  There is very similar code to this in a bunch of the
> other test_ext_downloads_*.js test files, off the top of my head I can't
> think of a reason why this one is special and it is failing while the others
> aren't.
> But in any case, I suspect you don't want to spend a bunch of time plumbing
> the depths of these particular tests, the actual fix seems reasonable
> enough, r+.

This patch can slightly delay the async os.file operations since they use a worker thread.  (This patch throttles worker threads.)  I think we were just getting lucky in the past.  Its possible we could wait for all the async file operations to complete, but I don't know the downloads.jsm well enough to trace it all through.  Since this is just test cleanup I think this is a reasonable fix.

> > +      if (!dir.exists() || count >= 25) {
> > +        return resolve();
> 
> If this hasn't succeeded after 25 tries, I think it should reject instead of
> resolving without actually removing the directory.

We don't treat it as an error in the other place I linked to in comment 36, but I can do it.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make test_ext_downloads_search.js wait for full window file removal. r=aswan

Updated to reject if we give up on the directory.

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

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62775b8b0e7b
P1 Expose a main thread TaskQueue WorkerPrivate. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/23830bda70ee
P2 Make WorkerRunnable's destined for main thread use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c66ad87e9f77
P3 Don't fail test_consoleAndBlobs.html if other console messages are received. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/466a5fb382ed
P4 Make dom/fetch use the worker MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/12ee5e6a9c9d
P5 Make service worker APIs use the MainThreadTaskQueue. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a3440336fa8
P6 Fix one place XHR used NS_DispatchToMainThread() directly from a worker. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7275e98b63b
P7 Make test_ext_downloads_search.js wait for full window file removal. r=aswan

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62775b8b0e7b
https://hg.mozilla.org/mozilla-central/rev/23830bda70ee
https://hg.mozilla.org/mozilla-central/rev/c66ad87e9f77
https://hg.mozilla.org/mozilla-central/rev/466a5fb382ed
https://hg.mozilla.org/mozilla-central/rev/12ee5e6a9c9d
https://hg.mozilla.org/mozilla-central/rev/0a3440336fa8
https://hg.mozilla.org/mozilla-central/rev/f7275e98b63b