Bug 175341 - LayoutTest accessibility/mac/select-element-selection-with-optgroups.html is a flaky failure
Summary: LayoutTest accessibility/mac/select-element-selection-with-optgroups.html is ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-08 14:08 PDT by Ryan Haddad
Modified: 2020-08-19 08:58 PDT (History)
20 users (show)

See Also:


Attachments
Patch (39.22 KB, patch)
2020-08-09 18:22 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (46.44 KB, patch)
2020-08-10 10:46 PDT, Darin Adler
cfleizach: review+
Details | Formatted Diff | Diff
Patch (51.12 KB, patch)
2020-08-11 12:34 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (52.24 KB, patch)
2020-08-11 12:43 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2017-08-08 14:08:39 PDT
LayoutTest accessibility/mac/select-element-selection-with-optgroups.html is a flaky failure

https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/builds/3600

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fmac%2Fselect-element-selection-with-optgroups.html

--- /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/accessibility/mac/select-element-selection-with-optgroups-expected.txt
+++ /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/accessibility/mac/select-element-selection-with-optgroups-actual.txt
@@ -8,7 +8,7 @@
 PASS selectElement.selectedChildrenCount became 1
 PASS selectElement.selectedChildAtIndex(0).isEqual(option1) is true
 PASS selectElement.selectedChildrenCount became 1
-PASS selectElement.selectedChildAtIndex(0).isEqual(option2) is true
+FAIL selectElement.selectedChildAtIndex(0).isEqual(option2) should be true. Was false.
 PASS selectElement.selectedChildrenCount became 1
 PASS selectElement.selectedChildAtIndex(0).isEqual(option3) is true
 PASS successfullyParsed is true
Comment 1 Matt Lewis 2017-08-17 16:33:17 PDT
Marked as flaky on Mac WK2
https://trac.webkit.org/changeset/220889/webkit
Comment 2 Radar WebKit Bug Importer 2020-08-09 17:30:16 PDT
<rdar://problem/66758661>
Comment 3 Darin Adler 2020-08-09 18:22:56 PDT Comment hidden (obsolete)
Comment 4 Darin Adler 2020-08-10 10:46:54 PDT
Created attachment 406311 [details]
Patch
Comment 5 Darin Adler 2020-08-10 12:10:38 PDT
The story being this patch is interesting. I was working on another patch and this test failed. I diagnosed and fixed it and then realized the failure had nothing to do with the changes I was making.

Turns out I had just encountered a flaky test, but in a configuration where the flakiness hadn’t been in the expectation file. Thank goodness it wasn’t because that led to me figuring it out.
Comment 6 Darin Adler 2020-08-10 12:34:45 PDT
I realize now that it’s possible that my fix doesn’t actually fix this flakiness, because my failure was all three, not just option2!
Comment 7 chris fleizach 2020-08-10 19:18:45 PDT
Andres can you review?
Comment 8 Andres Gonzalez 2020-08-11 06:16:15 PDT
(In reply to Darin Adler from comment #4)
> Created attachment 406311 [details]
> Patch

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ a/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -640,6 +643,24 @@ AccessibilityObject* AXObjectCache::getOrCreate(Node* node)
     if (!node->parentElement())
         return nullptr;

+    bool isOptionElement = is<HTMLOptionElement>(*node);
+    if (isOptionElement || is<HTMLOptGroupElement>(*node)) {
+        auto select = isOptionElement
+            ? downcast<HTMLOptionElement>(*node).ownerSelectElement()
+            : downcast<HTMLOptGroupElement>(*node).ownerSelectElement();
+        if (!select)
+            return nullptr;
+        RefPtr<AccessibilityObject> wrapper;

Within the accessibility scope, we try to reserve the name "wrapper" for platform accessibility wrappers like WebAccessibilityObjectWrapper. To avoid confusion, I would suggest naming this variable just object, axObject or accessibilityObject.
Comment 9 Andres Gonzalez 2020-08-11 06:21:17 PDT
(In reply to Darin Adler from comment #4)
> Created attachment 406311 [details]
> Patch

For instance, the call to cacheAndInitializeWrapper down below refers to initializing the platform wrapper, not the AccessibilityObject just created.

+        cacheAndInitializeWrapper(wrapper.get(), node);
Comment 10 Darin Adler 2020-08-11 08:38:22 PDT
OK. Will rename it “object”. Any other comments?
Comment 11 Andres Gonzalez 2020-08-11 09:27:56 PDT
(In reply to Darin Adler from comment #4)
> Created attachment 406311 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityMenuListOption.h
+++ a/Source/WebCore/accessibility/AccessibilityMenuListOption.h

-    RefPtr<HTMLElement> m_element;
+    HTMLOptionElement* m_element;

One question that apply to this and all the other classes that wrap a DOM object. Can we use something like a WeakPtr instead of a raw pointer? Relying on detachRemoteParts to make the raw pointer null is good, but I'm not sure that in all cases detachRemoteParts is called and that we are checking for null in all cases, so using the raw pointer could lead to crashes. Of course, we should find all of those possible cases if they exist, but I'm just concerned that we are making this change late and we won't find all the cases immediately.
Comment 12 Darin Adler 2020-08-11 09:45:53 PDT
(In reply to Andres Gonzalez from comment #11)
> One question that apply to this and all the other classes that wrap a DOM
> object. Can we use something like a WeakPtr instead of a raw pointer?
> Relying on detachRemoteParts to make the raw pointer null is good, but I'm
> not sure that in all cases detachRemoteParts is called and that we are
> checking for null in all cases, so using the raw pointer could lead to
> crashes. Of course, we should find all of those possible cases if they
> exist, but I'm just concerned that we are making this change late and we
> won't find all the cases immediately.

I think the reason we didn’t do that yet is that Node doesn’t have a WeakPtrFactory; adding it would make Node significantly bigger and have a significant effect on overall memory use. So that’s major decision and a major project. I think.

Given the other accessibility objects already rely on detachRemoteParts, and this is joining the idiom the others use rather than breaking new ground, I don’t think that updating these classes to match the pattern is highly risky.

I have studied how it gets called and it is straightforward and I see little room for failure:

    Node::~Node calls Document::willBeDeletedFrom
    Document::willBeDeletedFrom calls AXObjectCache::remove(Node&)
    AXObjectCache::remove calls AXObjectCache::remove(AXID)
    AXObjectCache::remove(AXID) calls AXCoreObject::detach
    AXCoreObject::detach calls virtual AXObject::detachRemoteParts

The only if statement here is getting the AXID from the Node and getting the AXCoreObject from the AXObjectCache. That’s the thing that is new for these two classes now, because before they weren’t stored in the cache.
Comment 13 Darin Adler 2020-08-11 09:47:23 PDT
If you are worried about late changes, I would be fine waiting a couple weeks until after the branch to fix this. I suppose the symptom must be mild since it’s been like this for a long time.
Comment 14 Andres Gonzalez 2020-08-11 10:11:58 PDT
(In reply to Darin Adler from comment #13)
> If you are worried about late changes, I would be fine waiting a couple
> weeks until after the branch to fix this. I suppose the symptom must be mild
> since it’s been like this for a long time.

Yes, my vote would be to wait for that couple weeks. On the other hand, I'm glad that you took on this, since this issue does have an impact for the isolated tree on Mac. Since the isolated Tre is a mirror of the AXObjectCache tree, these two classes are not expose in isolated tree mode at all. I'll be more Etna happy to take it from here and update and land the patch in a couple of weeks. Just let me know your preference. Thank you very much.
Comment 15 chris fleizach 2020-08-11 10:22:36 PDT
(In reply to Andres Gonzalez from comment #14)
> (In reply to Darin Adler from comment #13)
> > If you are worried about late changes, I would be fine waiting a couple
> > weeks until after the branch to fix this. I suppose the symptom must be mild
> > since it’s been like this for a long time.
> 
> Yes, my vote would be to wait for that couple weeks. On the other hand, I'm
> glad that you took on this, since this issue does have an impact for the
> isolated tree on Mac. Since the isolated Tre is a mirror of the
> AXObjectCache tree, these two classes are not expose in isolated tree mode
> at all. I'll be more Etna happy to take it from here and update and land the
> patch in a couple of weeks. Just let me know your preference. Thank you very
> much.

I think as long as iOS14 is out of the way we'll have enough time to live on and qualify this change
Comment 16 Darin Adler 2020-08-11 12:06:36 PDT
(In reply to Darin Adler from comment #12)
> I think the reason we didn’t do that yet is that Node doesn’t have a
> WeakPtrFactory; adding it would make Node significantly bigger and have a
> significant effect on overall memory use. So that’s major decision and a
> major project. I think.

Wait, I was wrong about this!

ContainerNode does have a WeakPtrFactory. So if we can refactor this so that we don’t have to allow non-container nodes like Text (and I think we can), we can start using WeakPtr instead of explicitly nulling out.

I’ll see if I can do that easily or if it makes the patch balloon in size.

I think it would *increase* the risk of the patch because it would change existing accessibility object behavior more (timing of the nulling), but it’s easier to reason about.
Comment 17 Darin Adler 2020-08-11 12:24:49 PDT
Since AccessibilityNodeObject is used with text nodes it can’t use WeakPtr. But the new code can and so I will update it.
Comment 18 Darin Adler 2020-08-11 12:34:46 PDT
Created attachment 406403 [details]
Patch
Comment 19 Darin Adler 2020-08-11 12:35:27 PDT
Uploaded a new patch that responds to both comments Andres made, so EWS can chew on it.
Comment 20 Darin Adler 2020-08-11 12:37:47 PDT
Oops, new version needs some more "get()" because of the WeakPtr change.
Comment 21 Darin Adler 2020-08-11 12:43:31 PDT
Created attachment 406405 [details]
Patch
Comment 22 Darin Adler 2020-08-11 13:58:34 PDT
Committed r265514: <https://trac.webkit.org/changeset/265514>