235384 2022-01-19 14:35:29 -0800 AXIsolatedTree::updateChildren childrenIDs and children local variables could get out of sync 2022-01-20 13:04:00 -0800 1 1 1 Unclassified WebKit Accessibility WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 tyler_w tyler_w aboxhall andresg_22 apinheiro cfleizach darin dmazzoni ews-watchlist jcraig jdiggs samuel_white webkit-bug-importer oldest_to_newest 1832236 0 tyler_w 2022-01-19 14:35:29 -0800 In AXIsolatedTree::updateChildren, we have this: const auto& axChildren = axAncestor->children(); auto axChildrenIDs = axAncestor->childrenIDs(); Because the current version of AXCoreObject::childrenIDs always updates the underlying children if necessary, these two variables could get out of sync if childrenIDs actually performs an update after we already got children(). 1832237 1 webkit-bug-importer 2022-01-19 14:35:46 -0800 <rdar://problem/87791765> 1832240 2 449519 tyler_w 2022-01-19 14:42:45 -0800 Created attachment 449519 Patch 1832492 3 andresg_22 2022-01-20 10:57:12 -0800 (In reply to Tyler Wilcock from comment #2) > Created attachment 449519 [details] > Patch No need to add a bool param to ChildrenIDs since it is never used with value true. Keeping it simple, we can say that childrenIDs returns the IDs of the current children. 1832498 4 tyler_w 2022-01-20 11:09:00 -0800 (In reply to Andres Gonzalez from comment #3) > (In reply to Tyler Wilcock from comment #2) > > Created attachment 449519 [details] > > Patch > > No need to add a bool param to ChildrenIDs since it is never used with value > true. Keeping it simple, we can say that childrenIDs returns the IDs of the > current children. Well, it's used in places outside this patch in ways that do expect childrenIDs to update if necessary. For example, this: void AXIsolatedTree::updateNode(AXCoreObject& axObject) { ...truncated... auto newObject = AXIsolatedObject::create(axObject, this, parentID); newObject->m_childrenIDs = axObject.childrenIDs(); ...truncated... } I did try to make childrenIDs be non-updating always, but that causes ~10 additional ITM failures. So I opted to give childrenIDs() the same bool API that children() has and fix the definitely wrong usage of it in AXIsolatedTree::updateChildren. 1832499 5 andresg_22 2022-01-20 11:14:50 -0800 (In reply to Tyler Wilcock from comment #4) > (In reply to Andres Gonzalez from comment #3) > > (In reply to Tyler Wilcock from comment #2) > > > Created attachment 449519 [details] > > > Patch > > > > No need to add a bool param to ChildrenIDs since it is never used with value > > true. Keeping it simple, we can say that childrenIDs returns the IDs of the > > current children. > Well, it's used in places outside this patch in ways that do expect > childrenIDs to update if necessary. For example, this: > > void AXIsolatedTree::updateNode(AXCoreObject& axObject) > { > ...truncated... > auto newObject = AXIsolatedObject::create(axObject, this, parentID); > newObject->m_childrenIDs = axObject.childrenIDs(); > ...truncated... > } > > I did try to make childrenIDs be non-updating always, but that causes ~10 > additional ITM failures. So I opted to give childrenIDs() the same bool API > that children() has and fix the definitely wrong usage of it in > AXIsolatedTree::updateChildren. OK, thanks. 1832507 6 tyler_w 2022-01-20 11:18:17 -0800 Thank you both for the review! 1832519 7 ews-feeder 2022-01-20 12:03:48 -0800 Committed r288313 (246229@main): <https://commits.webkit.org/246229@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449519. 1832541 8 449519 darin 2022-01-20 13:04:00 -0800 Comment on attachment 449519 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449519&action=review > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1612 > + auto& kids = children(updateChildrenIfNecessary); It’s more elegant to use Vector::map instead of a loop and easier to get the vector size allocation right; something to consider in future. > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1614 > + childrenIDs.reserveCapacity(kids.size()); This should use reserveInitialCapacity, slightly more efficient. > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1615 > + for (const auto& child : kids) This can just use auto&, no need for the const. > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1616 > childrenIDs.append(child->objectID()); This can use the slightly more efficient uncheckedAppend because of the reserveCapacity above. 449519 2022-01-19 14:42:45 -0800 2022-01-20 12:03:50 -0800 Patch bug-235384-20220119164244.patch text/plain 4225 tyler_w U3VidmVyc2lvbiBSZXZpc2lvbjogMjg4MjE2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNTA5ZWRmYzI4YjZmZDUy MTg5OGQ4YjdjMDRlZTY2ZmY3OTMzYzM5YS4uNmQyNDM1MDIzODZiNjJhM2JjOTEzNjMyNmJhOTkw YmYxZTAzZTZjOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDMwIEBACisyMDIyLTAxLTE5ICBUeWxl ciBXaWxjb2NrICA8dHlsZXJfd0BhcHBsZS5jb20+CisKKyAgICAgICAgQVhJc29sYXRlZFRyZWU6 OnVwZGF0ZUNoaWxkcmVuIGNoaWxkcmVuSURzIGFuZCBjaGlsZHJlbiBsb2NhbCB2YXJpYWJsZXMg Y291bGQgZ2V0IG91dCBvZiBzeW5jCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0yMzUzODQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBJbiBBWElzb2xhdGVkVHJlZTo6dXBkYXRlQ2hpbGRyZW4sIHdlIGhhdmUg dGhpczoKKworICAgICAgICBjb25zdCBhdXRvJiBheENoaWxkcmVuID0gYXhBbmNlc3Rvci0+Y2hp bGRyZW4oKTsKKyAgICAgICAgYXV0byBheENoaWxkcmVuSURzID0gYXhBbmNlc3Rvci0+Y2hpbGRy ZW5JRHMoKTsKKworICAgICAgICBCZWNhdXNlIHRoZSBjdXJyZW50IHZlcnNpb24gb2YgQVhDb3Jl T2JqZWN0OjpjaGlsZHJlbklEcworICAgICAgICBhbHdheXMgdXBkYXRlcyB0aGUgdW5kZXJseWlu ZyBjaGlsZHJlbiBpZiBuZWNlc3NhcnksIHRoZXNlCisgICAgICAgIHR3byB2YXJpYWJsZXMgY291 bGQgZ2V0IG91dCBvZiBzeW5jIGlmIGNoaWxkcmVuSURzIGFjdHVhbGx5CisgICAgICAgIHBlcmZv cm1zIGFuIHVwZGF0ZSBhZnRlciB3ZSBhbHJlYWR5IGdvdCBjaGlsZHJlbigpLgorCisgICAgICAg IFRoaXMgcGF0Y2ggY2hhbmdlcyBjaGlsZHJlbklEcyB0byBoYXZlIHRoZSBzYW1lIGludGVyZmFj ZSBhcworICAgICAgICBjaGlsZHJlbigpIGJ5IGFkZGluZyBhIGBib29sIHVwZGF0ZUNoaWxkcmVu SWZOZWNlc3NhcnlgIHBhcmFtZXRlciwKKyAgICAgICAgYW5kIHVzaW5nIGNoaWxkcmVuSURzKGZh bHNlKSBpbiB0aGUgYWJvdmUgbWV0aG9kLgorCisgICAgICAgICogYWNjZXNzaWJpbGl0eS9BY2Nl c3NpYmlsaXR5T2JqZWN0SW50ZXJmYWNlLmg6CisgICAgICAgIChXZWJDb3JlOjpBWENvcmVPYmpl Y3Q6OmNoaWxkcmVuSURzKToKKyAgICAgICAgQWRkIHVwZGF0ZUNoaWxkcmVuSWZOZWNlc3Nhcnkg cGFyYW1ldGVyIHRvIG1hdGNoIEFYQ29yZU9iamVjdDo6Y2hpbGRyZW4oYm9vbCkuCisgICAgICAg ICogYWNjZXNzaWJpbGl0eS9pc29sYXRlZHRyZWUvQVhJc29sYXRlZFRyZWUuY3BwOgorICAgICAg ICAoV2ViQ29yZTo6QVhJc29sYXRlZFRyZWU6OnVwZGF0ZUNoaWxkcmVuKToKKwogMjAyMi0wMS0x OSAgUm9iIEJ1aXMgIDxyYnVpc0BpZ2FsaWEuY29tPgogCiAgICAgICAgIE51bGwgY2hlY2sgbV9w cm9ncmVzc1RyYWNrZXIgaW4gY2xlYXJQcm92aXNpb25hbExvYWQKZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvQWNjZXNzaWJpbGl0eU9iamVjdEludGVyZmFjZS5oIGIv U291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9BY2Nlc3NpYmlsaXR5T2JqZWN0SW50ZXJmYWNl LmgKaW5kZXggZDcwNjI0MjU1NjdiZjM1YzA0MjhiN2Q0N2FmNDJjODJhOTQxMDlmMy4uNDUwZTkw NDlmZTc4ODRlYmQ4MWMwMTA4YTdiNDIzMzQ4ODJkMGY4OCAxMDA2NDQKLS0tIGEvU291cmNlL1dl YkNvcmUvYWNjZXNzaWJpbGl0eS9BY2Nlc3NpYmlsaXR5T2JqZWN0SW50ZXJmYWNlLmgKKysrIGIv U291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9BY2Nlc3NpYmlsaXR5T2JqZWN0SW50ZXJmYWNl LmgKQEAgLTEyNzIsNyArMTI3Miw3IEBAIHB1YmxpYzoKICAgICB2aXJ0dWFsIHZvaWQgYWRkQ2hp bGRyZW4oKSA9IDA7CiAgICAgdmlydHVhbCB2b2lkIGFkZENoaWxkKEFYQ29yZU9iamVjdCosIERl c2NlbmRJZklnbm9yZWQgPSBEZXNjZW5kSWZJZ25vcmVkOjpZZXMpID0gMDsKICAgICB2aXJ0dWFs IHZvaWQgaW5zZXJ0Q2hpbGQoQVhDb3JlT2JqZWN0KiwgdW5zaWduZWQsIERlc2NlbmRJZklnbm9y ZWQgPSBEZXNjZW5kSWZJZ25vcmVkOjpZZXMpID0gMDsKLSAgICBWZWN0b3I8QVhJRD4gY2hpbGRy ZW5JRHMoKTsKKyAgICBWZWN0b3I8QVhJRD4gY2hpbGRyZW5JRHMoYm9vbCB1cGRhdGVDaGlsZHJl bklmTmVjZXNzYXJ5ID0gdHJ1ZSk7CiAKICAgICB2aXJ0dWFsIGJvb2wgY2FuSGF2ZUNoaWxkcmVu KCkgY29uc3QgPSAwOwogICAgIHZpcnR1YWwgdm9pZCB1cGRhdGVDaGlsZHJlbklmTmVjZXNzYXJ5 KCkgPSAwOwpAQCAtMTYwNywxMCArMTYwNywxMiBAQCBpbmxpbmUgdm9pZCBBWENvcmVPYmplY3Q6 OmRldGFjaFdyYXBwZXIoQWNjZXNzaWJpbGl0eURldGFjaG1lbnRUeXBlIGRldGFjaG1lbnRUeQog fQogI2VuZGlmCiAKLWlubGluZSBWZWN0b3I8QVhJRD4gQVhDb3JlT2JqZWN0OjpjaGlsZHJlbklE cygpCitpbmxpbmUgVmVjdG9yPEFYSUQ+IEFYQ29yZU9iamVjdDo6Y2hpbGRyZW5JRHMoYm9vbCB1 cGRhdGVDaGlsZHJlbklmTmVjZXNzYXJ5KQogeworICAgIGF1dG8mIGtpZHMgPSBjaGlsZHJlbih1 cGRhdGVDaGlsZHJlbklmTmVjZXNzYXJ5KTsKICAgICBWZWN0b3I8QVhJRD4gY2hpbGRyZW5JRHM7 Ci0gICAgZm9yIChjb25zdCBhdXRvJiBjaGlsZCA6IGNoaWxkcmVuKCkpCisgICAgY2hpbGRyZW5J RHMucmVzZXJ2ZUNhcGFjaXR5KGtpZHMuc2l6ZSgpKTsKKyAgICBmb3IgKGNvbnN0IGF1dG8mIGNo aWxkIDoga2lkcykKICAgICAgICAgY2hpbGRyZW5JRHMuYXBwZW5kKGNoaWxkLT5vYmplY3RJRCgp KTsKICAgICByZXR1cm4gY2hpbGRyZW5JRHM7CiB9CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29y ZS9hY2Nlc3NpYmlsaXR5L2lzb2xhdGVkdHJlZS9BWElzb2xhdGVkVHJlZS5jcHAgYi9Tb3VyY2Uv V2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2lzb2xhdGVkdHJlZS9BWElzb2xhdGVkVHJlZS5jcHAKaW5k ZXggYzY3MWY3MWVkYjdmN2I5ZDNlNTZmNjU3Mzg5NjIwYTc3MjUxY2FmYS4uMzczZTc5YzVjZmM4 ODA1YTRmOGY3YzNjMzBlZGQxYWEwNzE1OWFlMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUv YWNjZXNzaWJpbGl0eS9pc29sYXRlZHRyZWUvQVhJc29sYXRlZFRyZWUuY3BwCisrKyBiL1NvdXJj ZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvaXNvbGF0ZWR0cmVlL0FYSXNvbGF0ZWRUcmVlLmNwcApA QCAtMzQxLDE1ICszNDEsMTYgQEAgdm9pZCBBWElzb2xhdGVkVHJlZTo6dXBkYXRlQ2hpbGRyZW4o QVhDb3JlT2JqZWN0JiBheE9iamVjdCkKICAgICBhdXRvIHJlbW92YWxzID0gaXRlcmF0b3ItPnZh bHVlOwogCiAgICAgY29uc3QgYXV0byYgYXhDaGlsZHJlbiA9IGF4QW5jZXN0b3ItPmNoaWxkcmVu KCk7Ci0gICAgYXV0byBheENoaWxkcmVuSURzID0gYXhBbmNlc3Rvci0+Y2hpbGRyZW5JRHMoKTsK KyAgICBhdXRvIGF4Q2hpbGRyZW5JRHMgPSBheEFuY2VzdG9yLT5jaGlsZHJlbklEcyhmYWxzZSk7 CiAKICAgICBib29sIHVwZGF0ZWRDaGlsZCA9IGZhbHNlOyAvLyBTZXQgdG8gdHJ1ZSBpZiBhdCBs ZWFzdCBvbmUgY2hpbGQncyBzdWJ0cmVlIGlzIHVwZGF0ZWQuCiAgICAgZm9yIChzaXplX3QgaSA9 IDA7IGkgPCBheENoaWxkcmVuLnNpemUoKSAmJiBpIDwgYXhDaGlsZHJlbklEcy5zaXplKCk7ICsr aSkgeworICAgICAgICBBU1NFUlQoYXhDaGlsZHJlbltpXS0+b2JqZWN0SUQoKSA9PSBheENoaWxk cmVuSURzW2ldKTsKKyAgICAgICAgQVNTRVJUKGF4Q2hpbGRyZW5JRHNbaV0uaXNWYWxpZCgpKTsK ICAgICAgICAgc2l6ZV90IGluZGV4ID0gcmVtb3ZhbHMuZmluZChheENoaWxkcmVuSURzW2ldKTsK ICAgICAgICAgaWYgKGluZGV4ICE9IG5vdEZvdW5kKQogICAgICAgICAgICAgcmVtb3ZhbHMucmVt b3ZlKGluZGV4KTsKICAgICAgICAgZWxzZSB7Ci0gICAgICAgICAgICBBU1NFUlQoYXhDaGlsZHJl bltpXS0+b2JqZWN0SUQoKSA9PSBheENoaWxkcmVuSURzW2ldKTsKICAgICAgICAgICAgIC8vIFRo aXMgaXMgYSBuZXcgY2hpbGQsIGFkZCBpdCB0byB0aGUgdHJlZS4KICAgICAgICAgICAgIEFYTE9H KCJBZGRpbmcgYSBuZXcgY2hpbGQgZm9yOiIpOwogICAgICAgICAgICAgQVhMT0coYXhDaGlsZHJl bltpXSk7Cg==