[Webkit-unassigned] [Bug 121558] AX: Crash when trying to retrieve textual information for a heading after hiding it

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=121558





--- Comment #5 from Mario Sanchez Prada <mario at webkit.org>  2013-09-19 04:07:43 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.