[Webkit-unassigned] [Bug 120416] [GTK] AccessibilityUIElement::addNotificationListener() crashes on debug build

[bugzilla-daemon at webkit.org]

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





--- Comment #10 from Denis Nomiyama (dnomi) <d.nomiyama at samsung.com>  2013-09-06 08:04:22 PST ---
(From update of attachment 210626)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.