Web Inspector: Web Sockets: Unable to inspect a WebSocket that receives >50 messages per second

[Nikita Vasilyev]

Steps:
1. In Web Inspector, open Network tab.
2. Navigate to http://demo2.kaazing.com/demo/forex/
3. Filter Web Sockets.
4. Click on the go to arrow right next to the WebSocket connection ("jms").

Expected:
WebSocket content view is shown.

Actual:
Nothing happens.

Notes:
This page sends a lot of WebSocket messages. You may not be able to reproduce the bug on a slow network connection.

[Radar WebKit Bug Importer]

<rdar://problem/32095863>

[Nikita Vasilyev]

This issue is similar to Bug 170036 - Web Inspector: Adding a WebSocket message may change the currently selected resource. Resource#increaseSize causes a chain reaction that results in ContentBrowser#showContentViewForRepresentedObject being called.

    showContentViewForRepresentedObject - ContentBrowser.js:146
    showRepresentedObject - ContentBrowserTabContentView.js:191
    showRepresentedObject - Main.js:1175
    _treeSelectionDidChange - NetworkGridContentView.js:397
    event: WebInspector.TreeOutline.Event.SelectionDidChange
    select - TreeElement.js:511
    _refreshDirtyDataGridNodes - TimelineDataGrid.js:279
    --- requestAnimationFrame ---
    dataGridNodeNeedsRefresh - TimelineDataGrid.js:198
    _needsRefresh - ResourceTimelineDataGridNode.js:277
    event: WebInspector.Resource.Event.SizeDidChange
    increaseSize - Resource.js:755
    addFrame - WebSocketResource.js:79

[Nikita Vasilyev]

TimelineDataGrid#_refreshDirtyDataGridNodes is called every time a resource size changes. This is unnecessary expensive.

I think the right strategy would be to avoid calling TimelineDataGrid#_refreshDirtyDataGridNodes when TimelineDataGrid isn't visible. We should, however, call this method when a TimelineDataGrid becomes visible.

[BJ Burg]

(In reply to Nikita Vasilyev from comment #3)
> TimelineDataGrid#_refreshDirtyDataGridNodes is called every time a resource
> size changes. This is unnecessary expensive.
> 
> I think the right strategy would be to avoid calling
> TimelineDataGrid#_refreshDirtyDataGridNodes when TimelineDataGrid isn't
> visible. We should, however, call this method when a TimelineDataGrid
> becomes visible.

We shouldn't be doing layout work / accessing DOM elements unless underneath an override of View.prototype.{initialLayout,layout}. Does DataGrid normally violate this, or is this code doing something special to cause this to happen?

[Nikita Vasilyev]

(In reply to Brian Burg from comment #4)
> (In reply to Nikita Vasilyev from comment #3)
> > TimelineDataGrid#_refreshDirtyDataGridNodes is called every time a resource
> > size changes. This is unnecessary expensive.
> > 
> > I think the right strategy would be to avoid calling
> > TimelineDataGrid#_refreshDirtyDataGridNodes when TimelineDataGrid isn't
> > visible. We should, however, call this method when a TimelineDataGrid
> > becomes visible.
> 
> We shouldn't be doing layout work / accessing DOM elements unless underneath
> an override of View.prototype.{initialLayout,layout}. Does DataGrid normally
> violate this, or is this code doing something special to cause this to
> happen?

DataGrid is fairly old and has a lot of layout work / DOM access outside of {initialLayout,layout}. Only TimelineDataGrid has _refreshDirtyDataGridNodes method.  I'm not entirely sure what are you asking.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #3)
> TimelineDataGrid#_refreshDirtyDataGridNodes is called every time a resource
> size changes. This is unnecessary expensive.
> 
> I think the right strategy would be to avoid calling
> TimelineDataGrid#_refreshDirtyDataGridNodes when TimelineDataGrid isn't
> visible. We should, however, call this method when a TimelineDataGrid
> becomes visible.

This alone didn't fix the problem :(

Another issue is that Resource#increaseSize causes NetworkSidebarPanel#treeElementAddedOrChanged 
to be called. Every increase of size in a resource causes a sidebar item to be removed and attached
to the DOM. On http://demo2.kaazing.com/demo/forex/ you can see that by hovering over the WebSocket
resource in the sidebar — the go-to arrow flickers!

Every resource, not just a WebSocket resource, that frequently increases its size must be unclickable.

[Nikita Vasilyev]

Created attachment 310583 [details]
Patch

[Matt Baker]

Comment on attachment 310583 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:349
> +    reattachIfIndexChanged(treeElement, insertionIndex)

If I understand correctly, this function reinserts the tree element if the index is changing OR the status element hasn't been initialized. Maybe the function should be called `reattachIfNeeded`, since the check involves more than just the index?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:354
> +            // insertionIndex.status and it can be triggered by re-attaching treeElement.

`insertionIndex.status` seems wrong. This comment could also be more clear: I don't understand why having a falsey status means the element has to be re-inserted.

[Nikita Vasilyev]

(In reply to Matt Baker from comment #8)
> Comment on attachment 310583 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310583&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:349
> > +    reattachIfIndexChanged(treeElement, insertionIndex)
> 
> If I understand correctly, this function reinserts the tree element if the
> index is changing OR the status element hasn't been initialized. Maybe the
> function should be called `reattachIfNeeded`, since the check involves more
> than just the index?
> 
> > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:354
> > +            // insertionIndex.status and it can be triggered by re-attaching treeElement.
> 
> `insertionIndex.status` seems wrong. This comment could also be more clear:
> I don't understand why having a falsey status means the element has to be
> re-inserted.

treeElement.status has the go-to button and close button elements. Re-inserting treeElement calls
NetworkSidebarPanel#treeElementAddedOrChanged, which adds these go-to button and close button elements.

    treeElementAddedOrChanged(treeElement)
    {
        if (treeElement.status || !treeElement.treeOutline)
            return;

        var fragment = document.createDocumentFragment();

        var closeButton = new WebInspector.TreeElementStatusButton(useSVGSymbol("Images/Close.svg", null, WebInspector.UIString("Close resource view")));
        closeButton.element.classList.add("close");
        closeButton.addEventListener(WebInspector.TreeElementStatusButton.Event.Clicked, this._treeElementCloseButtonClicked, this);
        fragment.appendChild(closeButton.element);

        let goToButton = new WebInspector.TreeElementStatusButton(WebInspector.createGoToArrowButton());
        goToButton[WebInspector.NetworkSidebarPanel.TreeElementSymbol] = treeElement;
        goToButton.addEventListener(WebInspector.TreeElementStatusButton.Event.Clicked, this._treeElementGoToArrowWasClicked, this);
        fragment.appendChild(goToButton.element);

        treeElement.status = fragment;
    }

https://github.com/WebKit/webkit/blob/f59f78f7e62bae5cd899a7e6943f5696b764e4cd/Source/WebInspectorUI/UserInterface/Views/NetworkSidebarPanel.js#L154

Now that I'm thinking about it, there should be no need to re-insert treeElements, they should have the go-to & close buttons already.
I'll try to implement that instead.

[Nikita Vasilyev]

Created attachment 310948 [details]
Patch

(In reply to Nikita Vasilyev from comment #9)
> Now that I'm thinking about it, there should be no need to re-insert
> treeElements, they should have the go-to & close buttons already.
> I'll try to implement that instead.

This isn't necessarily a good idea because both Network and Resources sidebar use the same TreeElement classes,
but tree elements in Network sidebar have the go-to arrows.

I think in the future we should re-consider the go-to arrow and close button UI in Network tab. I've seen at least 3 people
confused not knowing how to close a content view in the Network tab.

[Joseph Pecoraro]

Comment on attachment 310948 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:16
> +        This patch makes sure treeElement gets re-attached if:
> +        - Its position (index) changes.
> +        - treeElement.status, which contains the go-to arrow and the close resource icon, is not yet added to the DOM.

I understand re-attaching if the position changes. I do not understand why status has any affect on reattaching.

Why do we do the extra work if there is not a status icon?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:354
> +        if (treeElement.status && this.children.indexOf(treeElement) === insertionIndex)

This appears to be O(n) for what should be a single O(1) check.

Instead of:

    this.children.indexOf(treeElement) === insertionIndex

Can you just check:

    this.children[insertionIndex] === treeElement

[Nikita Vasilyev]

Comment on attachment 310948 [details]
Patch

(In reply to Joseph Pecoraro from comment #11)
> Comment on attachment 310948 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310948&action=review
> 
> > Source/WebInspectorUI/ChangeLog:16
> > +        This patch makes sure treeElement gets re-attached if:
> > +        - Its position (index) changes.
> > +        - treeElement.status, which contains the go-to arrow and the close resource icon, is not yet added to the DOM.
> 
> I understand re-attaching if the position changes. I do not understand why
> status has any affect on reattaching.
> 
> Why do we do the extra work if there is not a status icon?

Without this treeElement.status check, there was no go-to/close icon. I think I finally figured it why.

Were these icons removed or never added?

    ResourceTreeElement#_updateStatus:
        treeElement.status = <div class="indeterminate-progress-spinner">

    ResourceTreeElement#_updateStatus:
        treeElement.status = ""

Turns out they were removed by ResourceTreeElement#_updateStatus.

WebSocketResourceTreeElement#_updateConnectionStatus is used in Resources tab, but not Network tab.

[Nikita Vasilyev]

Created attachment 311077 [details]
Patch

[Nikita Vasilyev]

Created attachment 311653 [details]
Patch

The previous patch sometimes didn't show the go-to buttons.

[Matt Baker]

Comment on attachment 311653 [details]
Patch

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

r=me, with a comment.

> Source/WebInspectorUI/UserInterface/Views/NetworkSidebarPanel.js:156
> +        if (treeElement.status instanceof DocumentFragment || !treeElement.treeOutline)

The check for status instanceOf DocumentFragment is a little fragile, since status could be technically be a DocumentFragment other than the goto/close fragment that gets added below. This could be more future-proof by adding a property to the status element:

treeElementAddedOrChanged(treeElement)
{
    if (!treeElement.treeOutline)
        return;

    if (treeElement.status && treeElement.status[WebInspector.NetworkSidebarPanel.TreeElementStatusButtonSymbol])
        return;

    treeElement.status[WebInspector.NetworkSidebarPanel.TreeElementStatusButtonSymbol] = true;
    ...
}

Putting the property on status instead of treeElement also ensures that the property can't get out of sync by someone setting the status elsewhere.

[Nikita Vasilyev]

Comment on attachment 311653 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/NetworkSidebarPanel.js:156
>> +        if (treeElement.status instanceof DocumentFragment || !treeElement.treeOutline)
> 
> The check for status instanceOf DocumentFragment is a little fragile, since status could be technically be a DocumentFragment other than the goto/close fragment that gets added below. This could be more future-proof by adding a property to the status element:
> 
> treeElementAddedOrChanged(treeElement)
> {
>     if (!treeElement.treeOutline)
>         return;
> 
>     if (treeElement.status && treeElement.status[WebInspector.NetworkSidebarPanel.TreeElementStatusButtonSymbol])
>         return;
> 
>     treeElement.status[WebInspector.NetworkSidebarPanel.TreeElementStatusButtonSymbol] = true;
>     ...
> }
> 
> Putting the property on status instead of treeElement also ensures that the property can't get out of sync by someone setting the status elsewhere.

Agreed. I like the use a symbol.

[Nikita Vasilyev]

Created attachment 311762 [details]
Patch

[Matt Baker]

Comment on attachment 311762 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/NetworkSidebarPanel.js:160
> +        if (treeElement.status[WebInspector.NetworkSidebarPanel.TreeElementStatusButtonSymbol] || !treeElement.treeOutline)

You also need to check that status is defined before accessing the property.

[Nikita Vasilyev]

Created attachment 311764 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 311764 [details]
Patch

Clearing flags on attachment: 311764

Committed r217690: <http://trac.webkit.org/changeset/217690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.