Bug 37112 – aria-liveregion-notifications.html fails on leopard release bot

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

For some reason, adding new tests to accessibility causes this test to fail on the leopard release bot. 
disabling this test on leopard until my investigation turns up the reason

[chris fleizach]

disabled for now
http://trac.webkit.org/changeset/57094

[WebKit Review Bot]

http://trac.webkit.org/changeset/57094 might have broken Tiger Intel Release

[Adam Barth]

> http://trac.webkit.org/changeset/57094 might have broken Tiger Intel Release

This is svg/custom/getsvgdocument.html, which has recently become very flaky. 
I don't think it's related to your change.

[chris fleizach]

yea that would be bad if changing a skip list causes a test to fail

[Eric Seidel (no email)]

This test failed on Snow Leopard again just now:
http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57184%20(5687)/platform/mac/accessibility/aria-liveregions-notifications-diffs.txt

[chris fleizach]

this is the active bug. will work on a less flaky test tonite

[chris fleizach]

eric, was that test failure on a bot. i don't see in the current build logs

(In reply to comment #6)
> this is the active bug. will work on a less flaky test tonite

[Eric Seidel (no email)]

Yes, the test failure was on the Snow Leopard build bot, linked from http://build.webkit.org/console or the link above.

[chris fleizach]

looks like its intermittently failing on SL. My idea is to separate this test into its four parts and test each individually. that way, if something continues to fail, at least I'll know where its happening

[Adam Barth]

Two questions:

1) Did you make any changes in your patch since the last time you landed it and cause this same test to become flaky?

2) Why did you skip this test on Leopard?  There has been a lot of discussion on webkit-dev recently about how skipping tests is not the correct solution for dealing with flakiness.

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

[chris fleizach]

the test also started failing with another patch, completely unrelated. it then seemed to me that adding any more accessibility tests will cause this test to start failing intermittently. that tells me the test needs to be deconstructed so i can understand whats failing. until that time, i disabled the test on leopard so the precious bots wouldn't be red, and i opened this bug which i am actively investigating.

[chris fleizach]

i also don't want to hold up fixing other important bugs because of a test thats failing because of a DRT implementation most likely.

[Adam Barth]

Thanks for working on fixing flaky tests.  I couldn't find an explanation of
your thought process so it's difficult to understand the approach your're
taking.  Hopefully you'll be able to fix the flakiness soon.

Sometimes disabling a test is the best way to move forward with repairing it. 
However, our current approach is to try fixing the test without disabling it. 
If a test is preventing you from moving forward with other patches, that's a
good sign that fixing the test is relatively higher priority.  We've seen many
examples of surprisingly subtle interactions and regressions that appeared to
manifest themselves as increased flakiness in a test.

Instead of skipping a test in an unreviewed patch, it might be preferable in
the future to discuss the issue with the folks who've shown interest in the
issue in the past, ideally getting one of them to review your patch before
landing it.

[Adam Barth]

To add more context for where I'm coming from, yesterday we had an example of a quite important bug (a crash on the Facebook home page) that caused increased flakiness in a test on Tiger.  The change and the failure appeared entirely unrelated, but after some careful investigation, jamesr determined that the change actually caused a real regression involving when the load event was fired.  The interaction was quite complex and involved some re-entrancy issues in attaching elements to the DOM tree.  Had we just skipped the test and investigated later, it's entirely likely that we would have shipped the regression in our zeal to make Facebook's home page not crash.  After understanding the issue, jamesr was able to produce a correct version of the patch that fixed the crash and avoided the regression.

[Adam Barth]

Now, I don't know whether we're seeing a similar case here or whether the interaction is entirely the fault of the test.  However, we only know these things after investigating the situation carefully, which I'm glad you're now doing.

[chris fleizach]

The relative instability of the drt mechanism for testing accessibility notifications on the mac has been known for a while. We've seen flakiness before but it's gone away on it's own. It's most likely due to storing a js callback as a static and then having objects not be deallocated by drt. I understand the desire not to disable a test but it became clear to me after twounrelated patches went in that both caused issues that something else was at play. I'm working on rewriting the current mechanism. Unfortunately comcast decided to shut down and go home so I won't be able to post any patches tonite

[chris fleizach]

Created attachment 52742 [details]
patch

I've redone the mechanism for how accessibility notifications are handled.

Now, DRT will set a flag that tells WebCore it should send out a NSNotification. Now any DRT object can just listen for that notification when they want to and stop when they dont.
The previous method stored a static function pointer in WebCore that it called back, which I suspect was causing some race condition flakiness

I also required that tests using this mechanism inform DRT when they are done listening, which allows for easier cleanup.

I am also adding four tests which are basically the aria-liveregion-notifications.html broken into its components. In my testing, having these four tests
run really stressed the system and brought out bugs, so I'd like to leave them there to see if they help bring out bugs on these other platforms

[Adam Barth]

I would review you patch except I don't understand Objective-C.  How does the NSNotificationCenter work?  It calls back the specified function from the main event loop?  I ask because we have a similar desire to call back into tests when console messages get logged.

[chris fleizach]

notifications from NSNotificationCenter are on the same thread as the thing that posts them. In Accessibility's case, this is the main thread on the Mac. This seems like a better, more durable, way of calling back from WebCore to DRT in my opinion. Definitely, better than have a static function pointer


(In reply to comment #18)
> I would review you patch except I don't understand Objective-C.  How does the
> NSNotificationCenter work?  It calls back the specified function from the main
> event loop?  I ask because we have a similar desire to call back into tests
> when console messages get logged.

[WebKit Review Bot]

Attachment 52742 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1666209

[chris fleizach]

Created attachment 52746 [details]
patch

forgot to add the new DRT methods to GTK and win

[Eric Seidel (no email)]

Comment on attachment 52746 [details]
patch

I'm not sure the memory management is right in this change.  Why are either of these weak pointers safe:

0     id m_platformElement;
 91     JSObjectRef m_notificationFunctionCallback;

I'm glad you documented this:
 105     // This is to avoid any race conditions between tests turning this flag on and off. Instead
I agree with your choice to leave it on.

Why call it "notification" if it's really the "notificationName"?
 2670         NSDictionary* userInfo = [NSDictionary dictionaryWithObjectsAndKeys:notification, @"notification", nil];

Are we guaranteed that the entire AccessibilityUIElement tree will be destroyed when the page navigates?  Otherwise I worry about results possibly bleeding into other tests.

0     // Already added once.
 831     if (m_notificationHandler)
 832         return false;
This seems like a programmer error, we should ASSERT or log or something here, no?

We should document (comment) the memory management for these members:
180182     PlatformUIElement m_element;
181      JSObjectRef m_notificationFunctionCallback;
 183     void* m_notificationHandler;

Otherwise I think this is OK.  I didn't look at the tests very closely.  I'm concerned about the various memory management issues listed above, so r-.

I'm feeling mostly better now, so reviews should be much quicker now that I"m not sleeping most of the day. :)

[chris fleizach]

Created attachment 53032 [details]
patch

this change addresses comments eric made.

Two issues he raised
1) How are the ivars memory managed in AccessibilityUIElement.h. 
    m_platformElement is assigned/retained/released in platform specific code. That has always been that way.
    I'm adding (for mac only, unless others need it), an "id m_notificationHandler". This will also be assigned/retained/released by the platform
    specific implementation. 

2) Will AXUIElements be released when the page exits? The answer to that seems no, which is the reason for this bug. It seems bleed over was causing
the flakiness before. My changes address this by requiring layout test writers (on the mac) to add and remove notification listeners within the test. If the notification listen is not removed, then DRT will assert. Forcing adding and removal
ensures that we don't get bleed over of notifications at least.

[Alexey Proskuryakov]

> My changes address this by requiring layout test writers
> (on the mac) to add and remove notification listeners within the test.

This is not how we usually handle this - even if there is a fatal error running a test (such as a JS parsing error), subsequent tests shouldn't be affected. See code in runTest(), which resets environment to consistent state before each test.

[chris fleizach]

In the implementation I am replacing,  subsequent tests could be affected. However, in the new method, subsequent tests should NOT be affected even if the user does not remove the notification listener, because the new method does not use a function callback stored as a static, it just sends out a NSNotification. 

However, it seems good practice to remove notification listeners explicitly, much like NSNotificationCenter requires the developer to removeObserver. That allows for a tighter cleanup path, hence the assertion that the developer removes the notification listener before the Accessibility object disappears.

> a test (such as a JS parsing error), subsequent tests shouldn't be affected.
> See code in runTest(), which resets environment to consistent state before each
> test.

[Alexey Proskuryakov]

Comment on attachment 53032 [details]
patch

From what I see, it appears that one of subsequent tests may mysteriously quit during garbage collection (since you used assert and not ASSERT, there won't even be a stack trace).

Asserting in addNotificationListener() also seems wrong - the function needs to be renamed to something like setNotificationListener() if it doesn't allow "adding" more than one.

Requiring tests to clean up after themselves is really not what we do. In fact, I made the same mistake with some plug-in test methods a few week ago, and got bitten by it almost immediately! It was really confusing.

More comments, some of these minot nitpicks:

+static BOOL AXShouldRepostNotifications = NO;

Old code also had this issue, but variable names should start with lower case letter per coding style guidelines. I usually advise to not initialize static variables explicitly - since they are guaranteed to start with zero, explicit initialization is just visual noise. It's almost the same as initializing RefPtr members in constructors with zero.

+    // The PlatformUIElement should be retained.
     PlatformUIElement m_element;

I don't believe calling retain is sufficient. It's a no-op if garbage collection is enabled, and I don't think that Objective-C garbage collection looks inside C++ objects. So, it should use CFRetain/CFRelease.

It's a shame we can't use RetainPtr here.

+#if PLATFORM(MAC)
+#ifdef __OBJC__
+    id m_notificationHandler;
+#endif 
+#endif

This is wrong - the size of the object will be smaller if the header included in a C/C++ file. The correct way to handle this is how PlatformUIElement is handled.

+    // Once when object starts requesting notifications, its on for the duration of the program.

Typos: "once when", and "its" instead of "it's".

+    // This is to avoid any race conditions between tests turning this flag on and off. Instead

I'm not sure if I understand what race conditions exist here. Aren't notifications delivered synchronously?

+    NSString* notificationName = [[notification userInfo] objectForKey:@"notificationName"];

We still position stars away from ObjC class names, so this should be "NSString *notificationName". Same issue elsewhere.

+    JSObjectCallAsFunction([mainFrame globalContext], m_notificationFunctionCallback, NULL, 1, &argument, NULL);

0, not NULL.

+- (void)setCallback:(JSObjectRef)callback
+{
+    m_notificationFunctionCallback = callback;
+}

m_notificationFunctionCallback doesn't seem to be GC protected anywhere. This is not a practical issue for existing tests, as the callback is protected by the virtue of being defined on global object, but it would be a problem for JavaScript code like this:

liveRegion.addNotificationListener(function() { alert(0) } );
// Force garbage collection
// Do something that dispatches the notification.

[Eric Seidel (no email)]

I fear we may have scared Chris off.
platform/mac/accessibility/aria-liveregions-notifications.html
is still a high-frequency failure for both Leopard and Snow Leopard builders. :(  Failed twice on the commit-queue today and few times on SL today:
http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57622%20(5990)/results.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57595%20(5974)/results.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57589%20(5970)/results.html

[chris fleizach]

That would be part of it. At the end of the day, none of this code is used in production. It only exists to test that notifications are generated due to specific events on the mac platform. The old method did not reliably capture that because its implementation was based on some incorrect assumptions. The new one (hopefully) does not make those same mistakes

(In reply to comment #27)
> I fear we may have scared Chris off.
> platform/mac/accessibility/aria-liveregions-notifications.html
> is still a high-frequency failure for both Leopard and Snow Leopard builders.
> :(  Failed twice on the commit-queue today and few times on SL today:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57622%20(5990)/results.html
> http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57595%20(5974)/results.html
> http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r57589%20(5970)/results.html

[Alexey Proskuryakov]

I'm sorry if my comments came across as overly harsh. Most of them are indeed minor nitpicks.

I still think it's worth it to maintain coding style and behavior conventions we have, because hackability is equally important for WebKit code and for tools code. One of the reasons to avoid issues like the GC one I pointed out last is that DRT serves as one of examples of how to embed WebKit, so developers study it to learn how we do that, or just copy/paste its code.

[chris fleizach]

I don't mind the nits. Some of the other things are hard to deal with. 

(In reply to comment #26)
> (From update of attachment 53032 [details])
> From what I see, it appears that one of subsequent tests may mysteriously quit
> during garbage collection (since you used assert and not ASSERT, there won't
> even be a stack trace).

I can change to ASSERT. When I searched through DRT for assert, the lowercase version was used quite often, so I used that. Also when i was testing that assert, it always gave me the backtrace..

> 
> Requiring tests to clean up after themselves is really not what we do. In fact,
> I made the same mistake with some plug-in test methods a few week ago, and got
> bitten by it almost immediately! It was really confusing.
> 

I don't see why its a problem to require that tests be written correctly. With the asserts I put in the test writer will experience problems immediately. Given that I can't be assured that an AX element will be deallocated when the page deallocates, its risky to have a handler still hanging around.


> More comments, some of these minot nitpicks:
> 
> +static BOOL AXShouldRepostNotifications = NO;
> 
> Old code also had this issue, but variable names should start with lower case
> letter per coding style guidelines. I usually advise to not initialize static
> variables explicitly - since they are guaranteed to start with zero, explicit
> initialization is just visual noise. It's almost the same as initializing
> RefPtr members in constructors with zero.
> 

I would argue that clarity of what the starting value here is more important that visual noise. I also see TONS of usage of setting member pointers to zero in constructors throughout webkit


> +    // The PlatformUIElement should be retained.
>      PlatformUIElement m_element;
> 
> I don't believe calling retain is sufficient. It's a no-op if garbage
> collection is enabled, and I don't think that Objective-C garbage collection
> looks inside C++ objects. So, it should use CFRetain/CFRelease.
> 
> It's a shame we can't use RetainPtr here.

I am only working with the Mac side, so this is not a C++ object. THe platformUIElement code has been here since AX was added to DRT. I'm not about to change that. I just added a comment because Eric asked for one. On the mac it's an ObjC obj of type id.

> 
> +    // This is to avoid any race conditions between tests turning this flag on
> and off. Instead
> 
> I'm not sure if I understand what race conditions exist here. Aren't
> notifications delivered synchronously?
>

That flag is to control whether WebCore will send notifications. If i have two tests running in parallel, both try turning on, then one turns off while the other expects it to be on, things will fail. That's why once the flag is turned on it should stay on.

 
> +    NSString* notificationName = [[notification userInfo]
> objectForKey:@"notificationName"];
> 
> We still position stars away from ObjC class names, so this should be "NSString
> *notificationName". Same issue elsewhere.
> 

This business about stars changing location based on what type of object you're using is madness. I'll do it, but there's a ton of code that does not do it, and it's nearly impossible to keep track of what to do.


> +    JSObjectCallAsFunction([mainFrame globalContext],
> m_notificationFunctionCallback, NULL, 1, &argument, NULL);
> 
> 0, not NULL.
> 
> +- (void)setCallback:(JSObjectRef)callback
> +{
> +    m_notificationFunctionCallback = callback;
> +}
> 
> m_notificationFunctionCallback doesn't seem to be GC protected anywhere. This
> is not a practical issue for existing tests, as the callback is protected by
> the virtue of being defined on global object, but it would be a problem for
> JavaScript code like this:
> 
> liveRegion.addNotificationListener(function() { alert(0) } );
> // Force garbage collection
> // Do something that dispatches the notification.

A layout test writer would have no reason to do that. However, I do want memory to be managed correctly. Do I just need to put in the header

RefPtr<JSObjectRef> m_notificationFunctionCallback;

[Alexey Proskuryakov]

> I don't see why its a problem to require that tests be written correctly.

This just comes from past experience. Today, I spent half a day investigating fallback from a change where automatic cleanup in DRT wasn't happening correctly - there was a test that just had to finish in an inconsistent state to test cleanup paths in WebKit.

I don't have a big problem with landing this part as is for now. It can be revisited later if it becomes an annoyance.

> I am only working with the Mac side, so this is not a C++ object.

I was talking about AccessibilityUIElement, which is a C++ object, so one needs to use CFRetain/CFRelease for its ObjC data members, and not retain/release.

> If i have two tests running in parallel

There can never be two tests running in parallel in the same process - a lot of code in DumpRenderTree assumes that there is only one test running. Or would it be a problem even with two separate DumpRenderTree processes? In the latter case, we can have multiple tests running at once, indeed.

> This business about stars changing location based on what type of object you're
> using is madness.

I couldn't agree more! But we have to follow the coding style guidelines until we can fix them.

> RefPtr<JSObjectRef> m_notificationFunctionCallback;

The APIs to use are JSValueProtect/JSValueUnprotect. Unfortunately, there is no smart pointer that would do that (since JavaScriptCore API is pure C).

[chris fleizach]

Created attachment 53868 [details]
patch

This implements most of what "ap" was asking for. modulus the other issues I brought up in my subsequent reply.

[Alexey Proskuryakov]

Comment on attachment 53868 [details]
patch

+        * Scripts/prepare-ChangeLog:

This doesn't seem intentional. Actual prepare-ChangeLog changes are also in this patch.

+- (void)setCallback:(JSObjectRef)callback
+{
+    if (!callback)
+        return;
+    
+    m_notificationFunctionCallback = callback;
+    JSValueProtect([mainFrame globalContext], m_notificationFunctionCallback);
+}

My understanding is that you intentionally don't unprotect m_notificationFunctionCallback, because addNotificationListener() can't be called twice. I think it's worth an assertion or a comment here, because this is not a usual setter pattern.

+    // Mac programmers should not be adding more than one notification listener per element.
+    // Other platforms may be different.

As an aside, is there any chance of having cross-platform accessibility tests? I do not know if that even makes sense, but we've been able to make some cross-platform tests even for inline text input.

You can consider adding a FIXME about m_element and m_notificationHandler needing CFRetain/CFRelease. It would be good to make DRT work with garbage collected ObjC one day.

Also, I still don't understand what the race condition is here (see comment 31).

r=me

[chris fleizach]

 
(In reply to comment #33)
> (From update of attachment 53868 [details])
> +        * Scripts/prepare-ChangeLog:
> 
> This doesn't seem intentional. Actual prepare-ChangeLog changes are also in
> this patch.
> 

Did not mean to add that. Will remove

> +- (void)setCallback:(JSObjectRef)callback
> +{
> +    if (!callback)
> +        return;
> +    
> +    m_notificationFunctionCallback = callback;
> +    JSValueProtect([mainFrame globalContext], m_notificationFunctionCallback);
> +}
> 
> My understanding is that you intentionally don't unprotect
> m_notificationFunctionCallback, because addNotificationListener() can't be
> called twice. I think it's worth an assertion or a comment here, because this
> is not a usual setter pattern.
> 

I can call JSValueUnprotect here. this method can be called more than once.

> +    // Mac programmers should not be adding more than one notification
> listener per element.
> +    // Other platforms may be different.
> 
> As an aside, is there any chance of having cross-platform accessibility tests?
> I do not know if that even makes sense, but we've been able to make some
> cross-platform tests even for inline text input.

There are a lot of cross platform tests for accessibility. However, notifications are tied pretty tightly to the system on the mac at least.

> 
> You can consider adding a FIXME about m_element and m_notificationHandler
> needing CFRetain/CFRelease. It would be good to make DRT work with garbage
> collected ObjC one day.
> 
> Also, I still don't understand what the race condition is here (see comment
> 31).
> 

I think I've explained this. The race condition existed before because there was one static function callback stored in webcore. because AccessibilityUIElements were not reaped at the end of a layout test, but at some point later, it was indeterminate where the function callback was pointing to and where output was going to be sent to.


> r=me

[chris fleizach]

alright its in
http://trac.webkit.org/changeset/58031
hopefully this works.

[chris fleizach]

i dare say this looked like it worked. thanx to everyone for their help

[Eric Seidel (no email)]

Thank you for the fix!