Bug 144650 – Web Inspector: REGRESSION (Tabs): Issues reloading a resource with breakpoints

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

[Joseph Pecoraro]

* SUMMARY
REGRESSION(Tabs) Issues reloading a resource with breakpoints

* STEPS TO REPRODUCE
1. Inspect <http://bogojoker.com/shell/>
2. Set a breakpoint on easySlider.min.js:56:21 ("if (autoplay) {")
3. Cmd+R to reload
  => Duplicate breakpoints, with 1 inactive and one on the pre-pretty-printed line.

[Radar WebKit Bug Importer]

<rdar://problem/20829494>

[Joseph Pecoraro]

Tracing SourceCodeTextEditor.prototype.close calls I see ResourceSidebarPanel closes its views when the main resource changes:

    Views/SourceCodeTextEditor.js:130:20: CONSOLE LOG close called
    Views/SourceCodeTextEditor.js:131:22: CONSOLE TRACE
    0: close(Views/SourceCodeTextEditor.js:131:22)
    1: closed(Views/TextResourceContentView.js:120:31)
    2: _disassociateFromContentView(Views/ContentViewContainer.js:459:27)
    3: closeAllContentViews(Views/ContentViewContainer.js:384:46)
    4: closed(Views/ClusterContentView.js:85:56)
    5: closed(Views/ResourceClusterContentView.js:135:62)
    6: _disassociateFromContentView(Views/ContentViewContainer.js:459:27)
    7: closeAllContentViews(Views/ContentViewContainer.js:384:46)
    8: _mainFrameMainResourceDidChange(Views/ResourceSidebarPanel.js:170:70)
    9: _mainResourceDidChange(Views/ResourceSidebarPanel.js:160:45)
    10: dispatch(Base/Object.js:124:55)
    11: dispatchEventToListeners(Base/Object.js:139:21)
    12: _dispatchMainResourceDidChangeEvent(Models/Frame.js:474:38)
    13: commitProvisionalLoad(Models/Frame.js:138:53)
    14: frameDidNavigate(Controllers/FrameResourceManager.js:105:40)
    15: frameNavigated(Protocol/PageObserver.js:42:59)
    16: dispatchEvent(Protocol/InspectorBackend.js:353:42)
    17: _dispatchEvent(Protocol/InspectorBackend.js:233:32)
    18: dispatch(Protocol/InspectorBackend.js:88:32)
    19: dispatchNextQueuedMessageFromBackend(Protocol/MessageDispatcher.js:42:34)

Other panels, like the DebuggerSidebarPanel should probably be doing the same.

In this case, what I'm seeing is the old ContentView without a sidebar tree element, and selecting the _new_ easySlider.min.js tree element gives me what I expect.

[Joseph Pecoraro]

I count a jumble of issues investigating this:

1. DebuggerSidebarPanel needs to clear content views and tree elements on main resource changes.
  - closeAllContentViews
  - breakpointsContentTreeOutline, remove non-global folder tree elements

2. BreakpointTreeElement restoration doesn't work
  - display source code for a WebInspector.Breakpoint represented object

3. Cookie saving is cumulative instead of replacing
  - TabContentView adds new cookie state onto its cookie instead of replacing the cookie

4. TabContentView.restoreStateFromCookie(causedByReload) parameter is never set
  - sounds like a missed opportunity, but it is always unused

5. Order of operations on WebInspector.Frame.Event.MainResourceDidChange is problematic
  - Main.js performs state restoration on MainResourceDidChange
  - NavigationSidebarPanels (Resources, Debugger, etc) perform clean up MainResourceDidChange
  - Typically, Main.js will do its work first, which may be very problematic in restoration!

So, I'm going to hesitantly attach a patch, but Tim, feel free to take over this.

[Joseph Pecoraro]

Created attachment 252441 [details]
[PATCH] Progress

I noticed that DOM Node restoration is failing. I believe it was failing before my changes as well.

[Joseph Pecoraro]

> 5. Order of operations on WebInspector.Frame.Event.MainResourceDidChange is
> problematic
>   - Main.js performs state restoration on MainResourceDidChange
>   - NavigationSidebarPanels (Resources, Debugger, etc) perform clean up
> MainResourceDidChange
>   - Typically, Main.js will do its work first, which may be very problematic
> in restoration!

The reason this is problematic was because of this scenario:

  (1) DebuggerSidebarPanel wants to remove its ResourceTreeElements for the old Frame and add ResourceTreeElements for the new Frame
  (2) State restoration wants to find a ResourceTreeElement basically just via URL

On reload, in cases where (2) happens before (1), then the ResourceTreeElement found during restoration was the OLD ResourceTreeElement, that immediately gets removed.

[Timothy Hatcher]

Comment on attachment 252441 [details]
[PATCH] Progress

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

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:124
> +        console.error("Unknown ContentView", representedObject);
> +
>          throw "Can't make a ContentView for an unknown representedObject.";

I've been considering removing the throw here and just doing a console.error.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:393
> +        for (var i = this._breakpointsContentTreeOutline.children.length - 1; i >= 0; --i) {
> +            var treeElement = this._breakpointsContentTreeOutline.children[i];
> +            if (treeElement !== this._globalBreakpointsFolderTreeElement)
> +                this._breakpointsContentTreeOutline.removeChildAtIndex(i, true, true);
> +        }

You should not need to do this. DebuggerSidebarPanel opts-in to NavigationSidebarPanel's autoPruneOldTopLevelResourceTreeElements. That will remove ResourceTreeElements that go away on MainResourceDidChange, ChildFrameWasRemoved, and ResourceWasRemoved events. The closeAllContentViews is all we should need.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:429
> +        // FIXME: Is this needed, now that we clear everything on main resource changes?

Yes, since NavigationSidebarPanel does not auto-prune these.

> Source/WebInspectorUI/UserInterface/Views/TabContentView.js:127
> -        var cookie = this._cookieSetting.value || {};
> +        var cookie = {};

Ah, yes. I see how this was wrong now.

[Timothy Hatcher]

I'll take this over from Joe since he is out on vacation.

[Timothy Hatcher]

Created attachment 252894 [details]
Patch

[Brian Burg]

Comment on attachment 252894 [details]
Patch

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

r=me, with some speling mistakes. Naming suggestions are sort of advisory, since this patch didn't establish the naming convention.

> Source/WebInspectorUI/ChangeLog:5
> +

I really appreciate the big list of issues here :)

> Source/WebInspectorUI/UserInterface/Base/Main.js:1147
> +WebInspector._restoreCookieForOpenTabs = function(causedByNaviation)

"navigation"

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:379
> +        var oldCurrentContentView = this.currentContentView;

visibleContentView? shownContentView? oldCurrent is awkward.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:390
> +            // prune normally happen on a delay.

"happens" on an later event loop turn.

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:71
> +        this._autoPruneOldTopLevelResourceTreeElements = autoPruneOldTopLevelResourceTreeElements || false;

Why not !!autoPruneOldTopLevelResourceElements?

As for the autoPruneOldXXX, I think a naming convention like autoPruneStaleXXX better conveys why it's being pruned.

And for a flag, this._shouldAutoPruneTopLevelResourceTreeElements makes it clear what purpose it serves (a policy decision).

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:420
> +    pruneOldResourceTreeElements()

See naming note above.

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:424
> +            this._checkForOldResourcesTimeoutIdentifier = undefined;

Why not delete?

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:-620
> -        this._checkForOldResourcesTimeoutIdentifier = setTimeout(delayedWork.bind(this), 0);

(We really should make a helper method to do this coalescing, I go cross-eyed looking for mistakes every time it's reimplemented.)

> Source/WebInspectorUI/UserInterface/Views/TabContentView.js:110
> +    restoreStateFromCookie: function(causedByNaviation)

"navigation"

> Source/WebInspectorUI/UserInterface/Views/TabContentView.js:115
>          var matchTypeOnlyDelayForReload = 2000;

Might as well fix this variable name to be consistent.

[Timothy Hatcher]

Comment on attachment 252894 [details]
Patch

https://trac.webkit.org/r184130