Bug 121558

Summary:

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

Product:

WebKit

Reporter:

Mario Sanchez Prada <mario>

Component:

Accessibility

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, apinheiro, buildbot, cfleizach, commit-queue, dmazzoni, eflews.bot, gtk-ews, gyuyoung.kim, jdiggs, k.czech, rego+ews, rniwa, webkit-bug-importer, webkit-ews, xan.lopez, zan

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

121602

Bug Blocks:

111992, 121556

Attachments:

Description	Flags
Patch proposal plus new Layout test	cfleizach: review+, mario: commit-queue-
Patch proposal plus new Layout test	eflews.bot: commit-queue-
Patch proposal plus new Layout test	none
Patch proposal	cfleizach: review+, buildbot: commit-queue-

Mario Sanchez Prada

Reported 2013-09-18 08:46:03 PDT

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.

Attachments
Patch proposal plus new Layout test (6.59 KB, patch) 2013-09-18 09:47 PDT, Mario Sanchez Prada	cfleizach: review+ mario: commit-queue-	Details Formatted Diff Diff
Patch proposal plus new Layout test (6.21 KB, patch) 2013-09-20 08:33 PDT, Mario Sanchez Prada	eflews.bot: commit-queue-	Details Formatted Diff Diff
Patch proposal plus new Layout test (80.24 KB, patch) 2013-09-25 08:26 PDT, Mario Sanchez Prada	no flags	Details Formatted Diff Diff
Patch proposal (79.24 KB, patch) 2013-09-26 05:51 PDT, Mario Sanchez Prada	cfleizach: review+ buildbot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2013-09-18 08:46:20 PDT

<rdar://problem/15020597>

chris fleizach

Comment 2 2013-09-18 09:22:40 PDT

(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

Comment 3 2013-09-18 09:47:59 PDT

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 4 2013-09-18 10:36:18 PDT

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

Comment 5 2013-09-19 04:07:43 PDT

(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 6 2013-09-19 04:18:21 PDT

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

Comment 7 2013-09-20 08:33:55 PDT

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 8 2013-09-24 07:49:33 PDT

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 9 2013-09-24 07:49:35 PDT

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 10 2013-09-24 07:50:08 PDT

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 11 2013-09-24 07:51:25 PDT

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 12 2013-09-24 08:10:25 PDT

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 13 2013-09-24 08:10:30 PDT

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 14 2013-09-24 08:22:47 PDT

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 15 2013-09-24 09:12:14 PDT

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

Comment 16 2013-09-25 06:24:20 PDT

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

Mario Sanchez Prada

Comment 17 2013-09-25 06:55:30 PDT

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

Comment 18 2013-09-25 06:56:27 PDT

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

Mario Sanchez Prada

Comment 19 2013-09-25 08:26:32 PDT

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 20 2013-09-25 11:16:37 PDT

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

Comment 21 2013-09-26 05:20:28 PDT

(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

Comment 22 2013-09-26 05:51:59 PDT

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 23 2013-09-26 06:23:49 PDT

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

Comment 24 2013-09-27 02:43:12 PDT

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

Mario Sanchez Prada

Comment 25 2013-09-27 09:53:31 PDT

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

Note You need to log in before you can comment on or make changes to this bug.