Bug 224652 - Web Inspector: Tree Outlines: `ondetach` can be called without `onattach` ever being called
Summary: Web Inspector: Tree Outlines: `ondetach` can be called without `onattach` eve...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Razvan Caliman
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-15 23:45 PDT by Devin Rousso
Modified: 2021-04-20 10:17 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.64 KB, patch)
2021-04-19 10:23 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2021-04-19 12:08 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch (10.84 KB, patch)
2021-04-20 09:02 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-04-15 23:45:49 PDT
`onattach` is only called if the `WI.TreeElement` has a `_listItemNode` and its parent `_shouldRefreshChildren`, meaning that if `removeChild`/`removeChildren` is called before those conditions are met then `ondetach` will get called, which could result in bad consistency/state
Comment 1 Radar WebKit Bug Importer 2021-04-15 23:46:20 PDT
<rdar://problem/76746385>
Comment 2 Razvan Caliman 2021-04-19 10:23:13 PDT
Created attachment 426441 [details]
Patch

Hardened the checks guarding TreeElement.ondetach() and did some light manual testing to check for side-effects. If you know of regressions or bugs happening as a result of this, please let me know so I can check them.
Comment 3 Devin Rousso 2021-04-19 10:47:54 PDT
Comment on attachment 426441 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:293
> +        if (this.ondetach && this._listItemNode && !this.parent._shouldRefreshChildren)

I don't think the check for `!this.parent._shouldRefreshChildren` is really necessary since it defaults to a falsy value (`undefined`) and also usually gets set back to a falsy value (`false`) in the subclass `onpopulate` (or also in `WI.TreeElement.prototype.expand`).

I think checking for `_listItemNode` is probably enough :)

As such, you should be able to remove the `if (this.listItemElement)` in `WI.FrameTreeElement.prototype.ondetach`.

Also please remove all the `FIXME` comments I added in <https://webkit.org/b/224651> :P
Comment 4 Razvan Caliman 2021-04-19 12:08:45 PDT
Created attachment 426453 [details]
Patch
Comment 5 Devin Rousso 2021-04-19 12:15:16 PDT
Comment on attachment 426453 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:293
> +        if (this.ondetach && this._listItemNode)

So looking at this again, I think there may need to be one more change.  `onattach` is called if `!this._listItemNode` OR `this.parent._shouldRefreshChildren`.  The changes you've made handle the former, but in the case of the latter it's possible for `onattach` to be called more than once per-`ondetach`.

I think the simple solution in this case is to
```
-if (this._listItemNode && this._listItemNode.parentNode)
-    this._listItemNode.parentNode.removeChild(this._listItemNode);
+this._detach()
```
inside `WI.TreeElement.prototype._attach` so that if there's already a `this._listItemNode` when `this.parent._shouldRefreshChildren` is set, we make sure to call `ondetach` with the old state first.

> Source/WebInspectorUI/UserInterface/Views/WebSocketResourceTreeElement.js:41
>          super.ondetach();

NIT: While you're here, can you move this to the end of the function so that we don't accidentally tear down superclass state if this subclass needs it?
Comment 6 Razvan Caliman 2021-04-20 09:02:04 PDT
Created attachment 426553 [details]
Patch

address code review feedback
Comment 7 Razvan Caliman 2021-04-20 09:05:06 PDT
Comment on attachment 426453 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:293
>> +        if (this.ondetach && this._listItemNode)
> 
> So looking at this again, I think there may need to be one more change.  `onattach` is called if `!this._listItemNode` OR `this.parent._shouldRefreshChildren`.  The changes you've made handle the former, but in the case of the latter it's possible for `onattach` to be called more than once per-`ondetach`.
> 
> I think the simple solution in this case is to
> ```
> -if (this._listItemNode && this._listItemNode.parentNode)
> -    this._listItemNode.parentNode.removeChild(this._listItemNode);
> +this._detach()
> ```
> inside `WI.TreeElement.prototype._attach` so that if there's already a `this._listItemNode` when `this.parent._shouldRefreshChildren` is set, we make sure to call `ondetach` with the old state first.

Yes, this makes sense. I made the change but opted to keep a check just so it's immediately clear why we're calling _detach() in _attach()

```
if (this.parent._shouldRefreshChildren)
                this._detach();
```
Comment 8 Devin Rousso 2021-04-20 09:49:07 PDT
Comment on attachment 426553 [details]
Patch

r=me
Comment 9 EWS 2021-04-20 10:17:31 PDT
Committed r276310 (236792@main): <https://commits.webkit.org/236792@main>

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