Bug 158498 - Simplify Interpreter::StackFrame.
Summary: Simplify Interpreter::StackFrame.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-07 15:25 PDT by Mark Lam
Modified: 2016-06-08 14:04 PDT (History)
8 users (show)

See Also:


Attachments
proposed patch. (18.23 KB, patch)
2016-06-07 15:29 PDT, Mark Lam
mark.lam: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (854.96 KB, application/zip)
2016-06-07 16:27 PDT, Build Bot
no flags Details
new and improved. (18.20 KB, patch)
2016-06-08 11:42 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
Patch for landing. (18.65 KB, patch)
2016-06-08 13:00 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-06-07 15:25:37 PDT
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.
Comment 1 Mark Lam 2016-06-07 15:29:03 PDT
Created attachment 280741 [details]
proposed patch.
Comment 2 Build Bot 2016-06-07 16:26:57 PDT
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
Comment 3 Build Bot 2016-06-07 16:27:02 PDT
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 4 Mark Lam 2016-06-07 16:29:38 PDT
Comment on attachment 280741 [details]
proposed patch.

Taking out of review while I look at the failures.  Might just need to rebase some tests.
Comment 5 Mark Lam 2016-06-08 11:42:58 PDT
Created attachment 280819 [details]
new and improved.
Comment 6 Saam Barati 2016-06-08 12:42:22 PDT
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 7 Mark Lam 2016-06-08 12:56:58 PDT
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.
Comment 8 Mark Lam 2016-06-08 13:00:42 PDT
Created attachment 280826 [details]
Patch for landing.
Comment 9 Mark Lam 2016-06-08 14:04:30 PDT
Thanks for the review.  Landed in r201830: <http://trac.webkit.org/r201830>.