84927 2012-04-25 21:11:55 -0700 End of Interpreter::tryCacheGetByID can trigger the garbage collector 2012-04-30 12:19:23 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 litherum webkit-unassigned barraclough ggaren mhahnenberg oliver webkit.review.bot oldest_to_newest 610328 0 litherum 2012-04-25 21:11:55 -0700 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? 612758 1 139479 litherum 2012-04-30 10:58:58 -0700 Created attachment 139479 Patch 612780 2 ggaren 2012-04-30 11:17:51 -0700 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. 612867 3 139479 webkit.review.bot 2012-04-30 12:19:19 -0700 Comment on attachment 139479 Patch Clearing flags on attachment: 139479 Committed r115657: <http://trac.webkit.org/changeset/115657> 612868 4 webkit.review.bot 2012-04-30 12:19:23 -0700 All reviewed patches have been landed. Closing bug. 139479 2012-04-30 10:58:58 -0700 2012-04-30 12:19:18 -0700 Patch bug-84927-20120430105900.patch text/plain 1634 litherum SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTE1NjUyKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBA CisyMDEyLTA0LTMwICBNeWxlcyBNYXhmaWVsZCAgPG1tYXhmaWVsZEBnb29nbGUuY29tPgorCisg ICAgICAgIEVuZCBvZiBJbnRlcnByZXRlcjo6dHJ5Q2FjaGVHZXRCeUlEIGNhbiB0cmlnZ2VyIHRo ZSBnYXJiYWdlIGNvbGxlY3RvcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9ODQ5MjcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICAqIGludGVycHJldGVyL0ludGVycHJldGVyLmNwcDoKKyAgICAgICAgKEpTQzo6 SW50ZXJwcmV0ZXI6OnRyeUNhY2hlR2V0QnlJRCk6CisKIDIwMTItMDQtMzAgIENhcmxvcyBHYXJj aWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQuIEZp eCBtYWtlIGRpc3RjaGVjay4KSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9pbnRlcnByZXRl ci9JbnRlcnByZXRlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2lu dGVycHJldGVyL0ludGVycHJldGVyLmNwcAkocmV2aXNpb24gMTE1NjQ4KQorKysgU291cmNlL0ph dmFTY3JpcHRDb3JlL2ludGVycHJldGVyL0ludGVycHJldGVyLmNwcAkod29ya2luZyBjb3B5KQpA QCAtMTg2NSw2ICsxODY1LDcgQEAgTkVWRVJfSU5MSU5FIHZvaWQgSW50ZXJwcmV0ZXI6OnRyeUNh Y2hlRwogICAgIH0KIAogICAgIAorICAgIFN0cnVjdHVyZUNoYWluKiBwcm90b3R5cGVDaGFpbiA9 IHN0cnVjdHVyZS0+cHJvdG90eXBlQ2hhaW4oY2FsbEZyYW1lKTsKICAgICBzd2l0Y2ggKHNsb3Qu Y2FjaGVkUHJvcGVydHlUeXBlKCkpIHsKICAgICBjYXNlIFByb3BlcnR5U2xvdDo6R2V0dGVyOgog ICAgICAgICB2UENbMF0gPSBnZXRPcGNvZGUob3BfZ2V0X2J5X2lkX2dldHRlcl9jaGFpbik7CkBA IC0xODgwLDcgKzE4ODEsNyBAQCBORVZFUl9JTkxJTkUgdm9pZCBJbnRlcnByZXRlcjo6dHJ5Q2Fj aGVHCiAgICAgICAgIGJyZWFrOwogICAgIH0KICAgICB2UENbNF0udS5zdHJ1Y3R1cmUuc2V0KGNh bGxGcmFtZS0+Z2xvYmFsRGF0YSgpLCBjb2RlQmxvY2stPm93bmVyRXhlY3V0YWJsZSgpLCBzdHJ1 Y3R1cmUpOwotICAgIHZQQ1s1XS51LnN0cnVjdHVyZUNoYWluLnNldChjYWxsRnJhbWUtPmdsb2Jh bERhdGEoKSwgY29kZUJsb2NrLT5vd25lckV4ZWN1dGFibGUoKSwgc3RydWN0dXJlLT5wcm90b3R5 cGVDaGFpbihjYWxsRnJhbWUpKTsKKyAgICB2UENbNV0udS5zdHJ1Y3R1cmVDaGFpbi5zZXQoY2Fs bEZyYW1lLT5nbG9iYWxEYXRhKCksIGNvZGVCbG9jay0+b3duZXJFeGVjdXRhYmxlKCksIHByb3Rv dHlwZUNoYWluKTsKICAgICB2UENbNl0gPSBjb3VudDsKIH0KIAo=