Bug 45572 – WebAccessibilityCacheImpl needs to handle invalid accessibility object ids

• Component: Accessibility
• Status: RESOLVED FIXED

[Chris Guillory]

The Chromium renderer crashes when it accesses objects that have been removed from the webkit accessibility tree. 
http://code.google.com/p/chromium/issues/detail?id=54973

WebAccessibilityCacheImpl::addOrGetId should know that AccessibilityObject's with an id of 0 are invalid - they've been removed from the webkit accessibility tree.

[Chris Guillory]

Created attachment 67258 [details]
Return invalidObjectID in WebAccessibilityCacheImpl when an AccessibilityObject's id is 0

[Chris Guillory]

Created attachment 67280 [details]
Need to ensure updateBackingStore is called

Looks like updateBackingStore is where the accessibility object can become invalid.

[Chris Guillory]

Created attachment 67281 [details]
Compiles now

[Chris Guillory]

Created attachment 67283 [details]
Logic is correct now

[WebKit Review Bot]

Attachment 67283 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebAccessibilityObject.cpp:110:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Guillory]

Created attachment 67285 [details]
Passes style check now

[chris fleizach]

Comment on attachment 67285 [details]
Passes style check now

> Index: WebKit/chromium/ChangeLog
> ===================================================================
> --- WebKit/chromium/ChangeLog	(revision 67263)
> +++ WebKit/chromium/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> [phone]  Chris Guillory  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        WebAccessibilityCacheImpl needs to handle invalid accessibility object ids

period at end of sentence
> +    WEBKIT_API bool isAxObjectIdValid() const;

since this is inside of an ax object, the method name should probably just be "isIdValid()"

>  
>      WEBKIT_API unsigned childCount() const;
>  
> +bool WebAccessibilityObject::isAxObjectIdValid() const
> +{
> +    if (!m_private)
> +        return false;
> +
> +    m_private->updateBackingStore();

after calling updateBackingStore() it's possible that m_private will be invalidated (on the WebCore side at least, where it's called m_object). 
you should probably confirm that the object is still valid after calling this method

> +    return m_private->axObjectID();
> +}
> +

[Chris Guillory]

> after calling updateBackingStore() it's possible that m_private will be invalidated (on the WebCore side at least, where it's called m_object). 
> you should probably confirm that the object is still valid after calling this method
I'm adding this method to determine if the WebAccessibilityObject is valid. Perhaps I should just name this method isValid(). An invalid WebCore::AccessibilityObject has an ID of 0.

[chris fleizach]

(In reply to comment #8)
> > after calling updateBackingStore() it's possible that m_private will be invalidated (on the WebCore side at least, where it's called m_object). 
> > you should probably confirm that the object is still valid after calling this method
> I'm adding this method to determine if the WebAccessibilityObject is valid. Perhaps I should just name this method isValid(). An invalid WebCore::AccessibilityObject has an ID of 0.

yea, if updateBackingStore is just checking validity, it should be renamed.

[Chris Guillory]

Created attachment 67360 [details]
Naming new method isValid

[chris fleizach]

Comment on attachment 67360 [details]
Naming new method isValid

r=me

[WebKit Commit Bot]

Comment on attachment 67360 [details]
Naming new method isValid

Clearing flags on attachment: 67360

Committed r67418: <http://trac.webkit.org/changeset/67418>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.