bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

TM: Iterator cache is not GC-safe




JavaScript Engine
8 years ago
7 years ago


(Reporter: dmandelin, Assigned: bhackett)



Firefox Tracking Flags

(Not tracked)




8 years ago
I discovered a bug in the iterator caching mechanism related to GC. This causes testComparisons.js to fail in JM if gczeal is set to 2. 

The basic problem is that we build a shape list for the cache entry at one point in time, and then a GC can happen before we put it into the cache. So the entry is invalidated, but we put it in the cache anyway and can wrongly pick it up later.

In detail: If we have 2 key-iterator loops, one after the other, the order of events related to iterator caching is this (starred events are particularly important to the bug).

1. Call GetIterator to get the iterator for loop 1.
  1.1 first we check the cache.
  *** 1.2 while checking the cache, we build up the proto chain shape array
  1.3 assume this doesn't get a cache hit
  1.4 then we call Snapshot followed by VectorToKeyIterator to create a new
  1.5 in VectorToKeyIterator we call NativeIterator::init
  *** 1.6 init() stores the shape array.

2. Call CloseIterator when done with loop 1.
  2.1 here we store the NativeIterator in the cache without changing the shape

3. Call GetIterator to get the iterator for loop 2.
  3.1 check the cache

If a GC happens between point 2.1 and point 3.1, we are OK, because a GC clears the native iterator cache.

But if a GC happens between 1.2 and 2.1, it can regenerate shapes, which invalidates the shape entry in the NativeIterator. But we don't check for this, and just store it in the cache anyway.

It seems that patching js_CloseIterator to update the shape array fixes the problem. I think this also means NativeIterator::init doesn't need to update the shape array.

Comment 2

8 years ago
This looks (partially and inadvertently) fixed by the fast-pathing iterator patch in bug 578756 (which we discussed last week, but I didn't get around to rebasing and checking in).  This patch doesn't remove the NativeIterator from the cache while in use, but adds a JSITER_ACTIVE bit to NativeIterator.flags, and the iterator can be flushed from the cache during GC even while it is in use (though it won't be collected in the process).  There is still a hazard if the GC occurs before the NativeIterator goes in the cache, i.e. between 1.2 and 1.6.

Comment 3

8 years ago
I backed out the temp fix. It was causing a big regression on string-fasta. I don't know why yet.

Comment 4

8 years ago
I'll fixup the fast-pathing iterator patch to remove the remaining hazards, should be done and checked in tomorrow AM.
Assignee: general → bhackett1024

Comment 5

8 years ago
Patch for this in bug 578756.

Comment 6

7 years ago
So this can be marked as a duplicate, per comment 5?


7 years ago
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 578756
You need to log in before you can comment on or make changes to this bug.