Bug 232788 – [Live Text] Add a helper method to remove recognized text from an image element

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 443490 [details]
Patch

[Aditya Keerthi]

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?

[Wenson Hsieh]

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).

[Wenson Hsieh]

(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)

[Aditya Keerthi]

(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)

👍

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/85169058>