Bug 164304

Summary: Crash beneath SlotVisitor::drain @ cooksillustrated.com
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, saam
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 164320    
Bug Blocks:    
Attachments:
Description Flags
Patch
msaboff: review-
Updated Patch mark.lam: review+

Michael Saboff
Reported 2016-11-01 17:20:44 PDT
We are missing a write barrier on a base object in the LLInt put_by_id code when we have a structure transition on that object.
Attachments
Patch (2.96 KB, patch)
2016-11-02 07:44 PDT, Michael Saboff 		msaboff: review-
DetailsFormatted DiffDiff
Updated Patch (3.01 KB, patch)
2016-11-02 11:18 PDT, Michael Saboff 		mark.lam: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Michael Saboff
Comment 1 2016-11-01 17:21:05 PDT
<rdar://problem/21650959>
Michael Saboff
Comment 2 2016-11-02 07:44:38 PDT
Created attachment 293659 [details] Patch Added <https://bugs.webkit.org/show_bug.cgi?id=164320> to add a regression test.
Mark Lam
Comment 3 2016-11-02 08:03:40 PDT
Comment on attachment 293659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293659&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1409 > + # Reload base into t0 > + loadVariable(1, t0) Pardon my ignorance, but t0 was originally loaded with: loadisFromInstruction(1, t3) loadConstantOrVariableCell(t3, t0, .opPutByIdSlow) How do we know that it is not a ConstantCell?
Michael Saboff
Comment 4 2016-11-02 10:27:31 PDT
(In reply to comment #3) > Comment on attachment 293659 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293659&action=review > > > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1409 > > + # Reload base into t0 > > + loadVariable(1, t0) > > Pardon my ignorance, but t0 was originally loaded with: > loadisFromInstruction(1, t3) > loadConstantOrVariableCell(t3, t0, .opPutByIdSlow) > > How do we know that it is not a ConstantCell? This is on a path where we already checked that we have a variable cell.
Michael Saboff
Comment 5 2016-11-02 10:46:58 PDT
(In reply to comment #3) > Comment on attachment 293659 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293659&action=review > > > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1409 > > + # Reload base into t0 > > + loadVariable(1, t0) > > Pardon my ignorance, but t0 was originally loaded with: > loadisFromInstruction(1, t3) > loadConstantOrVariableCell(t3, t0, .opPutByIdSlow) > > How do we know that it is not a ConstantCell? Actually, you're right, the cell could be a constant. I'll upload a new patch shortly.
Michael Saboff
Comment 6 2016-11-02 11:18:08 PDT
Created attachment 293678 [details] Updated Patch
Mark Lam
Comment 7 2016-11-02 11:33:00 PDT
Comment on attachment 293678 [details] Updated Patch r=me
Michael Saboff
Comment 8 2016-11-02 14:12:33 PDT
Committed r208299: <http://trac.webkit.org/changeset/208299>
Note You need to log in before you can comment on or make changes to this bug.