Bug 119362 – Have vm's exceptionStack match java's vm's exceptionStack.

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

[Chris Curtis]

This will allow the inspector to see the previous stack trace as well as the current stack trace if desired.

[Chris Curtis]

Created attachment 207889 [details]
patch 1

[Early Warning System Bot]

Comment on attachment 207889 [details]
patch 1

Attachment 207889 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1288923

[Early Warning System Bot]

Comment on attachment 207889 [details]
patch 1

Attachment 207889 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1287846

[EFL EWS Bot]

Comment on attachment 207889 [details]
patch 1

Attachment 207889 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1299796

[kov's GTK+ EWS bot]

Comment on attachment 207889 [details]
patch 1

Attachment 207889 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1298864

[Geoffrey Garen]

Looks like you need an #include or two:

../../Source/WTF/wtf/RefCountedArray.h:128:23: error: invalid use of incomplete type 'struct JSC::StackFrame'
         return m_data + Header::fromPayload(m_data)->length;

[Geoffrey Garen]

Comment on attachment 207889 [details]
patch 1

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

> Source/JavaScriptCore/ChangeLog:10
> +        1. The vm->exceptionStack is unconditionally updated at each throw, and error 
> +            objects are only updated when they do not have the stack property.

You should mention the 'why' here: This matches other browsers, and Java VMs.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:617
> -        if (!callFrame->vm().exceptionStack().size()) {
> +        if (!callFrame->vm().getExceptionStack().size()) {

WebKit style only uses the "get" prefix for functions that have out parameters. This should be "exceptionStack()".

> Source/JavaScriptCore/runtime/VM.cpp:535
> +void VM::setExceptionStack(const RefCountedArray<StackFrame> newStack)

Passing an object like this will invoke its copy constructor. In the case of RefCountedArray, that will copy a pointer on the stack and then increment a refcount. When the temporary copy is destroyed, it will do some branching and possibly decrement the refcount. This is needless wasted performance.

You should pass "const RefCountedArray<StackFrame>&" to avoid this cost.

> Source/JavaScriptCore/runtime/VM.h:336
> +        RefCountedArray<StackFrame> getExceptionStack() const { return m_exceptionStack; } 
> +        void setExceptionStack(const RefCountedArray<StackFrame>);
> +        RefCountedArray<StackFrame>& lastExceptionStack() { return m_lastExceptionStack; }

I believe it's sufficient to have a single "lastExceptionStack". Any new exception can just overwrite the last lastExceptionStack. Is there any time when we want to know the current exception stack and the last simultaneously?

[Chris Curtis]

Created attachment 207933 [details]
patch 2

[Geoffrey Garen]

Comment on attachment 207933 [details]
patch 2

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

> Source/JavaScriptCore/interpreter/Interpreter.cpp:621
> -        if (!callFrame->vm().exceptionStack().size()) {
> +        if (!callFrame->vm().lastExceptionStack().size()) {
>              Vector<StackFrame> stack;
>              callFrame->vm().interpreter->getStackTrace(stack);
> -            callFrame->vm().exceptionStack() = RefCountedArray<StackFrame>(stack);
> +            callFrame->vm().lastExceptionStack() = RefCountedArray<StackFrame>(stack);
>          }

I think we want to set lastExceptionStack unconditionally, don't we? Isn't that what addErrorInfo() does in the "if" case above?

[Chris Curtis]

(In reply to comment #9)
> (From update of attachment 207933 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207933&action=review
> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:621
> > -        if (!callFrame->vm().exceptionStack().size()) {
> > +        if (!callFrame->vm().lastExceptionStack().size()) {
> >              Vector<StackFrame> stack;
> >              callFrame->vm().interpreter->getStackTrace(stack);
> > -            callFrame->vm().exceptionStack() = RefCountedArray<StackFrame>(stack);
> > +            callFrame->vm().lastExceptionStack() = RefCountedArray<StackFrame>(stack);
> >          }
> 
> I think we want to set lastExceptionStack unconditionally, don't we? Isn't that what addErrorInfo() does in the "if" case above?

I was not sure about this case, its an exception coming from a non object. Considering that all of the changes I made effected error objects only, and the original changes to the vm were reverted to its original state. It seemed like a bad idea to change this functionality. These changes are just the naming convention that was altered in the process.

[Chris Curtis]

(In reply to comment #9)
> (From update of attachment 207933 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207933&action=review
> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:621
> > -        if (!callFrame->vm().exceptionStack().size()) {
> > +        if (!callFrame->vm().lastExceptionStack().size()) {
> >              Vector<StackFrame> stack;
> >              callFrame->vm().interpreter->getStackTrace(stack);
> > -            callFrame->vm().exceptionStack() = RefCountedArray<StackFrame>(stack);
> > +            callFrame->vm().lastExceptionStack() = RefCountedArray<StackFrame>(stack);
> >          }
> 
> I think we want to set lastExceptionStack unconditionally, don't we? Isn't that what addErrorInfo() does in the "if" case above?

After running tests on this, it would be incorrect to remove the check. Seen in current regression tests:
storage/websql/sql-error-codes.html
fast/dom/exception-getting-event-handler.html

Throw error is called, which originally sets the vm exceptionStack. Later, Throw exception is called. Without that check, the exceptionStack overwrites its own current exception. This results in the lose of a few call frames.

[Geoffrey Garen]

> I was not sure about this case, its an exception coming from a non object.

Why do we want different behavior for throwing an object vs throwing, say, a string?

> These changes are just the naming convention that was altered in the process.

Right. But "lastExceptionStack" is a bad name if it doesn't hold the last exception stack. Something about our understanding here is wrong.

> After running tests on this, it would be incorrect to remove the check. Seen in current regression tests:
> storage/websql/sql-error-codes.html
> fast/dom/exception-getting-event-handler.html

If you change fast/dom/exception-getting-event-handler.html to throw an object instead of a number, does it have the same problem? If so, what's the right fix? Our goal is that all throwing should behave the same way, regardless of what value is thrown.

> Throw error is called, which originally sets the vm exceptionStack. Later, Throw exception is called. Without that check, the exceptionStack overwrites its own current exception. This results in the lose of a few call frames.

Do you know why throwException gets the wrong information about the call stack? That seems like the fundamental bug here.

[Chris Curtis]

(In reply to comment #12)
> > After running tests on this, it would be incorrect to remove the check. Seen in current regression tests:
> > storage/websql/sql-error-codes.html
> > fast/dom/exception-getting-event-handler.html
> 
> If you change fast/dom/exception-getting-event-handler.html to throw an object instead of a number, does it have the same problem? If so, what's the right fix? Our goal is that all throwing should behave the same way, regardless of what value is thrown.
> 
> > Throw error is called, which originally sets the vm exceptionStack. Later, Throw exception is called. Without that check, the exceptionStack overwrites its own current exception. This results in the lose of a few call frames.
> 
> Do you know why throwException gets the wrong information about the call stack? That seems like the fundamental bug here.

If you throw an object instead, the result gave the correct information, but that is because there was a similar check that looked to see if error information was already set.  The only difference was that it looks at the error object instead of the vm. By removing that check, the results matched, but exception stack was missing frames.

Both cases call throw exception a second time, and both had a check to see if the error was already set. 

So a solution could be to change the naming convention of lastExceptionStack back to exceptionStack and leave the functionality here. The other solution is to change what is causing throw exception to be called twice and remove the checks for both objects and non objects throws.

In general I think the first solution is probably better, but I am looking at the conditions for the second throw, to get a better understanding of what changes would need to be done.

[Geoffrey Garen]

> storage/websql/sql-error-codes.html
> fast/dom/exception-getting-event-handler.html

What these tests have in common is that they throw exceptions from within JavaScript callbacks. sql-error-codes.html throws from within a database transaction callback, and exception-getting-event-handler.html throws from within an event handler callback.

[Chris Curtis]

Created attachment 208044 [details]
Patch

[Geoffrey Garen]

Comment on attachment 208044 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 208044 [details]
Patch

Clearing flags on attachment: 208044

Committed r153669: <http://trac.webkit.org/changeset/153669>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.