Changeset 155192 in webkit


Ignore:
Timestamp:
Sep 6, 2013 9:32:30 AM (11 years ago)
Author:
commit-queue@webkit.org
Message:

[GTK] AccessibilityUIElement::addNotificationListener() crashes on debug build
https://bugs.webkit.org/show_bug.cgi?id=120416

Patch by Denis Nomiyama <d.nomiyama@samsung.com> on 2013-09-06
Reviewed by Mario Sanchez Prada.

Tools:

Fixed crashes when running debug DRT. Simplified loops at AccessibilityCallbackAtk.cpp where the HashMap
iterator was removed inside a loop. Fixed AccessibilityUIElement::addNotificationListener() where
m_notificationHandler expected RefPtr.

The global notification handler was stored in the HashMap with key 0. And this caused an assertion when
HashMap::add() or find() are called. To fix it, moved the global handler to a separated pointer.

  • DumpRenderTree/atk/AccessibilityCallbacks.h: Removed the global notification key.
  • DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:

(axObjectEventListener): Simplified the code by using HashMap::find() and a separate pointer for the
global notification handler.
(addAccessibilityNotificationHandler): Simplified the code by using HashMap::find() and a separate pointer
for the global notification handler.
(removeAccessibilityNotificationHandler): Added the removal for the global notification handler.

  • DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.h:

(AccessibilityNotificationHandler::create): Added static function to create
AccessibilityNotificationHandler.

  • DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:

(AccessibilityUIElement::addNotificationListener): Assigned m_notificationHandler with
AccessibilityNotificationHandler::create().

LayoutTests:

Fixed crashes when running debug DRT on tests that require an a11y notification handler.

  • platform/gtk/TestExpectations: Unskipped some a11y tests that were crashing before this fix.

Updated the expectation of accessibility/notification-listeners.html to expect failure on Debug build
since it is not crashing anymore. This other issue is tracked in a separate bug (bug 120669).

Location:
trunk
Files:
8 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r155191 r155192  
     12013-09-06  Denis Nomiyama  <d.nomiyama@samsung.com>
     2
     3        [GTK] AccessibilityUIElement::addNotificationListener() crashes on debug build
     4        https://bugs.webkit.org/show_bug.cgi?id=120416
     5
     6        Reviewed by Mario Sanchez Prada.
     7
     8        Fixed crashes when running debug DRT on tests that require an a11y notification handler.
     9
     10        * platform/gtk/TestExpectations: Unskipped some a11y tests that were crashing before this fix.
     11        Updated the expectation of accessibility/notification-listeners.html to expect failure on Debug build
     12        since it is not crashing anymore. This other issue is tracked in a separate bug (bug 120669).
     13
    1142013-09-06  Chris Fleizach  <cfleizach@apple.com>
    215
  • trunk/LayoutTests/platform/gtk/TestExpectations

    r155184 r155192  
    497497
    498498webkit.org/b/120200 [ Debug ] http/tests/security/XFrameOptions/x-frame-options-allowall.html [ Crash Pass ]
    499 
    500 webkit.org/b/120667 [ Debug ] accessibility/multiselect-list-reports-active-option.html [ Crash ]
    501 webkit.org/b/120667 [ Debug ] accessibility/notification-listeners.html [ Crash ]
    502 webkit.org/b/120667 [ Debug ] accessibility/menu-list-sends-change-notification.html [ Crash ]
    503 webkit.org/b/120667 [ Debug ] accessibility/aria-invalid.html [ Crash ]
    504 webkit.org/b/120667 [ Debug ] accessibility/aria-checkbox-sends-notification.html [ Crash ]
    505499
    506500#////////////////////////////////////////////////////////////////////////////////////////
     
    14601454webkit.org/b/120203 http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html [ Failure ]
    14611455
    1462 webkit.org/b/120669 [ Release ] accessibility/notification-listeners.html [ Failure ]
     1456webkit.org/b/120669 accessibility/notification-listeners.html [ Failure ]
    14631457
    14641458webkit.org/b/120670 accessibility/lists.html [ Failure ]
  • trunk/Tools/ChangeLog

    r155188 r155192  
     12013-09-06  Denis Nomiyama  <d.nomiyama@samsung.com>
     2
     3        [GTK] AccessibilityUIElement::addNotificationListener() crashes on debug build
     4        https://bugs.webkit.org/show_bug.cgi?id=120416
     5
     6        Reviewed by Mario Sanchez Prada.
     7
     8        Fixed crashes when running debug DRT. Simplified loops at AccessibilityCallbackAtk.cpp where the HashMap
     9        iterator was removed inside a loop. Fixed AccessibilityUIElement::addNotificationListener() where
     10        m_notificationHandler expected RefPtr.
     11
     12        The global notification handler was stored in the HashMap with key 0. And this caused an assertion when
     13        HashMap::add() or find() are called. To fix it, moved the global handler to a separated pointer.
     14
     15        * DumpRenderTree/atk/AccessibilityCallbacks.h: Removed the global notification key.
     16        * DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:
     17        (axObjectEventListener): Simplified the code by using HashMap::find() and a separate pointer for the
     18        global notification handler.
     19        (addAccessibilityNotificationHandler): Simplified the code by using HashMap::find() and a separate pointer
     20        for the global notification handler.
     21        (removeAccessibilityNotificationHandler): Added the removal for the global notification handler.
     22        * DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.h:
     23        (AccessibilityNotificationHandler::create): Added static function to create
     24        AccessibilityNotificationHandler.
     25        * DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:
     26        (AccessibilityUIElement::addNotificationListener): Assigned m_notificationHandler with
     27        AccessibilityNotificationHandler::create().
     28
    1292013-09-06  Gabor Abraham  <abrhm@inf.u-szeged.hu>
    230
  • trunk/Tools/DumpRenderTree/atk/AccessibilityCallbacks.h

    r154697 r155192  
    3333#include "AccessibilityUIElement.h"
    3434
    35 const PlatformUIElement GlobalNotificationKey = 0;
    36 
    3735void connectAccessibilityCallbacks();
    3836void disconnectAccessibilityCallbacks();
  • trunk/Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp

    r154960 r155192  
    4949#endif
    5050
     51typedef HashMap<PlatformUIElement, AccessibilityNotificationHandler*> NotificationHandlersMap;
     52
    5153static guint stateChangeListenerId = 0;
    5254static guint focusEventListenerId = 0;
     
    5557static guint propertyChangedListenerId = 0;
    5658static guint visibleDataChangedListenerId = 0;
    57 static HashMap<PlatformUIElement, AccessibilityNotificationHandler*> notificationHandlers;
     59static NotificationHandlersMap notificationHandlers;
     60static AccessibilityNotificationHandler* globalNotificationHandler = 0;
    5861
    5962extern bool loggingAccessibilityEvents;
     
    128131
    129132    if (notificationName.length()) {
    130         for (HashMap<PlatformUIElement, AccessibilityNotificationHandler*>::iterator it = notificationHandlers.begin(); it != notificationHandlers.end(); ++it) {
    131             if (it->key == accessible || it->key == GlobalNotificationKey) {
    132                 JSRetainPtr<JSStringRef> jsNotificationEventName(Adopt, JSStringCreateWithUTF8CString(reinterpret_cast<const char*>(notificationName.utf8().data())));
    133                 JSValueRef notificationNameArgument = JSValueMakeString(jsContext, jsNotificationEventName.get());
    134                 AccessibilityNotificationHandler* notificationHandler = it->value;
    135                 if (notificationHandler->platformElement()) {
    136                     JSValueRef argument = notificationNameArgument;
    137                     // Listener for one element just gets one argument, the notification name.
    138                     JSObjectCallAsFunction(jsContext, notificationHandler->notificationFunctionCallback(), 0, 1, &argument, 0);
    139                 } else {
    140                     // A global listener gets the element and the notification name as arguments.
    141                     JSValueRef arguments[2];
    142                     arguments[0] = AccessibilityUIElement::makeJSAccessibilityUIElement(jsContext, AccessibilityUIElement(accessible));
    143                     arguments[1] = notificationNameArgument;
    144                     JSObjectCallAsFunction(jsContext, notificationHandler->notificationFunctionCallback(), 0, 2, arguments, 0);
    145                 }
    146             }
     133        JSRetainPtr<JSStringRef> jsNotificationEventName(Adopt, JSStringCreateWithUTF8CString(notificationName.utf8().data()));
     134        JSValueRef notificationNameArgument = JSValueMakeString(jsContext, jsNotificationEventName.get());
     135        NotificationHandlersMap::iterator elementNotificationHandler = notificationHandlers.find(accessible);
     136        if (elementNotificationHandler != notificationHandlers.end()) {
     137            // Listener for one element just gets one argument, the notification name.
     138            JSObjectCallAsFunction(jsContext, elementNotificationHandler->value->notificationFunctionCallback(), 0, 1, &notificationNameArgument, 0);
     139        } else if (globalNotificationHandler) {
     140            // A global listener gets the element and the notification name as arguments.
     141            JSValueRef arguments[2];
     142            arguments[0] = AccessibilityUIElement::makeJSAccessibilityUIElement(jsContext, AccessibilityUIElement(accessible));
     143            arguments[1] = notificationNameArgument;
     144            JSObjectCallAsFunction(jsContext, globalNotificationHandler->notificationFunctionCallback(), 0, 2, arguments, 0);
    147145        }
    148146    }
     
    229227    // Check if this notification handler is related to a specific element.
    230228    if (notificationHandler->platformElement()) {
    231         if (notificationHandlers.contains(notificationHandler->platformElement())) {
    232             JSValueUnprotect(jsContext, notificationHandlers.find(notificationHandler->platformElement())->value->notificationFunctionCallback());
    233             notificationHandlers.remove(notificationHandler->platformElement());
     229        NotificationHandlersMap::iterator currentNotificationHandler = notificationHandlers.find(notificationHandler->platformElement());
     230        if (currentNotificationHandler != notificationHandlers.end()) {
     231            ASSERT(currentNotificationHandler->value->platformElement());
     232            JSValueUnprotect(jsContext, currentNotificationHandler->value->notificationFunctionCallback());
     233            notificationHandlers.remove(currentNotificationHandler->value->platformElement());
    234234        }
    235235        notificationHandlers.add(notificationHandler->platformElement(), notificationHandler);
    236236    } else {
    237         if (notificationHandlers.contains(GlobalNotificationKey)) {
    238             JSValueUnprotect(jsContext, notificationHandlers.find(GlobalNotificationKey)->value->notificationFunctionCallback());
    239             notificationHandlers.remove(GlobalNotificationKey);
    240         }
    241         notificationHandlers.add(GlobalNotificationKey, notificationHandler);
     237        if (globalNotificationHandler)
     238            JSValueUnprotect(jsContext, globalNotificationHandler->notificationFunctionCallback());
     239        globalNotificationHandler = notificationHandler;
    242240    }
    243241
     
    258256        return;
    259257
    260     for (HashMap<PlatformUIElement, AccessibilityNotificationHandler*>::iterator it = notificationHandlers.begin(); it != notificationHandlers.end(); ++it) {
    261         if (it->value == notificationHandler) {
    262             JSValueUnprotect(jsContext, notificationHandler->notificationFunctionCallback());
    263             notificationHandlers.remove(it);
     258    if (globalNotificationHandler == notificationHandler) {
     259        JSValueUnprotect(jsContext, globalNotificationHandler->notificationFunctionCallback());
     260        globalNotificationHandler = 0;
     261    } else if (notificationHandler->platformElement()) {
     262        NotificationHandlersMap::iterator removeNotificationHandler = notificationHandlers.find(notificationHandler->platformElement());
     263        if (removeNotificationHandler != notificationHandlers.end()) {
     264            JSValueUnprotect(jsContext, removeNotificationHandler->value->notificationFunctionCallback());
     265            notificationHandlers.remove(removeNotificationHandler);
    264266        }
    265267    }
  • trunk/Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.cpp

    r154697 r155192  
    2424
    2525AccessibilityNotificationHandler::AccessibilityNotificationHandler(void)
    26     : m_platformElement(GlobalNotificationKey)
     26    : m_platformElement(0)
    2727    , m_notificationFunctionCallback(0)
    2828{
  • trunk/Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.h

    r154697 r155192  
    2323#include <JavaScriptCore/JSObjectRef.h>
    2424#include <atk/atk.h>
     25#include <wtf/PassRefPtr.h>
    2526#include <wtf/RefCounted.h>
    2627
    2728class AccessibilityNotificationHandler : public RefCounted<AccessibilityNotificationHandler> {
    2829public:
     30    static PassRefPtr<AccessibilityNotificationHandler> create()
     31    {
     32        return adoptRef(new AccessibilityNotificationHandler());
     33    }
    2934    AccessibilityNotificationHandler(void);
    3035    ~AccessibilityNotificationHandler();
  • trunk/Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp

    r155041 r155192  
    10511051        return false;
    10521052
    1053     m_notificationHandler = new AccessibilityNotificationHandler();
     1053    m_notificationHandler = AccessibilityNotificationHandler::create();
    10541054    m_notificationHandler->setPlatformElement(platformUIElement());
    10551055    m_notificationHandler->setNotificationFunctionCallback(functionCallback);
Note: See TracChangeset for help on using the changeset viewer.