Bug 271406 – AX: Crash with VoiceOver when focusing a PDF annotation.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

Thread 1 Crashed:: com.apple.accessibility.secondary
0   WebCore                                  0x1a8ac37e8 WTFCrashWithInfo(int, char const*, char const*, int) + 20
1   WebCore                                  0x1aa8c8f20 WebCore::TimerBase::setNextFireTime(WTF::MonotonicTime) + 2572
2   WebCore                                  0x1aa089130 WebCore::Element::insertedIntoAncestor(WebCore::Node::InsertionType, WebCore::ContainerNode&) + 1256
3   WebCore                                  0x1aa2fe114 WebCore::HTMLFormControlElement::insertedIntoAncestor(WebCore::Node::InsertionType, WebCore::ContainerNode&) + 44
4   WebCore                                  0x1aa0044d4 WebCore::notifyNodeInsertedIntoDocument(WebCore::ContainerNode&, WebCore::Node&, WebCore::TreeScopeChange, WTF::Vector<WTF::Ref<WebCore::Node, WTF::RawPtrTraits<WebCore::Node>>, 11ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&) + 108
5   WebCore                                  0x1a9ffd8d8 WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&) + 1232
6   WebKit                                   0x1ac210f80 WebKit::PDFPluginAnnotation::attach(WebCore::Element*) + 868
7   WebKit                                   0x1ac20cdb4 WebKit::PDFPlugin::setActiveAnnotation(PDFAnnotation*) + 404

[Radar WebKit Bug Importer]

<rdar://problem/125187438>

[Andres Gonzalez]

rdar://124631171

[Andres Gonzalez]

Created attachment 470469 [details]
Patch

[Tyler Wilcock]

Comment on attachment 470469 [details]
Patch

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

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1197
> +    callOnMainRunLoop([annotation = WTFMove(annotation), this] {

This should probably be callOnMainRunLoopAndWait, as callOnMainRunLoop is a change in behavior for non-AT users. https://github.com/WebKit/WebKit/commit/83013c6d19ebb5430aa94c4a813ceeebc71951e2 (written by me) caused a regression by doing this same thing (using callOnMainRunLoop instead of callOnMainRunLoopAndWait) where PDFs loaded half-way scrolled down the first page.

[Andres Gonzalez]

Created attachment 470471 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 470469 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=470469&action=review
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1197
> > +    callOnMainRunLoop([annotation = WTFMove(annotation), this] {
> 
> This should probably be callOnMainRunLoopAndWait, as callOnMainRunLoop is a
> change in behavior for non-AT users.
> https://github.com/WebKit/WebKit/commit/
> 83013c6d19ebb5430aa94c4a813ceeebc71951e2 (written by me) caused a regression
> by doing this same thing (using callOnMainRunLoop instead of
> callOnMainRunLoopAndWait) where PDFs loaded half-way scrolled down the first
> page.

I saw that and was pondering the possible impact, but not clear to me if it would have a side effect in setting focus as it did in scrolling. So taking the safer path and taking your suggestion. Thanks.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #6)
> (In reply to Tyler Wilcock from comment #4)
> > Comment on attachment 470469 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=470469&action=review
> > 
> > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1197
> > > +    callOnMainRunLoop([annotation = WTFMove(annotation), this] {
> > 
> > This should probably be callOnMainRunLoopAndWait, as callOnMainRunLoop is a
> > change in behavior for non-AT users.
> > https://github.com/WebKit/WebKit/commit/
> > 83013c6d19ebb5430aa94c4a813ceeebc71951e2 (written by me) caused a regression
> > by doing this same thing (using callOnMainRunLoop instead of
> > callOnMainRunLoopAndWait) where PDFs loaded half-way scrolled down the first
> > page.
> 
> I saw that and was pondering the possible impact, but not clear to me if it
> would have a side effect in setting focus as it did in scrolling. So taking
> the safer path and taking your suggestion. Thanks.
TW: Yeah, certainly possible it would be OK here, but I thought the same thing with my last change :)

Maybe in a future patch, we could consider using callOnMainRunLoop if ITM is enabled, or maybe if this request is specifically from an ITM-enabled AT, and use callOnMainRunLoopAndWait otherwise? We can brainstorm some more and maybe a better idea will come.

[EWS]

Committed 276548@main (648225124a2c): <https://commits.webkit.org/276548@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 470471 [details].