239402 2022-04-15 14:03:59 -0700 AXIsolatedTree::updateNode cannot attach the platform wrapper to the replacement node on the main thread. 2022-04-16 15:06:42 -0700 1 1 1 Unclassified WebKit Accessibility WebKit Nightly Build Unspecified Unspecified NEW InRadar P2 Normal --- 1 andresg_22 andresg_22 aboxhall andresg_22 apinheiro cfleizach darin dmazzoni ews-watchlist godrimarcel.551 jcraig jdiggs samuel_white webkit-bug-importer oldest_to_newest 1861903 0 andresg_22 2022-04-15 14:03:59 -0700 AXIsolatedTree::updateNode cannot attach the platform wrapper to the replacement node on the main thread. 1861904 1 webkit-bug-importer 2022-04-15 14:04:09 -0700 <rdar://problem/91827992> 1861910 2 457724 andresg_22 2022-04-15 14:14:02 -0700 Created attachment 457724 Patch 1861944 3 457724 darin 2022-04-15 15:43:29 -0700 Comment on attachment 457724 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457724&action=review > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:299 > - auto change = nodeChangeForObject(axObject, parentID, true); > + auto change = nodeChangeForObject(axObject, parentID, false); This is why we work so hard to avoid boolean constants. The old code is not obviously wrong. The new code is not obviously right. 1861945 4 457724 darin 2022-04-15 15:44:02 -0700 Comment on attachment 457724 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457724&action=review > Source/WebCore/ChangeLog:14 > + AXIsolatedTree::updateNode is called for existing IsolatedObjects that > + are being accessed on the AX thread through their platform wrappers. > + For that reason, updateNode cannot attach a new IsolatedObject to the > + same wrapper on the main thread. Instead, the wrapper should be added to > + the NodeChange struct in order to be attached to the IsolatedObject on > + the AX thread. Can we make a regression test? 1862064 5 andresg_22 2022-04-16 11:56:14 -0700 (In reply to Darin Adler from comment #3) > Comment on attachment 457724 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457724&action=review > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:299 > > - auto change = nodeChangeForObject(axObject, parentID, true); > > + auto change = nodeChangeForObject(axObject, parentID, false); > > This is why we work so hard to avoid boolean constants. The old code is not > obviously wrong. The new code is not obviously right. Thanks Darin. We are merging this change with the patch for https://bugs.webkit.org/show_bug.cgi?id=239398, since they overlap on this code. Added: + enum class AttachWrapper : bool { OnMainThread, OnAXThread }; to make explicit what this parameter means. 1862065 6 andresg_22 2022-04-16 12:01:47 -0700 (In reply to Darin Adler from comment #4) > Comment on attachment 457724 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457724&action=review > > > Source/WebCore/ChangeLog:14 > > + AXIsolatedTree::updateNode is called for existing IsolatedObjects that > > + are being accessed on the AX thread through their platform wrappers. > > + For that reason, updateNode cannot attach a new IsolatedObject to the > > + same wrapper on the main thread. Instead, the wrapper should be added to > > + the NodeChange struct in order to be attached to the IsolatedObject on > > + the AX thread. > > Can we make a regression test? The patch in https://bugs.webkit.org/show_bug.cgi?id=239398 fixes several tests in isolated tree mode. Not sure though that any of those tests covers specifically this issue. This however goes straight to the thread safety of the isolated tree mode, even though it may be difficult to write a layout tests that fails without this change and passes with it. 1862084 7 darin 2022-04-16 15:06:42 -0700 (In reply to Andres Gonzalez from comment #6) > This however goes straight to the thread safety of > the isolated tree mode, even though it may be difficult to write a layout > tests that fails without this change and passes with it. Sure, agreed, very important! But the more *important* the fix is, the more value a regression test is. The test helps make sure that a future programmer doesn’t break this. I understand, though, if you can’t think of a good way to make a test. 457724 2022-04-15 14:14:02 -0700 2022-04-16 00:31:31 -0700 Patch bug-239402-20220415171401.patch text/plain 3121 andresg_22 U3VidmVyc2lvbiBSZXZpc2lvbjogMjkyODY3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggN2QxMDcyMjQxM2UxYWRl MTYzY2ZkNDAxYWRmYTRlOWQ1NjNlNWZjZC4uMjNjMDdiZGNjMThiYzkzYTM4ZjY1MDk5MzE3ZjAw MDJiNjllNmVjNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDIyLTA0LTE1ICBBbmRy ZXMgR29uemFsZXogIDxhbmRyZXNnXzIyQGFwcGxlLmNvbT4KKworICAgICAgICBBWElzb2xhdGVk VHJlZTo6dXBkYXRlTm9kZSBjYW5ub3QgYXR0YWNoIHRoZSBwbGF0Zm9ybSB3cmFwcGVyIHRvIHRo ZSByZXBsYWNlbWVudCBub2RlIG9uIHRoZSBtYWluIHRocmVhZC4KKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIzOTQwMgorICAgICAgICA8cmRhcjovL3By b2JsZW0vOTE4Mjc5OTI+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgQVhJc29sYXRlZFRyZWU6OnVwZGF0ZU5vZGUgaXMgY2FsbGVkIGZvciBleGlzdGlu ZyBJc29sYXRlZE9iamVjdHMgdGhhdAorICAgICAgICBhcmUgYmVpbmcgYWNjZXNzZWQgb24gdGhl IEFYIHRocmVhZCB0aHJvdWdoIHRoZWlyIHBsYXRmb3JtIHdyYXBwZXJzLgorICAgICAgICBGb3Ig dGhhdCByZWFzb24sIHVwZGF0ZU5vZGUgY2Fubm90IGF0dGFjaCBhIG5ldyBJc29sYXRlZE9iamVj dCB0byB0aGUKKyAgICAgICAgc2FtZSB3cmFwcGVyIG9uIHRoZSBtYWluIHRocmVhZC4gSW5zdGVh ZCwgdGhlIHdyYXBwZXIgc2hvdWxkIGJlIGFkZGVkIHRvCisgICAgICAgIHRoZSBOb2RlQ2hhbmdl IHN0cnVjdCBpbiBvcmRlciB0byBiZSBhdHRhY2hlZCB0byB0aGUgSXNvbGF0ZWRPYmplY3Qgb24K KyAgICAgICAgdGhlIEFYIHRocmVhZC4KKyAgICAgICAgSW4gdGhpcyBwYXRjaCBJJ20gYWxzbyBh ZGRpbmcgdGhlIHJvbGUgYXR0cmlidXRlIHRvIHRoZQorICAgICAgICBBWE9iamVjdENhY2hlOjp0 cmVlRGF0YSBtZXRob2QgZm9yIGRlYnVnZ2luZyBwdXJwb3Nlcy4KKworICAgICAgICAqIGFjY2Vz c2liaWxpdHkvQVhPYmplY3RDYWNoZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpBWE9iamVjdENh Y2hlOjp0cmVlRGF0YSk6CisgICAgICAgICogYWNjZXNzaWJpbGl0eS9pc29sYXRlZHRyZWUvQVhJ c29sYXRlZFRyZWUuY3BwOgorICAgICAgICAoV2ViQ29yZTo6QVhJc29sYXRlZFRyZWU6OnVwZGF0 ZU5vZGUpOgorCiAyMDIyLTA0LTE0ICBLYXRlIENoZW5leSAgPGthdGhlcmluZV9jaGVuZXlAYXBw bGUuY29tPgogCiAgICAgICAgIFdLV2ViVmlldzogbmF2aWdhdG9yLnNlcnZpY2VXb3JrZXIucmVn aXN0ZXIgbWV0aG9kIGZhaWxzIGZvciBhIG5ldyB2ZXJzaW9uIG9mIGFuIGFscmVhZHkgcmVnaXN0 ZXJlZCBzZXJ2aWNlIHdvcmtlci4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2FjY2Vzc2li aWxpdHkvQVhPYmplY3RDYWNoZS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L0FY T2JqZWN0Q2FjaGUuY3BwCmluZGV4IGZhMWU5NDNiNTIwZTlmMjE1NTQ3MTFmMDk3NzI4ODFiNGM1 YzgyYWIuLmIwMDdkNTE2MGI4YTEzNzM3ODYzNDgwMmM0NGRiMmRiN2RlY2NjYjYgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvQVhPYmplY3RDYWNoZS5jcHAKKysrIGIv U291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9BWE9iamVjdENhY2hlLmNwcApAQCAtMzU2NCw3 ICszNTY0LDcgQEAgQVhUcmVlRGF0YSBBWE9iamVjdENhY2hlOjp0cmVlRGF0YSgpCiAKICAgICBz dHJlYW0gPDwgIlxuQVhPYmplY3RUcmVlOlxuIjsKICAgICBpZiAoYXV0byogcm9vdCA9IGdldChk b2N1bWVudCgpLnZpZXcoKSkpIHsKLSAgICAgICAgY29uc3RleHByIE9wdGlvblNldDxBWFN0cmVh bU9wdGlvbnM+IG9wdGlvbnMgPSB7IEFYU3RyZWFtT3B0aW9uczo6T2JqZWN0SUQsIEFYU3RyZWFt T3B0aW9uczo6UGFyZW50SUQgfTsKKyAgICAgICAgY29uc3RleHByIE9wdGlvblNldDxBWFN0cmVh bU9wdGlvbnM+IG9wdGlvbnMgPSB7IEFYU3RyZWFtT3B0aW9uczo6T2JqZWN0SUQsIEFYU3RyZWFt T3B0aW9uczo6UGFyZW50SUQsIEFYU3RyZWFtT3B0aW9uczo6Um9sZSB9OwogICAgICAgICBzdHJl YW1TdWJ0cmVlKHN0cmVhbSwgcm9vdCwgb3B0aW9ucyk7CiAgICAgfSBlbHNlCiAgICAgICAgIHN0 cmVhbSA8PCAiTm8gcm9vdCEiOwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvYWNjZXNzaWJp bGl0eS9pc29sYXRlZHRyZWUvQVhJc29sYXRlZFRyZWUuY3BwIGIvU291cmNlL1dlYkNvcmUvYWNj ZXNzaWJpbGl0eS9pc29sYXRlZHRyZWUvQVhJc29sYXRlZFRyZWUuY3BwCmluZGV4IGE0NDM2MDc1 MGE2NjgyOWE0ZWMzODQ4YTU3OGM3ZWE5NzgwNjY3ZmQuLjZmMWUzYjA3YzhhMTRmMzU4ZTA3OGQ2 ZDkzMGZkNjhhYWRiYWEwNzAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxp dHkvaXNvbGF0ZWR0cmVlL0FYSXNvbGF0ZWRUcmVlLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9h Y2Nlc3NpYmlsaXR5L2lzb2xhdGVkdHJlZS9BWElzb2xhdGVkVHJlZS5jcHAKQEAgLTI5Niw3ICsy OTYsNyBAQCB2b2lkIEFYSXNvbGF0ZWRUcmVlOjp1cGRhdGVOb2RlKEFYQ29yZU9iamVjdCYgYXhP YmplY3QpCiAgICAgYXV0byogYXhQYXJlbnQgPSBheE9iamVjdC5wYXJlbnRPYmplY3RVbmlnbm9y ZWQoKTsKICAgICBBWElEIHBhcmVudElEID0gYXhQYXJlbnQgPyBheFBhcmVudC0+b2JqZWN0SUQo KSA6IEFYSUQoKTsKIAotICAgIGF1dG8gY2hhbmdlID0gbm9kZUNoYW5nZUZvck9iamVjdChheE9i amVjdCwgcGFyZW50SUQsIHRydWUpOworICAgIGF1dG8gY2hhbmdlID0gbm9kZUNoYW5nZUZvck9i amVjdChheE9iamVjdCwgcGFyZW50SUQsIGZhbHNlKTsKIAogICAgIC8vIFJlbW92ZSB0aGUgb2xk IG9iamVjdCBhbmQgc2V0IHRoZSBuZXcgb25lIHRvIGJlIHVwZGF0ZWQgb24gdGhlIEFYIHRocmVh ZC4KICAgICBMb2NrZXIgbG9ja2VyIHsgbV9jaGFuZ2VMb2dMb2NrIH07Cg==