.
Created attachment 443490 [details] Patch
Comment on attachment 443490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443490&action=review > Source/WebCore/html/HTMLElement.cpp:1341 > + RefPtr<HTMLDivElement> containerToRemove; Is this variable necessary? Why not just remove the container in the loop?
Comment on attachment 443490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443490&action=review Thanks for the review! >> Source/WebCore/html/HTMLElement.cpp:1341 >> + RefPtr<HTMLDivElement> containerToRemove; > > Is this variable necessary? Why not just remove the container in the loop? I'll need to double check, but IIRC mutating the DOM underneath the scope of a DOM iterator will fire debug assertions (though in this scenario, it is technically safe since we break immediately afterwards).
(In reply to Wenson Hsieh from comment #3) > Comment on attachment 443490 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443490&action=review > > Thanks for the review! > > >> Source/WebCore/html/HTMLElement.cpp:1341 > >> + RefPtr<HTMLDivElement> containerToRemove; > > > > Is this variable necessary? Why not just remove the container in the loop? > > I'll need to double check, but IIRC mutating the DOM underneath the scope of > a DOM iterator will fire debug assertions (though in this scenario, it is > technically safe since we break immediately afterwards). Yeah, directly removing the node underneath the iterator results in: ``` ASSERTION FAILED: ScriptDisallowedScope::InMainThread::isEventDispatchAllowedInSubtree(childToRemove) dom/ContainerNode.cpp(146) : bool WebCore::ContainerNode::removeNodeWithScriptAssertion(WebCore::Node &, ChildChange::Source) 1 0x37c1f4199 WTFCrash 2 0x37c1f41b9 WTFCrashWithSecurityImplication 3 0x3920467c1 WebCore::ContainerNode::removeNodeWithScriptAssertion(WebCore::Node&, WebCore::ContainerNode::ChildChange::Source) 4 0x392042e22 WebCore::ContainerNode::removeChild(WebCore::Node&) 5 0x392221136 WebCore::Node::remove() 6 0x3925543cf WebCore::HTMLElement::removeImageOverlaySoonIfNeeded()::$_4::operator()() const ``` (I'll keep the code as-is, at least for now)
(In reply to Wenson Hsieh from comment #4) > (In reply to Wenson Hsieh from comment #3) > > Comment on attachment 443490 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=443490&action=review > > > > Thanks for the review! > > > > >> Source/WebCore/html/HTMLElement.cpp:1341 > > >> + RefPtr<HTMLDivElement> containerToRemove; > > > > > > Is this variable necessary? Why not just remove the container in the loop? > > > > I'll need to double check, but IIRC mutating the DOM underneath the scope of > > a DOM iterator will fire debug assertions (though in this scenario, it is > > technically safe since we break immediately afterwards). > > Yeah, directly removing the node underneath the iterator results in: > > ``` > ASSERTION FAILED: > ScriptDisallowedScope::InMainThread:: > isEventDispatchAllowedInSubtree(childToRemove) > dom/ContainerNode.cpp(146) : bool > WebCore::ContainerNode::removeNodeWithScriptAssertion(WebCore::Node &, > ChildChange::Source) > 1 0x37c1f4199 WTFCrash > 2 0x37c1f41b9 WTFCrashWithSecurityImplication > 3 0x3920467c1 > WebCore::ContainerNode::removeNodeWithScriptAssertion(WebCore::Node&, > WebCore::ContainerNode::ChildChange::Source) > 4 0x392042e22 WebCore::ContainerNode::removeChild(WebCore::Node&) > 5 0x392221136 WebCore::Node::remove() > 6 0x3925543cf > WebCore::HTMLElement::removeImageOverlaySoonIfNeeded()::$_4::operator()() > const > ``` > > (I'll keep the code as-is, at least for now) 👍
Committed r285425 (243981@main): <https://commits.webkit.org/243981@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443490 [details].
<rdar://problem/85169058>