Summary: | Simplify Interpreter::StackFrame. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, fpizlo, ggaren, keith_miller, msaboff, rniwa, saam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Local Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Mark Lam
2016-06-07 15:25:37 PDT
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>. |