Bug 140348 – Local JSArray* "keys" in objectConstructorKeys() is not marked during garbage collection

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

When we mark objects, we conservatively mark the stack and live registers.  This is done by creating a local variable "void *dummy" in Heap::markRoots().  The address of dummy is passed as a parameter to Heap::gatherStackRoots(), which passes it to MachineThreads::gatherConservativeRoots() which in turn passes it to MachineThread::gatherFromCurrentThread().  The address of dummy is used to mark the "top of the current stack".  In MachineThread::gatherFromCurrentThread(), we use setjmp() to get all the register values.

If a value we want to mark is in a callee save register and that register is saved and subsequently overwritten in either Heap::gatherStackRoots() or MachineThreads::gatherConservativeRoots(), then it won't show up as saved on the stack or in the register value we get from calling setjmp().

This can be fixed by having MachineThread::gatherFromCurrentThread() use one of its locals for the top of stack.

rdar://problem/19440752

[Michael Saboff]

Created attachment 244433 [details]
Patch

[Mark Lam]

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"?

[Mark Lam]

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?

[Michael Saboff]

(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".

[Michael Saboff]

Committed r178266: <http://trac.webkit.org/changeset/178266>

[Alexey Proskuryakov]

This broke a JS test:

2015-01-12 09:21:06.385 testapi[45813:35152987] TEST: "weak value == nil": FAILED

[WebKit Commit Bot]

Re-opened since this is blocked by bug 140363

[Alexey Proskuryakov]

Comment on attachment 244433 [details]
Patch

Rolled out in <https://trac.webkit.org/r178284>.

[Michael Saboff]

Created attachment 244499 [details]
Updated patch that fixes broken API test

[Mark Lam]

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.

[Michael Saboff]

Committed r178364: <http://trac.webkit.org/changeset/178364>