122572 2013-10-09 13:46:57 -0700 AX: Crash at WebCore::accessibleNameForNode when visiting Facebook 2013-10-10 06:41:21 -0700 1 1 1 Unclassified WebKit Accessibility 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 1 cfleizach cfleizach aboxhall apinheiro commit-queue dmazzoni jdiggs mario webkit-bug-importer oldest_to_newest 938057 0 cfleizach 2013-10-09 13:46:57 -0700 > 1 com.apple.WebCore 0x10bdb025f WebCore::accessibleNameForNode(WebCore::Node*) + 0x12f (AccessibilityNodeObject.cpp:1806) 2 com.apple.WebCore 0x10bdb00e0 WebCore::AccessibilityNodeObject::accessibilityDescriptionForElements(WTF::Vector<WebCore::Element*, 0ul, WTF::CrashOnOverflow>&) const + 0x80 (AccessibilityNodeObject.cpp:1825) 3 com.apple.WebCore 0x10bdb0579 WebCore::AccessibilityNodeObject::ariaLabeledByAttribute() const + 0x49 (AccessibilityNodeObject.cpp:1877) 4 com.apple.WebCore 0x10bdae4f3 WebCore::AccessibilityNodeObject::ariaLabeledByText(WTF::Vector<WebCore::AccessibilityText, 0ul, WTF::CrashOnOverflow>&) const + 0x43 (AccessibilityNodeObject.cpp:1379) 5 com.apple.WebCore 0x10bdadf25 WebCore::AccessibilityNodeObject::alternativeText(WTF::Vector<WebCore::AccessibilityText, 0ul, WTF::CrashOnOverflow>&) const + 0xb5 (AccessibilityNodeObject.cpp:1236) 6 com.apple.WebCore 0x10bdaed03 WebCore::AccessibilityNodeObject::accessibilityText(WTF::Vector<WebCore::AccessibilityText, 0ul, WTF::CrashOnOverflow>&) + 0x43 (AccessibilityNodeObject.cpp:1368) 7 com.apple.WebCore 0x10d7725fd -[WebAccessibilityObjectWrapperBase accessibilityDescription] + 0xbd (WebAccessibilityObjectWrapperBase.mm:198) 8 com.apple.WebCore 0x10d77d195 -[WebAccessibilityObjectWrapper accessibilityAttributeValue:] + 0x1625 (WebAccessibilityObjectWrapperMac.mm:2177) 9 com.apple.AppKit 0x7fff8884feb1 -[NSObject(NSAccessibilityInternal) _accessibilityValueForAttribute:clientError:] + 0xf2 10 com.apple.AppKit 0x7fff88853f7d <rdar://problem/15189506> 938058 1 cfleizach 2013-10-09 13:47:32 -0700 Looks like they're probably doing something like <button aria-labeledby="item"> <div id="item" style="display:none">data</div> 938082 2 213818 cfleizach 2013-10-09 14:42:37 -0700 Created attachment 213818 patch 938262 3 213818 mario 2013-10-10 03:29:29 -0700 Comment on attachment 213818 patch View in context: https://bugs.webkit.org/attachment.cgi?id=213818&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1814 > - String text = node->document().axObjectCache()->getOrCreate(node)->textUnderElement(); > + // If the node can be turned into an AX object, we can use standard name computation rules. > + // If however, the node cannot (because there's no renderer e.g.) fallback to using the basic text underneath. > + AccessibilityObject* axObject = node->document().axObjectCache()->getOrCreate(node); > + String text; > + if (axObject) > + text = axObject->textUnderElement(); > + else if (node->isElementNode()) > + text = toElement(node)->innerText(); > + I wonder if we could fix this issue in a different way, by making AXObjectCache::getOrCreate(Node* node) able to handle this kind of situation, creating a AccessibilityNodeObject that wraps that not rendered node inside. What do you think? 938303 4 cfleizach 2013-10-10 05:56:54 -0700 (In reply to comment #3) > (From update of attachment 213818 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=213818&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1814 > > - String text = node->document().axObjectCache()->getOrCreate(node)->textUnderElement(); > > + // If the node can be turned into an AX object, we can use standard name computation rules. > > + // If however, the node cannot (because there's no renderer e.g.) fallback to using the basic text underneath. > > + AccessibilityObject* axObject = node->document().axObjectCache()->getOrCreate(node); > > + String text; > > + if (axObject) > > + text = axObject->textUnderElement(); > > + else if (node->isElementNode()) > > + text = toElement(node)->innerText(); > > + > > I wonder if we could fix this issue in a different way, by making AXObjectCache::getOrCreate(Node* node) able to handle this kind of situation, creating a AccessibilityNodeObject that wraps that not rendered node inside. > > What do you think? Certainly getOrCreate could return an object, but there's a bit of code to ensure we only return a AXNode based object under certain conditions (either in a canvas or aria-hidden=false). Hard for me to say if there's any negative consequence of doing that. It might allow more elements into the tree than we would like... For example, we definitely don't want to add objects that are just hidden to the tree, as in <div style="display:none;"> sub tree here </div> So it could be done, but I am less sure of the consequences of doing that right now. Thoughts 938306 5 213818 mario 2013-10-10 06:04:39 -0700 Comment on attachment 213818 patch (In reply to comment #4) > > I wonder if we could fix this issue in a different way, by making > > AXObjectCache::getOrCreate(Node* node) able to handle this kind of situation, > > creating a AccessibilityNodeObject that wraps that not rendered node inside. > > > > What do you think? > > Certainly getOrCreate could return an object, but there's a bit of code > to ensure we only return a AXNode based object under certain conditions > (either in a canvas or aria-hidden=false). Hard for me to say if there's > any negative consequence of doing that. It might allow more elements > into the tree than we would like... > Yes, I've seen that code and it's not clear to me either. That's why I ask. > For example, we definitely don't want to add objects that are just hidden > to the tree, as in <div style="display:none;"> sub tree here </div> > Agreed. > So it could be done, but I am less sure of the consequences of doing that > right now. > Fair enough. Let's give this a try then :) 938317 6 213818 commit-queue 2013-10-10 06:41:19 -0700 Comment on attachment 213818 patch Clearing flags on attachment: 213818 Committed r157221: <http://trac.webkit.org/changeset/157221> 938318 7 commit-queue 2013-10-10 06:41:21 -0700 All reviewed patches have been landed. Closing bug. 213818 2013-10-09 14:42:37 -0700 2013-10-10 06:41:19 -0700 patch patch text/plain 4226 cfleizach SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE1NzE4NSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDEzLTEwLTA5ICBDaHJpcyBG bGVpemFjaCAgPGNmbGVpemFjaEBhcHBsZS5jb20+CisKKyAgICAgICAgQVg6IENyYXNoIGF0IFdl YkNvcmU6OmFjY2Vzc2libGVOYW1lRm9yTm9kZSB3aGVuIHZpc2l0aW5nIEZhY2Vib29rCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjI1NzIKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUZXN0OiBhY2Nlc3Np YmlsaXR5L2FyaWEtbGFiZWxlZC13aXRoLWhpZGRlbi1ub2RlLmh0bWwKKworICAgICAgICBIYW5k bGUgdGhlIGNhc2Ugd2hlcmUgYXJpYS1sYWJlbGxlZGJ5IHJlZmVyZW5jZXMgYSBub24tcmVuZGVy ZWQgbm9kZS4KKworICAgICAgICAqIGFjY2Vzc2liaWxpdHkvQWNjZXNzaWJpbGl0eU5vZGVPYmpl Y3QuY3BwOgorICAgICAgICAoV2ViQ29yZTo6YWNjZXNzaWJsZU5hbWVGb3JOb2RlKToKKwogMjAx My0xMC0wOSAgU2VyZ2lvIENvcnJlaWEgIDxzZXJnaW8uY29ycmVpYUBvcGVuYm9zc2Eub3JnPgog CiAgICAgICAgIFtDb29yZGluYXRlZEdyYXBoaWNzXSBBU1NFUlRJT04gRkFJTEVEOiAhbV9mbHVz aGluZ0xheWVycyAoYWZ0ZXIgcjE1NjI5MSkKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2FjY2Vzc2li aWxpdHkvQWNjZXNzaWJpbGl0eU5vZGVPYmplY3QuY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9X ZWJDb3JlL2FjY2Vzc2liaWxpdHkvQWNjZXNzaWJpbGl0eU5vZGVPYmplY3QuY3BwCShyZXZpc2lv biAxNTcxODMpCisrKyBTb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L0FjY2Vzc2liaWxpdHlO b2RlT2JqZWN0LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTgwMyw3ICsxODAzLDE1IEBACiAgICAg aWYgKGlzSFRNTElucHV0RWxlbWVudChub2RlKSkKICAgICAgICAgcmV0dXJuIHRvSFRNTElucHV0 RWxlbWVudChub2RlKS0+dmFsdWUoKTsKICAgICAKLSAgICBTdHJpbmcgdGV4dCA9IG5vZGUtPmRv Y3VtZW50KCkuYXhPYmplY3RDYWNoZSgpLT5nZXRPckNyZWF0ZShub2RlKS0+dGV4dFVuZGVyRWxl bWVudCgpOworICAgIC8vIElmIHRoZSBub2RlIGNhbiBiZSB0dXJuZWQgaW50byBhbiBBWCBvYmpl Y3QsIHdlIGNhbiB1c2Ugc3RhbmRhcmQgbmFtZSBjb21wdXRhdGlvbiBydWxlcy4KKyAgICAvLyBJ ZiBob3dldmVyLCB0aGUgbm9kZSBjYW5ub3QgKGJlY2F1c2UgdGhlcmUncyBubyByZW5kZXJlciBl LmcuKSBmYWxsYmFjayB0byB1c2luZyB0aGUgYmFzaWMgdGV4dCB1bmRlcm5lYXRoLgorICAgIEFj Y2Vzc2liaWxpdHlPYmplY3QqIGF4T2JqZWN0ID0gbm9kZS0+ZG9jdW1lbnQoKS5heE9iamVjdENh Y2hlKCktPmdldE9yQ3JlYXRlKG5vZGUpOworICAgIFN0cmluZyB0ZXh0OworICAgIGlmIChheE9i amVjdCkKKyAgICAgICAgdGV4dCA9IGF4T2JqZWN0LT50ZXh0VW5kZXJFbGVtZW50KCk7CisgICAg ZWxzZSBpZiAobm9kZS0+aXNFbGVtZW50Tm9kZSgpKQorICAgICAgICB0ZXh0ID0gdG9FbGVtZW50 KG5vZGUpLT5pbm5lclRleHQoKTsKKyAgICAKICAgICBpZiAoIXRleHQuaXNFbXB0eSgpKQogICAg ICAgICByZXR1cm4gdGV4dDsKICAgICAKSW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHJldmlzaW9uIDE1NzE4NSkKKysrIExh eW91dFRlc3RzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSw1ICsxLDE1IEBACiAyMDEz LTEwLTA5ICBDaHJpcyBGbGVpemFjaCAgPGNmbGVpemFjaEBhcHBsZS5jb20+CiAKKyAgICAgICAg QVg6IENyYXNoIGF0IFdlYkNvcmU6OmFjY2Vzc2libGVOYW1lRm9yTm9kZSB3aGVuIHZpc2l0aW5n IEZhY2Vib29rCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMjI1NzIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICAqIGFjY2Vzc2liaWxpdHkvYXJpYS1sYWJlbGVkLXdpdGgtaGlkZGVuLW5vZGUtZXhwZWN0ZWQu dHh0OiBBZGRlZC4KKyAgICAgICAgKiBhY2Nlc3NpYmlsaXR5L2FyaWEtbGFiZWxlZC13aXRoLWhp ZGRlbi1ub2RlLmh0bWw6IEFkZGVkLgorCisyMDEzLTEwLTA5ICBDaHJpcyBGbGVpemFjaCAgPGNm bGVpemFjaEBhcHBsZS5jb20+CisKICAgICAgICAgQVg6IFZvaWNlT3ZlciBkb3VibGUgc3BlYWtz IGFsbCBpdGVtcyBpbiA8bGk+IG5vZGVzCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xMjI1NjQKIApJbmRleDogTGF5b3V0VGVzdHMvYWNjZXNzaWJpbGl0 eS9hcmlhLWxhYmVsZWQtd2l0aC1oaWRkZW4tbm9kZS1leHBlY3RlZC50eHQKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gTGF5b3V0VGVzdHMvYWNjZXNzaWJpbGl0eS9hcmlhLWxhYmVsZWQtd2l0aC1oaWRkZW4tbm9k ZS1leHBlY3RlZC50eHQJKHJldmlzaW9uIDApCisrKyBMYXlvdXRUZXN0cy9hY2Nlc3NpYmlsaXR5 L2FyaWEtbGFiZWxlZC13aXRoLWhpZGRlbi1ub2RlLWV4cGVjdGVkLnR4dAkod29ya2luZyBjb3B5 KQpAQCAtMCwwICsxLDExIEBACitHbworVGhpcyB0ZXN0cyB0aGF0IGlmIHRyeSB0byB1c2UgYSBu b24tcmVuZGVyZWQgbm9kZSBhcyB5b3VyIGxhYmVsbGVkYnkgZWxlbWVudCwgaXQgd29uJ3QgY3Jh c2ggYW5kIGl0IHdpbGwgd29yay4KKworT24gc3VjY2VzcywgeW91IHdpbGwgc2VlIGEgc2VyaWVz IG9mICJQQVNTIiBtZXNzYWdlcywgZm9sbG93ZWQgYnkgIlRFU1QgQ09NUExFVEUiLgorCisKK1BB U1MgYnV0dG9uLmRlc2NyaXB0aW9uIGlzICdBWERlc2NyaXB0aW9uOiBUSVRMRScKK1BBU1Mgc3Vj Y2Vzc2Z1bGx5UGFyc2VkIGlzIHRydWUKKworVEVTVCBDT01QTEVURQorCkluZGV4OiBMYXlvdXRU ZXN0cy9hY2Nlc3NpYmlsaXR5L2FyaWEtbGFiZWxlZC13aXRoLWhpZGRlbi1ub2RlLmh0bWwKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gTGF5b3V0VGVzdHMvYWNjZXNzaWJpbGl0eS9hcmlhLWxhYmVsZWQtd2l0aC1o aWRkZW4tbm9kZS5odG1sCShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMvYWNjZXNzaWJpbGl0 eS9hcmlhLWxhYmVsZWQtd2l0aC1oaWRkZW4tbm9kZS5odG1sCSh3b3JraW5nIGNvcHkpCkBAIC0w LDAgKzEsMjYgQEAKKzwhRE9DVFlQRSBodG1sPgorPGh0bWw+Cis8dGl0bGUgaWQ9InRpdGxlIj5U SVRMRTwvdGl0bGU+Cis8aGVhZD4KKzxzY3JpcHQgc3JjPSIuLi9yZXNvdXJjZXMvanMtdGVzdC1w cmUuanMiPjwvc2NyaXB0PgorPC9oZWFkPgorPGJvZHk+CisKKzxidXR0b24gaWQ9ImJ1dHRvbiIg YXJpYS1sYWJlbGxlZGJ5PSJ0aXRsZSI+R288L2J1dHRvbj4KKworCis8cCBpZD0iZGVzY3JpcHRp b24iPjwvcD4KKzxkaXYgaWQ9ImNvbnNvbGUiPjwvZGl2PgorCis8c2NyaXB0PgorICAgIGRlc2Ny aXB0aW9uKCJUaGlzIHRlc3RzIHRoYXQgaWYgdHJ5IHRvIHVzZSBhIG5vbi1yZW5kZXJlZCBub2Rl IGFzIHlvdXIgbGFiZWxsZWRieSBlbGVtZW50LCBpdCB3b24ndCBjcmFzaCBhbmQgaXQgd2lsbCB3 b3JrLiIpOworICAgIAorICAgIGlmICh3aW5kb3cuYWNjZXNzaWJpbGl0eUNvbnRyb2xsZXIpIHsK KyAgICAgICB2YXIgYnV0dG9uID0gYWNjZXNzaWJpbGl0eUNvbnRyb2xsZXIuYWNjZXNzaWJsZUVs ZW1lbnRCeUlkKCJidXR0b24iKTsKKyAgICAgICBzaG91bGRCZSgiYnV0dG9uLmRlc2NyaXB0aW9u IiwgIidBWERlc2NyaXB0aW9uOiBUSVRMRSciKTsKKyAgICB9Cis8L3NjcmlwdD4KKworPHNjcmlw dCBzcmM9Ii4uL3Jlc291cmNlcy9qcy10ZXN0LXBvc3QuanMiPjwvc2NyaXB0PgorPC9ib2R5Pgor PC9odG1sPgo=