RESOLVED FIXED 224652
Web Inspector: Tree Outlines: `ondetach` can be called without `onattach` ever being called
https://bugs.webkit.org/show_bug.cgi?id=224652
Summary Web Inspector: Tree Outlines: `ondetach` can be called without `onattach` eve...
Devin Rousso
Reported 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
Attachments
Patch (1.64 KB, patch)
2021-04-19 10:23 PDT, Razvan Caliman
no flags
Patch (10.11 KB, patch)
2021-04-19 12:08 PDT, Razvan Caliman
no flags
Patch (10.84 KB, patch)
2021-04-20 09:02 PDT, Razvan Caliman
no flags
Radar WebKit Bug Importer
Comment 1 2021-04-15 23:46:20 PDT
Razvan Caliman
Comment 2 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.
Devin Rousso
Comment 3 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
Razvan Caliman
Comment 4 2021-04-19 12:08:45 PDT
Devin Rousso
Comment 5 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?
Razvan Caliman
Comment 6 2021-04-20 09:02:04 PDT
Created attachment 426553 [details] Patch address code review feedback
Razvan Caliman
Comment 7 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(); ```
Devin Rousso
Comment 8 2021-04-20 09:49:07 PDT
Comment on attachment 426553 [details] Patch r=me
EWS
Comment 9 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].
Note You need to log in before you can comment on or make changes to this bug.