Bug 152348 – Web Inspector: Scope chain shouldn't show empty Closure sections

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

[Matt Baker]

Created attachment 267472 [details]
[Screenshot] Empty Closure section

* SUMMARY
Scope chain shouldn't show empty Closure sections. We don't display other scope types when they aren't present (With scopes).

* REDUCTION
<script>
(function f() {
    let a;
    var x;
    debugger;
})();
</script>

* STEPS TO REPRODUCE
1. Inspector -> Debugger tab
2. Inspect above test case, reload page
3. Open right-sidebar (Scope Chain panel)
  => An empty closure section appears at the bottom (see attached screenshot)

[Alexey Proskuryakov]

<rdar://problem/28213666>

[Devin Rousso]

I just tried this with the given reduction and I didn't see any empty closure sections.  Can you still reproduce it?

[Nikita Vasilyev]

Created attachment 288491 [details]
[HTML] Reduction

[Nikita Vasilyev]

Created attachment 288492 [details]
[Image] Empty Closure section

Still relevant with the updated reduction.

[Devin Rousso]

Created attachment 288495 [details]
Patch

[Matt Baker]

Comment on attachment 288495 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:164
>  

Nit: newlines aren't necessary between consecutive one-line getters.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:170
>  

What about _hasPropertyEntries instead? I think it's less ambiguous.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:171
> +    get expanded() { return this._expanded; }

Nit: add a space since the next statement is a method, not a property.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:292
> +        for (let propertyDescriptor of properties) {

I think we should resist the urge to refactor nearby code like this, as it makes it more time consuming to scan diffs.

[Matt Baker]

Based on our discussion over IRC, we should try deferring the creation of sections until properties are available. Ideally we shouldn't create DetailsSections that are just going to be removed right away.

[Matt Baker]

Comment on attachment 288495 [details]
Patch

r- since the goal is now to postpone the creation of DetailsSections so empty sections aren't created.

[Devin Rousso]

Created attachment 288504 [details]
Patch

[Build Bot]

Comment on attachment 288504 [details]
Patch

Attachment 288504 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2049844

New failing tests:
fast/scrolling/ios/scroll-events-back-forward.html

[Build Bot]

Created attachment 288511 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Joseph Pecoraro]

Comment on attachment 288504 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:249
> +            Promise.all(updatePromises)
> +            .then((updateEvents) => {

Err, I think the style here would be to indent the .then, and use .catch for a global function we use to show the uncaught exception reporter for errors:

    Promise.all([...])
        .then(() => {
            ...
        })
        .catch(handlePromiseException);

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:254
> +                    detailsSections.remove(detailsSection);
> +                    detailsSection.element.remove();
> +                    return;
> +                }

Does this mean the sidebar "flashes" removing this section? (I haven't tested).

[Devin Rousso]

Comment on attachment 288504 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:249
>> +            .then((updateEvents) => {
> 
> Err, I think the style here would be to indent the .then, and use .catch for a global function we use to show the uncaught exception reporter for errors:
> 
>     Promise.all([...])
>         .then(() => {
>             ...
>         })
>         .catch(handlePromiseException);

Brian and I have been talking about this, and we are not indenting .then() inside LayoutTests.  I think we should keep it consistent.

>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:254
>> +                }
> 
> Does this mean the sidebar "flashes" removing this section? (I haven't tested).

I have not seen any flashes.  I could make it so that each DetailsSection starts out hidden, just in case.  Another option would be to rework the entire function so that it only resolves the returned promise when every ObjectTreeView for each scope has been updated.

[Matt Baker]

Comment on attachment 288504 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:254
>>> +                }
>> 
>> Does this mean the sidebar "flashes" removing this section? (I haven't tested).
> 
> I have not seen any flashes.  I could make it so that each DetailsSection starts out hidden, just in case.  Another option would be to rework the entire function so that it only resolves the returned promise when every ObjectTreeView for each scope has been updated.

If possible we shouldn't add a section to the DOM at all until a promise resolves. Then we could add it or drop it.

[Devin Rousso]

Created attachment 288754 [details]
Patch

So I tried reworking _generateCallFramesSection to only resolve once every scope has updated all of its properties (thus only creating DetailsSection instances if a scope has at least one property), but it causes a noticeable delay to the rendering of the scope chain.  This is most likely due to the sheer number of calls to resolve PropertyDescriptor values for RemoteObject.  I think that a current "middle-ground" solution may be to have placeholder elements be added to the DOM (basically blank <div>) that we can remove if there are no properties or replace with a DetailsSection.

[Devin Rousso]

Created attachment 288769 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 288769 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:30
> +        * UserInterface/Views/ScopeChainDetailsSidebarPanel.js:
> +        (WebInspector.ScopeChainDetailsSidebarPanel.prototype._generateCallFramesSection):
> +        Changed logic to not resolve until every scope has updated all of its objects.
> +        DetailsSection elements are not created unless that section has scopes with properties.
> +        Also added sorting to the entries in each scope (it still keeps "this" at the top).

I think this is the wrong approach.

I think the backend should just tell the frontend "this scope is empty" so we don't have to do back and forth messages and delay updating the UI. Especially because these are very costly messages (get a list of all objects and their previews). Likewise I don't want us to delay updating the UI checking if the Global scope (a huge list of values) is empty or not.

I think this would be very easy to do, but it is non-obvious:

  1. Debugger.json - Update Debugger.Scope object type in the protocol to include an "isEmpty" property
  2. InjectedScriptSource.js - InjectedScript.CallFrameProxy._createScopeJson would check if "object" is empty or not
  3. Have the frontend handle this (DebuggerManager.prototype._scopeChainNodeFromPayload, WebInspector.ScopeChainNode, etc)

Let me know if you want to tackle this!

[Devin Rousso]

Created attachment 289187 [details]
Patch

[Matt Baker]

Comment on attachment 289187 [details]
Patch

Could DebuggerManager just drop empty payloads? If _scopeChainNodeFromPayload returned null for payloads marked as empty, _scopeChainFromPayload could ignore them and the `isEmpty` flag wouldn't need to leak into Model/View code.

[Joseph Pecoraro]

(In reply to comment #19)
> Comment on attachment 289187 [details]
> Patch
> 
> Could DebuggerManager just drop empty payloads? If
> _scopeChainNodeFromPayload returned null for payloads marked as empty,
> _scopeChainFromPayload could ignore them and the `isEmpty` flag wouldn't
> need to leak into Model/View code.

I don't think that would be a good idea. The frontend should have complete information when determining what to show. And an isEmpty Closure scope could be promoted to the Local scope which has `this` stuff into it. If empty scopes were dropped by DebuggerManager we would have chosen he wrong scope to make "Local".

[Joseph Pecoraro]

Comment on attachment 289187 [details]
Patch

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

r- because we should be able to write a test for this. The patch itself looks great though.

See https://bugs.webkit.org/show_bug.cgi?id=162113 for an example patch in this exact area.

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1339
> +    if (!Object.keys(object).length)

Object.keys is slightly wasteful and user overridable (which is a known issue with our injected script source right now). We should just write our own "isEmptyObject" function at the top with other utilities that cannot be defeated by code on the page:

    function isEmptyObject(object)
    {
        for (let key in object)
            return false;
        return true;
    }

> Source/JavaScriptCore/inspector/protocol/Debugger.json:88
> +                { "name": "isEmpty", "type": "boolean", "optional": true, "description": "Whether the object has any properties." }

What do you think about just naming this "empty"? We seem to use "is" sparingly, and I think empty would be clear.

> Source/WebInspectorUI/UserInterface/Models/ScopeChainNode.js:51
> +    get isEmpty() { return this._isEmpty; }

Likewise this would be "empty" and would read better.

[Devin Rousso]

Created attachment 292589 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 292589 [details]
Patch

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

r=me, this is way nicer and the test output is a nice improvement!

> Source/JavaScriptCore/inspector/protocol/Debugger.json:88
> +                { "name": "empty", "type": "boolean", "optional": true, "description": "Whether the object has any properties." }

"Whether the object has any properties" could be "Whether the scope has any variables". I believe that is the intent of this property, it just manifests as the object having no properties.

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:172
> +            if (scope.type !== WebInspector.ScopeChainNode.Type.Local && scope.empty)

I would flip these. The empty check first is the intent, the Local Scope is a condition.

    if (scope.empty && scope.type !== Local)

[Devin Rousso]

Created attachment 292653 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 292653 [details]
Patch

Rejecting attachment 292653 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 292653, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2360808

[Devin Rousso]

Created attachment 292654 [details]
Patch

Oops.  Forgot the "Oops". (hehehe)

[WebKit Commit Bot]

Comment on attachment 292654 [details]
Patch

Clearing flags on attachment: 292654

Committed r207784: <http://trac.webkit.org/changeset/207784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.