Closed Bug 260385 Opened 17 years ago Closed 17 years ago

beforeunload event can be used to open a popup window

Categories

(SeaMonkey :: UI Design, defect)

All
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: danm.moz, Assigned: jst)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.5)

Attachments

(4 files)

Reload a page containing a beforeunload handler that opens a window, you get a
new window. Note this is not the same as bug 259117.
Attachment #159412 - Attachment mime type: text/plain → text/html
Note this was found by XWire on the forums.
Ugh. More notes. Firefox Aviary branch build dated 20040918.
Hardware: PC → All
*** Bug 260581 has been marked as a duplicate of this bug. ***
Confirmed. This only works when clicking the refresh button, not with Ctrl-R.
This seems to have the same reasons as bug 259117.
This blocks popups from beforeunload events. And it undoes a change done for
the fix for the same problem for unload events by moving that code from the
global window code into the document viewer code where the unload event is
fired.
Attachment #159627 - Flags: superreview?(bzbarsky)
Attachment #159627 - Flags: review?(danm.moz)
Comment on attachment 159627 [details] [diff] [review]
Don't permit onloads from onbeforeunload event handlers

Yeah, I like this approach!
Attachment #159627 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 159627 [details] [diff] [review]
Don't permit onloads from onbeforeunload event handlers

Does this not allow you to also back out bug 259117's changes to nsDocShell? I
found them kind of worrisome:

@@ -3129,11 +3129,11 @@ nsDocShell::Create()
 NS_IMETHODIMP
 nsDocShell::Destroy()
 {
+    mIsBeingDestroyed = PR_TRUE;
+
     //Fire unload event before we blow anything away.
     (void) FireUnloadNotification();

-    mIsBeingDestroyed = PR_TRUE;
-
     // Stop any URLs that are currently being loaded...
     Stop(nsIWebNavigation::STOP_ALL);
     if (mDocLoader) {
@@ -3771,10 +3771,6 @@ nsDocShell::ScrollByPages(PRInt32 numPag
 nsIScriptGlobalObject*
 nsDocShell::GetScriptGlobalObject()
 {
-    if (mIsBeingDestroyed) {
-	 return nsnull;
-    }
-
     NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nsnull);

     return mScriptGlobal;
Attachment #159627 - Flags: review?(danm.moz) → review+
Yes, I could actually do that, but I'd rather not do that on the trunk as it's
IMO the right thing to do. I'd be happy to back that out on the branch though,
just in case. Sound ok?
I was especially concerned about the @@ -3771,10 +3771,6 change, which at first
glance looks like it could lead to referenced deleted object crashes. I'm happy
with what you think is best.
Attachment #159627 - Flags: approval1.8a4?
Attachment #159627 - Flags: approval1.7.x?
Attachment #159627 - Flags: approval-aviary?
Comment on attachment 159627 [details] [diff] [review]
Don't permit onloads from onbeforeunload event handlers

a=asa for 1.8a4 checkin.
Attachment #159627 - Flags: approval1.8a4? → approval1.8a4+
Fix checked in on the trunk.
Comment on attachment 159627 [details] [diff] [review]
Don't permit onloads from onbeforeunload event handlers

a=asa for branches checkin.
Attachment #159627 - Flags: approval1.7.x?
Attachment #159627 - Flags: approval1.7.x+
Attachment #159627 - Flags: approval-aviary?
Attachment #159627 - Flags: approval-aviary+
Fixed on trunk and aviary branch. Need to land other changes on the 1.7 branch
before this one can go in there.
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
Keywords: fixed1.7.x
Fixed on 1.7 branch
*** Bug 263271 has been marked as a duplicate of this bug. ***
*** Bug 265886 has been marked as a duplicate of this bug. ***
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.