Bug 23635 – Successively reloading Slashdot causes an assertion failure

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Cameron Zwarich (cpst)]

Consecutively reloading Slashdot causes an assertion failure of the following form:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xbbadbeef
0x0058e414 in JSC::asObject (value={m_ptr = 0x20896a80}) at JSObject.h:215
215	    ASSERT(asCell(value)->isObject());
(gdb) bt
#0  0x0058e414 in JSC::asObject (value={m_ptr = 0x20896a80}) at JSObject.h:215
#1  0x0061a3b3 in JSC::JIT::privateCompileGetByIdChainList (this=0xbfffd69c, stubInfo=0x1c2401c0, prototypeStructures=0x1c4eb7f0, currentIndex=1, structure=0x1e6e9ad0, chain=0x1f60ee20, count=2, cachedOffset=4, callFrame=0x208ed178) at /Users/Cameron/WebKit/JavaScriptCore/jit/JITPropertyAccess.cpp:540
#2  0x00602657 in JSC::JIT::compileGetByIdChainList (globalData=0x1fd1a400, callFrame=0x208ed178, codeBlock=0x1f680810, stubInfo=0x1c2401c0, prototypeStructureList=0x1c4eb7f0, currentIndex=1, structure=0x1e6e9ad0, chain=0x1f60ee20, count=2, cachedOffset=4) at JIT.h:313
#3  0x005ee107 in JSC::Interpreter::cti_op_get_by_id_proto_list (args=0x0) at /Users/Cameron/WebKit/JavaScriptCore/interpreter/Interpreter.cpp:4621

[Cameron Zwarich (cpst)]

Created attachment 27172 [details]
First steps at a reduction

Here's a pretty good reduction from the original page. I will try to reduce it further.

[Cameron Zwarich (cpst)]

Created attachment 27174 [details]
Further reduction

This is as good as I can make a reduction. In order for it to work, it needs to be the first thing I load with a local debug build of WebKit.

[Cameron Zwarich (cpst)]

Created attachment 27185 [details]
Even further reduction

Here's a way simpler reduction:

function f(o, propertyName)
{
    return o.propertyIsEnumerable(propertyName);
}

f("Slash", "split");
f("Slash", "length");

I can't reproduce it with jsc, and the same conditions apply as the previous reduction.

[Cameron Zwarich (cpst)]

This program demonstrates the problem as well, although it doesn't crash for me:

function f(o)
{
    return o.propertyIsEnumerable;
}

f("");
f("");

The problem is that a cached prototype chain is created for the JSString Structure, which is stored in JSGlobalData. Since the cached prototype chain is not marked (presumedly because it doesn't have to be marked), the object it is pointing to disappears and is replaced by another object.

I think the bug is that it is caching this prototype chain in the first place, since this Structure lives on JSGlobalData and persists across pages, but I am not sure about the best way to fix it.

A similar bug should occur with the number prototype.

[Cameron Zwarich (cpst)]

It appears that this only happens in the string case, not the number case. I am not sure why the number case doesn't get cached.

[Cameron Zwarich (cpst)]

Ah, the problem only occurs for non-immediate numbers, because immediates are automatically excluded from caching.

[Cameron Zwarich (cpst)]

Gavin, I was pondering adding a bit to Structure that says whether a Structure has been globally allocated to deal with this in the meantime. The fix would be to disable caching for such Structures. Does this sound like an acceptable fix?

[Alexey Proskuryakov]

<rdar://problem/6878952>

[Alexey Proskuryakov]

I cannot reproduce this with ToT. Can this bug be closed?

[Gavin Barraclough]

Having been through the code, I think this bug still exists (at least in theory), and should be fixed.

JSGlobalData contains a set of refpointers to structures, like this:

    RefPtr<Structure> activationStructure;

These Structures contain 

    JSValue m_prototype;

Which I think should be marked.
One simple fix would be to add:

    activationStructure->storedPrototype()

etc to the markStack every time we start a GC.  I'll check with Geoff & see what he thinks of this.

cheers,
G.

[Geoffrey Garen]

Seems like objects that have per-window prototype chains either need to have per-window structures, or need to not participate in prototype chain caching.

[Geoffrey Garen]

There's no need to mark a structure's prototype if it's null. All of the structures in that list have null prototypes. However, they might have non-null prototype chains. Prototype chains are the new thing here.

[Gavin Barraclough]

These do now get marked (Structure is a GC type now, so the RefPtr<Structure>s are all Strong<Structure>s, these are GC roots), so this bug no longer exists.