Closed Bug 336978 Opened 14 years ago Closed 14 years ago

Crash when window gets destroyed on pagehide event

Categories

(Core :: DOM: Navigation, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: smaug)

References

Details

(5 keywords)

Attachments

(4 files)

See upcoming testcase, which crashes Mozilla on load.
This doesn't happen in 2005-08-19 build, but happens in 2005-08-21 build:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-19+07&maxdate=2005-08-21+18&cvsroot=%2Fcvsroot
Maybe regression from bug 303765? I have no idea.

Talkback ID: TB18393341Q
DocumentViewerImpl::PageHide   nsDocShell::FirePageHideNotification   nsDocShell::CreateContentViewer   nsDSURIContentListener::DoContent   nsDocumentOpenInfo::TryContentListener   nsDocumentOpenInfo::DispatchContent   0x0012fa78
kModuleInfo
   0xc0d4e92c
So it looks like mDocument is null at that point.... (would have been nice to include the value of mDocument in the gdb output too).

Note that we have death-grips in the code that fires unload (both in docshell and in document viewer), but none here.  Should there be some?  Doesn't seem like it would help with mDocument being null, actually...

bryner, could you check this out?
Component: Event Handling → Embedding: Docshell
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?
OS: Windows XP → All
Hardware: PC → All
(In reply to comment #3)
> So it looks like mDocument is null at that point.... (would have been nice to
> include the value of mDocument in the gdb output too).
I get this:
(gdb) p mDocument
$1 = {mRawPtr = 0xfeeefeee}
Interesting.  Over here it's null.

Well, 0xfeeefeee is consistent with objects dying.  We definitely need this fixed on branches, in that case.
Martijn, could you test this, just to make sure ...
Assignee: events → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #223145 - Flags: superreview?(bryner)
Attachment #223145 - Flags: review?(bryner)
(In reply to comment #6)
> Created an attachment (id=223145) [edit]
> kungFuDeathGrip + null check
> 
> Martijn, could you test this, just to make sure ...

Yes, that seems to fix the crash.
This crash could be easily caught by the patch in bug#338362, here is the message reported by it:

Deleting a possible live ref (this) on the stack:
DocumentViewerImpl::PageHide(int) (/usr/local/google/home/fqian/workspace/firefox_src_kungfudeath/mozilla/layout/base/nsDocumentViewer.cpp:1238)
 

Stack traces:
nsLiveStackRefCheck::Release(unsigned int, void**, void*) (/usr/local/google/home/fqian/workspace/firefox_src_kungfudeath/mozilla/xpcom/base/nsLiveStackRefCheck.cpp:125)
DocumentViewerImpl::Release() (/usr/local/google/home/fqian/workspace/firefox_src_kungfudeath/mozilla/layout/base/nsDocumentViewer.cpp:521)
nsCOMPtr<nsIContentViewer>::assign_assuming_AddRef(nsIContentViewer*) (../../dist/include/xpcom/nsCOMPtr.h:568)

... stack trace skipped ...

nsDocument::OnPageHide(int) (/usr/local/google/home/fqian/workspace/firefox_src_kungfudeath/mozilla/content/base/src/nsDocument.cpp:5194)
DocumentViewerImpl::PageHide(int) (/usr/local/google/home/fqian/workspace/firefox_src_kungfudeath/mozilla/layout/base/nsDocumentViewer.cpp:1238) nsDocShell::FirePageHideNotification(int) (/usr/local/google/home/fqian/workspace/firefox_src_kungfudeath/mozilla/docshell/base/nsDocShell.cpp:923) 

...

(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=223145) [edit]
> > kungFuDeathGrip + null check
> > 
> > Martijn, could you test this, just to make sure ...
> 
> Yes, that seems to fix the crash.
> 
Comment on attachment 223145 [details] [diff] [review]
kungFuDeathGrip + null check

Feng pointed out that it's probably clearer to keep a reference to mContentViewer around the call to PageHide() from nsDocShell::FirePageHideNotification.
Attachment #223145 - Flags: superreview?(bryner)
Attachment #223145 - Flags: superreview-
Attachment #223145 - Flags: review?(bryner)
Attachment #223145 - Flags: review-
You meant like this?
Attachment #223193 - Flags: superreview?(bryner)
Attachment #223193 - Flags: review?(bryner)
Comment on attachment 223193 [details] [diff] [review]
kungFuDeathGrip in docshell

Exactly.  But add a comment saying why you need to hold the reference, e.g.

// Keep an explicit reference since calling PageHide could release mContentViewer
Attachment #223193 - Flags: superreview?(bryner)
Attachment #223193 - Flags: superreview+
Attachment #223193 - Flags: review?(bryner)
Attachment #223193 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #223193 - Flags: approval1.8.0.5?
Attachment #223193 - Flags: approval-branch-1.8.1?(bzbarsky)
Verified FIXED on trunk with build 2006-05-25-09 of SeaMonkey trunk/Windows XP.
Status: RESOLVED → VERIFIED
Comment on attachment 223193 [details] [diff] [review]
kungFuDeathGrip in docshell

Add a comment in the first hunk that we want to make sure that mContentViewer doesn't die while executing PageHide().
Attachment #223193 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
Keywords: fixed1.8.1
Comment on attachment 223193 [details] [diff] [review]
kungFuDeathGrip in docshell

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #223193 - Flags: approval1.8.0.5? → approval1.8.0.5+
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.5?
Flags: blocking1.8.0.5+
Flags: blocking1.9a1?
Keywords: fixed1.8.0.5
Verified fixed on mac 1.5.0. branch build 2006062008 and today's 2.0 branch nightly.
Depends on: 344121
You need to log in before you can comment on or make changes to this bug.