WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-04-15 23:46:20 PDT
<
rdar://problem/76746385
>
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
Created
attachment 426453
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug