Bug 127141 – platform/mac/accessibility/aria-multiline.html sometimes asserts in AccessibilityController::removeNotificationListener

• Component: Accessibility
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

platform/mac/accessibility/aria-multiline.html sometimes asserts on Mac WebKit1:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010674845a WTFCrash + 42 (Assertions.cpp:333)
1   DumpRenderTree                	0x0000000105f4e71a AccessibilityController::removeNotificationListener() + 74 (AccessibilityControllerMac.mm:141)
2   DumpRenderTree                	0x0000000105f4db06 removeNotificationListenerCallback(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**) + 54 (AccessibilityController.cpp:130)
3   com.apple.JavaScriptCore      	0x0000000106456007 long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*) + 455 (APICallbackFunction.h:61)
4   com.apple.JavaScriptCore      	0x000000010641b7d5 handleHostCall + 293 (JITOperations.cpp:654)
5   com.apple.JavaScriptCore      	0x000000010641c175 linkFor + 133 (JITOperations.cpp:697)
6   com.apple.JavaScriptCore      	0x00000001064170ca operationLinkCall + 26 (JITOperations.cpp:730)

Looks like this started today, perhaps with <http://trac.webkit.org/changeset/162132>.

[Radar WebKit Bug Importer]

<rdar://problem/15839077>

[chris fleizach]

now that all the notifications are perhaps being sent, there might be a test that is not cleaning up after itself

[chris fleizach]

(In reply to comment #2)
> now that all the notifications are perhaps being sent, there might be a test that is not cleaning up after itself

looking into it

[chris fleizach]

(In reply to comment #3)
> (In reply to comment #2)
> > now that all the notifications are perhaps being sent, there might be a test that is not cleaning up after itself
> 
> looking into it

this would indicate some test didn't call removeNotificationListener() when it finished, which means that maybe a test timed out as well?

if we can't figure this out and it becomes an issue, we can probably remove this assert

ASSERT(m_globalNotificationHandler); in DRT and be ok as well

[Alexey Proskuryakov]

Would it be possible to remove the listeners automatically? It is nearly always the responsibility of DumpRenderTree to ensure that tests don't affect each other, regardless of what they do.

[chris fleizach]

(In reply to comment #5)
> Would it be possible to remove the listeners automatically? It is nearly always the responsibility of DumpRenderTree to ensure that tests don't affect each other, regardless of what they do.

Seems reasonable. I forget the issues why I couldn't do it before like that

[Alexey Proskuryakov]

Affects other tests too: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r162151%20(12285)/platform/mac/accessibility/aria-menubar-crash-log.txt> (this one was reported as a failure in a different test from what CrashLog itself says).

[chris fleizach]

Created attachment 221429 [details]
patch

[WebKit Commit Bot]

Comment on attachment 221429 [details]
patch

Clearing flags on attachment: 221429

Committed r162179: <http://trac.webkit.org/changeset/162179>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Hmm, while this still looks like a good change to me, I'm not sure how it could fix the assertions - and indeed, they still occur:

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r162180%20(1934)/platform/mac/accessibility/aria-menubar-crash-log.txt

[chris fleizach]

(In reply to comment #11)
> Hmm, while this still looks like a good change to me, I'm not sure how it could fix the assertions - and indeed, they still occur:
> 
> http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r162180%20(1934)/platform/mac/accessibility/aria-menubar-crash-log.txt

Looks like there are two asserts we need to investigate.

[chris fleizach]

Created attachment 221478 [details]
patch

[chris fleizach]

(In reply to comment #13)
> Created an attachment (id=221478) [details]
> patch

So i was under the impression that AXController would dealloc when the page went away. That is not correct.

We need to clean up this stuff on the resetToConsistentState() method

[Alexey Proskuryakov]

Comment on attachment 221478 [details]
patch

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

r=me

> Tools/DumpRenderTree/ios/AccessibilityControllerIOS.mm:126
> +{
> +}

Not needed on iOS?

> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:145
> +    // No longer a need to cleanup for tests, since resetToConsistentState will remove the listener.

Should we remove this function completely, including from all tests?

[chris fleizach]

(In reply to comment #15)
> (From update of attachment 221478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221478&action=review
> 
> r=me
> 
> > Tools/DumpRenderTree/ios/AccessibilityControllerIOS.mm:126
> > +{
> > +}
> 
> Not needed on iOS?

It looks like iOS doesn't allow notifications on the axController (yet)

> 
> > Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:145
> > +    // No longer a need to cleanup for tests, since resetToConsistentState will remove the listener.
> 
> Should we remove this function completely, including from all tests?

I think so. I'll file a new bug

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

[WebKit Commit Bot]

Comment on attachment 221478 [details]
patch

Clearing flags on attachment: 221478

Committed r162222: <http://trac.webkit.org/changeset/162222>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.