Closed Bug 899759 Opened 7 years ago Closed 7 years ago

JS_NondeterministicGetWeakMapKeys can GC while iterating over a weakmap

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
blocking-b2g leo+
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- fixed
b2g18 --- unaffected
b2g18-v1.0.0 --- unaffected
b2g18-v1.0.1 --- unaffected
b2g-v1.1hd --- unaffected

People

(Reporter: billm, Assigned: billm)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files)

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.
Attached 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+
Doesn't look like b2g18 has AutoSuppressGC. Needs a branch-specific patch for uplift.
Attached 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.