Bug 33300 – REGRESSION(52819?): AXLoadComplete and AXLayoutComplete causes 4 tests fail on Snow Leopard Debug bot

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

two ARIA tests fail on Snow Leopard Debug bot

They probably just need new baselines:
platform/mac/accessibility/aria-required.html
platform/mac/accessibility/aria-slider-value-change.html

--- layout-test-results/platform/mac/accessibility/aria-required-expected.txt	2010-01-06 20:45:07.000000000 -0800
+++ layout-test-results/platform/mac/accessibility/aria-required-actual.txt	2010-01-06 20:45:07.000000000 -0800
@@ -1,3 +1,5 @@
+AXLayoutComplete
+AXLoadComplete
   
 This tests that aria-required is a usable attribute.
 
--- layout-test-results/platform/mac/accessibility/aria-slider-value-change-expected.txt	2010-01-06 20:45:08.000000000 -0800
+++ layout-test-results/platform/mac/accessibility/aria-slider-value-change-actual.txt	2010-01-06 20:45:08.000000000 -0800
@@ -1,3 +1,14 @@
+AXLayoutComplete
+AXLayoutComplete
+AXLayoutComplete
+AXSelectedTextChanged
+AXLayoutComplete
+AXLayoutComplete
+AXLayoutComplete
+AXSelectedTextChanged
+AXLayoutComplete
+AXLoadComplete
+AXLayoutComplete
 slider
 This tests that its possible to increment and decrement an aria slider and have the value updated correctly.

[Eric Seidel (no email)]

Looks like two storage tests are also failing due to Aria logs:

storage/domstorage/localstorage/window-open.html
storage/domstorage/sessionstorage/delete-removal.html

I suspect the logs are occurring after the test as "completed" and are bleeding into later tests.

[Eric Seidel (no email)]

The buildbot seems to think these failures started with http://trac.webkit.org/changeset/52896.  I suspect these failures are just intermittent as r52896 looks completely unrelated.

I suspect the actual catalyst of this regression is from http://trac.webkit.org/changeset/52819.  However the real culprit is probably that we aren't properly canceling all actions that might cause later logging between tests inside DRT.

[Eric Seidel (no email)]

Unfortunately r52819 is too long ago for the buildbots to still have data about that release, so I'm just guessing that that is related.

[Eric Seidel (no email)]

Yes, definitely intermittent.  52897 didn't fail, but 52898 did.

[Eric Seidel (no email)]

The AXLoadComplete disease has struck Leopard as well!

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r52899%20(8958)/results.html
svg/W3C-SVG-1.1/linking-a-04-t.svg
svg/W3C-SVG-1.1/linking-a-05-t.svg
svg/W3C-SVG-1.1/linking-a-07-t.svg
svg/W3C-SVG-1.1/linking-uri-01-b.svg
svg/W3C-SVG-1.1/linking-uri-02-b.svg

Those 5 all failed due to the same AXLoadComplete spew in their results.

[chris fleizach]

(In reply to comment #5)
> The AXLoadComplete disease has struck Leopard as well!


Did a NSLog get left in somewhere?

[Eric Seidel (no email)]

I'm not sure what these are from.  They seem to be intermittent, and only seem to happen on the slow bots (the Debug bots).

[Mark Rowe (bdash)]

AccessibilityUIElement::addNotificationListener is called to set a notification listener.  I don’t see anything that clears out the notification listener between tests.  I’m also not sure that the code is safe: it stores a reference to a JSObjectRef in to a global variable without first protecting it.

[chris fleizach]

(In reply to comment #8)
> AccessibilityUIElement::addNotificationListener is called to set a notification
> listener.  I don’t see anything that clears out the notification listener
> between tests.  I’m also not sure that the code is safe: it stores a reference
> to a JSObjectRef in to a global variable without first protecting it.

Mark, do you know what needs to be done to clear out the listener between tests. And how to protect the variable? if so, please file a bug for me

[Mark Rowe (bdash)]

(In reply to comment #8)
> AccessibilityUIElement::addNotificationListener is called to set a notification
> listener.  I don’t see anything that clears out the notification listener
> between tests.  I’m also not sure that the code is safe: it stores a reference
> to a JSObjectRef in to a global variable without first protecting it.

I guess that doesn’t explain how it would show up in the test output, since the only place addNotificationListener is used is in platform/mac/accessibility/aria-liveregions-notifications.html where the notification is not displayed anywhere.

[Eric Seidel (no email)]

Hit this again in another random test:
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r52967%20(9012)/svg/W3C-SVG-1.1/animate-elem-78-t-pretty-diff.html

Need to figure out how those logs are happening in the first place.

[Eric Seidel (no email)]

I was wrong, these are not ARIA* messages, they are AX* messages.  I don't see these anywhere in any layout test result.

[Eric Seidel (no email)]

I expect that some test is registering an AX NotificationListener, and then it's never getting unset (just as Mark said above).

Since 
static JSObjectRef AXNotificationFunctionCallback = 0;

is static in:
http://trac.webkit.org/browser/trunk/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm#L2662

we can just clear it in our "reset state" callback.

I can write that patch.  It will be a hack, but someone with more AX knowledge can also write a better patch which is less of a hack.  Either way, we need to make the bots green again asap.

[chris fleizach]

(In reply to comment #13)
> I expect that some test is registering an AX NotificationListener, and then
> it's never getting unset (just as Mark said above).
> 
> Since 
> static JSObjectRef AXNotificationFunctionCallback = 0;
> 
> is static in:
> http://trac.webkit.org/browser/trunk/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm#L2662

there's only one test that registers
platform/mac/accessibility/aria-liveregions-notifications.html

> 
> we can just clear it in our "reset state" callback.
> 
> I can write that patch.  It will be a hack, but someone with more AX knowledge
> can also write a better patch which is less of a hack.  Either way, we need to
> make the bots green again asap.

I'll be looking into this some more tonite so that the notification callbacks are not so static and get cleared approriately

[Eric Seidel (no email)]

I just wrote a hack patch.  Testing it now.  If you jump on IRC we can discuss.  I'm happy to leave this to you to fix. :)

[chris fleizach]

(In reply to comment #15)
> I just wrote a hack patch.  Testing it now.  If you jump on IRC we can discuss.
>  I'm happy to leave this to you to fix. :)

can't talk now. commit your patch and i'll work with it. it'll be helpful to see what you do. thanx

[Eric Seidel (no email)]

Created attachment 46110 [details]
Speculative (hackish) fix, work in progress

[Eric Seidel (no email)]

Created attachment 46111 [details]
Patch

[Eric Seidel (no email)]

Chris:  I've posted a patch to skip the test for now.  I also posted my (untested) work in progress hackish fix.  This way you now have no real rush, and can fix this when you have time.  We'll leave this bug open after the Skipped patch lands.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46111 [details] without any errors.

[chris fleizach]

Created attachment 46113 [details]
patch

This is my attempt to address the problems in the first iteration of this

We no longer use a single static callback variable, and we clear out the value being kept in AccessibilityObjectWrapper

[chris fleizach]

Comment on attachment 46113 [details]
patch

wrong patch

[chris fleizach]

Created attachment 46114 [details]
patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46114 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 46114 [details]
patch

I think this has a very strange API for this being static.  I think that the JS API needs to be reconsidered long term.  This should be a method on AccessibiltyController instead of ObjectWraper since it listens to all notifications, not just one.

But if this fixes the bug, then I'm OK with this.  If you agree, please file a new bug about fixing the API.

[Eric Seidel (no email)]

Comment on attachment 46111 [details]
Patch

I accidentally committed this patch:
Committed r52979: <http://trac.webkit.org/changeset/52979>

But I won't roll it out for now, because it will actually keep the tree greener for the next 12 hours before you wake and land yours. :)

[chris fleizach]

(In reply to comment #26)


https://bugs.webkit.org/show_bug.cgi?id=33388

[chris fleizach]

http://trac.webkit.org/changeset/52994