Fix crash when system retains an accessibility object.

Fix crash when system retains an accessibility object.

The system might retain instances to BrowserAccessibilityCocoa
objects even after they're no longer used. Previously, we handled
this by having BrowserAccessibilityCocoa own BrowserAccessibilityMac
so that its underlying data would persist as long as the wrapper.
However, this led to crashes when the system calls methods that
explore the tree hierarchy, like GetLocalBoundsRect.

The proper fix is that BrowserAccessibilityCocoa should detach
from BrowserAccessibilityMac with the latter is deleted, and
then do a NULL check in any NSAccessibility protocol implementation
so that it won't crash when a deleted object is queried.

BUG=242944

[David Tseng, 2013-06-20 17:49:36]

lgtm

Was this tested with VoiceOver/Accessibility Inspector? The change seems
reasonable, but not sure I've ever seen this crash. Do you know if webkit
returns some default set of attributes for accessibility objects that have been
detached instead of nil everywhere?

[dmazzoni, 2013-06-20 23:33:09]

On Thu, Jun 20, 2013 at 10:49 AM, <dtseng@chromium.org> wrote:

> lgtm
>
> Was this tested with VoiceOver/Accessibility Inspector?


Yes, but the crash is hard to reproduce. All I can be sure of is that this
doesn't seem to break anything under normal circumstances. FWIW, Shawn hits
this crash a lot, and we have a lot of crash reports with this stack trace.


> The change seems
> reasonable, but not sure I've ever seen this crash. Do you know if webkit
> returns some default set of attributes for accessibility objects that have
> been
> detached instead of nil everywhere?
>

If you look in WebAccessibilityObjectWrapperMac.mm, most of the methods
start with if (!m_object) return nil - so they're basically doing the same
thing.

I think we got away without it for a lot time because we were retaining the
reference to the underlying BrowserAccessibility object, so most functions
work fine and it rarely leads to a crash. In fact, it'd probably be
possible to fix it that way by making sure detached BrowserAccessibility
objects never have pointers to parents or children, but this fix is better.

- Dominic

[David Tseng, 2013-06-21 00:41:18]

>
> Still lgtm.
>


> I think we got away without it for a lot time because we were retaining
> the reference to the underlying BrowserAccessibility object, so most
> functions work fine and it rarely leads to a crash. In fact, it'd probably
> be possible to fix it that way by making sure detached BrowserAccessibility
> objects never have pointers to parents or children, but this fix is better.
>
> The only advantage of the current approach was that a client could
continue to query for information even for nodes that were invalidated (and
since they chose to retain the object, they shouldn't be surprised to get
stale data).

But, definitely stability is more important; I suppose folks using a mouse
might trigger this more often than a purely keyboard-based VoiceOver user
which could explain why I never saw it.

[dmazzoni, 2013-06-21 04:57:01]

On Thu, Jun 20, 2013 at 5:41 PM, David Tseng <dtseng@chromium.org> wrote:

> But, definitely stability is more important; I suppose folks using a mouse
>> might trigger this more often than a purely keyboard-based VoiceOver user
>> which could explain why I never saw it.
>>
>
That's a good theory, actually - perhaps hovering while the page is
changing triggers it.

[dmazzoni, 2013-06-21 06:24:06]

Landed r207709