Summary: | Local JSArray* "keys" in objectConstructorKeys() is not marked during garbage collection | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||||
Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, jnlehner, jonlee, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 312.x | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 140363 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Michael Saboff
2015-01-12 00:10:40 PST
Created attachment 244433 [details]
Patch
Comment on attachment 244433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244433&action=review r=me > Source/JavaScriptCore/ChangeLog:8 > + Move the address of the local variable that is used to mark the top of the stack for By “mark the top”, do you mean “demarcate the top"? Comment on attachment 244433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244433&action=review > Source/JavaScriptCore/heap/MachineStackMarker.cpp:242 > + // We need to mark the stack top in this function so that callee saves are either already on the stack, I think “mark” here should also be “demarcate”. Marking the top of the stack in GC code has a different meaning then demarcating the top of the stack. I think you meant "indicating the top of the stack" (demarcating) instead of "recording as live” (marking). Is that right? (In reply to comment #3) > Comment on attachment 244433 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244433&action=review > > > Source/JavaScriptCore/heap/MachineStackMarker.cpp:242 > > + // We need to mark the stack top in this function so that callee saves are either already on the stack, > > I think “mark” here should also be “demarcate”. Marking the top of the > stack in GC code has a different meaning then demarcating the top of the > stack. I think you meant "indicating the top of the stack" (demarcating) > instead of "recording as live” (marking). Is that right? I'll change the comment to "demarcate". Committed r178266: <http://trac.webkit.org/changeset/178266> This broke a JS test: 2015-01-12 09:21:06.385 testapi[45813:35152987] TEST: "weak value == nil": FAILED Re-opened since this is blocked by bug 140363 Comment on attachment 244433 [details] Patch Rolled out in <https://trac.webkit.org/r178284>. Created attachment 244499 [details]
Updated patch that fixes broken API test
Comment on attachment 244499 [details] Updated patch that fixes broken API test View in context: https://bugs.webkit.org/attachment.cgi?id=244499&action=review r=me > Source/JavaScriptCore/ChangeLog:16 > + We now get the register contents at the same place that we demarcate the current top of > + stack using the address of a local variable, in Heap::markRoots(). The register contents > + buffer is passed along with the demarcation pointer. These need to be done at this level > + in the call tree and no lower, as markRoots() calls various functions that visit object > + pointers that may be latter proven dead. Any of those pointers that are left on the > + stack or in registers could be incorrectly marked as live if we scan the stack contents > + from a called function or one of its callees. The stack demarcation pointer and register > + saving need to be done in the same function so that we have a consistent stack, active > + and spilled registers. I think you should also have a comment about the original bug and why this fix works i.e. that flushing in non-volatile registers in gatherFromCurrentThread() is too late because those registers may have been spilled on the stack and replaced with other values by the time the GC got to gatherFromCurrentThread(), and that this patch fixes it by ensuring that they are spilled into the MachineThreads::RegisterState where we demarcate the top of stack for GC scanning. Hence, any root values are guaranteed to be either on the stack or in the captured RegisterState. Committed r178364: <http://trac.webkit.org/changeset/178364> |