Closed Bug 547399 Opened 14 years ago Closed 14 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .4-fixed
status1.9.1 --- .10-fixed

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(4 files)

Attached patch Patch, v1Splinter Review
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
Attachment #427925 - Flags: superreview?(jst)
Attachment #427925 - Flags: superreview+
Attachment #427925 - Flags: review?(jst)
Attachment #427925 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #427925 - Flags: approval1.9.2.3?
Attachment #427925 - Flags: approval1.9.1.10?
Depends on: 552054
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+
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?
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.
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+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.