241650 2022-06-15 14:47:35 -0700 AX: AccessibilityObject::insertChild does not check the validity of the insertionIndex while processing grandchildren 2022-06-16 17:21:18 -0700 1 1 1 Unclassified WebKit Accessibility WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 tyler_w tyler_w aboxhall andresg_22 apinheiro cfleizach dmazzoni ews-watchlist jcraig jdiggs samuel_white webkit-bug-importer oldest_to_newest 1876087 0 tyler_w 2022-06-15 14:47:35 -0700 This can cause crashes. 1876088 1 webkit-bug-importer 2022-06-15 14:47:42 -0700 <rdar://problem/95240529> 1876093 2 460262 tyler_w 2022-06-15 15:03:03 -0700 Created attachment 460262 Patch 1876112 3 tyler_w 2022-06-15 16:27:10 -0700 rdar://94895437 1876137 4 460262 cfleizach 2022-06-15 17:48:12 -0700 Comment on attachment 460262 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=460262&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:636 > + if (insertionIndex > m_children.size()) do we want to insert this at position 0 in this case? or discard completely? 1876162 5 tyler_w 2022-06-15 18:52:39 -0700 (In reply to chris fleizach from comment #4) > Comment on attachment 460262 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=460262&action=review > > > Source/WebCore/accessibility/AccessibilityObject.cpp:636 > > + if (insertionIndex > m_children.size()) > > do we want to insert this at position 0 in this case? or discard completely? I think we want to discard this grandchild (and any following) entirely, since they may no longer be the right children after the layout that caused m_children to be cleared. Also, when this happens, m_childrenInitialized should always become false (it does in the crash I was chasing down), meaning we will add the actually-correct children in the next call to children(true). 1876234 6 andresg_22 2022-06-16 07:57:33 -0700 (In reply to Tyler Wilcock from comment #2) > Created attachment 460262 [details] > Patch It seems that what we are doing in this whole block is to insert the grandchildren if the newChild is ignored. Would it make it clearer if add a method called insertchildren(const Vector&, size_t index), and then we could write in the body of this block: if (descendIfIgnored == DescendIfIgnored::Yes && child->accessibilityIsIgnored()) insertChildren(child->children(), index); 1876277 7 tyler_w 2022-06-16 10:51:58 -0700 (In reply to Andres Gonzalez from comment #6) > (In reply to Tyler Wilcock from comment #2) > > Created attachment 460262 [details] > > Patch > > It seems that what we are doing in this whole block is to insert the > grandchildren if the newChild is ignored. Would it make it clearer if add a > method called insertchildren(const Vector&, size_t index), and then we could > write in the body of this block: > if (descendIfIgnored == DescendIfIgnored::Yes > && child->accessibilityIsIgnored()) > insertChildren(child->children(), index); I tried this out and unfortunately we'd also need to pass a third AXAncestorFlags parameter (to capture the flags of the ignored object), which IMO makes this refactor not worth it. 1876300 8 andresg_22 2022-06-16 12:31:09 -0700 (In reply to Tyler Wilcock from comment #7) > (In reply to Andres Gonzalez from comment #6) > > (In reply to Tyler Wilcock from comment #2) > > > Created attachment 460262 [details] > > > Patch > > > > It seems that what we are doing in this whole block is to insert the > > grandchildren if the newChild is ignored. Would it make it clearer if add a > > method called insertchildren(const Vector&, size_t index), and then we could > > write in the body of this block: > > if (descendIfIgnored == DescendIfIgnored::Yes > > && child->accessibilityIsIgnored()) > > insertChildren(child->children(), index); > I tried this out and unfortunately we'd also need to pass a third > AXAncestorFlags parameter (to capture the flags of the ignored object), > which IMO makes this refactor not worth it. No, you can do auto ancestorFlags = computeAncestorFlags(); inside insertObjects(...), no need to pass it as a param. Changed the name from insertchildren to insertObjects because in this case you would be inserting the grandChildren. I believe it would make the code maintainable. 1876304 9 andresg_22 2022-06-16 12:40:36 -0700 (In reply to Andres Gonzalez from comment #8) > (In reply to Tyler Wilcock from comment #7) > > (In reply to Andres Gonzalez from comment #6) > > > (In reply to Tyler Wilcock from comment #2) > > > > Created attachment 460262 [details] > > > > Patch > > > > > > It seems that what we are doing in this whole block is to insert the > > > grandchildren if the newChild is ignored. Would it make it clearer if add a > > > method called insertchildren(const Vector&, size_t index), and then we could > > > write in the body of this block: > > > if (descendIfIgnored == DescendIfIgnored::Yes > > > && child->accessibilityIsIgnored()) > > > insertChildren(child->children(), index); > > I tried this out and unfortunately we'd also need to pass a third > > AXAncestorFlags parameter (to capture the flags of the ignored object), > > which IMO makes this refactor not worth it. > > No, you can do > > auto ancestorFlags = computeAncestorFlags(); > > inside insertObjects(...), no need to pass it as a param. Changed the name > from insertchildren to insertObjects because in this case you would be > inserting the grandChildren. I believe it would make the code maintainable. The name insertChildren is fine, thought for a moment that insertObjects was better but it is not, since you are inserting children that in this case happen to be grandchildren. 1876358 10 tyler_w 2022-06-16 16:44:22 -0700 (In reply to Andres Gonzalez from comment #8) > (In reply to Tyler Wilcock from comment #7) > > (In reply to Andres Gonzalez from comment #6) > > > (In reply to Tyler Wilcock from comment #2) > > > > Created attachment 460262 [details] > > > > Patch > > > > > > It seems that what we are doing in this whole block is to insert the > > > grandchildren if the newChild is ignored. Would it make it clearer if add a > > > method called insertchildren(const Vector&, size_t index), and then we could > > > write in the body of this block: > > > if (descendIfIgnored == DescendIfIgnored::Yes > > > && child->accessibilityIsIgnored()) > > > insertChildren(child->children(), index); > > I tried this out and unfortunately we'd also need to pass a third > > AXAncestorFlags parameter (to capture the flags of the ignored object), > > which IMO makes this refactor not worth it. > > No, you can do > > auto ancestorFlags = computeAncestorFlags(); > > inside insertObjects(...), no need to pass it as a param. Changed the name > from insertchildren to insertObjects because in this case you would be > inserting the grandChildren. I believe it would make the code maintainable. The current behavior is to use the ancestor flags of the ignored child rather than `this` as you suggest. Using `this` ancestor flags might be OK...but I don't want to make that behavior change in this patch. Let's address this refactor in a separate patch. 1876372 11 ews-feeder 2022-06-16 17:21:16 -0700 Committed r295618 (251623@main): <https://commits.webkit.org/251623@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 460262. 460262 2022-06-15 15:03:03 -0700 2022-06-16 17:21:17 -0700 Patch bug-241650-20220615150303.patch text/plain 2233 tyler_w RnJvbSBlYWFlMDYyMDJjMmVkMTcwM2VlMjI1YzBkZTk3OTdhYjE0MWEzMjU2IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBUeWxlciBXaWxjb2NrIDx0eWxlcl93QGFwcGxlLmNvbT4KRGF0 ZTogV2VkLCAxNSBKdW4gMjAyMiAxNDo0Nzo1MCAtMDcwMApTdWJqZWN0OiBbUEFUQ0hdIEFYOiBB Y2Nlc3NpYmlsaXR5T2JqZWN0OjppbnNlcnRDaGlsZCBkb2VzIG5vdCBjaGVjayB0aGUKIHZhbGlk aXR5IG9mIHRoZSBpbnNlcnRpb25JbmRleCB3aGlsZSBwcm9jZXNzaW5nIGdyYW5kY2hpbGRyZW4K IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNDE2NTAKClJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgoKV2hlbiBBY2Nlc3NpYmlsaXR5T2JqZWN0OjppbnNlcnRDaGls ZCBpcyBhc2tlZCB0byBpbnNlcnQgYSBjaGlsZCB0aGF0J3MKaWdub3JlZCwgd2UgaW5zdGVhZCBh ZGQgdGhhdCBvYmplY3QncyBjaGlsZHJlbi4gSG93ZXZlciwgYm90aApgYWNjZXNzaWJpbGl0eUlz SWdub3JlZGAgYW5kIGBjaGlsZHJlbmAgY2FuIGNhdXNlIGxheW91dCwgYW5kIHNhaWQKbGF5b3V0 IGNvdWxkIGNhdXNlIEFjY2Vzc2liaWxpdHlPYmplY3Q6Om1fY2hpbGRyZW4gdG8gYmUgY2xlYXJl ZC4gVGhpcwptYWtlcyB0aGUgYGluc2VydGlvbkluZGV4YCBpbnZhbGlkLCB3aGljaCBjYXVzZXMg YSBjcmFzaC4KCkluIHRoaXMgcGF0Y2gsIHJpZ2h0IGJlZm9yZSBtX2NoaWxkcmVuLmluc2VydCgp LCB3ZSBjaGVjayB0byBtYWtlIHN1cmUKdGhlIGluZGV4IGlzIHN0aWxsIHZhbGlkLgoKSSB3YXNu J3QgYWJsZSB0byBtYWtlIGEgdGVzdCBmb3IgdGhpcyBidWcuIEl0IGlzIGRpZmZpY3VsdCB0byBy ZXByb2R1Y2UsCmFuZCB0aGUgY2lyY3Vtc3RhbmNlcyB0byByZXByb2R1Y2UgYXJlIGNvbXBsZXgu CgoqIFNvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvQWNjZXNzaWJpbGl0eU9iamVjdC5jcHA6 CihXZWJDb3JlOjpBY2Nlc3NpYmlsaXR5T2JqZWN0OjppbnNlcnRDaGlsZCk6Ci0tLQogU291cmNl L1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9BY2Nlc3NpYmlsaXR5T2JqZWN0LmNwcCB8IDMgKysrCiAx IGZpbGUgY2hhbmdlZCwgMyBpbnNlcnRpb25zKCspCgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNv cmUvYWNjZXNzaWJpbGl0eS9BY2Nlc3NpYmlsaXR5T2JqZWN0LmNwcCBiL1NvdXJjZS9XZWJDb3Jl L2FjY2Vzc2liaWxpdHkvQWNjZXNzaWJpbGl0eU9iamVjdC5jcHAKaW5kZXggODljYmYzZWI0NzI0 Li4xZTdmMDkwMDBjMGMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkv QWNjZXNzaWJpbGl0eU9iamVjdC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0 eS9BY2Nlc3NpYmlsaXR5T2JqZWN0LmNwcApAQCAtNjMyLDYgKzYzMiw5IEBAIHZvaWQgQWNjZXNz aWJpbGl0eU9iamVjdDo6aW5zZXJ0Q2hpbGQoQVhDb3JlT2JqZWN0KiBuZXdDaGlsZCwgdW5zaWdu ZWQgaW5kZXgsIERlCiAgICAgICAgICAgICAgICAgICAgIC8vIEV2ZW4gdGhvdWdoIGBjaGlsZGAg aXMgaWdub3JlZCwgd2Ugc3RpbGwgbmVlZCB0byBzZXQgYW5jZXN0cnkgZmxhZ3MgYmFzZWQgb24g aXQuCiAgICAgICAgICAgICAgICAgICAgIGdyYW5kY2hpbGQtPmluaXRpYWxpemVBbmNlc3RvckZs YWdzKGNoaWxkQW5jZXN0b3JGbGFncyk7CiAgICAgICAgICAgICAgICAgICAgIGdyYW5kY2hpbGQt PmFkZEFuY2VzdG9yRmxhZ3ModGhpc0FuY2VzdG9yRmxhZ3MpOworICAgICAgICAgICAgICAgICAg ICAvLyBDYWxscyB0byBgY2hpbGQtPmFjY2Vzc2liaWxpdHlJc0lnbm9yZWQoKWAgb3IgYGNoaWxk LT5jaGlsZHJlbigpYCBjYW4gY2F1c2UgbGF5b3V0LCB3aGljaCBpbiB0dXJuIGNhbiBjYXVzZSB0 aGlzIG9iamVjdCB0byBjbGVhciBpdHMgbV9jaGlsZHJlbi4gVGhpcyBjYW4gY2F1c2UgYGluc2Vy dGlvbkluZGV4YCB0byBubyBsb25nZXIgYmUgdmFsaWQuIERldGVjdCB0aGlzIGFuZCBicmVhayBl YXJseSBpZiBuZWNlc3NhcnkuCisgICAgICAgICAgICAgICAgICAgIGlmIChpbnNlcnRpb25JbmRl eCA+IG1fY2hpbGRyZW4uc2l6ZSgpKQorICAgICAgICAgICAgICAgICAgICAgICAgYnJlYWs7CiAg ICAgICAgICAgICAgICAgICAgIG1fY2hpbGRyZW4uaW5zZXJ0KGluc2VydGlvbkluZGV4LCBncmFu ZGNoaWxkKTsKICAgICAgICAgICAgICAgICAgICAgKytpbnNlcnRpb25JbmRleDsKICAgICAgICAg ICAgICAgICB9Cg==