Bug 234572 - Web Inspector: Assertion Failed removing subview in ContentViewContainer.prototype._disassociateFromContentView
Summary: Web Inspector: Assertion Failed removing subview in ContentViewContainer.prot...
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: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-21 11:40 PST by Patrick Angle
Modified: 2021-12-21 21:18 PST (History)
5 users (show)

See Also:


Attachments
Patch v1.0 (1.69 KB, patch)
2021-12-21 11:59 PST, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.1 (2.07 KB, patch)
2021-12-21 12:34 PST, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-12-21 11:40:40 PST
[Error] Assertion Failed: Subview DOM element must be a child of the parent view element.
	removeSubview (View.js:121)
	_disassociateFromContentView (ContentViewContainer.js:428)
	replaceContentView (ContentViewContainer.js:199)
	showContentView (ContentViewContainer.js:85)
	_showDOMTreeContentViewIfNeeded (ElementsTabContentView.js:144)
	_mainFrameDidChange (ElementsTabContentView.js:149)
	dispatch (Object.js:134)
	dispatchEventToListeners (Object.js:142)
	_mainFrameDidChange (NetworkManager.js:1384)
	_processMainFrameResourceTreePayload (NetworkManager.js:1240)
	_processMainFrameResourceTreePayload
	_dispatchResponseToCallback (Connection.js:152)
	_dispatchResponse (Connection.js:122)
	dispatch (Connection.js:77)
	dispatchProvisionalMessages (Connection.js:68)
	didCommitProvisionalTarget (TargetManager.js:151)
	didCommitProvisionalTarget (TargetObserver.js:37)
	_dispatchEvent (Connection.js:210)
	dispatch (Connection.js:79)
	dispatch (InspectorBackend.js:233)
	(anonymous function) (MessageDispatcher.js:42)

https://trac.webkit.org/changeset/283859/webkit removed the `isAttached` check for the content view by accident when refactoring this method to work with extension tabs, which should not be removed from the DOM, but it is not actually guaranteed that the content view is attached.
Comment 1 Radar WebKit Bug Importer 2021-12-21 11:41:04 PST
<rdar://problem/86778405>
Comment 2 Patrick Angle 2021-12-21 11:59:07 PST
Created attachment 447741 [details]
Patch v1.0
Comment 3 Devin Rousso 2021-12-21 12:18:03 PST
Comment on attachment 447741 [details]
Patch v1.0

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:429
> +            this.removeSubview(contentView);

So this call was added in r283728 and was originally guarded by `contentView.constructor.shouldNotRemoveFromDOMWhenHidden()`.  I'm not really sure if we want this to be something that _always_ happens outside of that check (especially since almost all callers of this will call `_hideEntry` right before, which also does `removeSubview`).  Maybe we should restructure this so that it's all underneath `contentView.constructor.shouldNotRemoveFromDOMWhenHidden()`?
```
// Hidden/non-visible extension tabs must remain attached to the DOM to avoid reloading.
if (contentView.constructor.shouldNotRemoveFromDOMWhenHidden()) {
    if (!contentView.visible)
        return;

    if (contentView.attached)
        this.removeSubview(contentView);
}
```
Comment 4 Patrick Angle 2021-12-21 12:34:13 PST
Created attachment 447743 [details]
Patch v1.1
Comment 5 EWS 2021-12-21 21:18:29 PST
Committed r287346 (245488@main): <https://commits.webkit.org/245488@main>

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