Bug 45927 – AX: CrashTracer: 1,382 crashes in Safari at com.apple.WebCore: WebCore::VisiblePosition::canonicalPosition + 78

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

[chris fleizach]

A lot of crashes for this one.

What happens is when someone holds onto a AXTextMarker and then tries to do something with it after the node that the
text marker references is deallocated.

we need a way to know whether a node is gone or not before trying to make a VisiblePosition out of it

[chris fleizach]

Created attachment 67850 [details]
patch

The solution in this patch is:


The AXObjectCache instance now keeps a HashSet of Node's being used. When a node becomes deallocated, it removes itself
from the HashSet. When creating a VisiblePosition from an AXTextMarker, the cache can then check if the node is valid
before proceeding.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/dom/Node.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 10 files


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

[chris fleizach]

Created attachment 67852 [details]
patch

[Eric Seidel (no email)]

The mac EWS hung processing this patch.  Suggesting it failed to build (we have a subprocess problem with our python, wherby if the subprocess -- in this case buildw-ebkit -- produces to much output, we deadlock.  Workign on a fix.)

[Eric Seidel (no email)]

Comment on attachment 67852 [details]
patch

The EWS is hanging on this patch, suggesting it's a build failure.  I'm still trying to diagnose the EWS hang and get you a build log, but for now i'm setting r- to not hang the queue overnight.

If this builds fine on your mac, feel free to set r? again and if the queue hangs that's my fault and I'll deal. :)

[Eric Seidel (no email)]

FYI the Mac EWS is currently Mac Leopard.

[chris fleizach]

(In reply to comment #5)
> (From update of attachment 67852 [details])
> The EWS is hanging on this patch, suggesting it's a build failure.  I'm still trying to diagnose the EWS hang and get you a build log, but for now i'm setting r- to not hang the queue overnight.
> 
> If this builds fine on your mac, feel free to set r? again and if the queue hangs that's my fault and I'll deal. :)

SnowLeopard was building for me. Possible it was leopard related.

[chris fleizach]

Created attachment 68101 [details]
patch

had no problems compiling this morning after updating. let's see if this compiles.

[chris fleizach]

looks like it built on mac alright now

[Simon Fraser (smfr)]

Comment on attachment 68101 [details]
patch

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

> WebCore/dom/Node.cpp:386
> +        m_document->axObjectCache()->removeNodeForUse(this);

What's the performance impact of this? For whom is AXObjectCache::accessibilityEnabled() true?

[chris fleizach]

(In reply to comment #10)
> (From update of attachment 68101 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68101&action=review
> 
> > WebCore/dom/Node.cpp:386
> > +        m_document->axObjectCache()->removeNodeForUse(this);
> 
> What's the performance impact of this? For whom is AXObjectCache::accessibilityEnabled() true?

Non that was noticeable. We're already doing the same thing for all RenderObjects, which hasn't proved to be a perf. bottleneck. so at worse this will be 2x what the RenderObject code imposes, but most likely will be less, since we don't need to actually store very many Nodes, so that HashMap will generally be small

accessibilityEnabled() only turns on when someone is using VoiceOver (or a screen reader) or using something accessing the accessibility info. so for the vast majority of customers, this is a no-op.

[chris fleizach]

http://trac.webkit.org/changeset/68541