331040 - Crash when removing parent iframe in onbeforunload handler

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Martijn Wargers (dead)]

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.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

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

Comment 2

[Martijn Wargers (dead)]

Attachment: Backtrace from debug build

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase2

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>

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Feng Qian (Google)]

Attachment: patch

Comment 6

[Darin Fisher]

-> feng

Comment 7

[Brian Ryner (not reading)]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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 9

[Feng Qian (Google)]

Attachment: revised patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Feng Qian (Google)]

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

Comment 12

[Feng Qian (Google)]

Attachment: revised path (2)

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Feng Qian (Google)]

Attachment: revised patch (3)

Added kungFuDeathGrip(this) in CreateAboutBlankContentViewer.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Feng Qian (Google)]

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

Comment 17

[Martijn Wargers (dead)]

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

Comment 18

[Martijn Wargers (dead)]

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 19

[Darin Fisher]

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.

Comment 20

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

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

Comment 21

[Darin Fisher]

fixed1.8.1

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 217305 [details] [diff] [review]
revised patch (3)

approved for 1.8.0 branch, a=dveditz for drivers

Comment 23

[Feng Qian (Google)]

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
> 

Comment 24

[Martijn Wargers (dead)]

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

Comment 25

[:Gavin Sharp [email: [e-mail]]]

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

Comment 26

[Tracy Walker [:tracy]]

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

Comment 27

[Boris Zbarsky [:bzbarsky]]

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.

Comment 28

[Tracy Walker [:tracy]]

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