58429 2011-04-13 04:13:26 -0700 [GTK] Missing nullchecks in GTK's a11y wrapper 2011-04-13 09:27:40 -0700 1 1 1 Unclassified WebKit Accessibility 528+ (Nightly build) PC Linux RESOLVED FIXED Gtk P2 Normal --- 1 mario webkit-unassigned mrobinson oldest_to_newest 384783 0 mario 2011-04-13 04:13:26 -0700 There are several places in the AccessibilityObjectWrapperAtk.cpp file where we are not null-checking the return of calling to coreObject->document() (coreObject as an instance of AccessibilityObject) and we're using it rightaway, assuming it won't ever be NULL, even though 'experience' tells us it can happen (I've observed some crashes recently because of that) Hence, it would be good to add some extra checks to prevent these situations. 384784 1 89363 mario 2011-04-13 04:18:55 -0700 Created attachment 89363 Patch proposal Attached patch proposal. No need to attach a test since it's just about adding some extra nullchecks to avoid potential problems. Other than that, it keeps passing all API and Layout tests up to date. 384903 2 89363 mrobinson 2011-04-13 08:22:48 -0700 Comment on attachment 89363 Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=89363&action=review Looks good, but consider my comments before landing. > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1560 > + Document* document = coreObject->document(); > switch(coords) { > case ATK_XY_SCREEN: > - extents = coreObject->document()->view()->contentsToScreen(extents); > + if (document) > + extents = document->view()->contentsToScreen(extents); > break; If you only use "document" in this if block you should probably do: if (Document* document = coreObject->document()) extents = document->view()->contentsToScreen(extents); and remove the first declaration. > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1836 > + Document* document = coreObject->document(); > + if (!document) > + return; > + > + if (!document->frame()) > return; Can't this just be: if (!document || !document->frame()) return; > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1867 > + Document* document = coreObject->document(); > + if (!document) > return; > + > + if (!document->frame()) > + return; > + Ditto. 384943 3 mario 2011-04-13 09:27:40 -0700 Committed r83746: <http://trac.webkit.org/changeset/83746> 89363 2011-04-13 04:18:55 -0700 2011-04-13 08:22:48 -0700 Patch proposal 0001-2011-04-13-Mario-Sanchez-Prada-msanchez-igalia.com.patch text/plain 6633 mario RnJvbSAyN2JiOWQyNmQ5MDEyOWJjOWIzMzAyMjUzMjExMzE1OWI4OTYwYTRkIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXJpbyBTYW5jaGV6IFByYWRhIDxtc2FuY2hlekBpZ2FsaWEu Y29tPgpEYXRlOiBXZWQsIDEzIEFwciAyMDExIDEzOjEzOjM4ICswMjAwClN1YmplY3Q6IFtQQVRD SF0gMjAxMS0wNC0xMyAgTWFyaW8gU2FuY2hleiBQcmFkYSAgPG1zYW5jaGV6QGlnYWxpYS5jb20+ CgogICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgoKICAgICAgICBbR1RLXSBNaXNz aW5nIG51bGxjaGVja3MgaW4gR1RLJ3MgYTExeSB3cmFwcGVyCiAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTU4NDI5CgogICAgICAgIEFkZCBtaXNzaW5nIG51 bGxjaGVja3MgZm9yIGNvcmVPYmplY3QtPmRvY3VtZW50KCkuCgogICAgICAgICogYWNjZXNzaWJp bGl0eS9ndGsvQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJBdGsuY3BwOgogICAgICAgIChhdGtQ YXJlbnRPZlJvb3RPYmplY3QpOiBBZGQgbWlzc2luZyBudWxsY2hlY2suCiAgICAgICAgKGdldFBh bmdvTGF5b3V0Rm9yQXRrKTogRGl0dG8uCiAgICAgICAgKHdlYmtpdF9hY2Nlc3NpYmxlX3RleHRf Z2V0X2NhcmV0X29mZnNldCk6IERpdHRvLgogICAgICAgICh0ZXh0RXh0ZW50cyk6IERpdHRvLgog ICAgICAgICh3ZWJraXRfYWNjZXNzaWJsZV9lZGl0YWJsZV90ZXh0X2luc2VydF90ZXh0KTogRGl0 dG8uCiAgICAgICAgKHdlYmtpdF9hY2Nlc3NpYmxlX2VkaXRhYmxlX3RleHRfZGVsZXRlX3RleHQp OiBEaXR0by4KLS0tCiBTb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cgICAgICAgICAgICAgICAgICAg ICAgICAgICB8ICAgMTcgKysrKysrKysrCiAuLi4vZ3RrL0FjY2Vzc2liaWxpdHlPYmplY3RXcmFw cGVyQXRrLmNwcCAgICAgICAgICB8ICAgMzkgKysrKysrKysrKysrKysrKy0tLS0KIDIgZmlsZXMg Y2hhbmdlZCwgNDggaW5zZXJ0aW9ucygrKSwgOCBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg ZjA2MGYzMC4uOWJlMTFiYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisr KyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMiw2ICsyLDIzIEBACiAKICAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCiAKKyAgICAgICAgW0dUS10gTWlzc2luZyBudWxs Y2hlY2tzIGluIEdUSydzIGExMXkgd3JhcHBlcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTg0MjkKKworICAgICAgICBBZGQgbWlzc2luZyBudWxsY2hl Y2tzIGZvciBjb3JlT2JqZWN0LT5kb2N1bWVudCgpLgorCisgICAgICAgICogYWNjZXNzaWJpbGl0 eS9ndGsvQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJBdGsuY3BwOgorICAgICAgICAoYXRrUGFy ZW50T2ZSb290T2JqZWN0KTogQWRkIG1pc3NpbmcgbnVsbGNoZWNrLgorICAgICAgICAoZ2V0UGFu Z29MYXlvdXRGb3JBdGspOiBEaXR0by4KKyAgICAgICAgKHdlYmtpdF9hY2Nlc3NpYmxlX3RleHRf Z2V0X2NhcmV0X29mZnNldCk6IERpdHRvLgorICAgICAgICAodGV4dEV4dGVudHMpOiBEaXR0by4K KyAgICAgICAgKHdlYmtpdF9hY2Nlc3NpYmxlX2VkaXRhYmxlX3RleHRfaW5zZXJ0X3RleHQpOiBE aXR0by4KKyAgICAgICAgKHdlYmtpdF9hY2Nlc3NpYmxlX2VkaXRhYmxlX3RleHRfZGVsZXRlX3Rl eHQpOiBEaXR0by4KKworMjAxMS0wNC0xMyAgTWFyaW8gU2FuY2hleiBQcmFkYSAgPG1zYW5jaGV6 QGlnYWxpYS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKICAg ICAgICAgQ29uc2lkZXIgcGFkZGluZyBhbmQgYm9yZGVyIHdoZW4gbG9va2luZyBmb3IgdGhlIG5l eHQvcHJldmlvdXMgbGluZSBwb3NpdGlvbgogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9NTU0ODEKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvYWNj ZXNzaWJpbGl0eS9ndGsvQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJBdGsuY3BwIGIvU291cmNl L1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9ndGsvQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJBdGsu Y3BwCmluZGV4IDQyYTg0NTYuLmQwZDgyNzggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2Fj Y2Vzc2liaWxpdHkvZ3RrL0FjY2Vzc2liaWxpdHlPYmplY3RXcmFwcGVyQXRrLmNwcAorKysgYi9T b3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2d0ay9BY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBl ckF0ay5jcHAKQEAgLTI2OSw3ICsyNjksMTEgQEAgc3RhdGljIEF0a09iamVjdCogYXRrUGFyZW50 T2ZSb290T2JqZWN0KEF0a09iamVjdCogb2JqZWN0KQogICAgIC8vIGltcG9zc2libGUgZm9yIGFz c2lzdGl2ZSB0ZWNobm9sb2dpZXMgdG8gYXNjZW5kIHRoZSBhY2Nlc3NpYmxlCiAgICAgLy8gaGll cmFyY2h5IGFsbCB0aGUgd2F5IHRvIHRoZSBhcHBsaWNhdGlvbi4gKEJ1ZyAzMDQ4OSkKICAgICBp ZiAoIWNvcmVQYXJlbnQgJiYgaXNSb290T2JqZWN0KGNvcmVPYmplY3QpKSB7Ci0gICAgICAgIEhv c3RXaW5kb3cqIGhvc3RXaW5kb3cgPSBjb3JlT2JqZWN0LT5kb2N1bWVudCgpLT52aWV3KCktPmhv c3RXaW5kb3coKTsKKyAgICAgICAgRG9jdW1lbnQqIGRvY3VtZW50ID0gY29yZU9iamVjdC0+ZG9j dW1lbnQoKTsKKyAgICAgICAgaWYgKCFkb2N1bWVudCkKKyAgICAgICAgICAgIHJldHVybiAwOwor CisgICAgICAgIEhvc3RXaW5kb3cqIGhvc3RXaW5kb3cgPSBkb2N1bWVudC0+dmlldygpLT5ob3N0 V2luZG93KCk7CiAgICAgICAgIGlmIChob3N0V2luZG93KSB7CiAgICAgICAgICAgICBQbGF0Zm9y bVBhZ2VDbGllbnQgc2Nyb2xsVmlldyA9IGhvc3RXaW5kb3ctPnBsYXRmb3JtUGFnZUNsaWVudCgp OwogICAgICAgICAgICAgaWYgKHNjcm9sbFZpZXcpIHsKQEAgLTExNzcsNyArMTE4MSwxMSBAQCBz dGF0aWMgUGFuZ29MYXlvdXQqIGdldFBhbmdvTGF5b3V0Rm9yQXRrKEF0a1RleHQqIHRleHRPYmpl Y3QpCiB7CiAgICAgQWNjZXNzaWJpbGl0eU9iamVjdCogY29yZU9iamVjdCA9IGNvcmUodGV4dE9i amVjdCk7CiAKLSAgICBIb3N0V2luZG93KiBob3N0V2luZG93ID0gY29yZU9iamVjdC0+ZG9jdW1l bnQoKS0+dmlldygpLT5ob3N0V2luZG93KCk7CisgICAgRG9jdW1lbnQqIGRvY3VtZW50ID0gY29y ZU9iamVjdC0+ZG9jdW1lbnQoKTsKKyAgICBpZiAoIWRvY3VtZW50KQorICAgICAgICByZXR1cm4g MDsKKworICAgIEhvc3RXaW5kb3cqIGhvc3RXaW5kb3cgPSBkb2N1bWVudC0+dmlldygpLT5ob3N0 V2luZG93KCk7CiAgICAgaWYgKCFob3N0V2luZG93KQogICAgICAgICByZXR1cm4gMDsKICAgICBQ bGF0Zm9ybVBhZ2VDbGllbnQgd2ViVmlldyA9IGhvc3RXaW5kb3ctPnBsYXRmb3JtUGFnZUNsaWVu dCgpOwpAQCAtMTIxOSwxMiArMTIyNywxNiBAQCBzdGF0aWMgZ2ludCB3ZWJraXRfYWNjZXNzaWJs ZV90ZXh0X2dldF9jYXJldF9vZmZzZXQoQXRrVGV4dCogdGV4dCkKICAgICBpZiAoIWNvcmVPYmpl Y3QtPmlzQWNjZXNzaWJpbGl0eVJlbmRlck9iamVjdCgpKQogICAgICAgICByZXR1cm4gMDsKIAor ICAgIERvY3VtZW50KiBkb2N1bWVudCA9IGNvcmVPYmplY3QtPmRvY3VtZW50KCk7CisgICAgaWYg KCFkb2N1bWVudCkKKyAgICAgICAgcmV0dXJuIDA7CisKICAgICBOb2RlKiBmb2N1c2VkTm9kZSA9 IGNvcmVPYmplY3QtPnNlbGVjdGlvbigpLmVuZCgpLmRlcHJlY2F0ZWROb2RlKCk7CiAgICAgaWYg KCFmb2N1c2VkTm9kZSkKICAgICAgICAgcmV0dXJuIDA7CiAKICAgICBSZW5kZXJPYmplY3QqIGZv Y3VzZWRSZW5kZXJlciA9IGZvY3VzZWROb2RlLT5yZW5kZXJlcigpOwotICAgIEFjY2Vzc2liaWxp dHlPYmplY3QqIGZvY3VzZWRPYmplY3QgPSBjb3JlT2JqZWN0LT5kb2N1bWVudCgpLT5heE9iamVj dENhY2hlKCktPmdldE9yQ3JlYXRlKGZvY3VzZWRSZW5kZXJlcik7CisgICAgQWNjZXNzaWJpbGl0 eU9iamVjdCogZm9jdXNlZE9iamVjdCA9IGRvY3VtZW50LT5heE9iamVjdENhY2hlKCktPmdldE9y Q3JlYXRlKGZvY3VzZWRSZW5kZXJlcik7CiAKICAgICBpbnQgb2Zmc2V0OwogICAgIC8vIERvbid0 IGlnbm9yZSBsaW5rcyBpZiB0aGUgb2Zmc2V0IGlzIGJlaW5nIHJlcXVlc3RlZCBmb3IgYSBsaW5r LgpAQCAtMTU0MCw5ICsxNTUyLDExIEBAIHN0YXRpYyBJbnRSZWN0IHRleHRFeHRlbnRzKEF0a1Rl eHQqIHRleHQsIGdpbnQgc3RhcnRPZmZzZXQsIGdpbnQgbGVuZ3RoLCBBdGtDb29yCiAgICAgQWNj ZXNzaWJpbGl0eU9iamVjdCogY29yZU9iamVjdCA9IGNvcmUodGV4dCk7CiAKICAgICBJbnRSZWN0 IGV4dGVudHMgPSBjb3JlT2JqZWN0LT5kb0FYQm91bmRzRm9yUmFuZ2UoUGxhaW5UZXh0UmFuZ2Uo c3RhcnRPZmZzZXQsIHJhbmdlTGVuZ3RoKSk7CisgICAgRG9jdW1lbnQqIGRvY3VtZW50ID0gY29y ZU9iamVjdC0+ZG9jdW1lbnQoKTsKICAgICBzd2l0Y2goY29vcmRzKSB7CiAgICAgY2FzZSBBVEtf WFlfU0NSRUVOOgotICAgICAgICBleHRlbnRzID0gY29yZU9iamVjdC0+ZG9jdW1lbnQoKS0+dmll dygpLT5jb250ZW50c1RvU2NyZWVuKGV4dGVudHMpOworICAgICAgICBpZiAoZG9jdW1lbnQpCisg ICAgICAgICAgICBleHRlbnRzID0gZG9jdW1lbnQtPnZpZXcoKS0+Y29udGVudHNUb1NjcmVlbihl eHRlbnRzKTsKICAgICAgICAgYnJlYWs7CiAgICAgY2FzZSBBVEtfWFlfV0lORE9XOgogICAgICAg ICAvLyBOby1vcApAQCAtMTgxNCwxMiArMTgyOCwxNiBAQCBzdGF0aWMgdm9pZCB3ZWJraXRfYWNj ZXNzaWJsZV9lZGl0YWJsZV90ZXh0X2luc2VydF90ZXh0KEF0a0VkaXRhYmxlVGV4dCogdGV4dCwg YwogICAgIC8vY29yZU9iamVjdC0+c2V0U2VsZWN0ZWRUZXh0UmFuZ2UoUGxhaW5UZXh0UmFuZ2Uo KnBvc2l0aW9uLCAwKSk7CiAgICAgLy9jb3JlT2JqZWN0LT5zZXRTZWxlY3RlZFRleHQoU3RyaW5n Ojpmcm9tVVRGOChzdHJpbmcpKTsKIAotICAgIGlmICghY29yZU9iamVjdC0+ZG9jdW1lbnQoKSB8 fCAhY29yZU9iamVjdC0+ZG9jdW1lbnQoKS0+ZnJhbWUoKSkKKyAgICBEb2N1bWVudCogZG9jdW1l bnQgPSBjb3JlT2JqZWN0LT5kb2N1bWVudCgpOworICAgIGlmICghZG9jdW1lbnQpCisgICAgICAg IHJldHVybjsKKworICAgIGlmICghZG9jdW1lbnQtPmZyYW1lKCkpCiAgICAgICAgIHJldHVybjsK ICAgICBjb3JlT2JqZWN0LT5zZXRTZWxlY3RlZFZpc2libGVQb3NpdGlvblJhbmdlKGNvcmVPYmpl Y3QtPnZpc2libGVQb3NpdGlvblJhbmdlRm9yUmFuZ2UoUGxhaW5UZXh0UmFuZ2UoKnBvc2l0aW9u LCAwKSkpOwogICAgIGNvcmVPYmplY3QtPnNldEZvY3VzZWQodHJ1ZSk7CiAgICAgLy8gRklYTUU6 IFdlIHNob3VsZCBzZXQgcG9zaXRpb24gdG8gdGhlIGFjdHVhbCBpbnNlcnRlZCB0ZXh0IGxlbmd0 aCwgd2hpY2ggbWF5IGJlIGxlc3MgdGhhbiB0aGF0IHJlcXVlc3RlZC4KLSAgICBpZiAoY29yZU9i amVjdC0+ZG9jdW1lbnQoKS0+ZnJhbWUoKS0+ZWRpdG9yKCktPmluc2VydFRleHRXaXRob3V0U2Vu ZGluZ1RleHRFdmVudChTdHJpbmc6OmZyb21VVEY4KHN0cmluZyksIGZhbHNlLCAwKSkKKyAgICBp ZiAoZG9jdW1lbnQtPmZyYW1lKCktPmVkaXRvcigpLT5pbnNlcnRUZXh0V2l0aG91dFNlbmRpbmdU ZXh0RXZlbnQoU3RyaW5nOjpmcm9tVVRGOChzdHJpbmcpLCBmYWxzZSwgMCkpCiAgICAgICAgICpw b3NpdGlvbiArPSBsZW5ndGg7CiB9CiAKQEAgLTE4NDAsMTEgKzE4NTgsMTYgQEAgc3RhdGljIHZv aWQgd2Via2l0X2FjY2Vzc2libGVfZWRpdGFibGVfdGV4dF9kZWxldGVfdGV4dChBdGtFZGl0YWJs ZVRleHQqIHRleHQsIGcKICAgICAvL2NvcmVPYmplY3QtPnNldFNlbGVjdGVkVGV4dFJhbmdlKFBs YWluVGV4dFJhbmdlKHN0YXJ0X3BvcywgZW5kX3BvcyAtIHN0YXJ0X3BvcykpOwogICAgIC8vY29y ZU9iamVjdC0+c2V0U2VsZWN0ZWRUZXh0KFN0cmluZygpKTsKIAotICAgIGlmICghY29yZU9iamVj dC0+ZG9jdW1lbnQoKSB8fCAhY29yZU9iamVjdC0+ZG9jdW1lbnQoKS0+ZnJhbWUoKSkKKyAgICBE b2N1bWVudCogZG9jdW1lbnQgPSBjb3JlT2JqZWN0LT5kb2N1bWVudCgpOworICAgIGlmICghZG9j dW1lbnQpCiAgICAgICAgIHJldHVybjsKKworICAgIGlmICghZG9jdW1lbnQtPmZyYW1lKCkpCisg ICAgICAgIHJldHVybjsKKwogICAgIGNvcmVPYmplY3QtPnNldFNlbGVjdGVkVmlzaWJsZVBvc2l0 aW9uUmFuZ2UoY29yZU9iamVjdC0+dmlzaWJsZVBvc2l0aW9uUmFuZ2VGb3JSYW5nZShQbGFpblRl eHRSYW5nZShzdGFydF9wb3MsIGVuZF9wb3MgLSBzdGFydF9wb3MpKSk7CiAgICAgY29yZU9iamVj dC0+c2V0Rm9jdXNlZCh0cnVlKTsKLSAgICBjb3JlT2JqZWN0LT5kb2N1bWVudCgpLT5mcmFtZSgp LT5lZGl0b3IoKS0+cGVyZm9ybURlbGV0ZSgpOworICAgIGRvY3VtZW50LT5mcmFtZSgpLT5lZGl0 b3IoKS0+cGVyZm9ybURlbGV0ZSgpOwogfQogCiBzdGF0aWMgdm9pZCB3ZWJraXRfYWNjZXNzaWJs ZV9lZGl0YWJsZV90ZXh0X3Bhc3RlX3RleHQoQXRrRWRpdGFibGVUZXh0KiB0ZXh0LCBnaW50IHBv c2l0aW9uKQotLSAKMS43LjQuMgoK