899759 - JS_NondeterministicGetWeakMapKeys can GC while iterating over a weakmap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Here's the relevant code:

        for (ObjectValueMap::Base::Range r = map->all(); !r.empty(); r.popFront()) {
            RootedObject key(cx, r.front().key);
            if (!JS_WrapObject(cx, key.address()))
                return false;
            if (!js_NewbornArrayPush(cx, arr, ObjectValue(*key)))
                return false;
        }

The problem is that if JS_WrapObject does a GC, then the GC could modify the weakmap, which would mess up the iteration.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: weakmap

This is the simplest possible solution. JS_WrapObject should never do much allocation, so we shouldn't need GC here.

Comment 2

[Andrew McCreight [:mccr8]]

It is probably okay to unhide this, as nondeterministicGetWeakMapKeys isn't used anywhere in Gecko or Gaia outside of tests, as far as I can see.

Comment 3

[Justin Lebar (not reading bugmail)]

Yet.  :)

Comment 4

[Andrew McCreight [:mccr8]]

Sure, but we won't be exposing anybody to danger as long as we land this at the same time as the other bug, and making it public will make the code landing waltz easier.

Comment 5

[Justin Lebar (not reading bugmail)]

Absolutely.  Sorry, I didn't mean to disagree.

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 783345 [details] [diff] [review]
weakmap

Review of attachment 783345 [details] [diff] [review]:
-----------------------------------------------------------------

Is it worth a comment? // Prevent GC from mutating the weakmap while iterating.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, opened up.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb36948223cc

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I guess even the simplest patch can burn the tree.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ce7cb22607a

Comment 10

[Justin Lebar (not reading bugmail)]

Blocks a blocker.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eb36948223cc
https://hg.mozilla.org/mozilla-central/rev/3ce7cb22607a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Doesn't look like b2g18 has AutoSuppressGC. Needs a branch-specific patch for uplift.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: weakmap2

OK, let's do it the hard way.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

No idea what branch this patch was made against, but it sure doesn't appear to have been b2g18.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, I feel stupid. b2g18 never had this problem at all.

Comment 16

[u279076]

Assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.