Skip to content

Commit

Permalink
Web Inspector: List of breakpoints in Sources tab disappears when ins…
Browse files Browse the repository at this point in the history
…pector reloads

rdar://123641994
https://bugs.webkit.org/show_bug.cgi?id=272098

Reviewed by Devin Rousso.

The reported bug can be exposed one of three ways, that the list of
breakpoints shown in the source sidebar disappears:
   (1) When you reload the webpage while the inspector is open,
   (2) When you navigate to a different page and come back while the
       inspector is open, and
   (3) When you close and reopen the inspector.

This fix addresses the three ways. Here's how:

   - For way (1), this happens because:
      a. The source sidebar has the logic to prune "stale" tree
         elements, which are elements that point to resources that no
         longer exists in the current frame. Reloading the webpage
         causes the main resource to change, which triggers this pruning
         logic. (Source: https://github.com/WebKit/WebKit/blob/c927c492d5b93193bc5cae300f1ca39eb36cd74c/Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js#L507-L508)
      b. When adding a breakpoint to show, the source sidebar first
         checks if there already exists a tree element representing
         the breakpoint. So, when the page reloads and its breakpoints
         are added (again), they don't actually get new corresponding
         tree elements created (Source: https://github.com/WebKit/WebKit/blob/c927c492d5b93193bc5cae300f1ca39eb36cd74c/Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js#L1407-L1408).
      c. The above two operations are in a race. If (a) occurs after
         (b), the _parents_ of the existing breakpoints will get pruned,
         since they now point to removed resources from the old webpage
         and are considered stale. Now, the reason (a) may happen after
         (b) is because NavigationSidebarPanel uses setTimeout to
         coalesce potentially-multiple calls of
         pruneStaleResourceTreeElements into just one, and setTimeout
         may put the call behind schedule. (Source: https://github.com/WebKit/WebKit/blob/27862391346cb4703757a860478d07f255cf5b57/Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js#L645-L649)
     So the fix is to remove the coalescence/buffer for calling
     pruneStaleResourceTreeElements and always allow it to be called.
     This does mean we're potentially pruning many times in a short
     time, but since pruning is only done for the top-level elements in
     the tree outlines, there shouldn't be a significant number of them
     in most cases. (Pruning nested tree elements when resources are
     sorted By Path is done by the code here: https://github.com/WebKit/WebKit/blob/bd841a09536799b70842b7e0a7e84890e9853fce/Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js#L1073-L1077)

   - For way (2), this happens because, when updating the source code
     resource for a breakpoint, which can happen as a result of loading
     a page a second time, that update is skipped unless either the
     old resource or the new resource is null. (Source: https://github.com/WebKit/WebKit/blob/c927c492d5b93193bc5cae300f1ca39eb36cd74c/Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js#L1668-L1669)
     The fix is to make it always allow breakpoints' resources to be
     updated, since when reloading the same page the resources will have
     new object references in the frame, and those have no reason to be
     blocked from being updated.

   - For way (3), this happens because:
      a. When the inspector is opened, the DebuggerManager loads the
         list of saved breakpoints from object stores. The manager tries
         to show them in the sidebar by dispatching the BreakpointAdded
         events, but they fail to get added because the breakpoints
         don't have their linked source code resources yet. (Source: https://github.com/WebKit/WebKit/blob/c927c492d5b93193bc5cae300f1ca39eb36cd74c/Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js#L128-L135)
      b. When a resource gets loaded, it links all loaded breakpoints
         that belong to this resource and show them in the sidebar.
         The breakpoints now succeed to show up because they first get
         linked to their resource. (Source: https://github.com/WebKit/WebKit/blob/c927c492d5b93193bc5cae300f1ca39eb36cd74c/Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js#L1187-L1188)
      c. The above two operations are in a race. If (a) occurs after
         (b), then during (b) there are no breakpoints loaded to link,
         and during (a) the loaded breakpoints can't get shown without
         linked resources.
     So the fix is, during (b), we also record the resource by its
     `contentIdentifier`, so in case (a) happens afterwards, we can
     look up the loaded resource to link to the loaded breakpoints so
     they can get added.

* Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:
(WI.NavigationSidebarPanel):
(WI.NavigationSidebarPanel.prototype.closed):
(WI.NavigationSidebarPanel.prototype.pruneStaleResourceTreeElements):
(WI.NavigationSidebarPanel.prototype._checkOutlinesForPendingViewStateCookie):
(WI.NavigationSidebarPanel.prototype._checkForStaleResourcesIfNeeded): Deleted.
(WI.NavigationSidebarPanel.prototype._checkForStaleResources): Deleted.
   - Remove the logic for coalescing multiple calls into
     pruneStaleResourceTreeElements. Pruning needs to happen immediately
     when conditions are met to prevent stale tree elements from being
     discovered by incoming breakpoints which will be removed by a later
     call to prune.

* Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:
(WI.DebuggerManager.prototype._associateBreakpointsWithSourceCode):
* Source/WebInspectorUI/UserInterface/Models/SourceCodeLocation.js:
(WI.SourceCodeLocation.prototype.setSourceCode):
   - Always allow breakpoints' linked-to resource to be updated since
     it might just be a newer reference to the same resource.
   - Move the `console.assert` into `setSourceCode` instead since it
     makes more sense being there instead.

* Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:
(WI.DebuggerManager.prototype.breakpointsForSourceCode):
   - If existing breakpoints haven't been loaded yet, record the
     resource by its `contentIdentifier` for later linking the loaded
     breakpoints.

* Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:
(WI.DebuggerManager.prototype.constructor):
   - When a breakpoint is loaded, see if its resource has already been
     loaded to be linked.

Canonical link: https://commits.webkit.org/279884@main
  • Loading branch information
the-chenergy authored and JonWBedard committed Jun 10, 2024
1 parent 77ce66b commit c6550e2
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 38 deletions.
16 changes: 10 additions & 6 deletions Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ WI.DebuggerManager = class DebuggerManager extends WI.Object
this._breakpointContentIdentifierMap = new Multimap;
this._breakpointScriptIdentifierMap = new Multimap;
this._breakpointIdMap = new Map;
this._sourceCodeContentIdentifierMap = new Map;

this._symbolicBreakpoints = [];

Expand Down Expand Up @@ -131,9 +132,14 @@ WI.DebuggerManager = class DebuggerManager extends WI.Object
const key = null;
WI.objectStores.breakpoints.associateObject(breakpoint, key, serializedBreakpoint);

let sourceCode = this._sourceCodeContentIdentifierMap.get(breakpoint.contentIdentifier);
if (sourceCode)
this._associateBreakpointsWithSourceCode([breakpoint], sourceCode);

this.addBreakpoint(breakpoint);
}
this._restoringBreakpoints = false;
this._sourceCodeContentIdentifierMap = undefined;
})());

if (WI.SymbolicBreakpoint.supported()) {
Expand Down Expand Up @@ -411,6 +417,8 @@ WI.DebuggerManager = class DebuggerManager extends WI.Object
if (sourceCode instanceof WI.SourceMapResource)
return Array.from(this.breakpointsForSourceCode(sourceCode.sourceMap.originalSourceCode)).filter((breakpoint) => breakpoint.sourceCodeLocation.displaySourceCode === sourceCode);

this._sourceCodeContentIdentifierMap?.set(sourceCode.contentIdentifier, sourceCode);

let contentIdentifierBreakpoints = this._breakpointContentIdentifierMap.get(sourceCode.contentIdentifier);
if (contentIdentifierBreakpoints) {
this._associateBreakpointsWithSourceCode(contentIdentifierBreakpoints, sourceCode);
Expand Down Expand Up @@ -1664,12 +1672,8 @@ WI.DebuggerManager = class DebuggerManager extends WI.Object
{
this._ignoreBreakpointDisplayLocationDidChangeEvent = true;

for (let breakpoint of breakpoints) {
if (!breakpoint.sourceCodeLocation.sourceCode)
breakpoint.sourceCodeLocation.sourceCode = sourceCode;
// SourceCodes can be unequal if the SourceCodeLocation is associated with a Script and we are looking at the Resource.
console.assert(breakpoint.sourceCodeLocation.sourceCode === sourceCode || breakpoint.sourceCodeLocation.sourceCode.contentIdentifier === sourceCode.contentIdentifier);
}
for (let breakpoint of breakpoints)
breakpoint.sourceCodeLocation.sourceCode = sourceCode;

this._ignoreBreakpointDisplayLocationDidChangeEvent = false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,11 +276,11 @@ WI.SourceCodeLocation = class SourceCodeLocation extends WI.Object

setSourceCode(sourceCode)
{
console.assert((this._sourceCode === null && sourceCode instanceof WI.SourceCode) || (this._sourceCode instanceof WI.SourceCode && sourceCode === null));

if (sourceCode === this._sourceCode)
return;

console.assert((this._sourceCode === null && sourceCode instanceof WI.SourceCode) || (this._sourceCode instanceof WI.SourceCode && sourceCode === null) || this._sourceCode?.contentIdentifier === sourceCode?.contentIdentifier);

this._makeChangeAndDispatchChangeEventIfNeeded(function() {
if (this._sourceCode) {
this._sourceCode.removeEventListener(WI.SourceCode.Event.SourceMapAdded, this._sourceCodeSourceMapAdded, this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ WI.NavigationSidebarPanel = class NavigationSidebarPanel extends WI.SidebarPanel
this._shouldAutoPruneStaleTopLevelResourceTreeElements = shouldAutoPruneStaleTopLevelResourceTreeElements || false;

if (this._shouldAutoPruneStaleTopLevelResourceTreeElements) {
WI.Frame.addEventListener(WI.Frame.Event.MainResourceDidChange, this._checkForStaleResources, this);
WI.Frame.addEventListener(WI.Frame.Event.ChildFrameWasRemoved, this._checkForStaleResources, this);
WI.Frame.addEventListener(WI.Frame.Event.ResourceWasRemoved, this._checkForStaleResources, this);
WI.Frame.addEventListener(WI.Frame.Event.MainResourceDidChange, this.pruneStaleResourceTreeElements, this);
WI.Frame.addEventListener(WI.Frame.Event.ChildFrameWasRemoved, this.pruneStaleResourceTreeElements, this);
WI.Frame.addEventListener(WI.Frame.Event.ResourceWasRemoved, this.pruneStaleResourceTreeElements, this);
}

this._pendingViewStateCookie = null;
Expand All @@ -86,9 +86,9 @@ WI.NavigationSidebarPanel = class NavigationSidebarPanel extends WI.SidebarPanel
window.removeEventListener("resize", this._boundUpdateContentOverflowShadowVisibilitySoon);

if (this._shouldAutoPruneStaleTopLevelResourceTreeElements) {
WI.Frame.removeEventListener(WI.Frame.Event.MainResourceDidChange, this._checkForStaleResources, this);
WI.Frame.removeEventListener(WI.Frame.Event.ChildFrameWasRemoved, this._checkForStaleResources, this);
WI.Frame.removeEventListener(WI.Frame.Event.ResourceWasRemoved, this._checkForStaleResources, this);
WI.Frame.removeEventListener(WI.Frame.Event.MainResourceDidChange, this.pruneStaleResourceTreeElements, this);
WI.Frame.removeEventListener(WI.Frame.Event.ChildFrameWasRemoved, this.pruneStaleResourceTreeElements, this);
WI.Frame.removeEventListener(WI.Frame.Event.ResourceWasRemoved, this.pruneStaleResourceTreeElements, this);
}
}

Expand Down Expand Up @@ -485,11 +485,6 @@ WI.NavigationSidebarPanel = class NavigationSidebarPanel extends WI.SidebarPanel

pruneStaleResourceTreeElements()
{
if (this._checkForStaleResourcesTimeoutIdentifier) {
clearTimeout(this._checkForStaleResourcesTimeoutIdentifier);
this._checkForStaleResourcesTimeoutIdentifier = undefined;
}

for (let contentTreeOutline of this.contentTreeOutlines) {
// Check all the ResourceTreeElements at the top level to make sure their Resource still has a parentFrame in the frame hierarchy.
// If the parentFrame is no longer in the frame hierarchy we know it was removed due to a navigation or some other page change and
Expand Down Expand Up @@ -631,24 +626,6 @@ WI.NavigationSidebarPanel = class NavigationSidebarPanel extends WI.SidebarPanel
this._updateContentOverflowShadowVisibilityDebouncer.delayForTime(0);
}

_checkForStaleResourcesIfNeeded()
{
if (!this._checkForStaleResourcesTimeoutIdentifier || !this._shouldAutoPruneStaleTopLevelResourceTreeElements)
return;
this.pruneStaleResourceTreeElements();
}

_checkForStaleResources(event)
{
console.assert(this._shouldAutoPruneStaleTopLevelResourceTreeElements);

if (this._checkForStaleResourcesTimeoutIdentifier)
return;

// Check on a delay to coalesce multiple calls to _checkForStaleResources.
this._checkForStaleResourcesTimeoutIdentifier = setTimeout(this.pruneStaleResourceTreeElements.bind(this));
}

_isTreeElementWithoutRepresentedObject(treeElement)
{
return treeElement instanceof WI.FolderTreeElement
Expand All @@ -671,7 +648,8 @@ WI.NavigationSidebarPanel = class NavigationSidebarPanel extends WI.SidebarPanel
if (!this._pendingViewStateCookie)
return;

this._checkForStaleResourcesIfNeeded();
if (this.shouldAutoPruneStaleTopLevelResourceTreeElements)
this.pruneStaleResourceTreeElements();

var visibleTreeElements = [];
this.contentTreeOutlines.forEach(function(outline) {
Expand Down

0 comments on commit c6550e2

Please sign in to comment.