Bug 84927 - End of Interpreter::tryCacheGetByID can trigger the garbage collector
Summary: End of Interpreter::tryCacheGetByID can trigger the garbage collector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-25 21:11 PDT by Myles C. Maxfield
Modified: 2012-04-30 12:19 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.60 KB, patch)
2012-04-30 10:58 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2012-04-25 21:11:55 PDT
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. oliver@apple.com: 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?
Comment 1 Myles C. Maxfield 2012-04-30 10:58:58 PDT
Created attachment 139479 [details]
Patch
Comment 2 Geoffrey Garen 2012-04-30 11:17:51 PDT
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.
Comment 3 WebKit Review Bot 2012-04-30 12:19:19 PDT
Comment on attachment 139479 [details]
Patch

Clearing flags on attachment: 139479

Committed r115657: <http://trac.webkit.org/changeset/115657>
Comment 4 WebKit Review Bot 2012-04-30 12:19:23 PDT
All reviewed patches have been landed.  Closing bug.