169036 - Browser crashes [@ nsXULDocument::GetPrincipal] N700

Status: RESOLVED FIXED

Description

[greer]

The talkback data is showing a lot of crashes at nsXULDocument::GetPrincipal.
All of the crashes are happening in the N7.0 data, but the code shows up in
MozillaTrunk. I don't see another bug for this. Is this a known issue?

From the registers it looks like |this| is null:
01d59111 8b442404         mov     eax,[esp+0x4]
01d59115 ff742408         push    dword ptr [esp+0x8]
01d59119 8b80dc010000     mov     eax,[eax+0x1dc]
01d5911f 50               push    eax
01d59120 8b08             mov     ecx,[eax]           <==Crashes here
01d59122 ff513c           call    dword ptr [ecx+0x3c]
01d59125 c20800           ret     0x8
 
Stack Trace: 

	 nsXULDocument::GetPrincipal
[d:\builds\seamonkey\mozilla\content\xul\document\src\nsXULDocument.cpp  line 846]
	 nsNodeInfoManager::DropDocumentReference
[d:\builds\seamonkey\mozilla\content\base\src\nsNodeInfoManager.cpp  line 181]
	 nsXULDocument::~nsXULDocument
[d:\builds\seamonkey\mozilla\content\xul\document\src\nsXULDocument.cpp  line 546]
	 nsXULDocument::`scalar deleting destructor'
	 nsXULDocument::Release
[d:\builds\seamonkey\mozilla\content\xul\document\src\nsXULDocument.cpp  line 585]
	 nsCOMPtr_base::~nsCOMPtr_base
[d:\builds\seamonkey\mozilla\xpcom\glue\nsCOMPtr.cpp  line 65]
	 nsContentDLF::CreateRDFDocument
[d:\builds\seamonkey\mozilla\content\build\nsContentDLF.cpp  line 530]
	 nsContentDLF::CreateInstance
[d:\builds\seamonkey\mozilla\content\build\nsContentDLF.cpp  line 280]
	 nsDocShell::NewContentViewerObj
[d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp  line 4176]
	 nsDocShell::CreateContentViewer
[d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp  line 4059]
	 nsDSURIContentListener::DoContent
[d:\builds\seamonkey\mozilla\docshell\base\nsDSURIContentListener.cpp  line 108]
	 nsDocumentOpenInfo::DispatchContent
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp  line 365]
	 nsDocumentOpenInfo::OnStartRequest
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp  line 229]
	 nsCachedChromeChannel::HandleStartLoadEvent
[d:\builds\seamonkey\mozilla\rdf\chrome\src\nsChromeProtocolHandler.cpp  line 451]
	 PL_HandleEvent	[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c  line 597]
	 PL_ProcessPendingEvents	[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c 
line 530]
	 _md_EventReceiverProc	[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c 
line 1078]
	 KERNEL32.DLL + 0x24a97 (0xbff84a97)
	 0x006a83fa
	 0x00058f64
 
 	Source File :
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/document/src/nsXULDocument.cpp
line : 846

Comment 1

[Brendan Eich [:brendan]]

This ought to be reassigned from hyatt, probably.  Any candidates or takers?

/be

Comment 2

[John Morrison]

I think that this crash came about with the fix for bug 156452.  In
~nsXULDocument, _after_ gRefCnt is 0, we do:

  if (mNodeInfoManager) {
      mNodeInfoManager->DropDocumentReference();
  }

The fix for bug 156452 added code to nsNodeInfoManager::DropDocumentReference
that makes it try to access the dying nsXULDocument's GetPrincipal. But I 
have no idea what the right thing to do instead.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

i'll have a look

Comment 4

[John Morrison]

Oops. That's not quite a true statement (gRefCnt isn't necessarily 0 at this 
point). Still, it is that code path that is being exercised.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

to me

Comment 6

[Jan Carpenter]

This stack signature is not showing up in the talkback database in any builds
since 2002.  Marking it fixed.
Reopen if you disagree.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

errr.. i thought we marked bugs fixed when they were fixed, not when people
stopped bumåing into them

Comment 8

[Jay Patel [:jay]]

Jonas: Yes...this should have not been marked fixed...but I think it might be
worth marking worksforme.  Unless you think we still need to fix this somehow. 
Talkback data doesn't show any crashes recently.  I'll leave it up to you to
resolve this bug.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Created attachment 114426 [details] [diff] [review]
null-safty

The only thing i could think of crashing is mMasterPrototype being null when
the nsXULDocument goes away and GetPrincipal is called.

I also moved up the mNodeInfoManager->DropDocumentReference call to the other
DropDocumentReference-calls just because it looked cleaner.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Created attachment 114427 [details] [diff] [review]
null-safety

ok, this time with a *sane* number of contextlines

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 114427 [details] [diff] [review]
null-safety

sr=jst

Comment 12

[John Morrison]

The patch seems fine as it is, but I don't know... Do we really want to add 
null check spackle to GetPrincipal when there isn't really much evidence of 
this being a top, or even "middle" crash in trunk builds. [This is a common 
crash on the branch, and would be really worth it there, but I only find one 
incident of this crash on the trunk, and that was in a build from early 
December]. nsXULDocument::GetPrincipal gets called a lot in the UI code.

Or maybe I'm just being a nitpicker :-)

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we should add the nullcheck anyway. Right now we will crash if you
create a xul-document, set it's url and then bail (thus releasing it). In order
to be fully safe you have to set the masterprototype before setting the url,
which IMHO isn't a good requirement to have.

Even though this isn't a topcrasher any more i bet there are several cases where
we would crash if something failed at the "right" moment, for example running
out of memory.  Better safe then sorry since i doubt that an extra |if| will be
noticeable perf-wise

Comment 14

[John Morrison]

Comment on attachment 114427 [details] [diff] [review]
null-safety

Okey-doke. r=jrgm

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 114427 [details] [diff] [review]
null-safety

fixes a potential crasher. This patch should be compleatly safe since all it
does is add a null-check.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

upps, i landed this a few days ago