547399 - Workers: Don't let worker messages run if the worker is suspended

Status: RESOLVED FIXED

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

Description

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

Attachment: Patch, v1

Right now a message posted by a worker to the main thread can race with the main thread suspending the worker. Attached patch queues any messages that arrive on the main thread after the worker has been suspended and then resubmits them when the worker is resumed. Also adds a test for suspend behavior.

Comment 1

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

http://hg.mozilla.org/mozilla-central/rev/f4ce6c16c48b

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: trivial followup fix: remove duplicate 'friend' declaration

This checking introduced the following compile warning, in debug builds:
> In file included from ../../../mozilla/dom/base/nsDOMClassInfo.cpp:456:
> dom/src/threads/nsDOMWorker.h:131: warning: 'nsDOMFireEventRunnable' is already a friend of 'nsDOMWorker'

The attached followup-patch fixes this by removing the old debug-only 'friend' statement.

Comment 3

[Daniel Holbert [:dholbert]]

pushed followup: http://hg.mozilla.org/mozilla-central/rev/d91d66514c2f

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 427925 [details] [diff] [review]
Patch, v1

Hey Ben, can we get a rollup here to make sure we don't miss the followup patch bits?

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Also, can you help us understand what the "bad" is if threads race? Deadlocks? Memory leaks? Armageddon?

Comment 8

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

Attachment: Rollup patch for 1.9.2

This is a patch for 1.9.2 that includes the fixes for this bug, bug 552054, and bug 553038. I know that sounds a little scary, but basically the patch here wasn't entirely complete. Bug 552054 closed the last loophole and then bug 553038 fixed a small problem with the test. Those patches have all baked on trunk for a while now with no further test failures.

I don't think that we should leave this bug unfixed on branches. The basic problem is that we can suspend a page and then JS will run on that page while it is in the bfcache. It's certainly not something that a page author will expect but more than that I'm betting that we could cause some weird corruption if the JS modifies the dom when we're not expecting it.

Comment 9

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

> I'm betting that we could cause some weird corruption
> if the JS modifies the dom when we're not expecting it.

Doh! Johnny tells me that this is in fact not a problem. So I guess it's just unexpected behavior, not anything dangerous.

Comment 10

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

Attachment: Rollup patch for 1.9.1

Here's the patch for 1.9.1

Comment 11

[Johnny Stenback (:jst)]

(In reply to comment #8)
> I don't think that we should leave this bug unfixed on branches. The basic
> problem is that we can suspend a page and then JS will run on that page while
> it is in the bfcache. It's certainly not something that a page author will
> expect but more than that I'm betting that we could cause some weird corruption
> if the JS modifies the dom when we're not expecting it.

Yeah, there's no added risk for any corruption here, this doesn't let the page do anything it can't already do by grabbing a reference to a DOM node in a page in an iframe and navigating that iframe and then mutating that DOM node.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 436556 [details] [diff] [review]
Rollup patch for 1.9.1

a=beltzner for 1.9.1.10 and 1.9.2.4

Comment 13

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9a9c3d0b69e8

Comment 14

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5077906c9b46