Bug 84927 – End of Interpreter::tryCacheGetByID can trigger the garbage collector

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Here is what I believe is going on:

1. The BytecodeGenerator runs emitGetById() in order to emit an op_get_by_id instruction.
        a. This calls m_codeBlock->addPropertyAccessInstruction, which saves the current index as a property access instruction (I'm using the classic interpreter).
2. Upon execution, Interpreter::privateExecute encounters the op_get_by_id and calls Interpreter::tryCacheGetByID.
3. Interpreter::tryCacheGetByID falls through to the very bottom, where it:
        a. Overwrites the opcode with a op_get_by_id_chain
        b. fills in vPC[7] and vPC[4]
        c. Tries to fill in vPC[5] with structure->prototypeChain(callFrame). Note that the call itself happens before vPC[5] gets assigned.
4. the cached prototype chain isn't valid, so Structure::prototypeChain calls StructureChain::create(), which uses placement new to call allocateCell<StructureChain>()
5. allocateCell can, under the right circumstances, cause m_heap->collect(Heap::DoNotSweep) in MarkedAllocator::allocateSlowCase
6. The collection routine walks the C++ stack, looking for valid pointers. One of the pointers it finds is a pointer to the CodeBlock/ScriptExecutable object.
7. While trying to drain the CodeBlock object, it tries to visit each of its children. It does this by going through each of the property access instructions (step 1) to access each of the properties. One of the indices it comes across points to the instruction that was just overwritten in step 3)
8. Believing the opcode to be a correctly-formed op_get_by_id_chain instruction, it attempts to append vPC[5], which hasn't been set yet.

The solution is probably to move the structure->prototypeChain(callFrame) before any modification to vPC in Interpreter::tryCacheGetByID. [e-mail]: How does this sound to you? I'll upload a patch if this sounds reasonable.

Should I try to create an example javascript program that triggers this?

[Myles C. Maxfield]

Created attachment 139479 [details]
Patch

[Geoffrey Garen]

I'm starting to think we should just generally prohibit GC in C++ code, except in certain rare cases like JSON processing or re-entry from C++ into the VM.

[WebKit Review Bot]

Comment on attachment 139479 [details]
Patch

Clearing flags on attachment: 139479

Committed r115657: <http://trac.webkit.org/changeset/115657>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.