Last Comment Bug 534051 - Workers: Don't change the global object while GC is running
: Workers: Don't change the global object while GC is running
Status: RESOLVED FIXED
[sg:critical?] fixes 533000
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Windows 7
: -- critical (vote)
: mozilla1.9.2
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
Depends on: 534308 536478 560144
Blocks: CVE-2010-0160
  Show dependency treegraph
 
Reported: 2009-12-10 14:11 PST by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2010-08-14 01:35 PDT (History)
12 users (show)
jst: blocking1.9.2+
dveditz: wanted1.9.0.x-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final-fixed
.8+
.8-fixed


Attachments
Patch (997 bytes, patch)
2009-12-10 14:27 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
jorendorff: review+
bent.mozilla: superreview+
dveditz: approval1.9.1.8+
Details | Diff | Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2009-12-10 14:11:09 PST
We're setting the global object on the context from another thread outside of a request. That's bad according to all sane peers, so we should stop. Also, I think we should check request depth inside JS_SetGlobalObject.
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-12-10 14:27:52 PST
Created attachment 416977 [details] [diff] [review]
Patch
Comment 2 Jason Orendorff [:jorendorff] 2009-12-10 15:47:41 PST
Comment on attachment 416977 [details] [diff] [review]
Patch

OK. The JS change is a bit risky though. At a first glance I couldn't be sure that all the *other* places that call this function are doing it right.
Comment 3 Jason Orendorff [:jorendorff] 2009-12-10 15:52:36 PST
Sorry--I meant to add: I'm OK with adding that CHECK_REQUEST if the tests pass. If they don't, that is if other code triggers the assertion, we need to look into it. People definitely shouldn't be racing with the GC to call that function. Shudder.
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-12-10 16:19:51 PST
I pushed the patch to the tryserver
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-12-11 12:46:36 PST
And tryserver loved it. Peterv and I think that this is the cause of bug 533000.
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-12-11 12:47:07 PST
Comment on attachment 416977 [details] [diff] [review]
Patch

Over-the-shoulder r+sr=jst.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2009-12-11 13:02:13 PST
We need this on the branches, fixes GC crashes.
Comment 8 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-12-11 13:23:14 PST
Pushed changeset 2000e14d36b2 to mozilla-central.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-12-11 13:24:40 PST
Comment on attachment 416977 [details] [diff] [review]
Patch

This patch should apply without any changes to 1.9.2 and 1.9.1.
Comment 10 Samuel Sidler (old account; do not CC) 2009-12-14 11:53:24 PST
This was blocking 1.9.2 (you accidentally removed the flag), so I'd go ahead and land there and ask jst to re-mark as blocking.
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2009-12-14 14:22:09 PST
blocking, still! :)
Comment 12 Daniel Veditz [:dveditz] 2009-12-14 15:02:10 PST
Comment on attachment 416977 [details] [diff] [review]
Patch

Approved for 1.9.1.7, a=dveditz for release-drivers
Comment 13 Justin Dolske [:Dolske] 2009-12-15 00:27:27 PST
Pushed to 192: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f8e3e9f4d7c4
Comment 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-12-15 15:40:36 PST
Pushed to 1.9.1 also. http://hg.mozilla.org/releases/mozilla-1.9.1/rev/620cfa6038af and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c3f393d2d171.
Comment 15 Henrik Skupin (:whimboo) 2010-01-27 14:32:22 PST
Ben, is there a way for QA to test this fix or do we have to trust hopefully existing tests?
Comment 16 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-01-27 19:08:44 PST
This is guarded by a hard assertion so we'll see crash reports if anyone violates it.
Comment 17 Henrik Skupin (:whimboo) 2010-01-28 02:54:17 PST
But that means there no way for us to verify this fix manually. Is that correct?
Comment 18 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-01-28 09:57:04 PST
Yes.

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