Workers: Don't change the global object while GC is running

RESOLVED FIXED in mozilla1.9.2

Status

()

Core
DOM
--
critical
RESOLVED FIXED
8 years ago
7 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
mozilla1.9.2
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
wanted1.9.0.x -

Firefox Tracking Flags

(status1.9.2 final-fixed, blocking1.9.1 .8+, status1.9.1 .8-fixed)

Details

(Whiteboard: [sg:critical?] fixes 533000)

Attachments

(1 attachment)

997 bytes, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
jorendorff
: review+
Ben Turner (not reading bugmail, use the needinfo flag!)
: superreview+
Details | Diff | Splinter Review
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.
Created attachment 416977 [details] [diff] [review]
Patch
Attachment #416977 - Flags: superreview?(jst)
Attachment #416977 - Flags: review?(jst)
Attachment #416977 - Flags: review?(jorendorff)
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.
Attachment #416977 - Flags: review?(jorendorff) → review+
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.
I pushed the patch to the tryserver
And tryserver loved it. Peterv and I think that this is the cause of bug 533000.
Severity: normal → critical
blocking1.9.1: --- → ?
Flags: blocking1.9.2?
Comment on attachment 416977 [details] [diff] [review]
Patch

Over-the-shoulder r+sr=jst.
Attachment #416977 - Flags: superreview?(jst)
Attachment #416977 - Flags: superreview+
Attachment #416977 - Flags: review?(jst)
Attachment #416977 - Flags: review+
We need this on the branches, fixes GC crashes.
Flags: blocking1.9.2? → blocking1.9.2+
Pushed changeset 2000e14d36b2 to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: blocking1.9.2+ → blocking1.9.2?
Resolution: --- → FIXED
Comment on attachment 416977 [details] [diff] [review]
Patch

This patch should apply without any changes to 1.9.2 and 1.9.1.
Attachment #416977 - Flags: approval1.9.2?
Attachment #416977 - Flags: approval1.9.1.7?
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.
blocking, still! :)
Flags: blocking1.9.2? → blocking1.9.2+
blocking1.9.1: ? → .7+
status1.9.1: --- → wanted
Flags: wanted1.9.0.x-
Comment on attachment 416977 [details] [diff] [review]
Patch

Approved for 1.9.1.7, a=dveditz for release-drivers
Attachment #416977 - Flags: approval1.9.2?
Attachment #416977 - Flags: approval1.9.1.7?
Attachment #416977 - Flags: approval1.9.1.7+
Whiteboard: [can land 1.9.2]
Pushed to 192: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f8e3e9f4d7c4
status1.9.2: --- → final-fixed
Whiteboard: [can land 1.9.2]
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.
status1.9.1: wanted → .7-fixed
Depends on: 536478
Ben, is there a way for QA to test this fix or do we have to trust hopefully existing tests?
This is guarded by a hard assertion so we'll see crash reports if anyone violates it.
But that means there no way for us to verify this fix manually. Is that correct?
Target Milestone: --- → mozilla1.9.2
Yes.
Blocks: 533000
Whiteboard: [sg:critical?] fixes 533000

Updated

7 years ago
Blocks: 560144

Updated

7 years ago
No longer blocks: 560144
Depends on: 560144

Updated

7 years ago
Depends on: 534308
Group: core-security
You need to log in before you can comment on or make changes to this bug.