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: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | 121602 | ||||||||||||
Bug Blocks: | 111992, 121556 | ||||||||||||
Attachments: |
|
Description
Mario Sanchez Prada
2013-09-18 08:46:03 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 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. 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(); } (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. 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. 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. 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 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 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 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 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 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 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 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 *** Bug 98360 has been marked as a duplicate of this bug. *** 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 Adding Krzysztof to CC, as he might be interested in knowing about this as well. 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!
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. (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. 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 Comment on attachment 212695 [details] Patch proposal Attachment 212695 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/2195306 Committed r156532: <http://trac.webkit.org/changeset/156532> *** Bug 111992 has been marked as a duplicate of this bug. *** |