Closed Bug 169036 Opened 23 years ago Closed 22 years ago

Browser crashes [@ nsXULDocument::GetPrincipal] N700

Categories

(Core :: XUL, defect)

x86
Windows 95
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: greer, Assigned: sicking)

Details

(Keywords: crash, qawanted, topcrash-)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
Keywords: crash, topcrash
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.
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
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
Closed: 22 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 → ---
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: topcrashqawanted, topcrash-
Attached patch null-safty (obsolete) — Splinter Review
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.
Attached patch null-safetySplinter Review
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+
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 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
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
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.

Attachment

General

Created:
Updated:
Size: