Bug 175341 – LayoutTest accessibility/mac/select-element-selection-with-optgroups.html is a flaky failure

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

[Ryan Haddad]

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

[Matt Lewis]

Marked as flaky on Mac WK2
https://trac.webkit.org/changeset/220889/webkit

[Radar WebKit Bug Importer]

<rdar://problem/66758661>

[Darin Adler]

Created attachment 406277 [details]
Patch

[Darin Adler]

Created attachment 406311 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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!

[chris fleizach]

Andres can you review?

[Andres Gonzalez]

(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.

[Andres Gonzalez]

(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);

[Darin Adler]

OK. Will rename it “object”. Any other comments?

[Andres Gonzalez]

(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.

[Darin Adler]

(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.

[Darin Adler]

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.

[Andres Gonzalez]

(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.

[chris fleizach]

(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

[Darin Adler]

(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.

[Darin Adler]

Since AccessibilityNodeObject is used with text nodes it can’t use WeakPtr. But the new code can and so I will update it.

[Darin Adler]

Created attachment 406403 [details]
Patch

[Darin Adler]

Uploaded a new patch that responds to both comments Andres made, so EWS can chew on it.

[Darin Adler]

Oops, new version needs some more "get()" because of the WeakPtr change.

[Darin Adler]

Created attachment 406405 [details]
Patch

[Darin Adler]

Committed r265514: <https://trac.webkit.org/changeset/265514>