Browser crashes [@ nsXULDocument::GetPrincipal] N700

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
16 years ago
8 years ago

People

(Reporter: greer, Assigned: sicking)

Tracking

({crash, qawanted, topcrash-})

Trunk
x86
Windows 95
crash, qawanted, topcrash-
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

16 years ago
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
This ought to be reassigned from hyatt, probably.  Any candidates or takers?

/be
Summary: Browser crashes [@ nsXULDocument::GetPrincipal] N700 → Browser crashes [@ nsXULDocument::GetPrincipal] N700
(Reporter)

Updated

16 years ago
Keywords: crash, topcrash

Comment 2

16 years ago
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 4

16 years ago
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.
to me
Assignee: hyatt → sicking

Comment 6

16 years ago
This stack signature is not showing up in the talkback database in any builds
since 2002.  Marking it fixed.
Reopen if you disagree.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
errr.. i thought we marked bugs fixed when they were fixed, not when people
stopped bumåing into them
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 8

16 years ago
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.

Keywords: topcrash → qawanted, topcrash-
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.
Created attachment 114427 [details] [diff] [review]
null-safety

ok, this time with a *sane* number of contextlines
Attachment #114426 - Attachment is obsolete: true
Attachment #114427 - Flags: superreview?(jst)
Attachment #114427 - Flags: review?(jrgm)
Comment on attachment 114427 [details] [diff] [review]
null-safety

sr=jst
Attachment #114427 - Flags: superreview?(jst) → superreview+

Comment 12

16 years ago
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 :-)
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

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

Okey-doke. r=jrgm
Attachment #114427 - Flags: review?(jrgm) → review+
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.
Attachment #114427 - Flags: approval1.3?
upps, i landed this a few days ago
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: shrir → xptoolkit.widgets
Crash Signature: [@ nsXULDocument::GetPrincipal]
You need to log in before you can comment on or make changes to this bug.