Bug 87426 – [Win] ASSERTION FAILED !m_ptr under AccessibilityController::winAddNotificationListener

• Component: Accessibility
• Status: RESOLVED FIXED

[Jessie Berlin]

The following tests all 'crash' when running the tests under debug:

accessibility/menu-list-sends-change-notification.html
accessibility/multiselect-list-reports-active-option.html
platform/win/accessibility/detached-object-notification-crash.html

Unhandled exception at 0x73728b2a (DumpRenderTree.dll) in DumpRenderTree.exe: 0xC0000005: Access violation writing location 0xbbadbeef.

>	DumpRenderTree.dll!COMPtr<IAccessible>::operator&()  Line 76 + 0x3a bytes	C++
 	DumpRenderTree.dll!WTF::HashTraits<COMPtr<IAccessible> >::constructDeletedValue(COMPtr<IAccessible> & slot={...})  Line 223 + 0x1c bytes	C++
 	DumpRenderTree.dll!WTF::PairHashTraits<WTF::HashTraits<COMPtr<IAccessible> >,WTF::HashTraits<OpaqueJSValue *> >::constructDeletedValue(std::pair<COMPtr<IAccessible>,OpaqueJSValue *> & slot=({m_ptr=0xcccccccc },0xcccccccc))  Line 185 + 0xc bytes	C++
 	DumpRenderTree.dll!WTF::HashTable<COMPtr<IAccessible>,std::pair<COMPtr<IAccessible>,OpaqueJSValue *>,WTF::PairFirstExtractor<std::pair<COMPtr<IAccessible>,OpaqueJSValue *> >,WTF::PtrHash<COMPtr<IAccessible> >,WTF::HashMapValueTraits<WTF::HashTraits<COMPtr<IAccessible> >,WTF::HashTraits<OpaqueJSValue *> >,WTF::HashTraits<COMPtr<IAccessible> > >::checkKey<WTF::HashMapTranslator<WTF::HashMapValueTraits<WTF::HashTraits<COMPtr<IAccessible> >,WTF::HashTraits<OpaqueJSValue *> >,WTF::PtrHash<COMPtr<IAccessible> > >,COMPtr<IAccessible> >(const COMPtr<IAccessible> & key={...})  Line 502 + 0x9 bytes	C++
 	DumpRenderTree.dll!WTF::HashTable<COMPtr<IAccessible>,std::pair<COMPtr<IAccessible>,OpaqueJSValue *>,WTF::PairFirstExtractor<std::pair<COMPtr<IAccessible>,OpaqueJSValue *> >,WTF::PtrHash<COMPtr<IAccessible> >,WTF::HashMapValueTraits<WTF::HashTraits<COMPtr<IAccessible> >,WTF::HashTraits<OpaqueJSValue *> >,WTF::HashTraits<COMPtr<IAccessible> > >::add<WTF::HashMapTranslator<WTF::HashMapValueTraits<WTF::HashTraits<COMPtr<IAccessible> >,WTF::HashTraits<OpaqueJSValue *> >,WTF::PtrHash<COMPtr<IAccessible> > >,COMPtr<IAccessible>,OpaqueJSValue *>(const COMPtr<IAccessible> & key={...}, OpaqueJSValue * const & extra=0x03c016c0)  Line 690	C++
 	DumpRenderTree.dll!WTF::HashMap<COMPtr<IAccessible>,OpaqueJSValue *,WTF::PtrHash<COMPtr<IAccessible> >,WTF::HashTraits<COMPtr<IAccessible> >,WTF::HashTraits<OpaqueJSValue *> >::inlineAdd(const COMPtr<IAccessible> & key={...}, OpaqueJSValue * const & mapped=0x03c016c0)  Line 335 + 0x14 bytes	C++
 	DumpRenderTree.dll!WTF::HashMap<COMPtr<IAccessible>,OpaqueJSValue *,WTF::PtrHash<COMPtr<IAccessible> >,WTF::HashTraits<COMPtr<IAccessible> >,WTF::HashTraits<OpaqueJSValue *> >::add(const COMPtr<IAccessible> & key={...}, OpaqueJSValue * const & mapped=0x03c016c0)  Line 362 + 0x14 bytes	C++
 	DumpRenderTree.dll!AccessibilityController::winAddNotificationListener(COMPtr<IAccessible> element={...}, OpaqueJSValue * functionCallback=0x03c016c0)  Line 329 + 0x17 bytes	C++
 	DumpRenderTree.dll!AccessibilityUIElement::addNotificationListener(OpaqueJSValue * functionCallback=0x03c016c0)  Line 622	C++
 	DumpRenderTree.dll!addNotificationListenerCallback(const OpaqueJSContext * context=0x036400b0, OpaqueJSValue * function=0x03c01500, OpaqueJSValue * thisObject=0x03c01540, unsigned int argumentCount=1, const OpaqueJSValue * const * arguments=0x001ee434, const OpaqueJSValue * * exception=0x001ee418)  Line 933 + 0x17 bytes	C++
 	JavaScriptCore.dll!JSC::JSCallbackFunction::call(JSC::ExecState * exec=0x036400b0)  Line 73 + 0x39 bytes	C++
 	JavaScriptCore.dll!cti_op_call_NotJSFunction(void * * args=0x001ee518)  Line 2308 + 0x8 bytes	C++
 	JavaScriptCore.dll!@cti_op_create_this@4()  + 0x16f bytes	C++
 	JavaScriptCore.dll!JSC::JITCode::execute(JSC::RegisterFile * registerFile=0x030040a4, JSC::ExecState * callFrame=0x03640038, JSC::JSGlobalData * globalData=0x02fbe198)  Line 127 + 0x2d bytes	C++
 	JavaScriptCore.dll!JSC::Interpreter::executeCall(JSC::ExecState * callFrame=0x0135c2b8, JSC::JSObject * function=0x03c01840, JSC::CallType callType=CallTypeJS, const JSC::CallData & callData={...}, JSC::JSValue thisValue={...}, const JSC::ArgList & args={...})  Line 1305 + 0x2a bytes	C++
 	JavaScriptCore.dll!JSC::call(JSC::ExecState * exec=0x0135c2b8, JSC::JSValue functionObject={...}, JSC::CallType callType=CallTypeJS, const JSC::CallData & callData={...}, JSC::JSValue thisValue={...}, const JSC::ArgList & args={...})  Line 39 + 0x3c bytes	C++
 	WebKit.dll!WebCore::JSMainThreadExecState::call(JSC::ExecState * exec=0x0135c2b8, JSC::JSValue functionObject={...}, JSC::CallType callType=CallTypeJS, const JSC::CallData & callData={...}, JSC::JSValue thisValue={...}, const JSC::ArgList & args={...})  Line 56 + 0x29 bytes	C++
 	WebKit.dll!WebCore::ScheduledAction::executeFunctionInContext(JSC::JSGlobalObject * globalObject=0x0135c240, JSC::JSValue thisValue={...}, WebCore::ScriptExecutionContext * context=0x04f2367c)  Line 115 + 0x4c bytes	C++
 	WebKit.dll!WebCore::ScheduledAction::execute(WebCore::Document * document=0x04f235c0)  Line 138	C++
 	WebKit.dll!WebCore::ScheduledAction::execute(WebCore::ScriptExecutionContext * context=0x04f2367c)  Line 85	C++
 	WebKit.dll!WebCore::DOMTimer::fired()  Line 151	C++
 	WebKit.dll!WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 115 + 0xf bytes	C++
 	WebKit.dll!WebCore::ThreadTimers::sharedTimerFired()  Line 94	C++
 	WebKit.dll!WebCore::TimerWindowWndProc(HWND__ * hWnd=0x000505ac, unsigned int message=49583, unsigned int wParam=0, long lParam=0)  Line 103 + 0x8 bytes	C++
 	user32.dll!75c26238() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]	
 	user32.dll!75c268ea() 	
 	user32.dll!75c26899() 	
 	user32.dll!75c27d31() 	
 	user32.dll!75c27dfa() 	
 	DumpRenderTree.dll!runTest(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & testPathOrURL="C:\cygwin\home\buildbot\OpenSource\LayoutTests\accessibility\menu-list-sends-change-notification.html")  Line 1053 + 0xf bytes	C++
 	DumpRenderTree.dll!dllLauncherEntryPoint(int argc=2, const char * * argv=0x00c72578)  Line 1435 + 0x28 bytes	C++
 	DumpRenderTree.exe!main(int argc=2, const char * * argv=0x00c72578)  Line 198 + 0x10 bytes	C++
 	DumpRenderTree.exe!__tmainCRTStartup()  Line 597 + 0x17 bytes	C
 	kernel32.dll!75103677() 	
 	ntdll.dll!77989f42() 	
 	ntdll.dll!77989f15()

[Radar WebKit Bug Importer]

<rdar://problem/11527899>

[Jessie Berlin]

Added the tests to the Windows Skipped list in http://trac.webkit.org/changeset/118417

[Brent Fulgham]

This appears to be some kind of bad interaction between COMPtr and the HashMap implementation. The assertion (that the pointer is non-null) fires during the first invocation of inserting the COMPtr.

[Brent Fulgham]

There may be a bug in COMPtr, or at least a design choice that makes it incompatible with HashMap. However, we can avoid the issue by storing the raw pointer of the IAccessible object in the HashMap.

This should be safe, because the actual ownership of the pointer being stored in the HashMap is not meant to be controlled by this lookup table.

[Brent Fulgham]

Created attachment 245326 [details]
Patch

[Darin Adler]

Comment on attachment 245326 [details]
Patch

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

Before recording to this I suggest fixing it by:

1) Adding a specialization for IsSmartPtr to COMPtr.h. Check out the one in RefPtr.h.
2) Removing the specialization of PtrHash from COMPtr.h.
3) Refreshing the specialization of HashTraits to more closely match the one for RefPtr in HashTraits.h using a PeekType and deriving from SimpleClassHashTraits instead of from GenericHashTraits. Although I’d suggest using nullptr instead of 0.

If you try these and they don’t work, then I guess you could go with what you posted here, but I think it’s unnecessary.

> Tools/DumpRenderTree/win/AccessibilityControllerWin.cpp:64
> +    for (auto it = m_notificationListeners.begin(); it != m_notificationListeners.end(); ++it) {

Could use a modern for loop. Either:

    for (auto& slot : m_notificationsListeners) {
        slot.key->Release();

Or if the COMPtr thing pans out then:

    for (auto& value : m_notificationsListeners.values())
        JSValueUnprotect(frame->globalContext(), it->value);

> Tools/DumpRenderTree/win/AccessibilityControllerWin.cpp:342
> +    for (auto it = m_notificationListeners.begin(); it != m_notificationListeners.end(); ++it) {

Same thing here:

    for (auto& slot : m_notificationsListeners) {

Then you can use "slot." instead of "it->"

[Anders Carlsson]

Comment on attachment 245326 [details]
Patch

I think what we really should do is make our containers work with keys/values that override operator&. This means replacing any uses of & in those containers with std::addressof.

[Brent Fulgham]

Created attachment 245381 [details]
Patch

[Brent Fulgham]

(In reply to comment #6)
> Comment on attachment 245326 [details]
> Before recording to this I suggest fixing it by:
> 
> 1) Adding a specialization for IsSmartPtr to COMPtr.h. Check out the one in
> RefPtr.h.
> 2) Removing the specialization of PtrHash from COMPtr.h.
> 3) Refreshing the specialization of HashTraits to more closely match the one
> for RefPtr in HashTraits.h using a PeekType and deriving from
> SimpleClassHashTraits instead of from GenericHashTraits. Although I’d
> suggest using nullptr instead of 0.

I made these changes, but still encountered the assertion. However, by moving from '&' to 'std::addressor' in our container classes the problem seems to be resolved.
 
[ ... ]
> Same thing here:
> 
>     for (auto& slot : m_notificationsListeners) {
> 
> Then you can use "slot." instead of "it->"

Yes! That is much nicer. Done.

[Darin Adler]

Comment on attachment 245381 [details]
Patch

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

Fantastic!

> Source/WebCore/platform/win/COMPtr.h:226
>  namespace WTF {
> +template<typename P> struct IsSmartPtr<COMPtr<P>> {

I think there should be a blank line here.

> Source/WebKit/win/WebView.cpp:4198
> -    if (m_page->isEditable() == flag)
> +    if (m_page->isEditable() == static_cast<bool>(flag))

How is this change related to the rest of the patch?

> Tools/DumpRenderTree/win/AccessibilityControllerWin.cpp:65
> +    for (auto& slot : m_notificationListeners)
> +        JSValueUnprotect(frame->globalContext(), slot.value);

This could be even better.

    for (auto& listener : m_notificationListeners.values())
        JSValueUnprotect(frame->globalContext(), listener);

[Brent Fulgham]

Comment on attachment 245381 [details]
Patch

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

>> Source/WebCore/platform/win/COMPtr.h:226
>> +template<typename P> struct IsSmartPtr<COMPtr<P>> {
> 
> I think there should be a blank line here.

OK!

>> Source/WebKit/win/WebView.cpp:4198
>> +    if (m_page->isEditable() == static_cast<bool>(flag))
> 
> How is this change related to the rest of the patch?

This was a drive-by fix. I was tired of seeing this warning during the compile.

>> Tools/DumpRenderTree/win/AccessibilityControllerWin.cpp:65
>> +        JSValueUnprotect(frame->globalContext(), slot.value);
> 
> This could be even better.
> 
>     for (auto& listener : m_notificationListeners.values())
>         JSValueUnprotect(frame->globalContext(), listener);

Very nice! Will do.

[Brent Fulgham]

Committed r179170: <http://trac.webkit.org/changeset/179170>