Bug 135965 – Web Inspector: Inspector frequently restores wrong view when opened (often Timelines instead of Resource)

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Inspector frequently restores wrong view when opened (often Timelines instead of Resources / DOM Tree)

* STEPS TO REPRODUCE
1. Inspect bogojoker.com
2. Select Timelines sidebar
3. Select Network timeline
4. Close Inspector
5. Develop > Show Web Inspector (inspector opens to Timelines View, good)
6. Select Resources sidebar
7. Select an Image resource in the sidebar
8. Close Inspector
9. Develop > Show Web Inspector
  => Inspector opens to Timelines view instead of the images view in Resources sidebar

* NOTES
- Inspector listens for window "pagehide" events to save the state of the last view you had open. Unfortunately this does not get called until you re-open the inspector. This is way late and we re-ran a restore of the previous, wrong state.

[Radar WebKit Bug Importer]

<rdar://problem/18026767>

[Joseph Pecoraro]

So, we could investigate why pagehide doesn't happen at the right time. But we could also investigate a workaround. For example, always update our cookie whenever the ContentView changes or navigation sidebar changes, instead of relying on pagehide. That kind of stinks, but at least it should fix this issue in common cases.

[Joseph Pecoraro]

A workaround still doesn't seem like enough. There is another actual bug here.

It looks like _loadNewRecording is triggering the Timelines panel to be the selected panel:

>    Resources/Views/Sidebar.js:143:20: CONSOLE LOG setting timeline
>    Resources/Views/Sidebar.js:144:22: CONSOLE TRACE
>    0: selectedSidebarPanel(Resources/Views/Sidebar.js:144:22)
>    1: _contentBrowserCurrentContentViewDidChange(Resources/Base/Main.js:1096:31)
>    2: dispatch(Resources/Base/Object.js:141:55)
>    3: dispatchEventToListeners(Resources/Base/Object.js:148:17)
>    4: _currentContentViewDidChange(Resources/Views/ContentBrowser.js:509:38)
>    5: dispatch(Resources/Base/Object.js:141:55)
>    6: dispatchEventToListeners(Resources/Base/Object.js:148:17)
>    7: showBackForwardEntryForIndex(Resources/Views/ContentViewContainer.js:207:38)
>    8: showContentView(Resources/Views/ContentViewContainer.js:179:42)
>    9: showContentView(Resources/Views/ContentBrowser.js:172:58)
>    10: _recordingLoaded(Resources/Views/TimelineSidebarPanel.js:436:52)
>    11: dispatch(Resources/Base/Object.js:141:55)
>    12: dispatchEventToListeners(Resources/Base/Object.js:148:17)
>    13: _loadNewRecording(Resources/Controllers/TimelineManager.js:421:38)
>    14: delayedWork(Resources/Controllers/TimelineManager.js:44:31)
>    15: delayedWork([native code])

[Joseph Pecoraro]

(In reply to comment #3)
> A workaround still doesn't seem like enough. There is another actual bug here.
> 
> It looks like _loadNewRecording is triggering the Timelines panel to be the selected panel:

Ugh, and even fixing that we run into another issue.

1. Open inspector
2. Switch to Timelines sidebar (puts a TimelineContentView into back forward history)
3. Switch to Resources sidebar
4. Select any resource (puts a ResourceView into back forward history)
5. Cmd+R to reload
  => ends up with Timelines sidebar selected

So again, looking at a Resource in the Resources sidebar, suddenly we get the Resource in Timelines.

The issue here is because of the back forward history:

  - ResourceViews are torn down
    => last valid view in the back forward list is selected, that is the TimelineContentView
    => timelines sidebar becomes selected
  - Page loads the old resource
  - Resource is restored
    => state restoration checks if timelines is a valid sidebar for this resource, it is.

We can't really fix one of these issues without fixing all of them. Each is easy to reproduce.

[Joseph Pecoraro]

Created attachment 236673 [details]
[PATCH] Debugging Patch

[Joseph Pecoraro]

Created attachment 236678 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

Comment on attachment 236678 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:871
>  WebInspector._pageHidden = function(event)
>  {
> -    this._updateCookieForInspectorViewState();
> +    // FIXME: pagehide events are not firing at the correct time.
> +    // Ignore the event for now, and instead update view state
> +    // every time the ContentView changes instead of only when we close.
> +    // this._updateCookieForInspectorViewState();
>  }

Unregister the event, remove the function and just file a bug?

> Source/WebInspectorUI/UserInterface/Base/Main.js:926
> +    // FIXME: Selecting a CallFrameTreeElement selects the resource, does the code below work?

Good catch. For the Debugger sidebar panel, which has two content tree outlines, selectedSidebarPanel.contentTreeOutline.processingSelectionChange is the wrong check.

The check should iterate over _visibleContentTreeOutlines and look for processingSelectionChange on any of them.

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:684
> +        if (!this._pendingViewStateCookie) {
> +            if (this._viewStateCookieMatchingRepresentedObject && !Array.isArray(treeElements) && treeElements.representedObject === this._viewStateCookieMatchingRepresentedObject) {
> +                treeElements.revealAndSelect();
> +                delete this._viewStateCookieMatchingRepresentedObject;
> +            }
>              return;
> +        }

We talked about a better fox for this in FrameTreeElement.

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:735
> +            // FIXME: Due to the order of tree elements getting inserted and onpopulate
> +            // getting called on the FrameTreeElement, this tree element could get
> +            // immediately removed and re-inserted. At which point, it will not
> +            // get its selected state restored. This is a workaround for that particular
> +            // chain of events. We should fix this in a cleaner way.
> +            this._viewStateCookieMatchingRepresentedObject = matchedElement.representedObject;

Ditto.

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:741
> +                delete this._viewStateCookieMatchingRepresentedObject;

Ditto.

[Joseph Pecoraro]

(In reply to comment #7)
> (From update of attachment 236678 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236678&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:871
> >  WebInspector._pageHidden = function(event)
> >  {
> > -    this._updateCookieForInspectorViewState();
> > +    // FIXME: pagehide events are not firing at the correct time.
> > +    // Ignore the event for now, and instead update view state
> > +    // every time the ContentView changes instead of only when we close.
> > +    // this._updateCookieForInspectorViewState();
> >  }
> 
> Unregister the event, remove the function and just file a bug?

Hmm, interestingly this is working for me now. I decided to keep this and NOT update view state on every content view change. Things are working well for now.


> > Source/WebInspectorUI/UserInterface/Base/Main.js:926
> > +    // FIXME: Selecting a CallFrameTreeElement selects the resource, does the code below work?
> 
> Good catch. For the Debugger sidebar panel, which has two content tree outlines, selectedSidebarPanel.contentTreeOutline.processingSelectionChange is the wrong check.

I'll take a look at this now.


> The check should iterate over _visibleContentTreeOutlines and look for processingSelectionChange on any of them.
> 
> > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:684
> > +        if (!this._pendingViewStateCookie) {
> > +            if (this._viewStateCookieMatchingRepresentedObject && !Array.isArray(treeElements) && treeElements.representedObject === this._viewStateCookieMatchingRepresentedObject) {
> > +                treeElements.revealAndSelect();
> > +                delete this._viewStateCookieMatchingRepresentedObject;
> > +            }
> >              return;
> > +        }
> 
> We talked about a better fox for this in FrameTreeElement.

Yep, this is much nicer. It does less work too.

[Joseph Pecoraro]

Created attachment 237811 [details]
[PATCH] Proposed Fix

Addresses Tim's comments. Also fixes the CallFrame issue, good suggestion Tim!

[WebKit Commit Bot]

Comment on attachment 237811 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 237811

Committed r173407: <http://trac.webkit.org/changeset/173407>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Yay!