The GGC store buffer records the address of heap locations containing pointers into the nursery. We do this automatically when a RelocatablePtr<T> is written to, using operator overloading. Since hashtable values are stored on the heap, if GC thing pointers are stored in hash tables they must be wrapped in a RelocatablePtr<T>. Such values can't be used on the stack because writing to them will store a pointer to their location in the store buffer, and for stack locations this will be invalid by the time the store buffer is traced. Unfortunately, the hashtable implementation does create value objects on the stack in a couple of places - while adding to the hashtable, and while rekeying. This is one of the reasons behind bug 913224.

I believe this particular usage to be safe: ~RelocatablePtr<T> "removes" the stack location from the store buffer so as long as we cannot GC while the RelocatablePtr<T> is on the stack, the next MinorGC should not be able to observe a stack location in the buffer.

I linked the wrong bug - this is showing up in a GC triggered from the operation callback in bug 913261.

Actually bug 913261 is not caused by this either. But I still think it's nasty to put stack locations in the storebuffer, even if it's only temporarily. Apart from anything else it's a waste of space. Anyway, maybe this is something to look at when we come to optimizing GGC.

Yeah, I totally agree, if we can do it cleanly. It is the way it is because it trades a bit of runtime complexity for significantly simpler implementation. Other options we considered are: 1) Have a custom BufferableRefs for all hash tables with associated gunk at every put. 2) Split the interface and heap types in HashTable and Vector. 3) Teach HashTable and Vector about the store buffer, presumably with a template argument. The first is easy to implement but impossible to maintain. The second would require an unacceptably large amount of extra type complexity in HashTable and Vector. The third was shot down by Luke before it got off the ground because of the extra template argument. Luckily, the relocatable buffers do not appear to be heavily used on any of the benchmarks I have looked at yet. Or alternatively, we used BufferableRefs instead of RelocatablePtr in all of the tables we know are hot.

Here's a patch to fix the places in jswatchpoint.cpp where we explicitly create these one the stack.

Comment on attachment 814913 [details] [diff] [review] watchpoint-fix Review of attachment 814913 [details] [diff] [review]: ----------------------------------------------------------------- Reasonable. r=me ::: js/src/jswatchpoint.cpp @@ +51,5 @@ > } /* anonymous namespace */ > > +/* > + * Watchpoint contains a RelocatablePtrObject member, which is conceptually a > + * heap-only class. It's preferable not to allocate these on the stack as they One space between sentences.

I think bug 989013 is probably the best way to fix this in the long term.