Bug 239402 – AXIsolatedTree::updateNode cannot attach the platform wrapper to the replacement node on the main thread.

• Component: Accessibility
• Status: NEW

[Andres Gonzalez]

AXIsolatedTree::updateNode cannot attach the platform wrapper to the replacement node on the main thread.

[Radar WebKit Bug Importer]

<rdar://problem/91827992>

[Andres Gonzalez]

Created attachment 457724 [details]
Patch

[Darin Adler]

Comment on attachment 457724 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:299
> -    auto change = nodeChangeForObject(axObject, parentID, true);
> +    auto change = nodeChangeForObject(axObject, parentID, false);

This is why we work so hard to avoid boolean constants. The old code is not obviously wrong. The new code is not obviously right.

[Darin Adler]

Comment on attachment 457724 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        AXIsolatedTree::updateNode is called for existing IsolatedObjects that
> +        are being accessed on the AX thread through their platform wrappers.
> +        For that reason, updateNode cannot attach a new IsolatedObject to the
> +        same wrapper on the main thread. Instead, the wrapper should be added to
> +        the NodeChange struct in order to be attached to the IsolatedObject on
> +        the AX thread.

Can we make a regression test?

[Andres Gonzalez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 457724 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457724&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:299
> > -    auto change = nodeChangeForObject(axObject, parentID, true);
> > +    auto change = nodeChangeForObject(axObject, parentID, false);
> 
> This is why we work so hard to avoid boolean constants. The old code is not
> obviously wrong. The new code is not obviously right.

Thanks Darin. We are merging this change with the patch for https://bugs.webkit.org/show_bug.cgi?id=239398, since they overlap on this code. Added:

+    enum class AttachWrapper : bool { OnMainThread, OnAXThread };

to make explicit what this parameter means.

[Andres Gonzalez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 457724 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457724&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        AXIsolatedTree::updateNode is called for existing IsolatedObjects that
> > +        are being accessed on the AX thread through their platform wrappers.
> > +        For that reason, updateNode cannot attach a new IsolatedObject to the
> > +        same wrapper on the main thread. Instead, the wrapper should be added to
> > +        the NodeChange struct in order to be attached to the IsolatedObject on
> > +        the AX thread.
> 
> Can we make a regression test?

The patch in https://bugs.webkit.org/show_bug.cgi?id=239398 fixes several tests in isolated tree mode. Not sure though that any of those tests covers specifically this issue. This however goes straight to the thread safety of the isolated tree mode, even though it may be difficult to write a layout tests that fails without this change and passes with it.

[Darin Adler]

(In reply to Andres Gonzalez from comment #6)
> This however goes straight to the thread safety of
> the isolated tree mode, even though it may be difficult to write a layout
> tests that fails without this change and passes with it.

Sure, agreed, very important!

But the more *important* the fix is, the more value a regression test is. The test helps make sure that a future programmer doesn’t break this. I understand, though, if you can’t think of a good way to make a test.