Bug 158210 - Web Inspector: Wrong function name next to scope
Summary: Web Inspector: Wrong function name next to scope
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
: 19048 159303 159304 (view as bug list)
Depends on: 159305
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-30 10:21 PDT by Saam Barati
Modified: 2016-09-16 21:30 PDT (History)
11 users (show)

See Also:


Attachments
image of bug (472.01 KB, image/png)
2016-05-30 10:21 PDT, Saam Barati
no flags Details
[PATCH] WIP (46.53 KB, patch)
2016-06-23 22:41 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (66.60 KB, patch)
2016-06-24 15:12 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (816.76 KB, application/zip)
2016-06-24 16:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.43 MB, application/zip)
2016-06-24 16:19 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (67.00 KB, patch)
2016-06-24 16:32 PDT, Joseph Pecoraro
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (66.93 KB, patch)
2016-06-28 11:22 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (66.91 KB, patch)
2016-06-28 11:45 PDT, Joseph Pecoraro
bburg: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (72.36 KB, patch)
2016-06-29 15:29 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (73.18 KB, patch)
2016-06-29 16:14 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff
[PATCH] Follow Up Changes (6.54 KB, patch)
2016-06-30 15:34 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (74.78 KB, patch)
2016-06-30 15:54 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-05-30 10:21:44 PDT
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`
Comment 1 Radar WebKit Bug Importer 2016-05-30 10:22:20 PDT
<rdar://problem/26543093>
Comment 2 Timothy Hatcher 2016-05-31 13:49:14 PDT
Joe worked on this earlier in the release. It might be from those changes.
Comment 3 Joseph Pecoraro 2016-05-31 14:31:20 PDT
Yeah, I've seen asserts from code in here that could lead to this.
Comment 4 Joseph Pecoraro 2016-06-23 14:56:22 PDT
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).
Comment 5 Joseph Pecoraro 2016-06-23 22:41:40 PDT
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.
Comment 6 Saam Barati 2016-06-24 11:29:09 PDT
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?
Comment 7 Joseph Pecoraro 2016-06-24 15:12:11 PDT
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.
Comment 8 WebKit Commit Bot 2016-06-24 15:13:55 PDT
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.
Comment 9 Build Bot 2016-06-24 16:09:27 PDT
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
Comment 10 Build Bot 2016-06-24 16:09:30 PDT
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
Comment 11 Build Bot 2016-06-24 16:19:33 PDT
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
Comment 12 Build Bot 2016-06-24 16:19:36 PDT
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
Comment 13 Joseph Pecoraro 2016-06-24 16:32:16 PDT
Created attachment 282028 [details]
[PATCH] Proposed Fix
Comment 14 Saam Barati 2016-06-27 11:56:06 PDT
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.
Comment 15 Joseph Pecoraro 2016-06-28 11:22:32 PDT
Created attachment 282260 [details]
[PATCH] Proposed Fix

Addressed Saam's comments.
Comment 16 Saam Barati 2016-06-28 11:35:29 PDT
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
Comment 17 Joseph Pecoraro 2016-06-28 11:44:50 PDT
> Nit: I don't think you need this const_cast

Correct! New patch up.
Comment 18 Joseph Pecoraro 2016-06-28 11:45:04 PDT
Created attachment 282263 [details]
[PATCH] Proposed Fix
Comment 19 Brian Burg 2016-06-29 11:32:33 PDT
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.
Comment 20 Brian Burg 2016-06-29 11:52:03 PDT
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
Comment 21 Joseph Pecoraro 2016-06-29 15:29:47 PDT
Created attachment 282382 [details]
[PATCH] Proposed Fix

Addressed Brian's feedback
Comment 22 Joseph Pecoraro 2016-06-29 16:14:00 PDT
Created attachment 282386 [details]
[PATCH] Proposed Fix

Tweaked test structure to be cleaner.
Comment 23 Brian Burg 2016-06-29 16:31:37 PDT
Comment on attachment 282386 [details]
[PATCH] Proposed Fix

r=me, Great work.
Comment 24 Joseph Pecoraro 2016-06-29 17:00:23 PDT
<https://trac.webkit.org/changeset/202659>
Comment 25 WebKit Commit Bot 2016-06-30 10:29:36 PDT
Re-opened since this is blocked by bug 159305
Comment 26 Ryan Haddad 2016-06-30 15:30:58 PDT
*** Bug 159304 has been marked as a duplicate of this bug. ***
Comment 27 Ryan Haddad 2016-06-30 15:31:19 PDT
*** Bug 159303 has been marked as a duplicate of this bug. ***
Comment 28 Ryan Haddad 2016-06-30 15:34:03 PDT
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
Comment 29 Joseph Pecoraro 2016-06-30 15:34:08 PDT
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.
Comment 30 Joseph Pecoraro 2016-06-30 15:36:31 PDT
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.
Comment 31 Joseph Pecoraro 2016-06-30 15:45:03 PDT
(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.
Comment 32 Joseph Pecoraro 2016-06-30 15:54:39 PDT
Created attachment 282484 [details]
[PATCH] Proposed Fix
Comment 33 WebKit Commit Bot 2016-06-30 16:25:24 PDT
Comment on attachment 282484 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 282484

Committed r202717: <http://trac.webkit.org/changeset/202717>
Comment 34 WebKit Commit Bot 2016-06-30 16:25:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Joseph Pecoraro 2016-09-16 21:30:18 PDT
*** Bug 19048 has been marked as a duplicate of this bug. ***