1066539 - crash in mozilla::a11y::DocManager::AddListeners(nsIDocument*, bool)

Status: RESOLVED WONTFIX

(Core :: Disability Access APIs, defect, P3)

Description

[Wayne Mery (:wsmwk)]

This signature is more common in Thunderbird 31 than TB24

This bug was filed from the Socorro interface and is 
report bp-0aad59d5-3687-4996-afd8-493672140821.
=============================================================
 0 	xul.dll	mozilla::a11y::DocManager::AddListeners(nsIDocument*, bool)	accessible/src/base/DocManager.cpp
1 	xul.dll	mozilla::a11y::DocManager::CreateDocOrRootAccessible(nsIDocument*)	accessible/src/base/DocManager.cpp
2 	xul.dll	mozilla::a11y::DocManager::GetDocAccessible(nsIDocument*)	accessible/src/base/DocManager.cpp
3 	xul.dll	mozilla::a11y::DocManager::GetDocAccessible(nsIPresShell const*)	objdir-tb/mozilla/dist/include/mozilla/a11y/DocManager.h
4 	xul.dll	nsAccessibilityService::ContentRangeInserted(nsIPresShell*, nsIContent*, nsIContent*, nsIContent*)	accessible/src/base/nsAccessibilityService.cpp 

http://hg.mozilla.org/releases/mozilla-esr31/annotate/51a9591df62f/accessible/src/base/DocManager.cpp#l326
surkov.alexander@43310 321 void
surkov.alexander@122870 322 DocManager::AddListeners(nsIDocument* aDocument,
surkov.alexander@122870 323 bool aAddDOMContentLoadedListener)
surkov.alexander@43310  324 {
dzbarsky@140331         325   nsPIDOMWindow* window = aDocument->GetWindow();
dzbarsky@140331         326   EventTarget* target = window->GetChromeEventHandler(); 

In firefox, 50% are startup crashes
eg bp-8c8d7340-512f-4c17-9135-2624d2140908
 0 	xul.dll	mozilla::a11y::DocManager::AddListeners(nsIDocument*, bool)	accessible/src/base/DocManager.cpp
1 	xul.dll	mozilla::a11y::DocManager::CreateDocOrRootAccessible(nsIDocument*)	accessible/src/base/DocManager.cpp
2 	xul.dll	mozilla::a11y::DocManager::HandleDOMDocumentLoad(nsIDocument*, unsigned int)	accessible/src/base/DocManager.cpp
3 	xul.dll	mozilla::a11y::DocManager::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult)	accessible/src/base/DocManager.cpp
4 	xul.dll	nsDocLoader::DoFireOnStateChange(nsIWebProgress* const, nsIRequest* const, int&, tag_nsresult)	uriloader/base/nsDocLoader.cpp
5 	xul.dll	nsDocLoader::doStopDocumentLoad(nsIRequest*, tag_nsresult)	uriloader/base/nsDocLoader.cpp
6 	xul.dll	nsDocLoader::DocLoaderIsEmpty(bool)	uriloader/base/nsDocLoader.cpp

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

null check

Comment 2

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

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

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

r=me

::: accessible/base/DocManager.cpp
@@ +323,5 @@
>                           bool aAddDOMContentLoadedListener)
>  {
>    nsPIDOMWindow* window = aDocument->GetWindow();
> +  if (!window)
> +    return;

OK. Can you make a comment something like:
// We need to null check here because Thunderbird is doing something we don't have time to debug.

Comment 3

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

If this is a regression, let's get a window.

Comment 4

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

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

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

Oh! This also happens in FF:
https://crash-stats.mozilla.com/report/index/2073b8a0-012f-4f8c-8f10-060ab2140908

Cancelling r+ for this and as there is further discussion.

Comment 5

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

Boris, we're not getting a window from an nsIDocument in these stacks. Do you happen to know if that should be expected (sometimes)? (and when etc)

Comment 6

[(no longer active)]

Yes, that should be expected.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> Yes, that should be expected.

Would it be expected on nsGlobalWindow::OpenInternal (stack from comment #0)?

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> Yes, that should be expected.

O_O why?

Comment 9

[(no longer active)]

I don't know.  For one thing, mWindow on nsDocument will be null before the SetScriptGlobalObject call from nsGlobalWindow::SetNewDocument.  Code inspection should tell you if that is what happens here.  If mWindow is null, there is a ton of ways in which getting a window might fail.  Look at all of the early returns in nsDocument::GetWindowInternal and its callees.

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9)
> I don't know.  For one thing, mWindow on nsDocument 

I'd say you should have some reason it should be expected, because off hand having a document and a presShell for it without a window sounds kind of insane.

will be null before the
> SetScriptGlobalObject call from nsGlobalWindow::SetNewDocument.  Code
> inspection should tell you if that is what happens here.  If mWindow is
> null, there is a ton of ways in which getting a window might fail.  Look at
> all of the early returns in nsDocument::GetWindowInternal and its callees.

Well, never actually returns early, and at least to me its unclear if it expects its callees to fail or not.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Do you have a way to reproduce this?  Because that would make it way easier to answer some of these questions.

> having a document and a presShell for it without a window sounds kind of insane.

You just described SVG images and SVG resource documents.  ;)

However, those should not have a docshell, and the stacks certainly look like a docshell is involved.

Furthermore, the document in question came from a GetDocument() on a window, right?

Comment 12

[Wayne Mery (:wsmwk)]

regression-wise:

thunderbird is no help - no crashes in 2014 for aurora, nightly or beta 

firefox signature summary https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Aa11y%3A%3ADocManager%3A%3AAddListeners%28nsIDocument*%2C+bool%29&product=Firefox&query_type=is_exactly&range_unit=weeks&process_type=any&hang_type=any&date=2014-09-12+15%3A00%3A00&range_value=4#reports claims crashes exist for version as old as 25 and 26. examples 

bp-740662e5-ff18-4a74-94a6-717d82140220 Build ID 	20131205075310 version 26

bp-17bca82c-d0cc-4d04-ac33-7b0432140528 Build ID 	20131112160018 version 25.0.1

there are also a few crashes that call out the next source line 
bp-37d3225b-45eb-4b38-b606-1d8532140223
 332  nsEventListenerManager* elm = target->GetOrCreateListenerManager();

Comment 13

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

(In reply to Boris Zbarsky [:bz] from comment #11)
> Do you have a way to reproduce this?  Because that would make it way easier
> to answer some of these questions.

Definitely.

> > having a document and a presShell for it without a window sounds kind of insane.
> 
> You just described SVG images and SVG resource documents.  ;)
> 
> However, those should not have a docshell, and the stacks certainly look
> like a docshell is involved.
> 
> Furthermore, the document in question came from a GetDocument() on a window,
> right?

I'm not sure. I think in this case we're getting it from the presshell here:
http://hg.mozilla.org/releases/mozilla-esr31/annotate/51a9591df62f/accessible/src/base/DocManager.h#l52

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hmm.  I was looking at https://crash-stats.mozilla.com/report/index/2073b8a0-012f-4f8c-8f10-060ab2140908 which doesn't go through that function, right?

Comment 15

[Trevor Saunders (:tbsaunde)]

Seems my comment friday didn't go through.

(In reply to David Bolter [:davidb] from comment #13)
> (In reply to Boris Zbarsky [:bz] from comment #11)
> > Do you have a way to reproduce this?  Because that would make it way easier
> > to answer some of these questions.
> 
> Definitely.

I'm not aware of anything.

> > > having a document and a presShell for it without a window sounds kind of insane.
> > 
> > You just described SVG images and SVG resource documents.  ;)
> > 
> > However, those should not have a docshell, and the stacks certainly look
> > like a docshell is involved.

yeah, the stacks do seem different, but seem to all involve a docshell somehow.

> > Furthermore, the document in question came from a GetDocument() on a window,
> > right?
> 
> I'm not sure. I think in this case we're getting it from the presshell here:
> http://hg.mozilla.org/releases/mozilla-esr31/annotate/51a9591df62f/
> accessible/src/base/DocManager.h#l52

yeah, I think the way it generally goes in these crashes is
window -> docShell -> presShell -> document

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky [:bz] from comment #14)
> Hmm.  I was looking at
> https://crash-stats.mozilla.com/report/index/2073b8a0-012f-4f8c-8f10-
> 060ab2140908 which doesn't go through that function, right?

err, yeah, that one is definitely just window -> document and then document->window fails

Comment 17

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

(In reply to Trevor Saunders (:tbsaunde) from comment #16)
> (In reply to Boris Zbarsky [:bz] from comment #14)
> > Hmm.  I was looking at
> > https://crash-stats.mozilla.com/report/index/2073b8a0-012f-4f8c-8f10-
> > 060ab2140908 which doesn't go through that function, right?
> 
> err, yeah, that one is definitely just window -> document and then
> document->window fails

Yeah weird. Is the QI failing somehow here?
http://hg.mozilla.org/releases/mozilla-release/annotate/44234f451065/accessible/src/base/DocManager.cpp#l148

Comment 18

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #17)
> (In reply to Trevor Saunders (:tbsaunde) from comment #16)
> > (In reply to Boris Zbarsky [:bz] from comment #14)
> > > Hmm.  I was looking at
> > > https://crash-stats.mozilla.com/report/index/2073b8a0-012f-4f8c-8f10-
> > > 060ab2140908 which doesn't go through that function, right?
> > 
> > err, yeah, that one is definitely just window -> document and then
> > document->window fails
> 
> Yeah weird. Is the QI failing somehow here?
> http://hg.mozilla.org/releases/mozilla-release/annotate/44234f451065/
> accessible/src/base/DocManager.cpp#l148

no, that would cause a crash much sooner the first time we call something on the document.

Comment 19

[BugBot [:suhaib / :marco]]

Crash volume for signature 'mozilla::a11y::DocManager::AddListeners':
 - nightly (version 50): 0 crash from 2016-06-06.
 - aurora  (version 49): 0 crash from 2016-06-07.
 - beta    (version 48): 1 crash from 2016-06-06.
 - release (version 47): 32 crashes from 2016-05-31.
 - esr     (version 45): 0 crash from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          0          0          0          0          0
 - aurora           0          0          0          0          0          0          0
 - beta             1          0          0          0          0          0          0
 - release          5          4          2          9          7          5          0
 - esr              0          0          0          0          0          0          0

Affected platform: Windows

Comment 20

[alexander :surkov (:asurkov)]

the crash moved down the road and not it crashes at target->GetOrCreateListenerManager() apparently because target = window.GetChromeEventHandler().

since don't have steps to reproduce, are there any suggestions for diagnostic assertions to add to investigate the issue further?

Comment 21

[alexander :surkov (:asurkov)]

I can see 56/52 crashes only. I guess we can close it as wontfix.