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

Status: RESOLVED FIXED

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

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!)]

Attachment: Patch

Comment 2

[Jason Orendorff [: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.

Comment 3

[Jason Orendorff [:jorendorff]]

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!)]

I pushed the patch to the tryserver

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!)]

Comment on attachment 416977 [details] [diff] [review]
Patch

Over-the-shoulder r+sr=jst.

Comment 7

[Johnny Stenback (:jst)]

We need this on the branches, fixes GC crashes.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset 2000e14d36b2 to mozilla-central.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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)]

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)]

blocking, still! :)

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 416977 [details] [diff] [review]
Patch

Approved for 1.9.1.7, a=dveditz for release-drivers

Comment 13

[Justin Dolske [:Dolske]]

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!)]

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][⌚️UTC+1]]

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!)]

This is guarded by a hard assertion so we'll see crash reports if anyone violates it.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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!)]

Yes.