David Mandelin [:dmandelin] Reporter
	Description • 14 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
    NativeIterator
  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
      array

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.

	David Mandelin [:dmandelin] Reporter
	Comment 1 • 14 years ago

Temporary fix pushed to moo:

http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/b9e62364729e

	Brian Hackett [Laid off!] Assignee
	Comment 2 • 14 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.

	David Mandelin [:dmandelin] Reporter
	Comment 3 • 14 years ago

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

	Brian Hackett [Laid off!] Assignee
	Comment 4 • 14 years ago

I'll fixup the fast-pathing iterator patch to remove the remaining hazards, should be done and checked in tomorrow AM.

	Brian Hackett [Laid off!] Assignee
	Comment 5 • 14 years ago

Patch for this in bug 578756.

	Paul Wright
	Comment 6 • 14 years ago

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