Bug 58429

Summary: [GTK] Missing nullchecks in GTK's a11y wrapper
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: mrobinson
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch proposal mrobinson: review+

Description Mario Sanchez Prada 2011-04-13 04:13:26 PDT
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.
Comment 1 Mario Sanchez Prada 2011-04-13 04:18:55 PDT
Created attachment 89363 [details]
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.
Comment 2 Martin Robinson 2011-04-13 08:22:48 PDT
Comment on attachment 89363 [details]
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.
Comment 3 Mario Sanchez Prada 2011-04-13 09:27:40 PDT
Committed r83746: <http://trac.webkit.org/changeset/83746>