681905 - Crash [@ nsDocAccessible::ProcessContentInserted]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

I got this crash while testing:

https://crash-stats.mozilla.com/report/index/bp-e232c518-b605-4a20-b1d5-9a24a2110825
0 	xul.dll 	nsDocAccessible::ProcessContentInserted 	accessible/src/base/nsDocAccessible.cpp:1805
1 	xul.dll 	NotificationController::TextEnumerator 	accessible/src/base/NotificationController.cpp:687
2 	xul.dll 	nsTHashtable<mozilla::plugins::PluginModuleChild::NPObjectData>::s_EnumStub 	obj-firefox/dist/include/nsTHashtable.h:420
3 	xul.dll 	PL_DHashTableEnumerate 	obj-firefox/xpcom/build/pldhash.cpp:754
4 	xul.dll 	nsTHashtable<NotificationController::nsCOMPtrHashKey<nsIContent> >::EnumerateEntries 	obj-firefox/dist/include/nsTHashtable.h:241
5 	xul.dll 	NotificationController::WillRefresh 	accessible/src/base/NotificationController.cpp:244
6 	xul.dll 	nsRefreshDriver::Notify 	layout/base/nsRefreshDriver.cpp:326
7 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:427
8 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:520
9 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:631
10 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
11 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:201
12 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:175
13 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
14 	xul.dll 	nsAppShell::Run 	widget/src/windows/nsAppShell.cpp:261
15 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:224
16 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3545
17 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:107
18 	firefox.exe 	__tmainCRTStartup 	obj-firefox/memory/jemalloc/crtsrc/crtexe.c:591
19 	kernel32.dll 	BaseThreadInitThunk 	
20 	ntdll.dll 	__RtlUserThreadStart 	
21 	ntdll.dll 	_RtlUserThreadStart

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Make sure GetContainerAccessible and GetAccessibleOrContainer aren't used without null check in cases when DOM node may not belong to accessible document.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 556585 [details] [diff] [review]
patch

Review of attachment 556585 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. If we're going to assert something we know happens we should probably have a plan to fix it - is there a bug filed?

::: accessible/src/base/NotificationController.cpp
@@ +685,3 @@
>      nsAccessible* container = document->GetAccessibleOrContainer(containerNode);
> +    NS_ASSERTION(container,
> +                 "Text node having rendered text hasn't accessible document!");

When does this happen?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #2)

> r=me. If we're going to assert something we know happens we should probably
> have a plan to fix it - is there a bug filed?

iirc there's something, see example below. In general having a bug for assertion that you're not able to reproduce is just bugzilla spamming. I often use assertions for something that crashes but have no idea why, hoping to be lucky to catch it in a wild.

> ::: accessible/src/base/NotificationController.cpp
> @@ +685,3 @@
> >      nsAccessible* container = document->GetAccessibleOrContainer(containerNode);
> > +    NS_ASSERTION(container,
> > +                 "Text node having rendered text hasn't accessible document!");
> 
> When does this happen?

for example, we don't create document accessible for initial documents, while these documents may be mutated and contain content that should be exposed to AT.

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 556585 [details] [diff] [review]
patch

nothing jumps out at me, David's review seems enough, reask if you still want me to look at something

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #3)
> (In reply to David Bolter [:davidb] from comment #2)
> 
> > r=me. If we're going to assert something we know happens we should probably
> > have a plan to fix it - is there a bug filed?
> 
> iirc there's something, see example below. In general having a bug for
> assertion that you're not able to reproduce is just bugzilla spamming. I
> often use assertions for something that crashes but have no idea why, hoping
> to be lucky to catch it in a wild.

It seems reasonable to me to do this to prevent crashes for now, and hopefully fuzzing or something will find us a test case eventually

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander surkov from comment #3)
> (In reply to David Bolter [:davidb] from comment #2)
> 
> > r=me. If we're going to assert something we know happens we should probably
> > have a plan to fix it - is there a bug filed?
> 
> iirc there's something, see example below. In general having a bug for
> assertion that you're not able to reproduce is just bugzilla spamming. I
> often use assertions for something that crashes but have no idea why, hoping
> to be lucky to catch it in a wild.

This is reasonable thinking. I'm tired of the assertion spam in my console but hopefully this case is really rare.

Comment 7

[alexander :surkov (:asurkov)]

inbound land: http://hg.mozilla.org/integration/mozilla-inbound/rev/e2a69e20bbc5

Comment 8

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/e2a69e20bbc5