Bug 224652 – Web Inspector: Tree Outlines: `ondetach` can be called without `onattach` ever being called

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

`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

[Radar WebKit Bug Importer]

<rdar://problem/76746385>

[Razvan Caliman]

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.

[Devin Rousso]

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

[Razvan Caliman]

Created attachment 426453 [details]
Patch

[Devin Rousso]

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?

[Razvan Caliman]

Created attachment 426553 [details]
Patch

address code review feedback

[Razvan Caliman]

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();
```

[Devin Rousso]

Comment on attachment 426553 [details]
Patch

r=me

[EWS]

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].