JS_NondeterministicGetWeakMapKeys can GC while iterating over a weakmap

RESOLVED FIXED in Firefox 25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 unaffected, b2g18-v1.0.0 unaffected, b2g18-v1.0.1 unaffected, b2g-v1.1hd unaffected)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments)

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.
Posted patch weakmapSplinter Review
This is the simplest possible solution. JS_WrapObject should never do much allocation, so we shouldn't need GC here.
Attachment #783345 - Flags: review?(sphink)
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.
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.
Absolutely.  Sorry, I didn't mean to disagree.
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.
Attachment #783345 - Flags: review?(sphink) → review+
OK, opened up.
Group: core-security
Blocks a blocker.
blocking-b2g: --- → leo+
https://hg.mozilla.org/mozilla-central/rev/eb36948223cc
https://hg.mozilla.org/mozilla-central/rev/3ce7cb22607a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Doesn't look like b2g18 has AutoSuppressGC. Needs a branch-specific patch for uplift.
Posted patch weakmap2Splinter Review
OK, let's do it the hard way.
Attachment #784480 - Flags: review?(sphink)
Flags: needinfo?(wmccloskey)
Attachment #784480 - Flags: review?(sphink) → review+
No idea what branch this patch was made against, but it sure doesn't appear to have been b2g18.
Flags: needinfo?(wmccloskey)
Well, I feel stupid. b2g18 never had this problem at all.
Assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.