Bug 87778 – [Chromium] AX: WebAccessibilityObject should check if an AccessibilityObject is detached

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

If an AccessibilityObject is detached, any WebAccessibilityObject methods should return an empty or default value.

[Dominic Mazzoni]

Created attachment 144622 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[chris fleizach]

Comment on attachment 144622 [details]
Patch

looks ok to me.

[Adam Barth]

Comment on attachment 144622 [details]
Patch

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

One minor comment, otherwise looks good.

> Source/WebKit/chromium/public/WebAccessibilityObject.h:72
> +    bool isDetached() const;

This needs to be marked WEBKIT_EXPORT

[WebKit Review Bot]

Comment on attachment 144622 [details]
Patch

Attachment 144622 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12839818

New failing tests:
accessibility/loading-iframe-sends-notification.html
platform/chromium/accessibility/add-to-menu-list-crashes.html
accessibility/image-map1.html
accessibility/selection-states.html
accessibility/multiselect-list-reports-active-option.html
accessibility/loading-iframe-updates-axtree.html

[WebKit Review Bot]

Created attachment 144670 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dominic Mazzoni]

Created attachment 144720 [details]
Patch

[Dominic Mazzoni]

Created attachment 144723 [details]
Patch

[Dominic Mazzoni]

Please take another look. It ended up being slightly more complicated, I didn't realize this would break existing tests, but this was a change that was needed anyway.

The original patch worked for AccessibilityRenderObject but not other subclasses of AccessibilityObject.

Explanation: for all other ports, there's one wrapper for every AccessibilityObject. When the AccessibilityObject is detached, it detaches the wrapper. The wrapper is typically a native refcounted object, so if the operating system is still holding onto a reference, the wrapper will persist, but its getters won't do anything because it's detached.

For the Chromium port, the native accessibility objects are in a different process, so we don't need native wrappers. The AccessibilityObjectWrapper code was never used, so I deleted it. Instead, AccessibilityObject just keeps track or whether or not it's detached - for all objects, not just AccessibilityRenderObject. That way the Chromium port can figure out if it has a detached object without needing a wrapper.

[chris fleizach]

Comment on attachment 144723 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:102
> +    return true;

should the default isDetached return whether the wrapper == nil?

> Source/WebKit/chromium/src/WebAccessibilityObject.cpp:107
>      m_private->updateBackingStore();

is it worth combining isDetached() and m_private->updateBackingStore()?

i see that would save you from duplicating that same code in every method. it would also help catch causes where updateBackingStore may not be called (like performDefaultAction)

[Dominic Mazzoni]

(In reply to comment #10)
> (From update of attachment 144723 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144723&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:102
> > +    return true;
> 
> should the default isDetached return whether the wrapper == nil?

I can't find any place where isDetached is used on Mac or any other platform. On any other platform, detaching directly disconnects the native wrapper so that it no longer has a pointer to the AccessibilityObject. Only in Chromium do we keep around a reference to an AccessibilityObject for a little while longer (and FYI, only while a notification is queued).

So I'm happy to add it, if it makes more sense, for future use?

> > Source/WebKit/chromium/src/WebAccessibilityObject.cpp:107
> >      m_private->updateBackingStore();
> 
> is it worth combining isDetached() and m_private->updateBackingStore()?
> 
> i see that would save you from duplicating that same code in every method. it would also help catch causes where updateBackingStore may not be called (like performDefaultAction)

Agreed! Let's do this in a separate change, though - I think the best solution would be to directly expose updateBackingStore in WebKit so that accessibility calls can be batched. Sound good?

[chris fleizach]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 144723 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=144723&action=review
> > 
> > > Source/WebCore/accessibility/AccessibilityObject.cpp:102
> > > +    return true;
> > 
> > should the default isDetached return whether the wrapper == nil?
> 
> I can't find any place where isDetached is used on Mac or any other platform. On any other platform, detaching directly disconnects the native wrapper so that it no longer has a pointer to the AccessibilityObject. Only in Chromium do we keep around a reference to an AccessibilityObject for a little while longer (and FYI, only while a notification is queued).
> 
> So I'm happy to add it, if it makes more sense, for future use?

I think we should change it now, because as it stands, it is returning false information.

> 
> > > Source/WebKit/chromium/src/WebAccessibilityObject.cpp:107
> > >      m_private->updateBackingStore();
> > 
> > is it worth combining isDetached() and m_private->updateBackingStore()?
> > 
> > i see that would save you from duplicating that same code in every method. it would also help catch causes where updateBackingStore may not be called (like performDefaultAction)
> 
> Agreed! Let's do this in a separate change, though - I think the best solution would be to directly expose updateBackingStore in WebKit so that accessibility calls can be batched. Sound good?

Sounds good

[Dominic Mazzoni]

Created attachment 144832 [details]
Patch

[Early Warning System Bot]

Comment on attachment 144832 [details]
Patch

Attachment 144832 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12847918

[Dominic Mazzoni]

Created attachment 144854 [details]
Patch for landing

[Adam Barth]

Comment on attachment 144854 [details]
Patch for landing

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:92
> -#if HAVE(ACCESSIBILITY)
> +#if PLATFORM(CHROMIUM)
> +    m_detached = true;
> +#elif HAVE(ACCESSIBILITY)

These ifdefs don't look quite right.  PLATFORM(CHROMIUM) can build with HAVE(ACCESSIBILITY) and !HAVE(ACCESSIBILITY).  It seems strange to couple these two ifdefs.

[Dominic Mazzoni]

Created attachment 144887 [details]
Patch

[Dominic Mazzoni]

(In reply to comment #16)
> (From update of attachment 144854 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144854&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:92
> > -#if HAVE(ACCESSIBILITY)
> > +#if PLATFORM(CHROMIUM)
> > +    m_detached = true;
> > +#elif HAVE(ACCESSIBILITY)
> 
> These ifdefs don't look quite right.  PLATFORM(CHROMIUM) can build with HAVE(ACCESSIBILITY) and !HAVE(ACCESSIBILITY).  It seems strange to couple these two ifdefs.

You're right. I fixed it, now the ifdefs are consistent with whether setWrapper() and wrapper() are defined in AccessibilityObject.h.

[Adam Barth]

Comment on attachment 144887 [details]
Patch

Thanks.  Do you want me to forward Chris' R+, or should we wait for him to R+ this patch himself?

[Dominic Mazzoni]

(In reply to comment #19)
> (From update of attachment 144887 [details])
> Thanks.  Do you want me to forward Chris' R+, or should we wait for him to R+ this patch himself?

I think it'd be okay to forward the R+ if you don't mind, since Chris was happy with it before and we only made this one small change. Thanks.

[Adam Barth]

Comment on attachment 144887 [details]
Patch

Done.

[WebKit Review Bot]

Comment on attachment 144887 [details]
Patch

Clearing flags on attachment: 144887

Committed r119012: <http://trac.webkit.org/changeset/119012>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.