Bug 158210 – Web Inspector: Wrong function name next to scope

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Saam Barati]

Created attachment 280102 [details]
image of bug

I saw this repro on the STP before ShadowChicken.
It also repros with ShadowChicken. I don't think the bug is ShadowChicken related.

run this file:
```
function foo() {
    let x = 10;
    bar();
    return baz();
}
function bar() {
    let z = 40;
    debugger;
    return 20;
}
foo();
```
And you will stop at the 'debugger' inside bar.
Show the scope chain for 'bar' to the right.
You will see the view for 'bar's scope chain
looks as follows:
`Closure Variables (foo)`
`z : 40`

[Radar WebKit Bug Importer]

<rdar://problem/26543093>

[Timothy Hatcher]

Joe worked on this earlier in the release. It might be from those changes.

[Joseph Pecoraro]

Yeah, I've seen asserts from code in here that could lead to this.

[Joseph Pecoraro]

Okay, so this code is just plain wrong for non-nested functions.

(1) Non-nested:

- Call Frame 1 (bar) has 4 Scopes [ bar lexical, bar var, global lexical, global ]
- Call Frame 2 (foo) has 4 Scopes [ foo lexical, foo var, global lexical, global ]

>    function foo() {
>        let x = 10;
>        bar();
>    }
>    function bar() {
>        let z = 40;
>        debugger;
>        return 20;
>    }
>    foo();


(2) Nested:

- Call Frame 1 (bar) has 6 Scopes [ bar lexical, bar var, foo lexical, foo var, global lexical, global ]
- Call Frame 2 (foo) has 4 Scopes [ foo lexical, foo var, global lexical, global ]

>    function foo() {
>        let x = 10;
>        bar();
>        function bar() {
>            let z = 40;
>            debugger;
>            return 20;
>        }
>    }
>    foo();

The algorithm in ScopeChainDetailsSidebarPanel.prototype._generateCallFramesSection was always assuming that the scope chain in a call frame always included the scope chain in an earlier call frame. That is _not_ the case when functions are not nested (1).

In order to get this to work, we will have to include more data on the Scope objects themselves. Currently it only includes the `object` and `type`. It can include other details such as whether or not it is lexical, and ideally some `name`.

That would be enough information for us to provide the correct sidebar details for scope chains in (1) and (2).

[Joseph Pecoraro]

Created attachment 281959 [details]
[PATCH] WIP

This approach adds a "name" to the Scope which the frontend uses to group scopes within a function. This passes the more involved tests.

This adds rare data to SymbolTable pointing to a CodeBlock during debugging. I don't know for certain if this pointer can go stale or not. I'll have to discuss this a bit with Saam. But I plan on cleaning this up a bit.

[Saam Barati]

Comment on attachment 281959 [details]
[PATCH] WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=281959&action=review

> Source/JavaScriptCore/debugger/DebuggerScope.cpp:224
> +String DebuggerScope::name() const
> +{
> +    SymbolTable* symbolTable = m_scope->symbolTable();
> +    if (!symbolTable)
> +        return String();
> +
> +    CodeBlock* codeBlock = symbolTable->rareDataCodeBlock();
> +    if (!codeBlock)
> +        return String();
> +
> +    return String::fromUTF8(codeBlock->inferredName());
> +}

You may want more than a name to identify a function. Maybe name + line + column + sourceID?

[Joseph Pecoraro]

Created attachment 282017 [details]
[PATCH] Proposed Fix

I took Saam's advice and also included the location where possible. This gives us the ability to distinguish between identically named / unnamed functions, and we can provide a goto arrow in the UI if needed (not implemented yet).

Extended the test to test identically named functions.

[WebKit Commit Bot]

Attachment 282017 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/debugger/DebuggerScope.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 282017 [details]
[PATCH] Proposed Fix

Attachment 282017 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1564573

New failing tests:
inspector/debugger/paused-scopes.html

[Build Bot]

Created attachment 282023 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282017 [details]
[PATCH] Proposed Fix

Attachment 282017 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1564595

New failing tests:
inspector/debugger/paused-scopes.html

[Build Bot]

Created attachment 282024 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

Created attachment 282028 [details]
[PATCH] Proposed Fix

[Saam Barati]

Comment on attachment 282028 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=282028&action=review

JSC bits look good to me. Just a few comments

> Source/JavaScriptCore/debugger/DebuggerLocation.h:35
> +struct DebuggerLocation {

Can this be any location? Or does it refer specifically to a function?
If it specifically refers to a functions location, I think the name of the class
should be modified to indicate that. If it can refer to any location, I think
the fields should be renamed from "firstLine"=>"line" and "startColumn"=>"column".

> Source/JavaScriptCore/runtime/SymbolTable.h:705
> +        CodeBlock* m_codeBlock { nullptr };

Even though this is currently only set right after a new symbol table is allocated, I think it's better for this
to be a WriteBarrier to future proof the setRareDataCodeBlock above.

[Joseph Pecoraro]

Created attachment 282260 [details]
[PATCH] Proposed Fix

Addressed Saam's comments.

[Saam Barati]

Comment on attachment 282260 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=282260&action=review

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2483
> +                    clone->setRareDataCodeBlock(const_cast<CodeBlock*>(this));

Nit: I don't think you need this const_cast

[Joseph Pecoraro]

> Nit: I don't think you need this const_cast

Correct! New patch up.

[Joseph Pecoraro]

Created attachment 282263 [details]
[PATCH] Proposed Fix

[Blaze Burg]

Comment on attachment 282263 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=282263&action=review

> LayoutTests/inspector/debugger/paused-scopes.html:13
> +    InspectorBackend.dumpInspectorProtocolMessages = true;

Pls remove

> LayoutTests/inspector/debugger/paused-scopes.html:48
> +        return Promise.all(promises).then(() => {

nice! Is there any reason to worry about nondeterminism here? I assume that the promises are going to be serviced in the same order every time...

> LayoutTests/inspector/debugger/paused-scopes.html:55
> +        let chain = Promise.resolve(null);

I don't think the null argument is necessary.

> LayoutTests/inspector/debugger/paused-scopes.html:58
> +                return dumpCallFrame(callFrame);

I would just inline this expression and drop the return and braces.

> LayoutTests/inspector/debugger/paused-scopes.html:70
> +                dumpCallFrames().then(() => {

I think you can do dumpCallFrames().then(resolve, reject). We definitely need to call reject if dumping call frames failed for some reason.

> LayoutTests/inspector/debugger/paused-scopes.html:85
> +                    resolve();

Same comments as above.

[Blaze Burg]

Comment on attachment 282263 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=282263&action=review

> Source/WebInspectorUI/UserInterface/Models/ScopeChainNode.js:62
> +    makeLocalScope()

Naming nit: convertToLocalScope(). makeXXX sounds like it creates a new object.

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:169
> +        let scopes = callFrame.scopeChain.slice();

This is neat but might we want to do it outside of the sidebar view class? That would let us test the merging algorithm (and maybe use it again in other places if needed)

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:221
> +            markAsBaseIfNeeded(scope, scopes[i+1]);

scopes[i+1] will always be undefined in the first iteration of the loop. Is this intentional?

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:230
> +                scopes.splice(i, 1); // Remove one of them.

It feels yucky to iterate over scopes while mutating it. It's a programming error in C++ but merely dodgy in JS. Can we put the merged scope list into its own array and adjust the value of i to skip ahead? Since we don't seem to merge more than consecutive two scopes (maybe I'm wrong), I don't think we need to use an worklist/accumulator-like setup here.

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:245
> +                    scope.makeLocalScope();

...so then this code would be scope.convertToLocalScope()

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:250
> +        for (let scope of scopes) {

Everything prior to here could be easily moved out of view code, which just needs to examine the merged scopes.

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:308
> +            // FIXME: This just puts two ObjectTreeViews next to eachother, but that means

Typo: eachother

[Joseph Pecoraro]

Created attachment 282382 [details]
[PATCH] Proposed Fix

Addressed Brian's feedback

[Joseph Pecoraro]

Created attachment 282386 [details]
[PATCH] Proposed Fix

Tweaked test structure to be cleaner.

[Blaze Burg]

Comment on attachment 282386 [details]
[PATCH] Proposed Fix

r=me, Great work.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/202659>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 159305

[Ryan Haddad]

*** Bug 159304 has been marked as a duplicate of this bug. ***

[Ryan Haddad]

*** Bug 159303 has been marked as a duplicate of this bug. ***

[Ryan Haddad]

Links from duped bugs:

LayoutTest inspector/model/scope-chain-node.html crashes

https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/builds/6239

LayoutTest inspector/debugger/paused-scopes.html timing out

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r202680%20(6239)/results.html

[Joseph Pecoraro]

Created attachment 282478 [details]
[PATCH] Follow Up Changes

This got rolled out because of test failures / crashes with:

  LayoutTests/inspector/debugger/paused-scopes.html
  LayoutTests/inspector/model/scope-chain-node.html

I'm making these two changes:

  1. JSSymbolTableObject was not declaring its own class info so it was getting JSScope's
     and JSWithScope was unexpectedly a JSSymbolTableObject instead of a JSScope object.

  2. Running the tests on debug builds causes timing differences that cause frontend
     event differences (DebuggerManager, Paused, Resumed, CallFramesDidChange). Made the
     tests more resilient, but a little slower to wait for explicit resumes.

This contains just the diffs I'm making to the original patch.

[Joseph Pecoraro]

Comment on attachment 282478 [details]
[PATCH] Follow Up Changes

View in context: https://bugs.webkit.org/attachment.cgi?id=282478&action=review

> Source/JavaScriptCore/runtime/JSSymbolTableObject.h:51
> +    DECLARE_EXPORT_INFO;

Probably doesn't need to be exported, this can probably be DECLARE_INFO. I'll try with that.

[Joseph Pecoraro]

(In reply to comment #30)
> Comment on attachment 282478 [details]
> [PATCH] Follow Up Changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282478&action=review
> 
> > Source/JavaScriptCore/runtime/JSSymbolTableObject.h:51
> > +    DECLARE_EXPORT_INFO;
> 
> Probably doesn't need to be exported, this can probably be DECLARE_INFO.
> I'll try with that.

It does need to be, since JSGlobalObject eventually inherits from it.

[Joseph Pecoraro]

Created attachment 282484 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 282484 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 282484

Committed r202717: <http://trac.webkit.org/changeset/202717>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

*** Bug 19048 has been marked as a duplicate of this bug. ***