336978 - Crash when window gets destroyed on pagehide event

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Martijn Wargers (dead)]

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

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase (crashes on load)

Comment 2

[Martijn Wargers (dead)]

Attachment: Backtrace from debug build

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Martijn Wargers (dead)]

(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}

Comment 5

[Boris Zbarsky [:bzbarsky]]

Interesting.  Over here it's null.

Well, 0xfeeefeee is consistent with objects dying.  We definitely need this fixed on branches, in that case.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: kungFuDeathGrip + null check

Martijn, could you test this, just to make sure ...

Comment 7

[Martijn Wargers (dead)]

(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 8

[Feng Qian (Google)]

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 9

[Brian Ryner (not reading)]

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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: kungFuDeathGrip in docshell

You meant like this?

Comment 11

[Brian Ryner (not reading)]

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

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on trunk with build 2006-05-25-09 of SeaMonkey trunk/Windows XP.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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().

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 223193 [details] [diff] [review]
kungFuDeathGrip in docshell

approved for 1.8.0 branch, a=dveditz for drivers

Comment 15

[Zach Lipton [:zach]]

Verified fixed on mac 1.5.0. branch build 2006062008 and today's 2.0 branch nightly.