Bug 239402 - AXIsolatedTree::updateNode cannot attach the platform wrapper to the replacement node on the main thread.
Summary: AXIsolatedTree::updateNode cannot attach the platform wrapper to the replacem...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-15 14:03 PDT by Andres Gonzalez
Modified: 2022-04-16 15:06 PDT (History)
12 users (show)

See Also:


Attachments
Patch (3.05 KB, patch)
2022-04-15 14:14 PDT, Andres Gonzalez
darin: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2022-04-15 14:03:59 PDT
AXIsolatedTree::updateNode cannot attach the platform wrapper to the replacement node on the main thread.
Comment 1 Radar WebKit Bug Importer 2022-04-15 14:04:09 PDT
<rdar://problem/91827992>
Comment 2 Andres Gonzalez 2022-04-15 14:14:02 PDT
Created attachment 457724 [details]
Patch
Comment 3 Darin Adler 2022-04-15 15:43:29 PDT
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.
Comment 4 Darin Adler 2022-04-15 15:44:02 PDT
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?
Comment 5 Andres Gonzalez 2022-04-16 11:56:14 PDT
(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.
Comment 6 Andres Gonzalez 2022-04-16 12:01:47 PDT
(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.
Comment 7 Darin Adler 2022-04-16 15:06:42 PDT
(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.