[ATK] [WK2] platform/gtk/accessibility/roles-exposed.html is failing

[Mario Sanchez Prada]

platform/gtk/accessibility/roles-exposed.html is failing in the WebKit2GTK+ bot due to printing a different output than in the WebKit1 bot for elements that are not exposed in the ATK hierarchy.

It looks like a problem in the DRT/WKTR

[Mario Sanchez Prada]

The problem seems to be related to this part of the test, since the output of the test using WKTR always prints "AXRole: " for every object in WebCore's accessibility tree, despite of that object being exposed or not through ATK:

    [...]
    axElement = accessibilityController.accessibleElementById(el.id);
    if (axElement)
        role = axElement.role;
    else
        role = 'AXRole: ';
    [...]


So, the problem is that there's a difference in what is returned by accessibleElementById() when the specified ID belongs to an element that is currently not being exposed in ATK (e.g. a row):

 - DRT: returns a valid AccessibilityUIElement, wrapping a null pointer (as there's no valid AtkObject to wrap).

 - WKTR: returns an undefined/null value, which is the expected behavior.


So, this error is happening now because I created the test expectations with DRT, which will go through the first branch of the if, actually returning an empty string '' when calling axElement.role, as there's no wrapped object.

In my opinion, we should make DRT consistent with what WKTR and return an undefined/null object in AccessibilityUIElement::makeJSAccessibilityUIElement() when the platform-specific accessibility object to be wrapped is not valid, in order to avoid this discrepancies between the two ports.

Furthermore, this change will probably help detect other issues that might have gone overlooked so far, so I think it's a good thing to do.

I'll upload a patch proposing a solution soon.

[Mario Sanchez Prada]

Created attachment 219413 [details]
Patch proposal

Patch attached. As you can see, the main change is this bit in DRT:

 JSObjectRef AccessibilityUIElement::makeJSAccessibilityUIElement(JSContextRef context, const AccessibilityUIElement& element)
 {
+    if (!element.platformUIElement())
+        return nullptr;
+
     return JSObjectMake(context, AccessibilityUIElement::getJSClass(), new AccessibilityUIElement(element));
 }

I understand that should be ok for the Mac too (for other platforms, PlatformUIElement is a pointer, so I assume it will be ok), but would be great to confirm that.

Also, I slightly modified two tests more (besides roles-exposed.html) to be able to test the same things while avoiding the differences in the accessibility hierarchy compared to ATK based ports. Hope that's fine as well.

Please review, thanks!

[Build Bot]

Comment on attachment 219413 [details]
Patch proposal

Attachment 219413 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/50118177

New failing tests:
accessibility/non-data-table-cell-title-ui-element.html

[Build Bot]

Created attachment 219415 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 219413 [details]
Patch proposal

Attachment 219413 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/48008246

New failing tests:
platform/mac/accessibility/search-when-element-starts-in-table.html
platform/mac/accessibility/search-predicate.html
accessibility/non-data-table-cell-title-ui-element.html
accessibility/loading-iframe-updates-axtree.html

[Build Bot]

Created attachment 219418 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Mario Sanchez Prada]

By looking at the output for the layout tests that are failing, it looks to me like there might be problems in the test themselves, probably overlooked by DRT due to always returning a valid AccessibilityUIElement, even when the wrapped platformUIElement is invalid:

--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/accessibility/loading-iframe-updates-axtree-expected.txt
+++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/accessibility/loading-iframe-updates-axtree-actual.txt
@@ -12,7 +12,7 @@
 
 PASS iframe.isEqual(newIframe) is true
 PASS scrollarea.isEqual(newScrollarea) is false
-PASS subwebarea.isEqual(newSubwebarea) is false
+FAIL subwebarea.isEqual(newSubwebarea) should be false. Threw exception TypeError: undefined is not an object (evaluating 'subwebarea.isEqual')
 PASS newSubwebarea.childrenCount > 0 is true
 
 TEST COMPLETE


--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/platform/mac/accessibility/search-predicate-expected.txt
+++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/platform/mac/accessibility/search-predicate-actual.txt
@@ -145,7 +145,7 @@
 PASS resultElement.title is 'AXTitle: test button 2'
 PASS resultElement.role is 'AXRole: AXButton'
 PASS resultElement.title is 'AXTitle: Submit'
-PASS resultElement.role is 'AXRole: '
+FAIL resultElement.role should be AXRole: . Threw exception TypeError: undefined is not an object (evaluating 'resultElement.role')
 PASS successfullyParsed is true
 
 TEST COMPLETE


--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/platform/mac/accessibility/search-when-element-starts-in-table-expected.txt
+++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/platform/mac/accessibility/search-when-element-starts-in-table-actual.txt
@@ -10,7 +10,7 @@
 
 PASS resultElement.role is 'AXRole: AXHeading'
 PASS resultElement.title is 'AXTitle: outside heading'
-PASS resultElement.role is 'AXRole: '
+FAIL resultElement.role should be AXRole: . Threw exception TypeError: undefined is not an object (evaluating 'resultElement.role')
 PASS resultElement.role is 'AXRole: AXHeading'
 PASS resultElement.role is 'AXRole: AXCell'
 PASS resultElement.role is 'AXRole: AXRow'


--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/accessibility/non-data-table-cell-title-ui-element-expected.txt
+++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/accessibility/non-data-table-cell-title-ui-element-actual.txt
@@ -1,5 +1,5 @@
+CONSOLE MESSAGE: line 32: TypeError: undefined is not an object (evaluating 'accessibilityController.accessibleElementById("table").childAtIndex')
 ATS
 
 pass	issue	fail	blocked	skip	test
-Test passed


I'll try to adapt those tests too, so we avoid the conflicting lines, but any help will be appreciated, since I do not have a Mac here to actually try them out

[Mario Sanchez Prada]

Created attachment 219427 [details]
Patch proposal

Let's see if the EWS is happier now...

[chris fleizach]

Comment on attachment 219427 [details]
Patch proposal

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

r+ with minor nits

> LayoutTests/accessibility/loading-iframe-updates-axtree.html:-28
> -                shouldBeFalse("subwebarea.isEqual(newSubwebarea)");

can we change this to
newSubwebarea.isEqual(subwebarea)

so that we check that subwebarea has actually disappeared

> Tools/ChangeLog:9
> +        wrapping invalid platform-specific accessibility objectsin DRT,

objectsin -> objects in

[Build Bot]

Comment on attachment 219427 [details]
Patch proposal

Attachment 219427 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/46118034

New failing tests:
platform/mac/accessibility/search-when-element-starts-in-table.html

[Build Bot]

Created attachment 219443 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Mario Sanchez Prada]

Created attachment 219515 [details]
Patch proposal (already reviewed)

(In reply to comment #9)
> (From update of attachment 219427 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219427&action=review
> 
> r+ with minor nits
> 
> > LayoutTests/accessibility/loading-iframe-updates-axtree.html:-28
> > -                shouldBeFalse("subwebarea.isEqual(newSubwebarea)");
> 
> can we change this to
> newSubwebarea.isEqual(subwebarea)
> 
> so that we check that subwebarea has actually disappeared

Ok, but let's try this through EWS before landing

[Build Bot]

Comment on attachment 219515 [details]
Patch proposal (already reviewed)

Attachment 219515 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/50028351

New failing tests:
platform/mac/accessibility/search-when-element-starts-in-table.html

[Build Bot]

Created attachment 219519 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Mario Sanchez Prada]

(In reply to comment #13)
> (From update of attachment 219515 [details])
> Attachment 219515 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/50028351
> 
> New failing tests:
> platform/mac/accessibility/search-when-element-starts-in-table.html

I think that this might be an issue in the implementation of uiElementForSearchPredicate(), whose result in the layout test is not consistent when executed in DRT and WKTR, despite of the change I did in DRT's makeJSAccessibilityUIElement:

  - DRT: if the search fails it *now* returns NULL 
  - WKTR: if the search fails it returns a valid AccessibilityUIElement wrapping a NULL

The thing that puzzled me a bit at the beginning is that search-predicate.html, which uses a similar functionality from DRT/WKTR, was failing in WK1 before I changed the test to use shouldBeUndefined() and is now not failing anymore in any port, but it seems the reason might be because it's generally skipped in WK2 ports (platform/wk2/TestExpectations).

Thus, the situation now is that I can't fix this thing properly because if I use shouldBe() to compare roles it will fail in DRT, and if I use shouldBeUndefined() it will fail in WKTR, because there's no consistent behaviour (which I think it should be to return an undefined object in both cases).

So, I'm thinking of reporting a bug about this inconsistency and adding this test to the TestExpectations file for Mac WK2's TestExpectations file, so someone with a Mac build can actually fix it in a better way than what I could.

How does it sound, Chris?

[chris fleizach]

(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 219515 [details] [details])
> > Attachment 219515 [details] [details] did not pass mac-wk2-ews (mac-wk2):
> > Output: http://webkit-queues.appspot.com/results/50028351
> > 
> > New failing tests:
> > platform/mac/accessibility/search-when-element-starts-in-table.html
> 
> I think that this might be an issue in the implementation of uiElementForSearchPredicate(), whose result in the layout test is not consistent when executed in DRT and WKTR, despite of the change I did in DRT's makeJSAccessibilityUIElement:
> 
>   - DRT: if the search fails it *now* returns NULL 
>   - WKTR: if the search fails it returns a valid AccessibilityUIElement wrapping a NULL
> 
> The thing that puzzled me a bit at the beginning is that search-predicate.html, which uses a similar functionality from DRT/WKTR, was failing in WK1 before I changed the test to use shouldBeUndefined() and is now not failing anymore in any port, but it seems the reason might be because it's generally skipped in WK2 ports (platform/wk2/TestExpectations).
> 
> Thus, the situation now is that I can't fix this thing properly because if I use shouldBe() to compare roles it will fail in DRT, and if I use shouldBeUndefined() it will fail in WKTR, because there's no consistent behaviour (which I think it should be to return an undefined object in both cases).
> 
> So, I'm thinking of reporting a bug about this inconsistency and adding this test to the TestExpectations file for Mac WK2's TestExpectations file, so someone with a Mac build can actually fix it in a better way than what I could.
> 
> How does it sound, Chris?

I think this sounds ok. thanks

[Mario Sanchez Prada]

Committed r160842: <http://trac.webkit.org/changeset/160842>

[Mario Sanchez Prada]

(In reply to comment #16)
> [...]
> > How does it sound, Chris?
> I think this sounds ok. thanks

Done. I already filed the bug 125996 for the issue in WKTR, and added a Failure expectation for that test in mac-wk2/TestExpectations

[WebKit Commit Bot]

Re-opened since this is blocked by bug 126003

[Alexey Proskuryakov]

This change caused a bad looking crash on Mac, rolling out.

http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r160845%20(1836)/results.html

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r160844%20(1222)/results.html

[Mario Sanchez Prada]

(In reply to comment #20)
> This change caused a bad looking crash on Mac, rolling out.
> 
> http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r160845%20(1836)/results.html
> 
> http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r160844%20(1222)/results.html

Sorry about that. I pushed this believing that it would not cause any issue (as per the output EWS), but something seems to be still wrong.

As I don't have a Mac here to try this out, I'd very much appreciate any help with it. I might be able to try it out at home in my 2010's Mac Mini (which has Mavericks installed), but I'm not sure how long it will take to download the code and build, so any hint would certainly be appreciated :)

[Radar WebKit Bug Importer]

<rdar://problem/15702461>

[chris fleizach]

(In reply to comment #21)
> (In reply to comment #20)
> > This change caused a bad looking crash on Mac, rolling out.
> > 
> > http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r160845%20(1836)/results.html
> > 
> > http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r160844%20(1222)/results.html
> 
> Sorry about that. I pushed this believing that it would not cause any issue (as per the output EWS), but something seems to be still wrong.
> 
> As I don't have a Mac here to try this out, I'd very much appreciate any help with it. I might be able to try it out at home in my 2010's Mac Mini (which has Mavericks installed), but I'm not sure how long it will take to download the code and build, so any hint would certainly be appreciated :)

Crash log looks like we're passing a nil element into isEqual in DRT

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010e4cba6d JSObjectGetPrivate + 13 (MarkedBlock.h:331)
1   DumpRenderTree                	0x000000010e1fa1a4 isEqualCallback(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**) + 58 (AccessibilityUIElement.cpp:41)
2   com.apple.JavaScriptCore      	0x000000010e498d02 long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*) + 530 (APICallbackFunction.h:61)
3   com.apple.JavaScriptCore      	0x000000010e525a01 JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) + 545 (LLIntSlowPaths.cpp:988)
4   com.apple.JavaScriptCore      	0x000000010e52a241 llint_op_call + 187

[Mario Sanchez Prada]

(In reply to comment #23)
> [...]
> Crash log looks like we're passing a nil element into isEqual in DRT
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.JavaScriptCore          0x000000010e4cba6d JSObjectGetPrivate + 13 (MarkedBlock.h:331)
> 1   DumpRenderTree                    0x000000010e1fa1a4 isEqualCallback(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**) + 58 (AccessibilityUIElement.cpp:41)
> 2   com.apple.JavaScriptCore          0x000000010e498d02 long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*) + 530 (APICallbackFunction.h:61)
> 3   com.apple.JavaScriptCore          0x000000010e525a01 JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) + 545 (LLIntSlowPaths.cpp:988)
> 4   com.apple.JavaScriptCore          0x000000010e52a241 llint_op_call + 187

Indeed, and now the question is which one of these is the faulty one:

    shouldBeTrue("iframe.isEqual(newIframe)");
    shouldBeFalse("scrollarea.isEqual(newScrollarea)");
    shouldBeFalse("newSubwebarea.isEqual(subwebarea)");

However, if this fails this way I think that it might be uncovering an issue in the test itself, since it might be that the isEqual() operation that we were doing until now were comparing two AccessibilityUIElements with invalid platformUIElement()'s, which is not what the test is supposed to check:

    description("This tests that if an iframe loads new content
                 after its accessibility object has already been
                 accessed, the iframe accessibility object's
                 descendants are the new scroll area and web
                 area, not the old deleted ones.");

So, I think I'll file a bug for this test too, and add it to mac/TestExpectations too.

[Mario Sanchez Prada]

Committed r160907: <http://trac.webkit.org/changeset/160907>

[Mario Sanchez Prada]

(In reply to comment #24)
> [...]
> So, I think I'll file a bug for this test too, and add it to mac/TestExpectations too.

I'll filed bug 126066 to track that issue down and have just pushed this patch again adding that test to the TestExpectations file:
http://trac.webkit.org/changeset/160907

I'll will monitor the buildbot and roll it out myself if I spot any issue

[Simon Fraser (smfr)]

31 failures:
http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r160908%20(11926)/results.html

[Alexey Proskuryakov]

Most of these 31 failures are unrelated, and due to lazy tree creation patch.