[ATK] Protect entry points in the ATK wrapper against outdated render trees

• Component: Accessibility
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

While working on bug 121556, I found that the reason why it crashes in AccessibilityNodeObject::textUnderElement() is because some nodes in the accessibility tree are being detached between one iteration and the following one inside the for loop in that function.

More specifically too, the problem happens exactly after we call child->textUnderElement() in the snipped of code below:

    StringBuilder builder;
    for  (AccessibilityObject* child = firstChild(); child; child = child->nextSibling()) {

        [...]

        String childText = child->textUnderElement(mode);
        if (childText.length()) {
            if (shouldAddSpaceBeforeAppendingNextElement(builder, childText))
                builder.append(' ');
            builder.append(childText);
        }
    }

Before the call to child->textUnderElement(), and considering the accessibility/heading-level.html test as our scenario, a hypothetical call to child->nextSibling() might return 0x0, which is fine, suggesting that there no more children after that one. However, if we call again child->nextSibling() after the call to textUnderElement(), we get a beautiful segfault error, which is the crashing we are seeing in the bots.

After some investigation, I found that the reason for that seems to be that in r155378 a forced call to update the layout has been added to the constructor of the TextIterator, which is used in AccessibilityRenderObject::textUnderElement() for text objects. And while that addition seems to be correct according to the comments in bug 120891, it's also causing that in certain scenarios (like the one detected with this test) we have the accessibility tree being altered between iterations of that for loop, causing that the 'child' pointer that we have at the beginning of the iteration is no longer valid, thus leading to the crash.

I have trying to get a simplified version of a layout test based on heading-level.html to proof the problem and also a fix for that, which I hope to attach soon.

Btw, this only seems to happen in GTK/EFL for some reason, probably because of the way DRT/WKTR is implemented, but in any case I think there might be a bug here and that it could be as easy to call to updateLayout() before the loop so we make sure that further uses of TextIterator won't cause this weird behaviour.

[Radar WebKit Bug Importer]

<rdar://problem/15020597>

[chris fleizach]

(In reply to comment #0)
> While working on bug 121556, I found that the reason why it crashes in AccessibilityNodeObject::textUnderElement() is because some nodes in the accessibility tree are being detached between one iteration and the following one inside the for loop in that function.
> 
> More specifically too, the problem happens exactly after we call child->textUnderElement() in the snipped of code below:
> 
>     StringBuilder builder;
>     for  (AccessibilityObject* child = firstChild(); child; child = child->nextSibling()) {
> 
>         [...]
> 
>         String childText = child->textUnderElement(mode);
>         if (childText.length()) {
>             if (shouldAddSpaceBeforeAppendingNextElement(builder, childText))
>                 builder.append(' ');
>             builder.append(childText);
>         }
>     }
> 
> Before the call to child->textUnderElement(), and considering the accessibility/heading-level.html test as our scenario, a hypothetical call to child->nextSibling() might return 0x0, which is fine, suggesting that there no more children after that one. However, if we call again child->nextSibling() after the call to textUnderElement(), we get a beautiful segfault error, which is the crashing we are seeing in the bots.
> 
> After some investigation, I found that the reason for that seems to be that in r155378 a forced call to update the layout has been added to the constructor of the TextIterator, which is used in AccessibilityRenderObject::textUnderElement() for text objects. And while that addition seems to be correct according to the comments in bug 120891, it's also causing that in certain scenarios (like the one detected with this test) we have the accessibility tree being altered between iterations of that for loop, causing that the 'child' pointer that we have at the beginning of the iteration is no longer valid, thus leading to the crash.
> 

Are we able to force the layout update before we start building the string? hopefully then calls within the textUnderElement() won't do anything because it will already be updated

[Mario Sanchez Prada]

Created attachment 212000 [details]
Patch proposal plus new Layout test

(In reply to comment #2)
> [...]
> Are we able to force the layout update before we start building the string? hopefully then calls within the textUnderElement() won't do anything because it will already be updated

Yes, that's exactly the idea behind the patch I'm attaching now.

[chris fleizach]

Comment on attachment 212000 [details]
Patch proposal plus new Layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=212000&action=review

> LayoutTests/accessibility/heading-crash-after-hidden.html:24
> +    axHeading.helpText

you should use semicolons at end of lines

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1593
> +    Document* document = this->document();

I think we can cut this comment down to something like
"Update the document's layout now, so that if a TextIterator updates, it won't invalidate our accessibility elements while iterating children"

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1596
> +    document->updateLayout();

we have a method already, we should probably use this one

void AccessibilityObject::updateBackingStore()
{
    // Updating the layout may delete this object.
    if (Document* document = this->document())
        document->updateLayoutIgnorePendingStylesheets();
}

[Mario Sanchez Prada]

(In reply to comment #4)
> [...]
> > LayoutTests/accessibility/heading-crash-after-hidden.html:24
> > +    axHeading.helpText
> 
> you should use semicolons at end of lines

Oops! Ok.

> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1593
> > +    Document* document = this->document();
> 
> I think we can cut this comment down to something like
> "Update the document's layout now, so that if a TextIterator updates, it 
> won't invalidate our accessibility elements while iterating children"

Agreed. I was definitely too verbose with my previous comment.

> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1596
> > +    document->updateLayout();
> 
> we have a method already, we should probably use this one
> 
> void AccessibilityObject::updateBackingStore()
> {
>     // Updating the layout may delete this object.
>     if (Document* document = this->document())
>         document->updateLayoutIgnorePendingStylesheets();
> }

I think you are right here too. However, this unveiled an issue in my previous patch, which is why I'm removing the flags for now and working on a new one:

In order to be able to use this non-const method, we would need to make textUnderElement() a non-cost function too, and chain that modification up in the callers of textUnderElement() until we are sure that any method from AccessibilityObject that might use this is declared as non-const too (e.g. title(), stringValue()...). 

Otherwise we will be hitting a build error due to trying to pass a const pointer to the non-cost method updateBackingStore().

This is a pity IMHO, and not very clear to people reading the accessibility object API, since we would be declaring methods like title() or stringValue() as non-const just because they might end up calling textUnderElement() for some cases, when those methods in principle should not modify the state.

However, this is a situation that was already there since bug 120891 got fixed, as now any time that a TextIterator is used will cause a call to updateLayout(), which will potentially modify the accessibility hierarchy, hence causing that these methods can't be declared as non-cost.

This means that the issue was already there, it was just not obvious. So, I guess the best way to move forward is to be more honest and declare as non-const all this methods that can potentially modify the a11y tree and then we will be able to finish this simple patch here by just calling updateBackingStore(), as you suggested.

[Mario Sanchez Prada]

Comment on attachment 212000 [details]
Patch proposal plus new Layout test

(In reply to comment #5)
> [...]
> This means that the issue was already there, it was just not 
> obvious. So, I guess the best way to move forward is to be
> more honest and declare as non-const all this methods that
> can potentially modify the a11y tree and then we will be
> able to finish this simple patch here by just calling
> updateBackingStore(), as you suggested.

I filed a new bug for dealing with that issue in bug 121602.

Now setting cq- so we make sure nobody lands this patch in the meantime, as it's wrong anyway since I should have realized that I should have made textUnderElement() a non-const method after adding the call to updateLayout() there.

[Mario Sanchez Prada]

Created attachment 212171 [details]
Patch proposal plus new Layout test

New patch based on the work done in bug 121602 (so it probably won't build due to that) and removing the call to helpText() from the layout test, since we are now in the process of cleaning up some issues we have with that in ATK based platforms (see bug 121675) and it's not needed anyway to make the point of this test.

Not setting the r? flag yet since first we need to get bug 121602 sorted out.

[EFL EWS Bot]

Comment on attachment 212171 [details]
Patch proposal plus new Layout test

Attachment 212171 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1908489

[EFL EWS Bot]

Comment on attachment 212171 [details]
Patch proposal plus new Layout test

Attachment 212171 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/2012350

[Early Warning System Bot]

Comment on attachment 212171 [details]
Patch proposal plus new Layout test

Attachment 212171 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/2053354

[Early Warning System Bot]

Comment on attachment 212171 [details]
Patch proposal plus new Layout test

Attachment 212171 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/2053355

[kov's GTK+ EWS bot]

Comment on attachment 212171 [details]
Patch proposal plus new Layout test

Attachment 212171 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/2092314

[Build Bot]

Comment on attachment 212171 [details]
Patch proposal plus new Layout test

Attachment 212171 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1960263

[Build Bot]

Comment on attachment 212171 [details]
Patch proposal plus new Layout test

Attachment 212171 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1864694

[Build Bot]

Comment on attachment 212171 [details]
Patch proposal plus new Layout test

Attachment 212171 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1960277

[Mario Sanchez Prada]

*** Bug 98360 has been marked as a duplicate of this bug. ***

[Mario Sanchez Prada]

As commented in bug 121602 (see [1]), it seems that a better way to proceed  is to ensure from the ATK wrapper that the render tree (and thus the a11y tree too) is not in an outdated state before proceeding to use the WebCore's accessibility API, by calling AccessibilityObject::updateBackingStore() in the entry points and then checking whether the ATK wrapper is detached or not to decide whether to go ahead or not.

So, this bug is no longer just about trying to avoid a crash after hiding a heading, but about fixing an important issue present in the ATK layer, which can potentially mean more issues than just that one.

Because of that, I'm now renaming this bug to reflect the reality better, and will be attaching a patch for review soon, which will probably fix more issues than just that crash.

[1] https://bugs.webkit.org/show_bug.cgi?id=121602#c4

[Mario Sanchez Prada]

Adding Krzysztof to CC, as he might be interested in knowing about this as well.

[Mario Sanchez Prada]

Created attachment 212582 [details]
Patch proposal plus new Layout test

Attaching a new patch for review now. As it's explained in the ChangeLog, it not only adds the new test to check that we no longer get that crash, but also helps us to pass a new one and get other two to print the right results (which were overlooked before, because of a wrong expectations file).

About the code, it's all changes in the ATK code with the exception of two asserts I added in AccessibilityNodeObject::textUnderElement(). Please Chris double check that's right also for the Mac:

    [...]
    // The render tree should be stable before going ahead. Otherwise, further uses of the
    // TextIterator will force a layout update, potentially altering the accessibility tree
    // and leading to crashes in the loop that computes the result text from the children.
    ASSERT(!document()->renderView()->layoutState());
    ASSERT(!document()->childNeedsStyleRecalc());
    [...]

It definitely works fine for GTK after the changes done here, but I'm not that sure about the mac. Also, besides your suggestion of asserting !document()->renderView()->layoutState(), I added the other one to make sure not only that an update is not currently in progress, but also that the render tree is not in the state of needing a new layout update, since that would be triggered when using the TextIterator.

Last, after some considerations, I think asserting here only should be enough, but feel free to point other places where you think it might be useful too (did not want to clutter all the code if not strictly needed).

Thanks!

[chris fleizach]

Comment on attachment 212582 [details]
Patch proposal plus new Layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=212582&action=review

> Source/WebCore/accessibility/atk/WebKitAccessibleUtil.h:38
> +#define returnIfObjectIsInvalid(webkitAccessible) G_STMT_START { \

I would just name this (webkitAccessible) "object", since the name of the macro is "returnIfObjectIs..."

> Source/WebCore/accessibility/atk/WebKitAccessibleUtil.h:44
> +            g_log(G_LOG_DOMAIN, \

do you think you need to log this event? it doesn't happen often, but it does seem like a valid use case to me.

[Mario Sanchez Prada]

(In reply to comment #20)
> (From update of attachment 212582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212582&action=review
> 
> > Source/WebCore/accessibility/atk/WebKitAccessibleUtil.h:38
> > +#define returnIfObjectIsInvalid(webkitAccessible) G_STMT_START { \
> 
Ok. Another option would be to rename the method to returnIfAccessibleIsInvalid, or even returnIfWebKitAccessibleIsInvalid, but I guess just changing the parameter name should be enough.

> > Source/WebCore/accessibility/atk/WebKitAccessibleUtil.h:44
> > +            g_log(G_LOG_DOMAIN, \
> 
> do you think you need to log this event? it doesn't happen often, but it does seem like a valid use case to me.

I think you are right here too. I took this idea from g_return_if_fail(), where they do log events, but it's right that this case does not have to be an error per-se, so logging does not add any value (if it makes sense at all)

I'll change those now.

[Mario Sanchez Prada]

Created attachment 212695 [details]
Patch proposal

Attaching new patch addressing the issues pointed out by Chris.

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 212582 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=212582&action=review
> > 
> > > Source/WebCore/accessibility/atk/WebKitAccessibleUtil.h:38
> > > +#define returnIfObjectIsInvalid(webkitAccessible) G_STMT_START { \
> > 
> Ok. Another option would be to rename the method to returnIfAccessibleIsInvalid, or even returnIfWebKitAccessibleIsInvalid, but I guess just changing the parameter name should be enough.

Actually I think I really prefer returnIfWebKitAccessibleIsInvalid() since that way is clear that a WebKitAccessible (subclass of AtkObject) is expected. I changed that in this new patch

[Build Bot]

Comment on attachment 212695 [details]
Patch proposal

Attachment 212695 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/2195306

[Mario Sanchez Prada]

Committed r156532: <http://trac.webkit.org/changeset/156532>

[Mario Sanchez Prada]

*** Bug 111992 has been marked as a duplicate of this bug. ***