609042 - Workers: Don't let events posted from workers drown the main thread

Status: NEW

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

Description

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

Attachment: Patch, v1

A simple worker that just postMessage's something to the main thread relentlessly can slow down the main thread a lot as each event is a separate runnable. We've gotten several reports of slowdown and our response thus far has been "well, there's already lots of ways to drown the main thread from script". I think this is easy enough to prevent that it's worth the slight additional complexity.

The attached patch ensures that only one worker event is actually scheduled at a time unless there are no other pending events. This may slow down workers' responsiveness but it will keep the UI zippy.

Comment 1

[Boris Zbarsky [:bzbarsky]]

What effect, if any, does this have on perf tests that use workers?

Comment 2

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

(In reply to comment #1)
> What effect, if any, does this have on perf tests that use workers?

I'm not aware of any perf tests in our tree that use workers, do you mean something in the wild?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yes, in the wild.  There are various perf benchmarks out there that use workers...

Comment 4

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

(In reply to comment #3)
> Yes, in the wild.  There are various perf benchmarks out there that use
> workers...

Hm. I'm not sure how we'll weigh main thread responsiveness vs. worker performance in the end. This patch is tweakable to a certain extent, though, as we could choose to run two events at a time, or five, or some percentage of the remaining events. We'd need to look at some benchmark numbers I guess.

Comment 5

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

I'm also not convinced this is worth doing. Is the concern here that people will accidentally swamp the main thread, as this certainly won't help with any malicious pages.

Comment 6

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

(In reply to comment #5)
> I'm also not convinced this is worth doing. Is the concern here that people
> will accidentally swamp the main thread, as this certainly won't help with any
> malicious pages.

It most certainly *will* help with malicious pages. By keeping the main thread more responsive the user can close malicious pages much easier, thereby cancelling and discarding any excess messages that were posted to the main thread. When the main thread gets flooded it's very difficult to close tabs, windows, etc.

Comment 7

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

Surely a malicious page can spam the main event loop in other ways (create tons of timeouts or call window.postMessage a million times), or simply use a good old

setInterval("while(1) {}", 10);

Comment 8

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

Forgot to say:

I guess I'm not entirely opposed to limiting how many events can be posted to the main event loop, but I think the limits in this patch is a bit harsh and might affect performance of well written pages that for example send events in small but rare bursts.

Comment 9

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

Would a solution here be to do something like the following:

When a worker sends a message to the main thread, put the message at the end of a queue. All of this happens on the worker thread. Then send a runnable to the main thread telling it to process one item from the queue.

However, only send the runnable to the main thread if there isn't already such a runnable pending.

Once the runnable runs, process one item off the queue. If the queue isn't empty, schedule the runnable again.

That way we can never end up with a huge list of runnables posted to the main thread, causing it to starve other events. Instead we always process all pending other events inbetween each worker runnable.

For the case when there isn't any other events pending, the result will be to do exactly what we are doing now. But if the user or page starts interacting with the browser, those events are given priority.

Comment 10

[(dormant account)]

(In reply to Jonas Sicking (:sicking) from comment #9)
> Would a solution here be to do something like the following:
> 
> When a worker sends a message to the main thread, put the message at the end
> of a queue. All of this happens on the worker thread. Then send a runnable
> to the main thread telling it to process one item from the queue.
> 
> However, only send the runnable to the main thread if there isn't already
> such a runnable pending.
> 
> Once the runnable runs, process one item off the queue. If the queue isn't
> empty, schedule the runnable again.
> 
> That way we can never end up with a huge list of runnables posted to the
> main thread, causing it to starve other events. Instead we always process
> all pending other events inbetween each worker runnable.
> 
> For the case when there isn't any other events pending, the result will be
> to do exactly what we are doing now. But if the user or page starts
> interacting with the browser, those events are given priority.

Sounds like workers should get own event queue in addition to the per-window event queues in bug 715376

Comment 11

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

Maybe.  I might be ok with letting workers starve the event queue of the window that created them.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.