Last Comment Bug 547399 - Workers: Don't let worker messages run if the worker is suspended
: Workers: Don't let worker messages run if the worker is suspended
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
: Andrew Overholt [:overholt]
Mentors:
: 552224 553451 (view as bug list)
Depends on: 552054
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-20 01:56 PST by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2010-04-05 15:38 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.4-fixed
.10-fixed


Attachments
Patch, v1 (11.06 KB, patch)
2010-02-20 01:56 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
jst: review+
jst: superreview+
mbeltzner: approval1.9.2.4-
mbeltzner: approval1.9.1.10-
Details | Diff | Splinter Review
trivial followup fix: remove duplicate 'friend' declaration (1.07 KB, patch)
2010-03-17 13:29 PDT, Daniel Holbert [:dholbert]
bent.mozilla: review+
Details | Diff | Splinter Review
Rollup patch for 1.9.2 (10.76 KB, patch)
2010-04-01 12:20 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
mbeltzner: approval1.9.2.4+
Details | Diff | Splinter Review
Rollup patch for 1.9.1 (12.20 KB, patch)
2010-04-01 13:54 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
mbeltzner: approval1.9.1.10+
Details | Diff | Splinter Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2010-02-20 01:56:51 PST
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.
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-03-12 13:55:58 PST
http://hg.mozilla.org/mozilla-central/rev/f4ce6c16c48b
Comment 2 Daniel Holbert [:dholbert] 2010-03-17 13:29:29 PDT
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.
Comment 3 Daniel Holbert [:dholbert] 2010-03-17 21:49:31 PDT
pushed followup: http://hg.mozilla.org/mozilla-central/rev/d91d66514c2f
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-03-19 13:45:26 PDT
*** Bug 553451 has been marked as a duplicate of this bug. ***
Comment 5 timeless 2010-03-21 06:40:43 PDT
*** Bug 552224 has been marked as a duplicate of this bug. ***
Comment 6 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-29 10:49:45 PDT
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] 2010-03-29 10:50:15 PDT
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!) 2010-04-01 12:20:16 PDT
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.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-04-01 13:53:22 PDT
> 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!) 2010-04-01 13:54:08 PDT
Created attachment 436556 [details] [diff] [review]
Rollup patch for 1.9.1

Here's the patch for 1.9.1
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2010-04-01 13:58:46 PDT
(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] 2010-04-05 10:24:42 PDT
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!) 2010-04-05 15:37:00 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9a9c3d0b69e8
Comment 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-04-05 15:38:55 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5077906c9b46

Note You need to log in before you can comment on or make changes to this bug.