Bug 158498 – Simplify Interpreter::StackFrame.

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

[Mark Lam]

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.

[Mark Lam]

Created attachment 280741 [details]
proposed patch.

[Build Bot]

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

[Build Bot]

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

[Mark Lam]

Comment on attachment 280741 [details]
proposed patch.

Taking out of review while I look at the failures.  Might just need to rebase some tests.

[Mark Lam]

Created attachment 280819 [details]
new and improved.

[Saam Barati]

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.

[Mark Lam]

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.

[Mark Lam]

Created attachment 280826 [details]
Patch for landing.

[Mark Lam]

Thanks for the review.  Landed in r201830: <http://trac.webkit.org/r201830>.