Bug 120537 - Wrong for SlowPathCall to load callFrame reg from vm.topCallFrame after call
Summary: Wrong for SlowPathCall to load callFrame reg from vm.topCallFrame after call
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-30 10:04 PDT by Michael Saboff
Modified: 2013-09-09 16:02 PDT (History)
0 users

See Also:


Attachments
Patch (2.46 KB, patch)
2013-08-30 10:09 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2013-08-30 10:04:58 PDT
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.
Comment 1 Michael Saboff 2013-08-30 10:09:32 PDT
Created attachment 210127 [details]
Patch
Comment 2 Mark Lam 2013-08-30 11:14:51 PDT
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?
Comment 3 Geoffrey Garen 2013-09-04 15:44:57 PDT
Comment on attachment 210127 [details]
Patch

Michael, what's the right solution here?
Comment 4 Michael Saboff 2013-09-04 16:03:33 PDT
(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.
Comment 5 Michael Saboff 2013-09-09 10:25:39 PDT
(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.
Comment 6 Geoffrey Garen 2013-09-09 15:26:05 PDT
Comment on attachment 210127 [details]
Patch

r=me
Comment 7 Oliver Hunt 2013-09-09 15:42:49 PDT
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.
Comment 8 Michael Saboff 2013-09-09 16:02:22 PDT
Committed r155399: <http://trac.webkit.org/changeset/155399>