Bug 120416

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

Created attachment 209884 [details] Patch

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

Created attachment 210448 [details] Patch

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)

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.

Created attachment 210609 [details] Patch

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.

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()->'/'->'

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.

Created attachment 210757 [details] Patch Modified the patch according to review comments.

Comment on attachment 210757 [details] Patch Thanks Denis. It looks good to me now.

Comment on attachment 210757 [details] Patch Clearing flags on attachment: 210757 Committed r155192: <http://trac.webkit.org/changeset/155192>

All reviewed patches have been landed. Closing bug.