Bug 120416 – [GTK] AccessibilityUIElement::addNotificationListener() crashes on debug build

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Denis Nomiyama (dnomi)]

The following tests are crashing on debug build after http://trac.webkit.org/changeset/154697.

accessibility/aria-checkbox-sends-notification-crash-log.txt
accessibility/multiselect-list-reports-active-option-crash-log.txt
accessibility/aria-invalid-crash-log.txt
accessibility/notification-listeners-crash-log.txt
accessibility/menu-list-sends-change-notification-crash-log.txt

Callstack is copied below.

#0  0x00007f65139aaec9 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:342
#1  0x00000000004a14b1 in WTF::RefCountedBase::ref (this=0xe94740) at ../../Source/WTF/wtf/RefCounted.h:59
#2  0x00000000004ad6e1 in WTF::refIfNotNull<AccessibilityNotificationHandler> (ptr=0xe94740) at ../../Source/WTF/wtf/PassRefPtr.h:46
#3  0x00000000004ad4e7 in WTF::RefPtr<AccessibilityNotificationHandler>::RefPtr (this=0x7fff62b2a330, ptr=0xe94740) at ../../Source/WTF/wtf/RefPtr.h:43
#4  0x00000000004ad1a0 in WTF::RefPtr<AccessibilityNotificationHandler>::operator= (this=0xe8dd78, optr=0xe94740) at ../../Source/WTF/wtf/RefPtr.h:126
#5  0x00000000004ac370 in AccessibilityUIElement::addNotificationListener (this=0xe8dd70, functionCallback=0x7f64c009dbf0) at ../../Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1038
#6  0x0000000000499742 in addNotificationListenerCallback (context=0x7f64b3c00108, function=0x7f64c005fbb0, thisObject=0x7f64c005fbf0, argumentCount=1, arguments=0x7fff62b2a400, exception=0x7fff62b2a498) at ../../Tools/DumpRenderTree/AccessibilityUIElement.cpp:1010
#7  0x00007f6513545921 in JSC::APICallbackFunction::call<JSC::JSCallbackFunction> (exec=0x7f64b3c00108) at ../../Source/JavaScriptCore/API/APICallbackFunction.h:59
#8  0x00007f6513800fd2 in JSC::LLInt::handleHostCall (execCallee=0x7f64b3c00108, pc=0xe18ec0, callee=..., kind=JSC::CodeForCall) at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:949
#9  0x00007f6513804454 in JSC::LLInt::setUpCall (execCallee=0x7f64b3c00108, pc=0xe18ec0, kind=JSC::CodeForCall, calleeAsValue=..., callLinkInfo=0xdd0cd8) at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:993
#10 0x00007f65138048f6 in JSC::LLInt::genericCall (exec=0x7f64b3c000a0, pc=0xe18ec0, kind=JSC::CodeForCall) at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1054
#11 0x00007f65138013b2 in JSC::LLInt::llint_slow_path_call (exec=0x7f64b3c000a0, pc=0xe18ec0) at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1060
...

[Denis Nomiyama (dnomi)]

I found a additional problem at AccessibilityCallbacksAtk.cpp where the HashMap iterator is removed inside the for loop and causing a crash at ++it.

[Denis Nomiyama (dnomi)]

Created attachment 209884 [details]
Patch

[Mario Sanchez Prada]

*** Bug 120667 has been marked as a duplicate of this bug. ***

[Denis Nomiyama (dnomi)]

Created attachment 210448 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 210448 [details]
Patch

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

Thanks for taking care of this. See my comments below...

> Tools/ChangeLog:10
> +        m_notificationHandler expected RefPtr.

Nit. You might probably use an extra line here to separate paragraphs

> Tools/ChangeLog:14
> +        * DumpRenderTree/atk/AccessibilityCallbacks.h: modified the global notification key from 0 to 1

Please use proper sentences starting with capital letters and ending with a period.

> Tools/ChangeLog:16
> +        (axObjectEventListener): Moved the call to the callback function out of the loop

Missing period. Same applies to the remaining bullet points from this entry.

> Tools/DumpRenderTree/atk/AccessibilityCallbacks.h:35
> -const PlatformUIElement GlobalNotificationKey = 0;
> +const PlatformUIElement GlobalNotificationKey = reinterpret_cast<PlatformUIElement>(0x1);

This change seems unrelated to this fix, so please do not include it if it's not needed.

Besides, the whole thing of having this cryptic value in a header file to be used as a key in a hashmap present in the implementatoin file (hence private) it's a bit too obscure too me. Perhaps it would be nice to have make a patch in the future just for either find a better way to do it or at least document it?

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:136
> +        AccessibilityNotificationHandler* notificationHandler = 0;
>          for (HashMap<PlatformUIElement, AccessibilityNotificationHandler*>::iterator it = notificationHandlers.begin(); it != notificationHandlers.end(); ++it) {
> -            if (it->key == accessible || it->key == GlobalNotificationKey) {
> -                JSRetainPtr<JSStringRef> jsNotificationEventName(Adopt, JSStringCreateWithUTF8CString(reinterpret_cast<const char*>(notificationName.utf8().data())));
> -                JSValueRef notificationNameArgument = JSValueMakeString(jsContext, jsNotificationEventName.get());
> -                AccessibilityNotificationHandler* notificationHandler = it->value;
> -                if (notificationHandler->platformElement()) {
> -                    JSValueRef argument = notificationNameArgument;
> -                    // Listener for one element just gets one argument, the notification name.
> -                    JSObjectCallAsFunction(jsContext, notificationHandler->notificationFunctionCallback(), 0, 1, &argument, 0);
> -                } else {
> -                    // A global listener gets the element and the notification name as arguments.
> -                    JSValueRef arguments[2];
> -                    arguments[0] = AccessibilityUIElement::makeJSAccessibilityUIElement(jsContext, AccessibilityUIElement(accessible));
> -                    arguments[1] = notificationNameArgument;
> -                    JSObjectCallAsFunction(jsContext, notificationHandler->notificationFunctionCallback(), 0, 2, arguments, 0);
> -                }
> +            if (it->key == accessible) {
> +                notificationHandler = it->value;
> +                break;
>              }
>          }

It looks to me like you could simplify this code by replacing it with a single line:

  AccessibilityNotificationHandler* notificationHandler = notificationHandlers.find(accessible);

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:138
> +        JSRetainPtr<JSStringRef> jsNotificationEventName(Adopt, JSStringCreateWithUTF8CString(reinterpret_cast<const char*>(notificationName.utf8().data())));

CString::data() already returns a const char*, so I don't think you need this reinterpret_cast here.

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:140
> +        if (notificationHandler)  {

It seems notificationHandler, if present, is only used inside this if branch.

Considering my previous comment, perhaps you could do just something like the following?

  if (AccessibilityNotificationHandler* notificationHandler = notificationHandlers.find(accessible)) {
     ...
  }

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:141
> +            JSValueRef argument = notificationNameArgument;

You don't need argument variable, you can use notificationNameArgument directly below, giving you a one-line if branch (so you can avoid the {} too)

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:152
> +        if (notificationHandlers.contains(GlobalNotificationKey)) {
> +            // A global listener gets the element and the notification name as arguments.
> +            JSValueRef arguments[2];
> +            arguments[0] = AccessibilityUIElement::makeJSAccessibilityUIElement(jsContext, AccessibilityUIElement(accessible));
> +            arguments[1] = notificationNameArgument;
> +            JSObjectCallAsFunction(jsContext, notificationHandlers.find(GlobalNotificationKey)->value->notificationFunctionCallback(), 0, 2, arguments, 0);
> +        }

I would probably do this without using the contains() method, by doing something like this:

  if (AccessibilityNotificationHandler* globalHandler = notificationHandlers.find(GlobalNotificationKey)) {
      // A global listener gets the element and the notification name as arguments.
      JSValueRef arguments[2];
      arguments[0] = AccessibilityUIElement::makeJSAccessibilityUIElement(jsContext, AccessibilityUIElement(accessible));
      arguments[1] = notificationNameArgument;
      JSObjectCallAsFunction(jsContext, globalHandler->value->notificationFunctionCallback(), 0, 2, arguments, 0);
  }

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:235
> +    if (notificationHandlers.contains(notificationHandler->platformElement())) {
> +        JSValueUnprotect(jsContext, notificationHandlers.find(notificationHandler->platformElement())->value->notificationFunctionCallback());
> +        notificationHandlers.remove(notificationHandler->platformElement());

Following the idea of my previous comment, you can probably replace this two calls to contains() and find() with just one call to find() here as well.

Additionally, perhaps it would be interesting to check first that notificationHandler->platformElement() is not null?

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:265
> +    HashMap<PlatformUIElement, AccessibilityNotificationHandler*>::iterator it;
> +    for (it = notificationHandlers.begin(); it != notificationHandlers.end(); ++it) {
>          if (it->value == notificationHandler) {
>              JSValueUnprotect(jsContext, notificationHandler->notificationFunctionCallback());
> -            notificationHandlers.remove(it);
> +            break;
>          }
>      }
> +
> +    if (it.get())
> +        notificationHandlers.remove(it);

It looks to me like the key thing here is the missing break and that, if you add that, you can keep the call to remove inside the loop and avoid doing it after it, leading to something more compact such as:

  for (HashMap<PlatformUIElement, AccessibilityNotificationHandler*>::iterator it = notificationHandlers.begin(); it != notificationHandlers.end(); ++it) {
     if (it->value == notificationHandler) {
        JSValueUnprotect(jsContext, notificationHandler->notificationFunctionCallback());
        notificationHandlers.remove(it);
        break;
     }
  }

> LayoutTests/ChangeLog:15
> +        - accessibility/multiselect-list-reports-active-option.html
> +        - accessibility/notification-listeners.html
> +        - accessibility/menu-list-sends-change-notification.html
> +        - accessibility/aria-invalid.html
> +        - accessibility/aria-checkbox-sends-notification.html

You don't have to explicitly list these tests here if you want. You can be more generic in this case :)

> LayoutTests/ChangeLog:18
> +        Skipped accessibility/notification-listeners.html both on Release and Debug builds since it is not
> +        crashing on Debug anymore.

You are not actually skipping tests, just updating the expectations to expect a failure. It might be worth commenting here that this issue is being tracked in a separate bug (bug 120669)

[Denis Nomiyama (dnomi)]

Comment on attachment 210448 [details]
Patch

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

>> Tools/ChangeLog:10
>> +        m_notificationHandler expected RefPtr.
> 
> Nit. You might probably use an extra line here to separate paragraphs

Ok. I'll do.

>> Tools/ChangeLog:14
>> +        * DumpRenderTree/atk/AccessibilityCallbacks.h: modified the global notification key from 0 to 1
> 
> Please use proper sentences starting with capital letters and ending with a period.

Ok.

>> Tools/ChangeLog:16
>> +        (axObjectEventListener): Moved the call to the callback function out of the loop
> 
> Missing period. Same applies to the remaining bullet points from this entry.

Ok.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacks.h:35
>> +const PlatformUIElement GlobalNotificationKey = reinterpret_cast<PlatformUIElement>(0x1);
> 
> This change seems unrelated to this fix, so please do not include it if it's not needed.
> 
> Besides, the whole thing of having this cryptic value in a header file to be used as a key in a hashmap present in the implementatoin file (hence private) it's a bit too obscure too me. Perhaps it would be nice to have make a patch in the future just for either find a better way to do it or at least document it?

Before the changes for this review, I could move this fix to bug 70606, where the implementation of global notifications is being done. But the new patch will have a call to find() at axObjectEventListener() and the debug build will crash if the key is zero.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:136
>>          }
> 
> It looks to me like you could simplify this code by replacing it with a single line:
> 
>   AccessibilityNotificationHandler* notificationHandler = notificationHandlers.find(accessible);

Nice one. I'll do something similar. Just have to modify it a bit because find() returns an iterator.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:138
>> +        JSRetainPtr<JSStringRef> jsNotificationEventName(Adopt, JSStringCreateWithUTF8CString(reinterpret_cast<const char*>(notificationName.utf8().data())));
> 
> CString::data() already returns a const char*, so I don't think you need this reinterpret_cast here.

Ops. Sorry.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:140
>> +        if (notificationHandler)  {
> 
> It seems notificationHandler, if present, is only used inside this if branch.
> 
> Considering my previous comment, perhaps you could do just something like the following?
> 
>   if (AccessibilityNotificationHandler* notificationHandler = notificationHandlers.find(accessible)) {
>      ...
>   }

Ok I'll simplify it.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:141
>> +            JSValueRef argument = notificationNameArgument;
> 
> You don't need argument variable, you can use notificationNameArgument directly below, giving you a one-line if branch (so you can avoid the {} too)

Good point. I'll do it.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:152
>> +        }
> 
> I would probably do this without using the contains() method, by doing something like this:
> 
>   if (AccessibilityNotificationHandler* globalHandler = notificationHandlers.find(GlobalNotificationKey)) {
>       // A global listener gets the element and the notification name as arguments.
>       JSValueRef arguments[2];
>       arguments[0] = AccessibilityUIElement::makeJSAccessibilityUIElement(jsContext, AccessibilityUIElement(accessible));
>       arguments[1] = notificationNameArgument;
>       JSObjectCallAsFunction(jsContext, globalHandler->value->notificationFunctionCallback(), 0, 2, arguments, 0);
>   }

Nice one. I'll do something similar, just because find() returns an iterator.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:235
>> +        notificationHandlers.remove(notificationHandler->platformElement());
> 
> Following the idea of my previous comment, you can probably replace this two calls to contains() and find() with just one call to find() here as well.
> 
> Additionally, perhaps it would be interesting to check first that notificationHandler->platformElement() is not null?

Sure. I'll do it.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:265
>> +        notificationHandlers.remove(it);
> 
> It looks to me like the key thing here is the missing break and that, if you add that, you can keep the call to remove inside the loop and avoid doing it after it, leading to something more compact such as:
> 
>   for (HashMap<PlatformUIElement, AccessibilityNotificationHandler*>::iterator it = notificationHandlers.begin(); it != notificationHandlers.end(); ++it) {
>      if (it->value == notificationHandler) {
>         JSValueUnprotect(jsContext, notificationHandler->notificationFunctionCallback());
>         notificationHandlers.remove(it);
>         break;
>      }
>   }

Ok. I think we can use find() here as well. I'll give it a try.

>> LayoutTests/ChangeLog:15
>> +        - accessibility/aria-checkbox-sends-notification.html
> 
> You don't have to explicitly list these tests here if you want. You can be more generic in this case :)

Cool ok.

>> LayoutTests/ChangeLog:18
>> +        crashing on Debug anymore.
> 
> You are not actually skipping tests, just updating the expectations to expect a failure. It might be worth commenting here that this issue is being tracked in a separate bug (bug 120669)

Ok. I will fix the comment and will add the reference to the other bug.

[Denis Nomiyama (dnomi)]

Created attachment 210609 [details]
Patch

[Denis Nomiyama (dnomi)]

Created attachment 210626 [details]
Patch

Just realised the code can be more readable and concise by moving the global notification handler from the HashTable to a separate pointer.

[Mario Sanchez Prada]

Comment on attachment 210626 [details]
Patch

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

Thanks Denis for working on this. I think the patch is better and cleaner now. Just setting the r- because of the small issues found and pointed below

> Tools/DumpRenderTree/atk/AccessibilityCallbacks.h:-36
> -const PlatformUIElement GlobalNotificationKey = 0;
> -

I love this change :)

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:51
> +typedef HashMap<PlatformUIElement, AccessibilityNotificationHandler*> NotificationHandlers;

'NotificationHandlers' does not seem to me like a good name for a type, it looks more like a variable name to me.

What about NotificationHandlersMap?

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:138
> +            JSObjectCallAsFunction(jsContext, elementNotificationHandler.get()->value->notificationFunctionCallback(), 0, 1, &notificationNameArgument, 0);

HashTableIteratorAdapter has the '->' operator overloaded to behave as get(), so you can't replace ".get()->" here for just "->"

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:232
> +            JSValueUnprotect(jsContext, currentNotificationHandler.get()->value->notificationFunctionCallback());
> +            notificationHandlers.remove(currentNotificationHandler.get()->value->platformElement());

Replace ".get()->" here for just "->" here as well

Additionally, you might want to check that currentNotificationHandler->value->platformElement() is valid before using it?

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:261
> +        NotificationHandlers::iterator removeNotificationHandler = notificationHandlers.find(notificationHandler->platformElement());

Same concern about notificationHandler->platformElement() here. It might be interesting to check that it's a valid value before using it.

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:263
> +            JSValueUnprotect(jsContext, removeNotificationHandler.get()->value->notificationFunctionCallback());

s/'.get()->'/'->'

[Denis Nomiyama (dnomi)]

Comment on attachment 210626 [details]
Patch

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

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:51
>> +typedef HashMap<PlatformUIElement, AccessibilityNotificationHandler*> NotificationHandlers;
> 
> 'NotificationHandlers' does not seem to me like a good name for a type, it looks more like a variable name to me.
> 
> What about NotificationHandlersMap?

Agree, the name doesn't help much. I'll change it.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:138
>> +            JSObjectCallAsFunction(jsContext, elementNotificationHandler.get()->value->notificationFunctionCallback(), 0, 1, &notificationNameArgument, 0);
> 
> HashTableIteratorAdapter has the '->' operator overloaded to behave as get(), so you can't replace ".get()->" here for just "->"

Ok nice one.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:232
>> +            notificationHandlers.remove(currentNotificationHandler.get()->value->platformElement());
> 
> Replace ".get()->" here for just "->" here as well
> 
> Additionally, you might want to check that currentNotificationHandler->value->platformElement() is valid before using it?

Ops ok. I will replace get() with ->.
In case of checking platformElement() before using it, I think we can add an assertion because if it is zero, then there is problem in the find() implementation.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:261
>> +        NotificationHandlers::iterator removeNotificationHandler = notificationHandlers.find(notificationHandler->platformElement());
> 
> Same concern about notificationHandler->platformElement() here. It might be interesting to check that it's a valid value before using it.

Ok. I'll add a check here.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:263
>> +            JSValueUnprotect(jsContext, removeNotificationHandler.get()->value->notificationFunctionCallback());
> 
> s/'.get()->'/'->'

Ok.

[Denis Nomiyama (dnomi)]

Created attachment 210757 [details]
Patch

Modified the patch according to review comments.

[Mario Sanchez Prada]

Comment on attachment 210757 [details]
Patch

Thanks Denis. It looks good to me now.

[WebKit Commit Bot]

Comment on attachment 210757 [details]
Patch

Clearing flags on attachment: 210757

Committed r155192: <http://trac.webkit.org/changeset/155192>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.