Bug 211790 - AXIsolatedTree::updateChildren needs to apply pending changes before updating the given node.
Summary: AXIsolatedTree::updateChildren needs to apply pending changes before updating...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-12 11:31 PDT by Andres Gonzalez
Modified: 2020-05-18 10:46 PDT (History)
10 users (show)

See Also:


Attachments
Patch (11.48 KB, patch)
2020-05-12 11:43 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (11.49 KB, patch)
2020-05-13 07:35 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2020-05-12 11:31:34 PDT
AXIsolatedTree::updateChildren needs to apply pending changes before updating the given node.
Comment 1 Andres Gonzalez 2020-05-12 11:43:40 PDT
Created attachment 399151 [details]
Patch
Comment 2 chris fleizach 2020-05-12 11:50:14 PDT
Comment on attachment 399151 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        AXIsolatedTree::updateChildren may be fired for a and isolated object

"for a and isolated"

> Source/WebCore/ChangeLog:12
> +        fail, causing that the isolated tree is not updated. This patch calls

rewrite:

"causing the isolated tree to not be updated."
Comment 3 Darin Adler 2020-05-12 12:12:53 PDT
Comment on attachment 399151 [details]
Patch

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

> Source/WebCore/accessibility/AXLogger.cpp:63
> -void AXLogger::log(const AXCoreObject& object)
> +void AXLogger::log(const RefPtr<AXCoreObject>& object)

This goes from const to non-const (not sure why), from non-null to allowing null (makes sense), and from raw pointer to a smart pointer (no good reason I can think of for that).
Comment 4 Andres Gonzalez 2020-05-13 07:35:44 PDT
Created attachment 399259 [details]
Patch
Comment 5 Andres Gonzalez 2020-05-13 07:39:13 PDT
(In reply to chris fleizach from comment #2)
> Comment on attachment 399151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399151&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        AXIsolatedTree::updateChildren may be fired for a and isolated object
> 
> "for a and isolated"

Fixe.
> 
> > Source/WebCore/ChangeLog:12
> > +        fail, causing that the isolated tree is not updated. This patch calls
> 
> rewrite:
> 
> "causing the isolated tree to not be updated."

Fixed.
Comment 6 Andres Gonzalez 2020-05-13 07:44:47 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 399151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399151&action=review
> 
> > Source/WebCore/accessibility/AXLogger.cpp:63
> > -void AXLogger::log(const AXCoreObject& object)
> > +void AXLogger::log(const RefPtr<AXCoreObject>& object)
> 
> This goes from const to non-const (not sure why), from non-null to allowing
> null (makes sense), and from raw pointer to a smart pointer (no good reason
> I can think of for that).

There are some non-const properties in AXCoreObject that we want to log. The smart pointer is an attempt to hold the object while logging it on the secondary thread when it may be released on the main thread.
Comment 7 EWS 2020-05-14 08:35:50 PDT
Committed r261694: <https://trac.webkit.org/changeset/261694>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399259 [details].
Comment 8 Radar WebKit Bug Importer 2020-05-14 08:36:14 PDT
<rdar://problem/63229053>
Comment 9 Darin Adler 2020-05-18 10:46:41 PDT
Comment on attachment 399151 [details]
Patch

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

>>> Source/WebCore/accessibility/AXLogger.cpp:63
>>> +void AXLogger::log(const RefPtr<AXCoreObject>& object)
>> 
>> This goes from const to non-const (not sure why), from non-null to allowing null (makes sense), and from raw pointer to a smart pointer (no good reason I can think of for that).
> 
> There are some non-const properties in AXCoreObject that we want to log. The smart pointer is an attempt to hold the object while logging it on the secondary thread when it may be released on the main thread.

Using RefPtr is fine for the reason you cite above, but making the argument type be const RefPtr& doesn’t add value, as far as I can tell.