AX: Selection change as a result of focusing an element may cause Safari to crash

[Nan Wang]

If an accessibilityObject is focused, it calls axObjectCache()->setIsSynchronizingSelection(true) assuming that the focus will cause a selection change. But once the selection is done, there is a chance that axObjectCache() is nullptr and cause axObjectCache()->setIsSynchronizingSelection(false) to crash the browser.

TO REPRO
* Goto Facebook.com
* Click the messanger icon
* Click “see all”
* Enable VoiceOver
* Put the VO cursor in the facebook search field
* PRess VO-Right a lot (or just hold it down)

[Radar WebKit Bug Importer]

<rdar://problem/21881458>

[Nan Wang]

<rdar://problem/21778212>

[Nan Wang]

Created attachment 257011 [details]
patch

[chris fleizach]

Comment on attachment 257011 [details]
patch

does this test actually reproduce the crash? it seems like an on focus handler would have to remove the element in order to trigger this crash

[Nan Wang]

(In reply to comment #4)
> Comment on attachment 257011 [details]
> patch
> 
> does this test actually reproduce the crash? it seems like an on focus
> handler would have to remove the element in order to trigger this crash

Do you mean by removing the element before focusing it? Ok, I'll work on the testcase.

[chris fleizach]

The test case should cause the crash to happen if the fix is not applied

[Nan Wang]

(In reply to comment #6)
> The test case should cause the crash to happen if the fix is not applied

So setting focus can trigger a deferred layout that invalidates the associated render element which cause asking axObjectCache() either found no document, or no axObjectCache at that document. I tried removing the element in the test as you suggested, but seemed no luck to crash it. 

Any other suggestion to come up a way to reproduce it in the layout test?

Thanks.

[Nan Wang]

Created attachment 257126 [details]
patch

Fixed the testcase and it will reproduce the crash.

[chris fleizach]

Comment on attachment 257126 [details]
patch

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

> LayoutTests/platform/mac/accessibility/focus-crash.html:16
> +    testRunner.dumpAsText();

this is probably not necessary

> LayoutTests/platform/mac/accessibility/focus-crash.html:20
> +    axElement.takeFocus();

looks like you can combine this into one line

[Nan Wang]

(In reply to comment #9)
> Comment on attachment 257126 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257126&action=review
> 
> > LayoutTests/platform/mac/accessibility/focus-crash.html:16
> > +    testRunner.dumpAsText();
> 
> this is probably not necessary
> 
> > LayoutTests/platform/mac/accessibility/focus-crash.html:20
> > +    axElement.takeFocus();
> 
> looks like you can combine this into one line

But I'm accessing axElement in the finishTest() function.

[chris fleizach]

Comment on attachment 257126 [details]
patch

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

>>> LayoutTests/platform/mac/accessibility/focus-crash.html:16
>>> +    testRunner.dumpAsText();
>> 
>> this is probably not necessary
> 
> But I'm accessing axElement in the finishTest() function.

not sure how that even works since the scope is totally different. you're creating the element here then accessing inside a different function

[Nan Wang]

Created attachment 257129 [details]
patch

[Nan Wang]

(In reply to comment #11)
> Comment on attachment 257126 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257126&action=review
> 
> >>> LayoutTests/platform/mac/accessibility/focus-crash.html:16
> >>> +    testRunner.dumpAsText();
> >> 
> >> this is probably not necessary
> > 
> > But I'm accessing axElement in the finishTest() function.
> 
> not sure how that even works since the scope is totally different. you're
> creating the element here then accessing inside a different function

Fixed.

[chris fleizach]

Comment on attachment 257129 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1687
> +    // To fix it, added a RefPtr to hold the object long enough and also created a cache for the axObjectCache().

Comment -->

"When a node is told to set focus, that can cause it to be deallocated, which means that doing anything else inside this object will crash.
To fix this, we added a RefPtr to protect this object long enough for duration. We can also locally cache the axObjectCache."

[Nan Wang]

Created attachment 257138 [details]
patch

Fixed the comment.

[Build Bot]

Comment on attachment 257138 [details]
patch

Attachment 257138 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6503519745998848

New failing tests:
platform/mac/accessibility/focus-crash.html

[Build Bot]

Created attachment 257142 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Nan Wang]

Created attachment 257147 [details]
patch

Fix Mavericks test failure.

[WebKit Commit Bot]

Comment on attachment 257147 [details]
patch

Clearing flags on attachment: 257147

Committed r187053: <http://trac.webkit.org/changeset/187053>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.