Previously, Interpreter::StackFrame (which is used to capture info for Error.stack) eagerly extracts info out of CodeBlock and duplicate the work that CodeBlock does to compute line and column numbers (amongst other things). This patch does away with the eager extraction and only stashes the CodeBlock pointer in the Interpreter::StackFrame. Instead, Interpreter::StackFrame will provide methods for computing the desired values on request later.
Created attachment 280741 [details] proposed patch.
Comment on attachment 280741 [details] proposed patch. Attachment 280741 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1462327 New failing tests: fast/events/attribute-listener-deletion-crash.html html5lib/generated/run-tests7-write.html html5lib/generated/run-tests1-write.html html5lib/generated/run-tests18-write.html fast/dom/attribute-event-listener-errors.html fast/events/window-onerror-syntax-error-in-attr.html fast/text/text-combine-crash.html
Created attachment 280744 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 280741 [details] proposed patch. Taking out of review while I look at the failures. Might just need to rebase some tests.
Created attachment 280819 [details] new and improved.
Comment on attachment 280819 [details] new and improved. View in context: https://bugs.webkit.org/attachment.cgi?id=280819&action=review > Source/JavaScriptCore/ChangeLog:9 > + Error.stack) eagerly extracts info out of CodeBlock and duplicate the work that s/duplicate/duplicates > Source/JavaScriptCore/interpreter/Interpreter.cpp:120 > + } Style: I would add a ``` default: ASSERT_NOT_REACHED() ``` case > Source/JavaScriptCore/interpreter/Interpreter.cpp:524 > + StackFrame s = { > + Strong<JSObject>(vm, visitor->callee()), > + Strong<CodeBlock>(), > + 0 > + }; Style: I would say unsigned bytecodeOffset = 0; and then pass that into the constructor. (Or as I said before, make this an Optional<unsigned> > Source/JavaScriptCore/runtime/Error.cpp:151 > + StackFrame* stackFrame; I would call this firstNonNativeFrame. > Source/JavaScriptCore/runtime/Error.cpp:154 > for (unsigned i = 0 ; i < stackTrace.size(); ++i) { > stackFrame = &stackTrace.at(i); > if (stackFrame->bytecodeOffset) As we discussed in person, it's worth changing this to !stackFrame->isNative() Using bytecodeOffset == 0 is just weird and confusing because the bytecodeOffset is allowed to be zero. If anything, we can make it an Optional<unsigned>. > Source/JavaScriptCore/runtime/ErrorInstance.cpp:147 > + bool hasTrace = addErrorInfoAndGetBytecodeOffset(exec, vm, this, useCurrentFrame, callFrame, hasSourceAppender() ? &bytecodeOffset : nullptr); nice change, the previous code was quite an anti-pattern.
Comment on attachment 280819 [details] new and improved. View in context: https://bugs.webkit.org/attachment.cgi?id=280819&action=review >> Source/JavaScriptCore/ChangeLog:9 >> + Error.stack) eagerly extracts info out of CodeBlock and duplicate the work that > > s/duplicate/duplicates Fixed. > Source/JavaScriptCore/ChangeLog:31 > + In the old code, the Error object's addErrorInfoAndGetBytecodeOffset() and the > + inspector's createScriptCallStackFromException() would check if sourceURL is > + empty. If so, they will use this as an indicator to use alternate source info > + in the Error object e.g. url and line numbers from the parser that produced a > + SyntaxError. > + > + In the new implementation, StackFrame only has a sourceURL() function that > + behaves like the old friendlySourceURL(). The client code which were relying > + on sourceURL being empty, will now explicitly check if the StackFrame is for > + native code using a new isNative() query in addition to the sourceURL being > + empty. This achieve functional parity with the old behavior. I indented these 2 paragraphs and added some text to the previous paragraph to make it clearer that these are details elaborating on that previous paragraph. >> Source/JavaScriptCore/interpreter/Interpreter.cpp:120 >> + } > > Style: I would add a > ``` > default: > ASSERT_NOT_REACHED() > ``` > case Applied. >> Source/JavaScriptCore/interpreter/Interpreter.cpp:524 >> + }; > > Style: I would say > unsigned bytecodeOffset = 0; > and then pass that into the constructor. > (Or as I said before, make this an Optional<unsigned> I added a "// unused value because codeBlock is null." comment next to the 0. >> Source/JavaScriptCore/runtime/Error.cpp:151 >> + StackFrame* stackFrame; > > I would call this firstNonNativeFrame. Applied. >> Source/JavaScriptCore/runtime/Error.cpp:154 >> if (stackFrame->bytecodeOffset) > > As we discussed in person, it's worth changing this to !stackFrame->isNative() > Using bytecodeOffset == 0 is just weird and confusing because the bytecodeOffset is allowed to be zero. > If anything, we can make it an Optional<unsigned>. I changed this to: if (!firstNonNativeFrame->isNative()) > Source/JavaScriptCore/runtime/Error.cpp:173 > + if (!stackFrame->isNative() && !frameSourceURL.isEmpty()) Now that we realize that the above loop is looking for the first non-native frame, then we know that this "!stackFrame->isNative()" is superflous. I removed it.
Created attachment 280826 [details] Patch for landing.
Thanks for the review. Landed in r201830: <http://trac.webkit.org/r201830>.