Summary: | Have vm's exceptionStack match java's vm's exceptionStack. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Curtis <chris_curtis> | ||||||||
Component: | JavaScriptCore | Assignee: | Chris Curtis <chris_curtis> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, eflews.bot, ggaren, gtk-ews, gyuyoung.kim, philn, webkit-ews, xan.lopez | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Chris Curtis
2013-07-31 17:28:48 PDT
Created attachment 207889 [details]
patch 1
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 Comment on attachment 207889 [details] patch 1 Attachment 207889 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1287846 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 Comment on attachment 207889 [details] patch 1 Attachment 207889 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1298864 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; 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? Created attachment 207933 [details]
patch 2
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? (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. (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. > 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. (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. > 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.
Created attachment 208044 [details]
Patch
Comment on attachment 208044 [details]
Patch
r=me
Comment on attachment 208044 [details] Patch Clearing flags on attachment: 208044 Committed r153669: <http://trac.webkit.org/changeset/153669> All reviewed patches have been landed. Closing bug. |