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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(status1.9.2 .4-fixed, status1.9.1 .10-fixed)

Details

Attachments

(4 attachments)

Created attachment 427925 [details] [diff] [review]
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.
Attachment #427925 - Flags: superreview?(jst)
Attachment #427925 - Flags: review?(jst)
Status: NEW → ASSIGNED

Updated

8 years ago
Attachment #427925 - Flags: superreview?(jst)
Attachment #427925 - Flags: superreview+
Attachment #427925 - Flags: review?(jst)
Attachment #427925 - Flags: review+
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/mozilla-central/rev/f4ce6c16c48b
Attachment #427925 - Flags: approval1.9.2.3?
Attachment #427925 - Flags: approval1.9.1.10?
Depends on: 552054
Created attachment 433153 [details] [diff] [review]
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.
Attachment #433153 - Flags: review?(bent.mozilla)
Attachment #433153 - Flags: review?(bent.mozilla) → review+
pushed followup: http://hg.mozilla.org/mozilla-central/rev/d91d66514c2f
Duplicate of this bug: 553451

Updated

8 years ago
Duplicate of this bug: 552224
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?
Attachment #427925 - Flags: approval1.9.2.3?
Attachment #427925 - Flags: approval1.9.2.3-
Attachment #427925 - Flags: approval1.9.1.10?
Attachment #427925 - Flags: approval1.9.1.10-
Also, can you help us understand what the "bad" is if threads race? Deadlocks? Memory leaks? Armageddon?
Created attachment 436540 [details] [diff] [review]
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.
Attachment #436540 - Flags: approval1.9.2.4?
> 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.
Created attachment 436556 [details] [diff] [review]
Rollup patch for 1.9.1

Here's the patch for 1.9.1
Attachment #436556 - Flags: approval1.9.1.10?
(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.
Attachment #436540 - Flags: approval1.9.2.4? → approval1.9.2.4+
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
Attachment #436556 - Flags: approval1.9.1.10? → approval1.9.1.10+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9a9c3d0b69e8
status1.9.2: --- → .4-fixed
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5077906c9b46
status1.9.1: --- → .10-fixed
You need to log in before you can comment on or make changes to this bug.