Web Inspector: `FrameDOMTreeContentView` may update after it has `closed` called, causing hangs on some webpages on reload

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

<rdar://82962890>

[Patrick Angle]

Created attachment 437942 [details]
Patch v1.0

[Myles C. Maxfield]

Comment on attachment 437942 [details]
Patch v1.0

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

> Source/WebInspectorUI/ChangeLog:18
> +

No test?

[Patrick Angle]

Comment on attachment 437942 [details]
Patch v1.0

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

>> Source/WebInspectorUI/ChangeLog:18
>> +
> 
> No test?

Not yet… I haven’t been able to produce a broken reduction to test working yet.

[Devin Rousso]

Comment on attachment 437942 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:522
> -            if (item)
> -                item.onpopulate();
> +            if (!item)
> +                return null;
> +
> +            item.onpopulate();

So i think the only way for us to get here is if the caller has provided both `isAncestor` and `getParent`.  Without `isAncestor`, the only way to reach this code is if (somehow) a direct child (i.e. something in `this.children`) exactly matches/corresponds to the given `representedObject`, which means that we'd only iterate inside the above `while` once.  As such, we'd only iterate this `for` once as well, and therefore only call `onpopulate` once.

Can you explain a bit more what you mean by "infinitely trying to populate that item"?  Are you saying that some `WI.TreeElement` subclass has an `onpopulate` that calls `findTreeElement`?  If so, that should probably be a reentrant error (e.g. `WI.setReentrantCheck`).  Is the issue perhaps more that `_shouldRefreshChildren` is not being set to `false` after in this case?

I ask these things because IIRC most callers do not provide `isAncestor` or `getParent`, but those that do (e.g. DOM tree, source maps, etc.) may expect this population behavior in order to ensure that `WI.TreeElement` are created for the entire path from the root to the given `representedObject` (e.g. revealing a deeply nested DOM node).

Tho, I do kinda see your point with this change in that "if we can't find a tree element for one of our ancestors, then we really shouldn't be able to find a tree element for the given `representedObject`, so why even bother trying".  I'm just wondering if maybe some caller is relying on the current (arguably incorrect) logic.

[Patrick Angle]

Comment on attachment 437942 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:522
>> +            item.onpopulate();
> 
> So i think the only way for us to get here is if the caller has provided both `isAncestor` and `getParent`.  Without `isAncestor`, the only way to reach this code is if (somehow) a direct child (i.e. something in `this.children`) exactly matches/corresponds to the given `representedObject`, which means that we'd only iterate inside the above `while` once.  As such, we'd only iterate this `for` once as well, and therefore only call `onpopulate` once.
> 
> Can you explain a bit more what you mean by "infinitely trying to populate that item"?  Are you saying that some `WI.TreeElement` subclass has an `onpopulate` that calls `findTreeElement`?  If so, that should probably be a reentrant error (e.g. `WI.setReentrantCheck`).  Is the issue perhaps more that `_shouldRefreshChildren` is not being set to `false` after in this case?
> 
> I ask these things because IIRC most callers do not provide `isAncestor` or `getParent`, but those that do (e.g. DOM tree, source maps, etc.) may expect this population behavior in order to ensure that `WI.TreeElement` are created for the entire path from the root to the given `representedObject` (e.g. revealing a deeply nested DOM node).
> 
> Tho, I do kinda see your point with this change in that "if we can't find a tree element for one of our ancestors, then we really shouldn't be able to find a tree element for the given `representedObject`, so why even bother trying".  I'm just wondering if maybe some caller is relying on the current (arguably incorrect) logic.

The situations in which we are seeing this reentrancy issue are with DOMTreeOutline/DOMTreeElement. DOMTreeOutline provides an `isAncestor` and `getParent` into `findTreeElement`. The issue is that a call to on populate may not be populating in all the expected elements that are in the current `ancestors` we are trying to populate.

Let's say we have E1 through E4, each of which is a child of the previous, and are trying to find E4 in the tree. We populate E1, then try and populate E2. After E2 is populated though, it doesn't populate in its supposed child E3 (I suspect because we are in middle of mutating the DOM tree, so the tree model is not matching the current represented object model). Previously, we would then try to find E3 in the tree, which would then attempt to populate in children of E2 again, as it is the closest ancestor in the tree. We try again to populate E2's children, even though nothing else has changed since. We then take another go at populating in E3, but still can't find it in the tree, with E2 being the closest ancestor yet again, and the cycle repeats itself.

I'm under no illusion that there isn't another issue here that leads to this being possible in the first place (something isn't happening in a happy order when the DOM tree mutates), but as you mention, we really shouldn't continue down the rabbit hole past an element we can't find anyways since it shouldn't be possible for a deeper child to be found when we couldn't populate in the current ancestor in the tree. I'm not sure I see a path where a caller could be relying on this logic without getting stuck like we sometimes do. I don't see a way in which the current behavior could be correct given that it will continue to attempt the same thing endlessly without this early return in what I believe to be exceptional cases.

[Patrick Angle]

Overall fix for this issue is going to be multi-faceted. Updating bug title to reflect what this bug will be fixing.

[Patrick Angle]

Created attachment 438203 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 438203 [details]
Patch v1.0

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

r=me

Ideally in the future I think it'd be nice to entirely get rid of the `closed` concept, but until that day this is a good step in the right direction :)

> Source/WebInspectorUI/ChangeLog:10
> +        DOM tree. to combat this, add a flag to `ContentView` to mark a closed `ContentView` as such, and then return

s/to/To

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:397
> +        this._isClosed = true;

Please make sure that all subclasses call `super.closed()` so that we won't miss any scenarios where something should be `isClosed` but isn't.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:402
> +    get isClosed() {
> +        return this._isClosed;
>      }

Style: `{` on a separate line
Style: you can make this all on one line at the top of the `// Public` section since it's a simple `get` (i.e. only returns a variable with the same name as the `get`)

> Source/WebInspectorUI/UserInterface/Views/FrameDOMTreeContentView.js:65
> +        if (this.isClosed)

I wonder how many other things could have something like this too 🤔

[Patrick Angle]

Created attachment 438294 [details]
Patch v1.2 - Review nits

[EWS]

Committed r282607 (241768@main): <https://commits.webkit.org/241768@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438294 [details].