Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Crash [@ nsDocAccessible::ProcessContentInserted]

RESOLVED FIXED in mozilla9

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: surkov)

Tracking

({crash})

Trunk
mozilla9
x86
Windows 7
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
Created attachment 556585 [details] [diff] [review]
patch

Make sure GetContainerAccessible and GetAccessibleOrContainer aren't used without null check in cases when DOM node may not belong to accessible document.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #556585 - Flags: review?(trev.saunders)
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?
Attachment #556585 - Flags: review+
(Assignee)

Comment 3

6 years ago
(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 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
Attachment #556585 - Flags: review?(trev.saunders)
(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
(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.
(Assignee)

Comment 7

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

Comment 8

6 years ago
http://hg.mozilla.org/mozilla-central/rev/e2a69e20bbc5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.