597343 - Crash [@ nsCoreUtils::IsRootDocument(nsIDocument*) ]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Scoobidiver (away)]

Build : Mozilla/5.0 (Windows NT 6.1; rv:2.0b7pre) Gecko/20100916 Firefox/4.0b7pre

This is a new crash signature that is present at different daily rate depending on builds :
20100909, 20100910, 20100911, 20100912, 2010913 : 1 crash/day
20100914, 20100915, 20100916 : 10 crashes/day
It is #15 top crasher for this build.

Signature	nsCoreUtils::IsRootDocument(nsIDocument*)
UUID	b5530da3-d1e8-4f27-9858-e14522100915
Process Type	
Time 	2010-09-15 03:21:53.243346
Uptime	17852
Install Age	21103 seconds (5.9 hours) since version was first installed.
Product	Firefox
Version	4.0b7pre
Build ID	20100914041908
Branch	2.0
OS	Windows NT
OS Version	6.1.7600
CPU	x86
CPU Info	GenuineIntel family 6 model 23 stepping 10
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0	
App Notes 	AdapterVendorID: 1002, AdapterDeviceID: 9553

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsCoreUtils::IsRootDocument 	accessible/src/base/nsCoreUtils.cpp:486
1 	xul.dll 	nsAccDocManager::CreateDocOrRootAccessible 	accessible/src/base/nsAccDocManager.cpp:456
2 	xul.dll 	nsAccDocManager::GetDocAccessible 	accessible/src/base/nsAccDocManager.cpp:80
3 	xul.dll 	nsAccessibilityService::InvalidateSubtreeFor 	accessible/src/base/nsAccessibilityService.cpp:1706
4 	xul.dll 	nsFrameManager::ReResolveStyleContext 	
5 	xul.dll 	nsFrameManager::ComputeStyleChangeFor 	layout/base/nsFrameManager.cpp:1552
6 	xul.dll 	mozilla::css::RestyleTracker::ProcessOneRestyle 	layout/base/RestyleTracker.cpp:156
7 	xul.dll 	mozilla::css::RestyleTracker::ProcessRestyles 	layout/base/RestyleTracker.cpp:240
8 	xul.dll 	nsCSSFrameConstructor::ProcessPendingRestyles 	layout/base/nsCSSFrameConstructor.cpp:11566
9 	xul.dll 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:4790
10 	xul.dll 	PresShell::HandlePostedReflowCallbacks 	layout/base/nsPresShell.cpp:4683
11 	xul.dll 	PresShell::ProcessReflowCommands 	layout/base/nsPresShell.cpp:7712
12 	xul.dll 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:4821
13 	xul.dll 	nsRefreshDriver::Notify 	layout/base/nsRefreshDriver.cpp:310
14 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:428
15 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:517
16 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:547
17 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
18 	xul.dll 	xul.dll@be68db 	
19 	xul.dll 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:219
20 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:202
21 	xul.dll 	UpdateDepth 	js/src/jsemit.cpp:246
22 	xul.dll 	_SEH_epilog4 	
23 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:176
24 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:180
25 	xul.dll 	nsAppShell::Run 	widget/src/windows/nsAppShell.cpp:243

Comment 1

[alexander :surkov (:asurkov)]

It sounds like nsCOMPtr<nsIDocShellTreeItem> treeitem =
do_QueryInterface(nsIDocument->GetContainer()) is null
(http://hg.mozilla.org/mozilla-central/annotate/5588c9796f0b/accessible/src/base/nsCoreUtils.cpp#l486).
Boris do you have any idea why I can't get docshell treeitem from the document,
what's the possible cases when it happens? It must be a document we want to
create an accessible for, it's not initial, it's visible, it has presshell
(http://hg.mozilla.org/mozilla-central/annotate/5588c9796f0b/accessible/src/base/nsAccDocManager.cpp#l456).

Comment 2

[alexander :surkov (:asurkov)]

request blocking fx4 based on comment #0

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Boris do you have any idea why I can't get docshell treeitem from the document,
> what's the possible cases when it happens?

Hmm...  There are all sorts of cases where it can happen: The document never had a docshell (e.g. a resource document or data document of some sort), the document used to be in a docshell and then got unloaded.

> It must be a document we want to create an accessible for, it's not initial,
> it's visible, it has presshell

I dunno what you use to decide whether you want to create an accessible.  

An obvious document that would pass your check but have no docshell is SVG-as-<img>.  To catch those, you want to check IsResourceDoc() as opposed to GetDisplayDocument().

Comment 4

[Boris Zbarsky [:bzbarsky]]

Do we have any URIs to work with here?

Comment 5

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

Note we used to null check prior to:
http://hg.mozilla.org/mozilla-central/rev/a2ba3a7cec03

Seems like we need to add those back.

Comment 6

[Ginn Chen]

The patch in Bug 597963 should fix the crash.
I can reproduce the crash with make reftest with a11y on Solaris.

Comment 7

[Ginn Chen]

Reproduce step:

dist/bin/firefox -no-remote -reftest ../layout/reftests/backgrounds/reftest.list

Stack:

 fd78d61c int nsCoreUtils::IsRootDocument(nsIDocument*) (a636cc8, 23, 1, fd77b1de) + 74
 fd77b244 nsDocAccessible*nsAccDocManager::CreateDocOrRootAccessible(nsIDocument*) (8e0c5f8, a636cc8, 0, fd77a021) + 74
 fd77a061 nsDocAccessible*nsAccDocManager::GetDocAccessible(nsIDocument*) (8e0c5f8, a636cc8, 8045f68, fd79c620) + 5d
 fd79c63c unsigned nsAccessibilityService::InvalidateSubtreeFor(nsIPresShell*,nsIContent*,unsigned) (8e0c5f8, a5fc3f8, a6113b8, 5) + 28
 fc87cc2a nsChangeHint nsFrameManager::ReResolveStyleContext(nsPresContext*,nsIFrame*,nsIContent*,nsStyleChangeList*,nsChangeHint,nsRestyleHint,int,mozilla::css::RestyleTracker&) (a5fc414, a5fb978, a639098, 0, 8046000, 0) + c46
 fc87cecf void nsFrameManager::ComputeStyleChangeFor(nsIFrame*,nsStyleChangeList*,nsChangeHint,mozilla::css::RestyleTracker&,int) (a5fc414, a639098, 8046000, 0, a5fc788, 0) + 7f
 fc841902 void nsCSSFrameConstructor::RestyleElement(mozilla::dom::Element*,nsIFrame*,nsChangeHint,mozilla::css::RestyleTracker&,int) (a5fc730, a6113b8, a639098, 0, a5fc788, 0) + 62
 fc8331d2 void mozilla::css::RestyleTracker::ProcessRestyles() (a5fc788, fcb194dc, 8046ae4, fc848d5a) + 786
 fc848d80 void nsCSSFrameConstructor::ProcessPendingRestyles() (a5fc730, fdfc2068, 8046b98, fc8a20c0) + 30
 fc8a20e2 void PresShell::FlushPendingNotifications(mozFlushType) (a5fc3f8, 5, a606398, fc8ad0b3) + 1ea
 fc8ad178 int PresShell::ProcessReflowCommands(int) (a5fc3f8, 0, 8046e58, fc8a2147) + b0c
 fc8a21b1 void PresShell::FlushPendingNotifications(mozFlushType) (a5fc3f8, 5, fe0aa6d0, fc8056fa) + 2b9
 fc805783 unsigned mozilla::imagelib::SVGDocumentWrapper::OnStopRequest(nsIRequest*,nsISupports*,unsigned) (a5e67a8) + 97
 fc806fd5 unsigned mozilla::imagelib::VectorImage::OnStopRequest(nsIRequest*,nsISupports*,unsigned) (a63fde8) + 45
 fc813fe2 unsigned imgRequest::OnStopRequest(nsIRequest*,nsISupports*,unsigned) (a6375e8, a60f0b4, 0, 0) + 112
 fc80f53f unsigned ProxyListener::OnStopRequest(nsIRequest*,nsISupports*,unsigned) (9fc7f68, a60f0b4) + 1f
 fc66c877 unsigned nsBaseChannel::OnStopRequest(nsIRequest*,nsISupports*,unsigned) (a60f088, a63fae8) + 63
 fc67846d unsigned nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (a63fae8, a62b9a8, 0, fd8b3fe2) + 35d
 fd8b4007 unsigned nsInputStreamReadyEvent::Run() (a61deb0, 1, 804707c, 0) + 2f
 fd8d6a89 unsigned nsThread::ProcessNextEvent(int,int*) (80fcd70, 1, 80470bc, fd87fb05) + 151
 fd87fb3a int NS_ProcessNextEvent_P(nsIThread*,int) (80fcd70, 1, 80470f8, fd74db2f) + 42
 fd74db47 unsigned nsBaseAppShell::Run() (813ed18, 0, 0, fd4f4714) + 47
 fd4f473a unsigned nsAppStartup::Run() (84a4d78, 806a790, 80fe3f0, 0) + 32
 fc656e3e XRE_main (4) + 25da
 08051e23 main     (4, 80475b4, 80475c8, 8051a68) + 263
 08051afd _start   (4, 8047724, 8047744, 804774d, 0, 0) + 7d

Comment 8

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

Assigning to Ginn based on comment 6. Ginn can you work on this? Let us know.

Comment 9

[Ginn Chen]

I'll find some time.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #3)

> An obvious document that would pass your check but have no docshell is
> SVG-as-<img>.  To catch those, you want to check IsResourceDoc() as opposed to
> GetDisplayDocument().

just to make things clear, we should replace GetDisplayDocument() check on IsResourceDoc() check, right?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)

> just to make things clear, we should replace GetDisplayDocument() check on
> IsResourceDoc() check, right?

I see IsResourceDoc is enough (http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIDocument.h#1160)

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Reproduce step:
> 
> dist/bin/firefox -no-remote -reftest
> ../layout/reftests/backgrounds/reftest.list

(In reply to comment #3)
> An obvious document that would pass your check but have no docshell is
> SVG-as-<img>.  To catch those, you want to check IsResourceDoc() as opposed to
> GetDisplayDocument().

You're right. That's the case of IsResourceDoc(), mIsBeingUsedAsImage is true.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 14

[Boris Zbarsky [:bzbarsky]]

> we should replace GetDisplayDocument() check on IsResourceDoc() check, right?

Yep.

Comment 15

[Ginn Chen]

surkov, the patch works.
I think we'd better also add null check and assertion after GetContainer().
What do you think?

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> surkov, the patch works.
> I think we'd better also add null check and assertion after GetContainer().
> What do you think?

I'm not sure whether this is going to happen ever. Though we might want to have this since Boris mentioned the case "the document used to be in a docshell and then got unloaded."

Boris, why and when does this happen?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Boris, why and when does this happen?

Any time you navigate away from a page?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> > Boris, why and when does this happen?
> 
> Any time you navigate away from a page?

the presshell should be destroyed as well, right? We have check for null presshell and check for IsVisible, is this enough?

Comment 19

[Boris Zbarsky [:bzbarsky]]

I _think_ so....  The ordering there is a bit hairy.  :(

Comment 20

[alexander :surkov (:asurkov)]

Ok, then it makes sense to put additional GetContainer() check just to be safe.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> I _think_ so....  The ordering there is a bit hairy.  :(

Boris, would it be ok to check nsIDocument::IsActive() instead of nsIDocument::GetContainer()?

Comment 22

[Boris Zbarsky [:bzbarsky]]

I don't know offhand.

Comment 23

[alexander :surkov (:asurkov)]

Attachment: patch2

IsActive should fine here, it returns false for document that doesn't have docshell and for document with removed docshell. Asking additional reivew from Boris to be sure.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 483054 [details] [diff] [review]
patch2

r=me

Comment 25

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/9f342c85157a