Summary: | [Live Text] Add a helper method to remove recognized text from an image element | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||
Component: | Platform | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | akeerthi, cdumez, changseok, esprehn+autocc, ews-watchlist, gyuyoung.kim, hi, megan_gardner, thorton, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Wenson Hsieh
2021-11-06 12:22:17 PDT
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]. |