Web Inspector: Resources disappear from the network tab when iframe gets removed from DOM

• Component: Web Inspector
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Reproduction steps:
1. Open https://polymerlabs.github.io/polymer-tests/polymer-20161208/components/polymer/test/runner.html?grep=unit%2Fbind%5C.html
2. Open Web Inspector’s Network tab
3. Reload

Expected Result:
It shows the list of all resources being loaded

Actual Result:
Inspector momentarily shows but removes a whole skew of resources at some point.

[Nikita Vasilyev]

I was able to reproduce it.

[Radar WebKit Bug Importer]

<rdar://problem/29909640>

[Ryosuke Niwa]

Note that this page doesn’t run tests at all after r210052. So use STP20 or a local build before r210052 to see this problem.

[Nikita Vasilyev]

The test runner page runs tests in iframes and removes them when a test is finished.

[Nikita Vasilyev]

Reduction: http://nv.github.io/webkit-inspector-bugs/166776_iframes-data-disappear/

[Nikita Vasilyev]

Created attachment 298388 [details]
Patch

cq- because I'm not yet sure if this doesn't break anything. I need more time to better understand how it works.

[Nikita Vasilyev]

Created attachment 298391 [details]
[Animated GIF] With patch applied

This patch keeps removed iframe resources in the Network tab. It doesn't seem to affect Resources and Debugger.

Chrome, for instance, keeps removed iframe resources in their Sources tab. We should probably do the same.

[Joseph Pecoraro]

(In reply to comment #7)
> Created attachment 298391 [details]
> [Animated GIF] With patch applied
> 
> This patch keeps removed iframe resources in the Network tab. It doesn't
> seem to affect Resources and Debugger.
> 
> Chrome, for instance, keeps removed iframe resources in their Sources tab.
> We should probably do the same.

I think this would be some kind of filter of the Resources tab. To show All resources or Active / Live resources. I don't think that by default we should show iframes / Resources in the sidebar that don't exist on the page.

[Nikita Vasilyev]

Created attachment 298518 [details]
Patch

[Nikita Vasilyev]

(In reply to comment #8)
> (In reply to comment #7)
> > Created attachment 298391 [details]
> > [Animated GIF] With patch applied
> > 
> > This patch keeps removed iframe resources in the Network tab. It doesn't
> > seem to affect Resources and Debugger.
> > 
> > Chrome, for instance, keeps removed iframe resources in their Sources tab.
> > We should probably do the same.
> 
> I think this would be some kind of filter of the Resources tab. To show All
> resources or Active / Live resources. I don't think that by default we
> should show iframes / Resources in the sidebar that don't exist on the page.

I think in Network tab we should always show resources regardless if they're still attached to the DOM or not.

I'm not opposed of the idea of adding a filter button to Resources (and Debugger tab?). However, I've never felt the need for it and I don't know what's the use case for it. I think it should be done as a separate task.

[BJ Burg]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Created attachment 298391 [details]
> > > [Animated GIF] With patch applied
> > > 
> > > This patch keeps removed iframe resources in the Network tab. It doesn't
> > > seem to affect Resources and Debugger.
> > > 
> > > Chrome, for instance, keeps removed iframe resources in their Sources tab.
> > > We should probably do the same.
> > 
> > I think this would be some kind of filter of the Resources tab. To show All
> > resources or Active / Live resources. I don't think that by default we
> > should show iframes / Resources in the sidebar that don't exist on the page.
> 
> I think in Network tab we should always show resources regardless if they're
> still attached to the DOM or not.

Resources are (conceptually) attached to a ScriptExecutionContext or something similarly per-document or per-frame, not the DOM. In reality, CachedResource objects can be kept alive by all kinds of things.

> 
> I'm not opposed of the idea of adding a filter button to Resources (and
> Debugger tab?). However, I've never felt the need for it and I don't know
> what's the use case for it. I think it should be done as a separate task.

The use case is to debug sites like the one linked by Ryosuke. This is not the normal case, so it should be an optional behavior. Furthermore, I would argue that even with this special filter that shows more resources, we should discard all resources when the main frame navigates. His bug was about subframe resources being lost/inaccessible for setting breakpoints.

[BJ Burg]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Created attachment 298391 [details]
> > > [Animated GIF] With patch applied
> > > 
> > > This patch keeps removed iframe resources in the Network tab. It doesn't
> > > seem to affect Resources and Debugger.
> > > 
> > > Chrome, for instance, keeps removed iframe resources in their Sources tab.
> > > We should probably do the same.
> > 
> > I think this would be some kind of filter of the Resources tab. To show All
> > resources or Active / Live resources. I don't think that by default we
> > should show iframes / Resources in the sidebar that don't exist on the page.
> 
> I think in Network tab we should always show resources regardless if they're
> still attached to the DOM or not.

Technicalities aside, I think reviewers (per IRC) are in agreement that Network Tab should be a transcript of all resource loads, regardless of whether or not the page uses them any more. So this bug can move forward. See <https://bugs.webkit.org/show_bug.cgi?id=167038> to discuss how this should be handled in other tabs that show resources (not loads).

[Joseph Pecoraro]

Comment on attachment 298518 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/NetworkSidebarPanel.js:31
> +        const shouldAutoPruneStaleTopLevelResourceTreeElements = false;
> +        super("network", WebInspector.UIString("Network"), shouldAutoPruneStaleTopLevelResourceTreeElements);

What happens on main frame navigations? shouldAutoPrune included a handler for MainResourceDidChange. Does the Network Tab get reset in another way on main frame navigations?

[Nikita Vasilyev]

(In reply to comment #13)
> Comment on attachment 298518 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298518&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/NetworkSidebarPanel.js:31
> > +        const shouldAutoPruneStaleTopLevelResourceTreeElements = false;
> > +        super("network", WebInspector.UIString("Network"), shouldAutoPruneStaleTopLevelResourceTreeElements);
> 
> What happens on main frame navigations? shouldAutoPrune included a handler
> for MainResourceDidChange. Does the Network Tab get reset in another way on
> main frame navigations?

Yes, Network Tab gets reset by TimelineManager:

    _mainResourceDidChange(event)
    {
        let frame = event.target;
        if (frame.isMainFrame())
            this._persistentNetworkTimeline.reset();

[Matt Baker]

Comment on attachment 298518 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/NetworkSidebarPanel.js:31
>>> +        super("network", WebInspector.UIString("Network"), shouldAutoPruneStaleTopLevelResourceTreeElements);
>> 
>> What happens on main frame navigations? shouldAutoPrune included a handler for MainResourceDidChange. Does the Network Tab get reset in another way on main frame navigations?
> 
> Yes, Network Tab gets reset by TimelineManager:
> 
>     _mainResourceDidChange(event)
>     {
>         let frame = event.target;
>         if (frame.isMainFrame())
>             this._persistentNetworkTimeline.reset();

Let's be consistent with StorageSidebarPanel and omit the third argument rather than passing false explicitly.

[Nikita Vasilyev]

Created attachment 298827 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 298827 [details]
Patch

Clearing flags on attachment: 298827

Committed r210759: <http://trac.webkit.org/changeset/210759>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.