The contained objects may be a different arena and, therefore, may already be finalized. I split this off from bug 839673 to avoid confusion between the two hash maps and make separating the review easier.

Since MapObject is only ever owned by a GC thing, this is easy: it is only ever destructed during a GC, the types of the key and value are known and the destructor is not needed on them, and we know that it is never necessary to call the pre-barrier here.

Comment on attachment 713546 [details] [diff] [review] v0 Hmm. It'd be nice if we could avoid making OrderedHashTable less generic. How are we *supposed* to do links from JS heap to C++ heap to JS heap? Is there a way that doesn't involve skipping destructors? Reassigning to billm for the GC stuff since I don't know if skipping these destructors is actually sufficient.

Comment on attachment 713546 [details] [diff] [review] v0 Review of attachment 713546 [details] [diff] [review]: ----------------------------------------------------------------- Could we add a method to OrderedHashTable that would clear it without calling destructors? Then we could call that from whatever GC code actually destroys these things.

No problem. I didn't initially because it's more code. Sorry it took me so long to get back to this.

Comment on attachment 723598 [details] [diff] [review] v1 Review of attachment 723598 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/MapObject.cpp @@ +225,5 @@ > * > * Returns false on OOM, leaving the OrderedHashTable and any live Ranges > * in the old state. > * > + * With DoCallDestructors The effect on live Ranges is the same as removing This seems to have gotten mangled or something. The original second paragraph of the comment seems to apply regardless of callDestructors. @@ +244,1 @@ > bool clear() { Using a template here seems like overkill that will just bloat the code. Please use a normal argument. @@ +257,5 @@ > alloc.free_(oldHashTable); > + if (callDestructors) > + freeData(oldData, oldDataLength); > + else > + alloc.free_(oldData); It seems a bit clearer to me if you conditionally call destroyData and always call alloc.free_.