[webkit-changes] [WebKit/WebKit] c6550e: Web Inspector: List of breakpoints in Sources tab ...

[Qianlang Chen]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: c6550e29032bf25d2d2dd78a05e29a498d99fc02
      https://github.com/WebKit/WebKit/commit/c6550e29032bf25d2d2dd78a05e29a498d99fc02
  Author: Qianlang Chen <qianlangchen at apple.com>
  Date:   2024-06-10 (Mon, 10 Jun 2024)

  Changed paths:
    M Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js
    M Source/WebInspectorUI/UserInterface/Models/SourceCodeLocation.js
    M Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js

  Log Message:
  -----------
  Web Inspector: List of breakpoints in Sources tab disappears when inspector 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



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications