See upcoming testcase which crashes current Mozilla trunk build. Doesn't crash in 2005-08-11 build, crashes in 2005-08-13 build. I think a regression from bug 296639. Also crashes in Firefox1.5.0.1.

The testcase crashes on load for me. Talkback ID: TB16568299Y

I guess this is more or less the same issue (it has the same regression range). I'm being redirected to google with this testcase, which should not happen. You need to allow popup windows. Basically the source of the opened window consists of this: <script> window.onbeforeunload=function() {window.close();} window.location="http://google.com"; </script>

Does it help to hold a kungfudeathgrip to |this| through the PermitUnload method? It's not quite clear to me why we didn't have that all alongl we should have....

-> feng

Comment on attachment 216860 [details] [diff] [review] patch >Index: nsDocShell.cpp >=================================================================== >RCS file: /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v >retrieving revision 1.782 >diff -u -u -8 -p -r1.782 nsDocShell.cpp >--- nsDocShell.cpp 24 Mar 2006 19:38:16 -0000 1.782 >+++ nsDocShell.cpp 31 Mar 2006 20:11:51 -0000 >@@ -6478,16 +6478,21 @@ nsDocShell::InternalLoad(nsIURI * aURI, > if (shEntry) > shEntry->SetTitle(mTitle); > } > > return NS_OK; > } > } > >+ // mContentViewer->PermitUnload can destroy |this| docShell, which >+ // causes the next call of CanSavePresentation to crash. Temporarily >+ // hold |this| to prevent crash happen. (bug#331040) >+ nsCOMPtr<nsIDocShell> holder(NS_STATIC_CAST(nsIDocShell*, this)); >+ So actually I told you wrong, you don't need the NS_STATIC_CAST here, just write: nsCOMPtr<nsIDocShell> holder(this); then fix up the second sentence of the comment a bit, maybe "Hold onto |this| until we return, to prevent a crash from happening." r=me with that change.

I should be able to get to this on Sunday. I still think that we might need a better kungFuDeathGrip in nsDocumentViewer as well, but I'll check on that.

Comment on attachment 216881 [details] [diff] [review] revised patch This is necessary, but not sufficient. For example, the stack Martijn posted shows him crashing inside DocumentViewerImpl::PermitUnload where we have an existing kungFuDeathGrip that doesn't extend to cover all member access after possible destruction... Adding the death grip in docshell may or may not help with that crash; better to fix both spots. Also, I think we generally name stack nsCOMPtrs that are supposed to keep |this| alive kungFuDeathGrip (just so it's clear what they're doing). That will fix the crashes. We probably want to have a followup bug (or also roll it into this patch?) on the fact that the load happens in totally the wrong window here. I suspect the right thing will at least require that nsDSURIContentListener::OnStartURIOpen abort if mDocShell is null. I think that should be enough, since destroying a docshell will cancel any existing in-progress loads...

Hold back my previous patch, I only reproduced the crash and fixed it on Linux. Now I reproduced the crash on Windows, and see the same backtrace as Martijn's, it is clearly different from Linux. (In reply to comment #10) > (From update of attachment 216881 [details] [diff] [review] [edit]) > This is necessary, but not sufficient. For example, the stack Martijn posted > shows him crashing inside DocumentViewerImpl::PermitUnload where we have an > existing kungFuDeathGrip that doesn't extend to cover all member access after > possible destruction... Adding the death grip in docshell may or may not help > with that crash; better to fix both spots. > > Also, I think we generally name stack nsCOMPtrs that are supposed to keep > |this| alive kungFuDeathGrip (just so it's clear what they're doing). > > That will fix the crashes. We probably want to have a followup bug (or also > roll it into this patch?) on the fact that the load happens in totally the > wrong window here. I suspect the right thing will at least require that > nsDSURIContentListener::OnStartURIOpen abort if mDocShell is null. I think > that should be enough, since destroying a docshell will cancel any existing > in-progress loads... >

On Linux, Firefox is able to finish PermitUnload, crashes in CanSavePresentation. Its behavior is slightly different from Windows. I am not familiar with nsDSURIContentListener::OnStartURIOpen, probably it is better to open another bug. The nsDocShell::CreateAboutBlankContentViewer() method has a similar pattern of calling mContentViewer->PermitUnload as in nsDocShell::InternalLoad, it might be possible to crash Firefox. But Brain and I didn't come up with a test case to verify that.

I'd add the kungFuDeathGrip in CreateAboutBlankContentViewer, just to be safe...

Added kungFuDeathGrip(this) in CreateAboutBlankContentViewer.

Comment on attachment 217305 [details] [diff] [review] revised patch (3) Seems reasonable. Please make sure to file a followup bug on the loads happening in the wrong window -- that's a major problem that we might want to fix on branches too....

(In reply to comment #15) > (From update of attachment 217305 [details] [diff] [review] [edit]) > Seems reasonable. Please make sure to file a followup bug on the loads > happening in the wrong window -- that's a major problem that we might want to > fix on branches too.... > Who can kindly help me commit the patch, I don't have cvs write access. Boris, can you file the followup bug with some descriptions? I am new and not sure if I fully understood the problem.

(In reply to comment #16) > Boris, can you file the followup bug with some descriptions? I am new and not > sure if I fully understood the problem. I think Boris meant the issue I described in comment 3, I filed bug 332901 for that. I can check the patch in as soon as I'm finished building.

Checking in docshell/base/nsDocShell.cpp; /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp new revision: 1.785; previous revision: 1.784 done Checking in layout/base/nsDocumentViewer.cpp; /cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v <-- nsDocumentViewer.cpp new revision: 1.478; previous revision: 1.477 done Checked into trunk.

Comment on attachment 217305 [details] [diff] [review] revised patch (3) I think we want this for FF2 as well. This should also be considered for FF 1.5.0.3.

Verified FIXED using build 2006-04-06-09 on SeaMonkey trunk using Windows XP. No crash for either testcase, and with popups enabled, the 2nd testcase didn't redirect to http://www.google.com

fixed1.8.1

Comment on attachment 217305 [details] [diff] [review] revised patch (3) approved for 1.8.0 branch, a=dveditz for drivers

Dan, I have not write permission to CVS tree yet, can you or Martjin Wargers help me check in the patch to 1.8.0.3? Thanks, Feng (In reply to comment #18) > Checking in docshell/base/nsDocShell.cpp; > /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp > new revision: 1.785; previous revision: 1.784 > done > Checking in layout/base/nsDocumentViewer.cpp; > /cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v <-- nsDocumentViewer.cpp > new revision: 1.478; previous revision: 1.477 > done > > Checked into trunk. > (In reply to comment #22) > (From update of attachment 217305 [details] [diff] [review] [edit]) > approved for 1.8.0 branch, a=dveditz for drivers >

(In reply to comment #23) > I have not write permission to CVS tree yet, can you or Martjin Wargers help me > check in the patch to 1.8.0.3? Sorry, I don't have a 1.8.0.3 tree.

Checked in on the 1.8.0 branch. mozilla/layout/base/nsDocumentViewer.cpp 1.442.4.7.2.1 mozilla/docshell/base/nsDocShell.cpp 1.719.2.21.2.6

this is not fixed in Firefox 1.5.0.4rc3 builds. The second testcase redirects to google.

Tracy, the redirect to google issue is bug 332901, not this bug. Please see comment 17. This bug is just about the crash, like the summary says.

okay, cool, no crash on 1.5.0.4 or 2.0a2(per bc)