232788 2021-11-06 12:22:17 -0700 [Live Text] Add a helper method to remove recognized text from an image element 2021-11-08 12:56:20 -0800 1 1 1 Unclassified WebKit Platform WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 wenson_hsieh wenson_hsieh akeerthi cdumez changseok esprehn+autocc ews-watchlist gyuyoung.kim hi megan_gardner thorton webkit-bug-importer oldest_to_newest 1812406 0 wenson_hsieh 2021-11-06 12:22:17 -0700 . 1812408 1 443490 wenson_hsieh 2021-11-06 12:24:39 -0700 Created attachment 443490 Patch 1812688 2 443490 akeerthi 2021-11-08 10:34:24 -0800 Comment on attachment 443490 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? 1812693 3 443490 wenson_hsieh 2021-11-08 10:38:19 -0800 Comment on attachment 443490 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). 1812740 4 wenson_hsieh 2021-11-08 11:49:29 -0800 (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) 1812750 5 akeerthi 2021-11-08 12:08:48 -0800 (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) 👍 1812776 6 ews-feeder 2021-11-08 12:55:34 -0800 Committed r285425 (243981@main): <https://commits.webkit.org/243981@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443490. 1812777 7 webkit-bug-importer 2021-11-08 12:56:20 -0800 <rdar://problem/85169058> 443490 2021-11-06 12:24:39 -0700 2021-11-08 12:55:35 -0800 Patch bug-232788-20211106122438.patch text/plain 4359 wenson_hsieh U3VidmVyc2lvbiBSZXZpc2lvbjogMjg1MzU5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYTg3MmVjOGMwM2ZkNWEx ZTAxZGM2OWI0ZDM3YzhjNDYxYmJjMzUyNi4uM2YzYzZlZWFiMDBlYjE5MDBlNmU5NzBiOWQ3NDZj ZDVhYWRjMzMyMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDIxLTExLTA2ICBXZW5z b24gSHNpZWggIDx3ZW5zb25faHNpZWhAYXBwbGUuY29tPgorCisgICAgICAgIFtMaXZlIFRleHRd IEFkZCBhIGhlbHBlciBtZXRob2QgdG8gcmVtb3ZlIHJlY29nbml6ZWQgdGV4dCBmcm9tIGFuIGlt YWdlIGVsZW1lbnQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTIzMjc4OAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIEFkZCBhIGNvdXBsZSBvZiBoZWxwZXIgZnVuY3Rpb25zIGZvciByZXNldHRpbmcgY2FjaGVk IHRleHQgcmVjb2duaXRpb24gcmVzdWx0cywgYW5kIGZvciByZW1vdmluZyByZWNvZ25pemVkIHRl eHQKKyAgICAgICAgZnJvbSB0aGUgc2hhZG93IHJvb3Qgb2YgYW4gaW1hZ2UgZWxlbWVudC4gVGhl c2Ugd2lsbCBiZSB1c2VkIHRvIHN1cHBvcnQgZW5oYW5jZW1lbnRzIHRvIExpdmUgVGV4dCBpbiBh IGZ1dHVyZQorICAgICAgICBwYXRjaC4gTm8gY2hhbmdlIGluIGJlaGF2aW9yLgorCisgICAgICAg ICogaHRtbC9IVE1MRWxlbWVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpIVE1MRWxlbWVudDo6 cmVtb3ZlSW1hZ2VPdmVybGF5U29vbklmTmVlZGVkKToKKyAgICAgICAgKiBodG1sL0hUTUxFbGVt ZW50Lmg6CisgICAgICAgICogcGFnZS9QYWdlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlBhZ2U6 OnJlc2V0VGV4dFJlY29nbml0aW9uUmVzdWx0KToKKyAgICAgICAgKiBwYWdlL1BhZ2UuaDoKKwog MjAyMS0xMS0wNSAgV2Vuc29uIEhzaWVoICA8d2Vuc29uX2hzaWVoQGFwcGxlLmNvbT4KIAogICAg ICAgICBbaU9TXSBNYWlsIGNvbXBvc2UgYmVjb21lcyB1bnJlc3BvbnNpdmUgYWZ0ZXIgcGFzdGlu ZyBpbiB0ZXh0IGFuZCBhdHRlbXB0aW5nIHRvIHR5cGUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJD b3JlL2h0bWwvSFRNTEVsZW1lbnQuY3BwIGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MRWxlbWVu dC5jcHAKaW5kZXggYzkzNWVlMWQ3MjcyYzY3NGU3MTVlZmJmNmVmNTBmYTBjYTllY2YwOS4uMWVl OGI1ZTc1M2RmZjVjMmQ2MjczYjE4OGI3YjRjYzQ5MWJlODIwNSAxMDA2NDQKLS0tIGEvU291cmNl L1dlYkNvcmUvaHRtbC9IVE1MRWxlbWVudC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvaHRtbC9I VE1MRWxlbWVudC5jcHAKQEAgLTEzMjQsNiArMTMyNCwzOCBAQCBib29sIEhUTUxFbGVtZW50Ojpp c0ltYWdlT3ZlcmxheVRleHQoY29uc3QgTm9kZSYgbm9kZSkKICAgICByZXR1cm4gZmFsc2U7CiB9 CiAKK3ZvaWQgSFRNTEVsZW1lbnQ6OnJlbW92ZUltYWdlT3ZlcmxheVNvb25JZk5lZWRlZCgpCit7 CisgICAgaWYgKCFoYXNJbWFnZU92ZXJsYXkoKSkKKyAgICAgICAgcmV0dXJuOworCisgICAgZG9j dW1lbnQoKS5ldmVudExvb3AoKS5xdWV1ZVRhc2soVGFza1NvdXJjZTo6SW50ZXJuYWxBc3luY1Rh c2ssIFt3ZWFrVGhpcyA9IFdlYWtQdHIgeyAqdGhpcyB9XSB7CisgICAgICAgIFJlZlB0ciBwcm90 ZWN0ZWRUaGlzID0gd2Vha1RoaXMuZ2V0KCk7CisgICAgICAgIGlmICghcHJvdGVjdGVkVGhpcykK KyAgICAgICAgICAgIHJldHVybjsKKworICAgICAgICBSZWZQdHIgc2hhZG93Um9vdCA9IHByb3Rl Y3RlZFRoaXMtPnVzZXJBZ2VudFNoYWRvd1Jvb3QoKTsKKyAgICAgICAgaWYgKCFzaGFkb3dSb290 KQorICAgICAgICAgICAgcmV0dXJuOworCisgICAgICAgIFJlZlB0cjxIVE1MRGl2RWxlbWVudD4g Y29udGFpbmVyVG9SZW1vdmU7CisgICAgICAgIGZvciAoYXV0byYgY2hpbGQgOiBjaGlsZHJlbk9m VHlwZTxIVE1MRGl2RWxlbWVudD4oKnNoYWRvd1Jvb3QpKSB7CisgICAgICAgICAgICBpZiAoY2hp bGQuZ2V0SWRBdHRyaWJ1dGUoKSA9PSBpbWFnZU92ZXJsYXlFbGVtZW50SWRlbnRpZmllcigpKSB7 CisgICAgICAgICAgICAgICAgY29udGFpbmVyVG9SZW1vdmUgPSAmY2hpbGQ7CisgICAgICAgICAg ICAgICAgYnJlYWs7CisgICAgICAgICAgICB9CisgICAgICAgIH0KKworICAgICAgICBpZiAoY29u dGFpbmVyVG9SZW1vdmUpCisgICAgICAgICAgICBjb250YWluZXJUb1JlbW92ZS0+cmVtb3ZlKCk7 CisKKyNpZiBFTkFCTEUoSU1BR0VfQU5BTFlTSVMpCisgICAgICAgIGlmIChhdXRvIHBhZ2UgPSBw cm90ZWN0ZWRUaGlzLT5kb2N1bWVudCgpLnBhZ2UoKSkKKyAgICAgICAgICAgIHBhZ2UtPnJlc2V0 VGV4dFJlY29nbml0aW9uUmVzdWx0KCpwcm90ZWN0ZWRUaGlzKTsKKyNlbmRpZgorICAgIH0pOwor fQorCiAjaWYgRU5BQkxFKElNQUdFX0FOQUxZU0lTKQogCiBJbnRSZWN0IEhUTUxFbGVtZW50Ojpj b250YWluZXJSZWN0Rm9yVGV4dFJlY29nbml0aW9uKCkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJD b3JlL2h0bWwvSFRNTEVsZW1lbnQuaCBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTEVsZW1lbnQu aAppbmRleCBjOTY1ZTIzMDE2ZTI1ZjY0M2Y1MzBlMzgwOTIxZTc1NjMzNjVmMWI5Li5jNjQyZDA2 MjUzNTRkNGY0Y2E4OTdkNzllMDYyNzlkNmMxODkwZmE3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi Q29yZS9odG1sL0hUTUxFbGVtZW50LmgKKysrIGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MRWxl bWVudC5oCkBAIC0xMzgsNiArMTM4LDcgQEAgcHVibGljOgogICAgIFdFQkNPUkVfRVhQT1JUIHN0 YXRpYyBib29sIGlzSW5zaWRlSW1hZ2VPdmVybGF5KGNvbnN0IE5vZGUmKTsKICAgICBXRUJDT1JF X0VYUE9SVCBzdGF0aWMgYm9vbCBpc0ltYWdlT3ZlcmxheVRleHQoY29uc3QgTm9kZSYpOwogICAg IFdFQkNPUkVfRVhQT1JUIHN0YXRpYyBib29sIGlzSW1hZ2VPdmVybGF5VGV4dChjb25zdCBOb2Rl Kik7CisgICAgdm9pZCByZW1vdmVJbWFnZU92ZXJsYXlTb29uSWZOZWVkZWQoKTsKIAogI2lmIEVO QUJMRShJTUFHRV9BTkFMWVNJUykKICAgICBJbnRSZWN0IGNvbnRhaW5lclJlY3RGb3JUZXh0UmVj b2duaXRpb24oKTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvUGFnZS5jcHAgYi9T b3VyY2UvV2ViQ29yZS9wYWdlL1BhZ2UuY3BwCmluZGV4IGE3YjAyZTBhZWI5YWMxNWYxOGNjMTg4 NzA0MDFjZDUwMDI1ZjZlY2IuLjg1ZDBhYTc0OWM0MzBhYjYyZWQxYjliMDJkNTE2MGZlMWExZGRj ZDkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvUGFnZS5jcHAKKysrIGIvU291cmNl L1dlYkNvcmUvcGFnZS9QYWdlLmNwcApAQCAtMzcxNSw2ICszNzE1LDExIEBAIHZvaWQgUGFnZTo6 cmVzZXRUZXh0UmVjb2duaXRpb25SZXN1bHRzKCkKICAgICBtX3RleHRSZWNvZ25pdGlvblJlc3Vs dHMuY2xlYXIoKTsKIH0KIAordm9pZCBQYWdlOjpyZXNldFRleHRSZWNvZ25pdGlvblJlc3VsdChj b25zdCBIVE1MRWxlbWVudCYgZWxlbWVudCkKK3sKKyAgICBtX3RleHRSZWNvZ25pdGlvblJlc3Vs dHMucmVtb3ZlKGVsZW1lbnQpOworfQorCiAjZW5kaWYgLy8gRU5BQkxFKElNQUdFX0FOQUxZU0lT KQogCiAjaWYgRU5BQkxFKFNFUlZJQ0VfV09SS0VSKQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNv cmUvcGFnZS9QYWdlLmggYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL1BhZ2UuaAppbmRleCAxMTUxY2Ni OGIxNzdlNjM2ZDNiYmYyMzQ5YzQyOGUxM2UwZjdmNmQ3Li5jZGY3Y2IzNzRhZGMzMTAwYjZmMDY1 M2I3N2QxNmMzYWJkYzEzYzE2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wYWdlL1BhZ2Uu aAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL1BhZ2UuaApAQCAtODkwLDYgKzg5MCw3IEBAIHB1 YmxpYzoKICNpZiBFTkFCTEUoSU1BR0VfQU5BTFlTSVMpCiAgICAgV0VCQ09SRV9FWFBPUlQgYm9v bCBoYXNDYWNoZWRUZXh0UmVjb2duaXRpb25SZXN1bHQoY29uc3QgSFRNTEVsZW1lbnQmKSBjb25z dDsKICAgICB2b2lkIGNhY2hlVGV4dFJlY29nbml0aW9uUmVzdWx0KGNvbnN0IEhUTUxFbGVtZW50 JiwgY29uc3QgSW50UmVjdCYgY29udGFpbmVyUmVjdCwgY29uc3QgVGV4dFJlY29nbml0aW9uUmVz dWx0Jik7CisgICAgdm9pZCByZXNldFRleHRSZWNvZ25pdGlvblJlc3VsdChjb25zdCBIVE1MRWxl bWVudCYpOwogI2VuZGlmCiAKICAgICBXRUJDT1JFX0VYUE9SVCBQZXJtaXNzaW9uQ29udHJvbGxl ciYgcGVybWlzc2lvbkNvbnRyb2xsZXIoKTsK