`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
<rdar://problem/76746385>
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 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
Created attachment 426453 [details] Patch
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?
Created attachment 426553 [details] Patch address code review feedback
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 on attachment 426553 [details] Patch r=me
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].