627099 - Crash [@ nsDocAccessible::RecreateAccessible(nsINode*) ][@ nsDocAccessible::RecreateAccessible ]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Scoobidiver (away)]

It is a new crash signature introduced by 4.0b10pre/20110119.
It is currently #6 top crasher in today's build.

Signature	nsDocAccessible::RecreateAccessible(nsINode*)
UUID	4a28058c-0a5d-46cb-8a85-192612110119
Time 	2011-01-19 08:40:40.265540
Uptime	2309
Last Crash	77032 seconds (21.4 hours) before submission
Install Age	2309 seconds (38.5 minutes) since version was first installed.
Product	Firefox
Version	4.0b10pre
Build ID	20110119030331
Branch	2.0
OS	Windows NT
OS Version	5.1.2600 Dodatek Service Pack 3
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 6
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	AdapterVendorID: 10de, AdapterDeviceID: 0298

Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsDocAccessible::RecreateAccessible 	accessible/src/base/nsDocAccessible.cpp:1461
1 	xul.dll 	TNotification<nsDocAccessible,nsINode>::Process 	accessible/src/base/NotificationController.h:99
2 	xul.dll 	NotificationController::WillRefresh 	accessible/src/base/NotificationController.cpp:206
3 	xul.dll 	nsRefreshDriver::Notify 	layout/base/nsRefreshDriver.cpp:254
4 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:428
5 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:517
6 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:633
7 	nspr4.dll 	PR_AssertCurrentThreadOwnsLock 	nsprpub/pr/src/threads/combined/prulock.c:404
8 	nspr4.dll 	PR_AssertCurrentThreadOwnsLock 	nsprpub/pr/src/threads/combined/prulock.c:404
9 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
10 	xul.dll 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:219
11 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:202
12 	mozcrt19.dll 	_VEC_memzero 	
13 	xul.dll 	xul.dll@0x35815d 	
14 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:176
15 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:192
16 	xul.dll 	xul.dll@0xb22323 	
17 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:217
18 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3699
19 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:128
20 	firefox.exe 	__tmainCRTStartup 	obj-firefox/memory/jemalloc/crtsrc/crtexe.c:591
21 	kernel32.dll 	BaseProcessStart 	

The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=eb105fe0e41c&tochange=e807269acaa3

More reports at:
http://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=exact&query=&range_value=4&range_unit=weeks&hang_type=any&process_type=any&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&admin=&signature=nsDocAccessible%3A%3ARecreateAccessible%28nsINode*%29

Comment 1

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

Regression in part from bug 617300.

Since we're now calling RecreateAccessible asyncronously (via HandleNotification) we can no longer assume a parent exists. We could null check here or further up the stack.

Comment 2

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

Attachment: nullcheck

This was an end of day quick fix. Really just hanging this patch here in case we need to take it. There might be a deeper fix.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 505161 [details] [diff] [review]
nullcheck

Nullcheck is a shortest way. It's correct but there's a better fix. I will extend comments in the patch. Also nullcheck should be moved above.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: nullcheck2

Comment 5

[alexander :surkov (:asurkov)]

Attachment: correct nullcheck

Comment 6

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

Sure. I thought about putting it there to make it explicit where it might fail, but leaned the other way. I'm fine with either patch -- please land at will.

Comment 7

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/8d45de3deeef

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Sure. I thought about putting it there to make it explicit where it might fail,
> but leaned the other way. 

Actually the other way doesn't make a sense because if parent is null when oldAccessible is not null then it will crash early, in other words your null check won't ever work in this case.

Comment 9

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

(In reply to comment #8)
> (In reply to comment #6)
> > Sure. I thought about putting it there to make it explicit where it might fail,
> > but leaned the other way. 
> 
> Actually the other way doesn't make a sense because if parent is null when
> oldAccessible is not null then it will crash early, in other words your null
> check won't ever work in this case.

Right, it would only be useful for this bug's code path and would be a needless check otherwise, which I as okay with. I had thought about adding an additional null check for the oldAccessible case but that wasn't biting us.

Anyways, thanks for landing!

Comment 10

[Scoobidiver (away)]

It happens again from 4.0b11pre/20110129:
https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=4&range_unit=weeks&signature=nsDocAccessible%3A%3ARecreateAccessible%28nsINode*%29

Comment 11

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

This is the other place where we don't null check. I can prepare a patch for that.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> This is the other place where we don't null check. I can prepare a patch for
> that.

perhaps not necessary since bug 606924 part 7 changes this code.

Comment 13

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

Attachment: the other null check

What are the cases we don't have a parent?

I think the Application root and perhaps children of windows other than the emulated windows?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> Created attachment 508413 [details] [diff] [review]
> the other null check

It doens't make sense to fix this code since this code is going be removed (see comment #13). After that patch is landed the crash perhaps should be coalesced with bug 630202.

Once bug 606924 is landed this bug can be marked as fixed since signature will be changed if crash stays.

> What are the cases we don't have a parent?
> 
> I think the Application root and perhaps children of windows other than the
> emulated windows?

only application accessible, normally, it's not a subject of this call.

Comment 15

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

(In reply to comment #14)
> (In reply to comment #13)
> > Created attachment 508413 [details] [diff] [review]
> > the other null check
> 
> It doens't make sense to fix this code since this code is going be removed (see
> comment #13).

OK that comment happened while I was attaching the wallpaper.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)

> OK that comment happened while I was attaching the wallpaper.

Ok, I think I'll land bug 606924 tomorrow and mark this one as fixed.

We should think how it may happen that we don't have a parent (bug 630202).

Comment 17

[alexander :surkov (:asurkov)]

fixed by bug 606924 part7