Bug 58429 - [GTK] Missing nullchecks in GTK's a11y wrapper
Summary: [GTK] Missing nullchecks in GTK's a11y wrapper
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-04-13 04:13 PDT by Mario Sanchez Prada
Modified: 2011-04-13 09:27 PDT (History)
1 user (show)

See Also:


Attachments
Patch proposal (6.48 KB, patch)
2011-04-13 04:18 PDT, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>