Bug 152348

Summary: Web Inspector: Scope chain shouldn't show empty Closure sections
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, hi, joepeck, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, saam, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[Screenshot] Empty Closure section
none
[HTML] Reduction
none
[Image] Empty Closure section
none
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2
none
Patch
none
Patch
none
Patch
none
Patch
joepeck: review+
Patch
commit-queue: commit-queue-
Patch none

Description Matt Baker 2015-12-16 11:14:25 PST
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)
Comment 1 Alexey Proskuryakov 2016-09-08 14:00:20 PDT
<rdar://problem/28213666>
Comment 2 Devin Rousso 2016-09-10 02:19:19 PDT
I just tried this with the given reduction and I didn't see any empty closure sections.  Can you still reproduce it?
Comment 3 Nikita Vasilyev 2016-09-10 13:17:22 PDT
Created attachment 288491 [details]
[HTML] Reduction
Comment 4 Nikita Vasilyev 2016-09-10 13:18:57 PDT
Created attachment 288492 [details]
[Image] Empty Closure section

Still relevant with the updated reduction.
Comment 5 Devin Rousso 2016-09-10 14:01:29 PDT
Created attachment 288495 [details]
Patch
Comment 6 Matt Baker 2016-09-10 14:16:16 PDT
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.
Comment 7 Matt Baker 2016-09-10 14:30:26 PDT
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.
Comment 8 Matt Baker 2016-09-10 14:33:23 PDT
Comment on attachment 288495 [details]
Patch

r- since the goal is now to postpone the creation of DetailsSections so empty sections aren't created.
Comment 9 Devin Rousso 2016-09-10 15:40:11 PDT
Created attachment 288504 [details]
Patch
Comment 10 Build Bot 2016-09-10 17:12:33 PDT
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
Comment 11 Build Bot 2016-09-10 17:12:36 PDT
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
Comment 12 Joseph Pecoraro 2016-09-12 13:34:20 PDT
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).
Comment 13 Devin Rousso 2016-09-12 13:39:06 PDT
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.
Comment 14 Matt Baker 2016-09-13 12:46:11 PDT
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.
Comment 15 Devin Rousso 2016-09-13 17:27:48 PDT
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.
Comment 16 Devin Rousso 2016-09-13 21:47:29 PDT
Created attachment 288769 [details]
Patch
Comment 17 Joseph Pecoraro 2016-09-16 21:40:08 PDT
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!
Comment 18 Devin Rousso 2016-09-17 18:59:04 PDT
Created attachment 289187 [details]
Patch
Comment 19 Matt Baker 2016-09-18 10:29:53 PDT
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.
Comment 20 Joseph Pecoraro 2016-09-18 18:32:28 PDT
(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".
Comment 21 Joseph Pecoraro 2016-09-18 18:39:14 PDT
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.
Comment 22 Devin Rousso 2016-10-24 00:03:16 PDT
Created attachment 292589 [details]
Patch
Comment 23 Joseph Pecoraro 2016-10-24 14:27:32 PDT
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)
Comment 24 Devin Rousso 2016-10-24 14:34:58 PDT
Created attachment 292653 [details]
Patch
Comment 25 WebKit Commit Bot 2016-10-24 14:36:02 PDT
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
Comment 26 Devin Rousso 2016-10-24 14:37:16 PDT
Created attachment 292654 [details]
Patch

Oops.  Forgot the "Oops". (hehehe)
Comment 27 WebKit Commit Bot 2016-10-24 15:12:18 PDT
Comment on attachment 292654 [details]
Patch

Clearing flags on attachment: 292654

Committed r207784: <http://trac.webkit.org/changeset/207784>
Comment 28 WebKit Commit Bot 2016-10-24 15:12:25 PDT
All reviewed patches have been landed.  Closing bug.