Bug 120537 – Wrong for SlowPathCall to load callFrame reg from vm.topCallFrame after call

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

[Michael Saboff]

After making a slow path call in the exception path, we load the call frame register from vm.topCallFrame.  This isn't needed and wrong. The callFrame register is a callee save and therefore its contents will be preserved across the slow path call. Also, there are no guarantees that vm.topCallFrame is still valid.  Given these two facts, we actually need to be storing the callFrameRegister TO vm.topCallFrame.

[Michael Saboff]

Created attachment 210127 [details]
Patch

[Mark Lam]

Comment on attachment 210127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210127&action=review

> Source/JavaScriptCore/jit/SlowPathCall.h:91
> -        m_jit->reloadCallFrameFromTopCallFrame();
> +        m_jit->updateTopCallFrame();

This looks strange to me.  At this point, given that we have an exception, the caller may have unwound the stack already.  We should not assume that the current stack frame is even valid.  Have you tested against the tests in https://bugs.webkit.org/show_bug.cgi?id=119848 yet?

[Geoffrey Garen]

Comment on attachment 210127 [details]
Patch

Michael, what's the right solution here?

[Michael Saboff]

(In reply to comment #3)
> (From update of attachment 210127 [details])
> Michael, what's the right solution here?

I need to run the tests that Mark suggests to verify this change.

From what I've seen, both locations have the same (correct) value.  It seem to make more sense that we update the one in vm.topCallFrame from the register.

I don't think Mark's concern is valid as this is the code that first discovers that there was an exception in the called C code.  Therefore there shouldn't have been any unwinding.  We're about ready to call ctiVMHandleException() which will call cti_vm_handle_exception() which will call jitThrow() which will unwind.

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 210127 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210127&action=review
> 
> > Source/JavaScriptCore/jit/SlowPathCall.h:91
> > -        m_jit->reloadCallFrameFromTopCallFrame();
> > +        m_jit->updateTopCallFrame();
> 
> This looks strange to me.  At this point, given that we have an exception, the caller may have unwound the stack already.  We should not assume that the current stack frame is even valid.  Have you tested against the tests in https://bugs.webkit.org/show_bug.cgi?id=119848 yet?

I tested the patch with w276-reduced.js attached to https://bugs.webkit.org/show_bug.cgi?id=119848 and it worked fine.

[Geoffrey Garen]

Comment on attachment 210127 [details]
Patch

r=me

[Oliver Hunt]

Comment on attachment 210127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210127&action=review

>>> Source/JavaScriptCore/jit/SlowPathCall.h:91
>>> +        m_jit->updateTopCallFrame();
>> 
>> This looks strange to me.  At this point, given that we have an exception, the caller may have unwound the stack already.  We should not assume that the current stack frame is even valid.  Have you tested against the tests in https://bugs.webkit.org/show_bug.cgi?id=119848 yet?
> 
> I tested the patch with w276-reduced.js attached to https://bugs.webkit.org/show_bug.cgi?id=119848 and it worked fine.

Mark, we cannot unwind the stack through host code - if execution is returning to js we have by definition rolled back only as far the the frame we're moving into.

[Michael Saboff]

Committed r155399: <http://trac.webkit.org/changeset/155399>