The interpreter engine for the core JavaScript language, independent of the browser's object model. File ONLY core JavaScript language bugs in this category. For bugs involving browser objects such as "window" and "document", use the "DOM" component. For bugs involving calls between JavaScript and C++, use the "XPConnect" component.

Right, so I tracked down the one failure: it was a simple C++ thinko. In StoreBuffer::coalesceForVerification, we call MonoTypeBuffer::accumulateEdges, which calls the method |this->compact()|: I forgot to declare this as virtual, so we did not call the specialized RelocatableMonoTypeBuffer::compact. This was causing us not to remove tagged pointers.
The presence of the tagged pointers was not the problem: we only call inNursery() on the tagged pointer. The problem is that when we dereference the pointer we hit memory that was shifted or deleted. We crash (or not) based on whether what is there happens to look like a GCThing, which (sometimes) asserts when we try to convert it to a pointer if the bottom bits are set.
I have identified three problems:
1) Missing virtual keyword.
2) inNursery should assert that the thing we are checking is not tagged.
3) (unrelated) When we coalesce we should check for overflow first. On overflow, we set the overflow bit on the compartment and set pos=base. This works fine for non-relocatable buffers, but for relocatable buffers it could lead to more removes than puts, which will trigger an assertion during compactRemoved.

Sadly, looks like (2) is inviable because JSRope::flatten rewrites the rope object inline into a different type of string. The pointer that was |right| in the rope becomes |capacity| in the flat string, making the target side of the edge not a pointer. Our assertion then triggers because the "pointer" looks tagged for odd-sized string.
This is annoying, but I don't think it is particularly dangerous, as long as we check that all pointer targets point to a real, correctly aligned thing before we move the target. This could be problematic if we move to a bump-allocated nursery.
Bill, does this seem right, or should we maybe make the store buffer for the manual barriers on ropes relocatable so we can delete these entries when we transform the rope inline?

(In reply to Terrence Cole [:terrence] from comment #5)
> Bill, does this seem right, or should we maybe make the store buffer for the
> manual barriers on ropes relocatable so we can delete these entries when we
> transform the rope inline?
I prefer this solution--relocating the pointers when we flatten.

Created attachment 651887[details][diff][review]
v4: Ensuring no non-pointer targets.
Try run is up at:
https://tbpl.mozilla.org/?tree=Try&rev=4239dface5b0
This turned out to be a bit more involved than I thought. In order to safely add this assert I needed to fix ropes, shape getter/setter, and setPrivate.
I've added a strong assertion to setPrivate and the new setPrivateGCThing that the passed pointer is or is not in the GC heap. The JS_SetPrivate usages all appear to be for non-gcthing pointers. I wasn't 100% sure about was the one in js/xpconnect/src/dombindings.cpp, but the try run should tell us if I guessed correctly.

(In reply to Bill McCloskey (:billm) from comment #13)
> The const_casts are kinda gross, but let's figure out how to fix that in
> another bug. I don't want to hold this up any longer.
It is ugly, but to be fair, we *are* writing to a hashtable key in-place. A const cast is probably a good signal for the absolute insanity occurring there.
One last try run to ensure that I didn't miss anything important hacking on both the marking and hashtable code, at the same time:
https://tbpl.mozilla.org/?tree=Try&rev=15ab37bf3fc5

Ultimately it's going to need more fixing than that: notice the assertion. Once we start moving objects we're going to want to update the key inline. For now it doesn't really matter: we don't move and the assertion will ensure that code gets rewritten when we do.

You need to log in
before you can comment on or make changes to this bug.