332901 - [FIX]popup window with js redirect after onbeforeunload and window.close causes the opener window to redirect

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

This is a follow-up from bug 331040, see bug 331040, comment 3.

See url testcase, it has a regression range between 2005-08-11 and 2005-08-13, so most likely a regression from bug 296639.
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 1

[Boris Zbarsky [:bzbarsky]]

darin, biesi, what do you think of my proposed fix in bug 332901 comment 10?

Comment 2

[Martijn Wargers (dead)]

(In reply to comment #1)
> darin, biesi, what do you think of my proposed fix in bug 332901 comment 10?
Boris meant bug 331040, comment 10, of course.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

yeah, sounds good.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this?

I'd also like to hear what you guys think of this for branch.  I'm frankly not sure how safe this change is.  :(

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 217504 [details] [diff] [review]
Something like this?

this doesn't exactly sound like a low-risk change to me...

Comment 6

[Darin Fisher]

Comment on attachment 217504 [details] [diff] [review]
Something like this?

>Index: docshell/base/nsDSURIContentListener.cpp

> NS_IMETHODIMP
> nsDSURIContentListener::OnStartURIOpen(nsIURI* aURI, PRBool* aAbortOpen)
> {
>+    // If mDocShell is null here, that means someone's starting a load
>+    // in our docshell after it's already been destroyed.  Don't let
>+    // that happen.
>+    if (!mDocShell) {
>+        *aAbortOpen = PR_TRUE;
>+        return NS_OK;
>+    }

Should we emit a warning here?


sr=darin

Comment 7

[Boris Zbarsky [:bzbarsky]]

If the load had started before the window.close() then we'd just Stop() the load with no warnings.  I'm not sure it's worth warning just because the order of those two statements was reversed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  I'm really not sure enough about the safety of this to want it for branch.