Bug 19418 - onbeforeunload is broken for framesets
: onbeforeunload is broken for framesets
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 525.x (Safari 3.1)
: All All
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2008-06-06 16:00 PST by
Modified: 2011-03-11 13:49 PST (History)


Attachments
The frameset file (863 bytes, text/html)
2008-06-06 16:01 PST, William J. Edney
no flags Details
The frame content file (562 bytes, text/html)
2008-06-06 16:04 PST, William J. Edney
no flags Details
The frameset file v2 (904 bytes, text/html)
2009-11-21 16:34 PST, William J. Edney
no flags Details
The frame content file v2 (954 bytes, text/html)
2009-11-21 16:34 PST, William J. Edney
no flags Details
Works on FF 3.6 and IE8. Please put parent.html and iframe.html in the same folder and load parent.html. When changing the child url with the button you'll see PASS on FF. (617 bytes, application/zip)
2010-11-18 15:53 PST, pablo
no flags Details
work in progress (746 bytes, patch)
2010-11-18 16:34 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
initial patch (4.16 KB, patch)
2010-12-06 12:34 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
Added more tests per darin's comment and to address ap's concern (10.87 KB, patch)
2010-12-17 14:25 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
Removed before-unload-in-subframe-fail.html (10.38 KB, patch)
2010-12-17 14:38 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
test case (zip) (771 bytes, application/octet-stream)
2011-01-04 10:42 PST, Ryosuke Niwa
no flags Details
fixes the bug (23.86 KB, patch)
2011-01-04 17:53 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
Updated per Adam's comment (23.42 KB, patch)
2011-01-05 19:34 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
added more tests (34.69 KB, patch)
2011-01-06 19:42 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
added one more test per abarth's request (37.00 KB, patch)
2011-01-07 14:06 PST, Ryosuke Niwa
abarth: review+
Review Patch | Details | Formatted Diff | Diff
change framecontent with document.write (1.75 KB, application/zip)
2011-03-11 07:41 PST, Holger Hees
no flags Details


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-06-06 16:00:55 PST
Folks -

Even though the IE documentation for onbeforeunload (since there's no real W3C standard for this event, we go by their stuff) says that onbeforeunload should work on 'frameset's as well as 'body's, ever since Safari 3.1 (on both Windows and Mac), this has been broken.

I've attached two testcase files - one is the frameset file and the other is the framecontent file. If you launch the frameset file and click the link (which is contained in the frame content file), you are immediately navigated to Google, which is incorrect - the onbeforeunload dialog box should stop you first. Curiously enough, if you try to return to the test page from Google by hitting the back button, you get the frameset's onbeforeunload message then - but its too late.

Note that if you load the frame content file directly and then try to click the link to go to Google, you get the proper behavior which is to present the onbeforeunload dialog box *before* you navigate to Google.

Marking this as P1 / Major since data loss could result from a loss of state in the client (prevelant amongst AJAX applications).

Cheers,

- Bill
------- Comment #1 From 2008-06-06 16:01:46 PST -------
Created an attachment (id=21535) [details]
The frameset file
------- Comment #2 From 2008-06-06 16:04:21 PST -------
Created an attachment (id=21536) [details]
The frame content file
------- Comment #3 From 2008-11-08 17:13:15 PST -------
This issue has stricken us as well.  Can someone mark this confirmed?
------- Comment #4 From 2008-11-15 02:17:50 PST -------
Confirmed with r38387.

See also: bug 15652.
------- Comment #5 From 2008-11-15 02:19:54 PST -------
...and bug 21699.
------- Comment #6 From 2008-11-15 02:30:04 PST -------
Note however that fixing this may give a way to ads in iframes to be even more nasty than they are now, and open windows when navigating away from a page with an ad. They could be other undesirable consequences, as well.
------- Comment #7 From 2008-11-18 11:28:41 PST -------
Alexey -

As one of the guys who supported the Mozilla crew adding this to their browser many moons ago (https://bugzilla.mozilla.org/show_bug.cgi?id=68215), I should speak up here.

This was originally added to IE 4.0 in 1997. It is a Microsoft extension and one that has been very handy for those of us who are building 'stateful client' applications. It allows us to warn the user that by either closing the window, quitting the browser or navigating away from the 'page' that they will lose data and that they should use the approved method of quitting the page.

In the intervening 11 years that this feature has been around, it has not been abused. I have not once been to a page that uses this mechanism in an abusive measure. I surmise that, because of the design of this feature (i.e. just popping up a panel that allows an application-supplied message along with a browser-supplied one and allowing the user to proceed or cancel), that its not really a sufficiently 'spoofable' mechanism for the evildoers.

Cheers,

- Bill
------- Comment #8 From 2008-11-18 11:47:21 PST -------
What I had in mind was something different - an onload/onbeforeunload handler that opens a new window with window.open(). This is pretty common on the Web, and very annoying. Fixing this bug can let any 3rd party ad in an iframe do this.
------- Comment #9 From 2008-11-18 16:46:36 PST -------
I'm not sure I see the significance.  There are already onunload handlers that nefarious people could use to open windows on exit.  And onbeforeunload already exists at the top-level, this is just asking for it to work properly for frames.
------- Comment #10 From 2009-07-15 01:52:42 PST -------
The onbeforeunload is different then the onunload event because it let you for example add a cookie to the request. The unload event fires after the request is ready so you can't modify it. 

Why not just enable the event when the iframe and the top frame are from the same domain?
------- Comment #11 From 2009-07-16 02:40:08 PST -------
*** Bug 21699 has been marked as a duplicate of this bug. ***
------- Comment #12 From 2009-08-27 15:39:52 PST -------
Any updates on this bug report?

The use case for enabling embedded iframes to handle onbeforeunload seems valid, especially since normal windows support the event just fine.

We are using -- rather, trying to use -- onbeforeunload to track a user's progress as they navigate through an embedded iframe.

We would really like to see this bug updated.  I would be happy to help further in any way I can.
------- Comment #13 From 2009-08-27 15:56:57 PST -------
Ditto to comment #12 ... just hoping enough *bumps* will put this on someone's radar.  Very frustrating that there are legitimate uses for this, but our requests are largely ignored.
------- Comment #14 From 2009-08-27 16:11:46 PST -------
<rdar://problem/7177160>
------- Comment #15 From 2009-10-19 04:23:47 PST -------
I must say: wow, open for over a year now. I would love to label my solutions Safari, Chrome, etc. compatible. But still no luck.

Could somebody please fix this?

Now for some actual input: the "same-domain-constraint" mentioned is very reasonable. 

Developers: if there are any other considerations preventing this bug from getting fixed, please share them with us so we can come to a solution.
------- Comment #16 From 2009-11-21 16:31:23 PST -------
In hopes that someone out there on the Webkit team can address this issue soon, here's my latest set of test files. I tried to make the messages a bit more consistent and clear. These are marked as 'The frameset file v2' and 'The frame content file v2'.

Unfortunately, the behavior that I noted in comment #1 hasn't improved. It is consistently broken in the same way.


As of today, I also noticed the following behavior, which is also inconsistent with how both IE and Mozilla work, and is yet another reason we're broken on Webkit-based browsers.

In IE / Mozilla:

- Launch the v2 versions of these pages.
- Attempt to close the page.
- You'll see a dialog containing the 'onbeforeunload' message installed on the frameset.
- Choose 'Ok' to 'continue closing' (if you choose 'Cancel', the system will - properly in my opinion - not show you the second dialog)
- Then you'll see a *second* dialog containing the 'onbeforeunload' message installed on the nested frame.

In Webkit-based browsers you *only see the first dialog*. The onbeforeunload handler for the nested frame is not being executed.


The Chromium team has upstreamed their bug report on this behavior back to this bug. Their report is here:

http://code.google.com/p/chromium/issues/detail?id=17157

They also mention this bug in their bug report. It's probably related:

https://bugs.webkit.org/show_bug.cgi?id=27481


To anyone on the Webkit team - this continues to badly break us. *Please* let me know if I can help in any way insofar as more test cases, etc. I haven't done C in many moons, so I can't help there unfortunately. Sam, I'm not sure if the 'rdar' bug system reflects changes here, but if not please pass along these comments. Also, feel free to email me or engage me in any fashion to help you all fix this bug - I'll spend time explaining it over the phone, if that's necessary - just let me know. As has been noted previously, its been a year. I know you all are amazingly busy but for those of us writing stateful applications, this is a deal breaker for Safari/Chrome/etc. support.

For my part, I've tested this on the following browsers. It is (thankfully) consistent between all of them:

Safari 4.0.4 / Mac
Webkit 51280 / Mac
Google Chrome 4.0.249.4 / Mac
Google Chrome 4.0.249.4 / Windows

Safari 4.0.4 / Windows is a strange case indeed. Launching the frameset and then using the link to go to Google doesn't light up the 'Back' button, such that you can't traverse back. This very well may be another bug. I don't have a Webkit nightly for Windows, but if there's interest, I can get one and retry on that.

Thanks to all in advance. Let me know if beer and/or wine bribery will be involved :-) (I know for a fact that the Mozillians can be bribed ;-) ).

Cheers,

- Bill
------- Comment #17 From 2009-11-21 16:34:04 PST -------
Created an attachment (id=43664) [details]
The frameset file v2
------- Comment #18 From 2009-11-21 16:34:34 PST -------
Created an attachment (id=43665) [details]
The frame content file v2
------- Comment #19 From 2010-04-07 08:54:19 PST -------
I've also ran into problems due to this event not firing in frames, and seeing as I've never seen the event hijacked in other browsers, I don't think it's much of a security concern (although I also agree that the domain constraint is reasonable).  I doubt this will see resolution any time soon, considering how long this has been open, but I wanted to add myself to the list in case it is.
------- Comment #20 From 2010-07-03 16:14:19 PST -------
Bump. Please fix.
------- Comment #21 From 2010-10-12 01:31:53 PST -------
Please fix.
------- Comment #22 From 2010-11-18 15:53:12 PST -------
Created an attachment (id=74307) [details]
Works on FF 3.6 and IE8. Please put parent.html and iframe.html in the same folder and load parent.html. When changing the child url with the button you'll see PASS on FF.
------- Comment #23 From 2010-11-18 16:03:51 PST -------
We need the beforeunload event on iframes in addition to the unload event from the same reasons we need both on any page.

The events serve different purpose.
Code in the beforeunload event listener can effect the request while code in the unload event listener can't.
For example, if we set a cookie in the beforeunload listener, it'll be sent with that request but if we set the cookie in the unload listener, it'll only effect the next request.

If the parent and the iframe are both from the same origin, there is no security issue.
------- Comment #24 From 2010-11-18 16:34:40 PST -------
Created an attachment (id=74319) [details]
work in progress

Here's very easy change to enable this feature.  I have to go check with other contributors to see if enabling this feature makes sense.
------- Comment #25 From 2010-12-06 10:44:14 PST -------
Alexey, while I agree that (In reply to comment #8)
> What I had in mind was something different - an onload/onbeforeunload handler that opens a new window with window.open(). This is pretty common on the Web, and very annoying. Fixing this bug can let any 3rd party ad in an iframe do this.

Can't the 3rd party add just window.open without waiting for the unload event? This doesn't seem to make things worse.
------- Comment #26 From 2010-12-06 12:34:27 PST -------
Created an attachment (id=75730) [details]
initial patch
------- Comment #27 From 2010-12-06 12:41:15 PST -------
(From update of attachment 75730 [details])
This patch looks good to me. Alexey, you OK with this?
------- Comment #28 From 2010-12-06 12:54:06 PST -------
(In reply to comment #26)
> Created an attachment (id=75730) [details] [details]
> initial patch

I know there are some controversy around this feature so this is more like a starting point.
------- Comment #29 From 2010-12-17 11:30:19 PST -------
(From update of attachment 75730 [details])
Change seems OK to me. Testing seems a little light? Did you test this with the browsers too? What did Safari do? What about a page that has multiple frames that all have beforeunload handlers?

Alexey, any further comment?
------- Comment #30 From 2010-12-17 11:58:03 PST -------
Darin -

Comment #16 has my observations from Nov 2009 regarding nested frames (iframes) from the same domain.

It uses the 'The frameset file v2' and 'The frame content file v2' files attached to this bug.

Cheers,

- Bill
------- Comment #31 From 2010-12-17 12:01:46 PST -------
I think that we should ensure that there is at most one OK/Cancel dialog displayed by onbeforeunload. Otherwise, what does it mean for the user to OK closing a subframe, but to say Cancel about the main frame? That's confusing to the user, and potentially leaves WebKit in an inconsistent state.

From event handing standpoint, what should a subframe do when it gets a beforeunload event? It doesn't know whether it's really going to be navigated away, since the user may cancel navigation from another frame's beforeunload event handler.
------- Comment #32 From 2010-12-17 12:44:11 PST -------
Thanks for the comments, Darin & Alexey.

(In reply to comment #29)
> Change seems OK to me. Testing seems a little light? Did you test this with the browsers too? What did Safari do? What about a page that has multiple frames that all have beforeunload handlers?

Thanks.  I'll add some more tests.

(In reply to comment #31)
> I think that we should ensure that there is at most one OK/Cancel dialog displayed by onbeforeunload. Otherwise, what does it mean for the user to OK closing a subframe, but to say Cancel about the main frame? That's confusing to the user, and potentially leaves WebKit in an inconsistent state.

As far as I can tell, we don't show multiple OK/Cancel dialogs when we navigate away the main frame (tested on WebKit TOT Mac port with my patch). loadWithNavigationAction isn't even called for subframes.

> From event handing standpoint, what should a subframe do when it gets a beforeunload event? It doesn't know whether it's really going to be navigated away, since the user may cancel navigation from another frame's beforeunload event handler.

Because of the above behavior, I think we don't have to worry about this issue.
------- Comment #33 From 2010-12-17 14:25:37 PST -------
Created an attachment (id=76910) [details]
Added more tests per darin's comment and to address ap's concern
------- Comment #34 From 2010-12-17 14:32:31 PST -------
(From update of attachment 76910 [details])
I think I can eliminate one file from this patch.
------- Comment #35 From 2010-12-17 14:38:07 PST -------
Created an attachment (id=76911) [details]
Removed before-unload-in-subframe-fail.html
------- Comment #36 From 2010-12-23 16:36:34 PST -------
Per ap's request on IRC, I'll explain the new behavior of beforeunload (on WebKit mac port):

* beforeunload is fired for each subframe when subframe's content is replaced by other content
* beforeunload is NOT fired for each subframe if the main frame is to be replaced by another content
* beforeunload continues to be fired for the main frame when it is replaced by another content
------- Comment #37 From 2010-12-24 10:17:03 PST -------
(From update of attachment 76911 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=76911&action=review

Seems sane to me, but given the long thread here I may be missing context.

> LayoutTests/ChangeLog:22
> +2010-12-17  Ryosuke Niwa  <rniwa@webkit.org>
> +
> +        Reviewed by NOBODY (OOPS!).

Double entry in your ChangeLog.
------- Comment #38 From 2010-12-24 13:10:31 PST -------
I think Ryosuke was going to do some more testing based on an IRC discussion.
------- Comment #39 From 2010-12-24 13:12:51 PST -------
(In reply to comment #38)
> I think Ryosuke was going to do some more testing based on an IRC discussion.

Yes, I'm planning to do some research about IE/FF's behavior and probably add more test cases based on ap's suggestion on IRC.
------- Comment #40 From 2010-12-27 18:47:27 PST -------
I'm pretty sure that all other major browsers fire onbeforeunload on subframes when the main frame's content is being replaced. Not having WebKit follow suit will be a major pain in the butt.
------- Comment #41 From 2010-12-28 00:59:26 PST -------
I would like to confirm Jonathan Hurshmans comment. Both IE (all common versions) and FF (current and several older versions) trigger onbeforeunload on all subframes.

Even if the event would only be triggered on the top frame on replacement, I would be able to work around that one. But following IEs & FFs behaviour would save me that hassle ;). Nonetheless, thank you for finally following up on this bug.
------- Comment #42 From 2011-01-04 10:15:22 PST -------
IE and FF both consistently fire onbeforeunload for all subframes prior to initiating navigation. If any of subframe's beforeunload event handler returns false, then a confirmation dialog is popped up for each and every frame that returned false. Confirmation cascades so canceling any one dialog results in not initiating any navigation at all.

When location.href is rewritten on beforeunload of a child frame, IE and FF behave differently.  Let us suppose that the main frame is navigation away to some external page A, and one of child frame rewrites the main frame's location.href to some other external page B.

Case 1, user clicks "leave" on each confirmation dialog:
IE: ignores the rewrite and navigates to A
FF: navigates first to B but then quickly navigates back to A. I haven't tested the case where B had beforeunload event, etc... but FF remembers B in its history stack.

Case 2, user clicks "stay" on the subframe which attempts the rewrite:
IE: ignores the rewrite and does not navigate any page at all.
FF: Rewrite happens before the confirmation dialog is prompted for the subframe which initiated the rewrite so FF tries to navigate to B. Of course, this triggers onbeforeunload on each frame except the subframe from which the rewrite was initiated (for which I click "leave" for the sake of discussion). However, after moving to B, the confirmation for the subframe that initiated rewrite appears and clicking "leave" results in moving to A and clicking "stay" results in staying on B.

I think IE's behavior is much saner. As a user, I can't make sense of FF's behavior at all.
------- Comment #43 From 2011-01-04 10:42:34 PST -------
Created an attachment (id=77903) [details]
test case (zip)
------- Comment #44 From 2011-01-04 16:21:38 PST -------
I experimented with showing just one dialog but it didn't work because multiple sub frames can return different strings. There are a couple of options to deal with this situation:

1. Show confirmation dialog for each string - this is what IE does. It might be too noisy for users but WebKit browsers can add a check box to suppress dialogs.

2. Show a dialog only for the fist non-null string - this guarantees that exactly one dialog pops up but it limits usage to some extent.  On web apps with multiple iframe gadget for example, the only one of them will be able to show a dialog to users.

3. Show a dialog with concatenated string - this not only shows exactly one dialog but also shows all messages returned by each subframe, allowing each subframe to provide useful information.  However, it's somewhat questionable if web authors had considered such implementation when determining which string to return.

I'll go make a patch with option 1 because I discussed about this with ojan and we concluded that option 1 is the simplest yet most compatible.  Please give us a feedback if you prefer other options.
------- Comment #45 From 2011-01-04 16:29:21 PST -------
As the original reporter of this bug, I'll also vote for option #1. I agree that its noisy, but its most compatible and allows each sub-frame to show its message as intended. I wouldn't even worry about a checkbox. None of the other browsers do it and its not what users would expect.

Option #2 is untenable because you never know what is loaded in which sub-frames and they each 'need their shot' at giving the user a chance to *not* unload them

Option #3 is just confusing, IMHO.

My 2 cents.

IIRC, if any window or sub-frame returns a null string, the dialog box is *not* shown for that window/sub-frame. I think that's true in all browsers.

My thanks for fixing this! It's been kind of languishing, but is extremely important in apps that hold state and want to give the user a chance to not flush that state before taking other actions.

Cheers,

- Bill
------- Comment #46 From 2011-01-04 17:53:20 PST -------
Created an attachment (id=77955) [details]
fixes the bug
------- Comment #47 From 2011-01-04 17:54:40 PST -------
(In reply to comment #46)
> Created an attachment (id=77955) [details] [details]
> fixes the bug

Unfortunately, I could not figure out how to test location.reload and history.back/history.forward.
------- Comment #48 From 2011-01-05 14:12:48 PST -------
(From update of attachment 77955 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=77955&action=review

Some minor comments below.

> WebCore/loader/FrameLoader.cpp:2879
> +bool FrameLoader::fireBeforeUnloadEvent(Chrome* chrome, Frame* frame)
> +{

I'm confused.  Is this a static?  Why isn't this a member function?

> WebCore/loader/FrameLoader.cpp:2886
> +    if (!document->body())
>          return true;

That seems goofy, but I guess it's here already.

> WebCore/loader/FrameLoader.h:366
> +    static bool fireBeforeUnloadEvent(Chrome*, Frame*);

This shouldn't be a static.  You're pass the frame, but you should just use m_frame.

> WebCore/loader/NavigationScheduler.cpp:268
> -    if (!m_frame->page())
> +    if (!m_frame->page() || (!protocolIsJavaScript(url) && !m_frame->page()->navigationEnabled()))

This check is copy/pasted four times.  Maybe it should be factored out?

> WebCore/page/Page.h:349
> +        bool m_navigationEnabled;

This seems slightly sketchy to me, but I'm not sure.  It might be ok.
------- Comment #49 From 2011-01-05 14:51:57 PST -------
(From update of attachment 77955 [details])
Thanks for the feedback!

View in context: https://bugs.webkit.org/attachment.cgi?id=77955&action=review

>> WebCore/loader/FrameLoader.cpp:2879
>> +{
> 
> I'm confused.  Is this a static?  Why isn't this a member function?

Yes.  This static member function is called by shouldClose on m_frame AND m_frame's descendents.  So it certainly cannot be a member function because it is called on different frames.  However, it can't be a non-member static function either because it sets values of m_pageDismissalEventBeingDispatched, which is a private member variable of FrameLoader.

>> WebCore/loader/FrameLoader.h:366
>> +    static bool fireBeforeUnloadEvent(Chrome*, Frame*);
> 
> This shouldn't be a static.  You're pass the frame, but you should just use m_frame.

For the reasons I stated above, it cannot use m_frame.

>> WebCore/loader/NavigationScheduler.cpp:268
>> +    if (!m_frame->page() || (!protocolIsJavaScript(url) && !m_frame->page()->navigationEnabled()))
> 
> This check is copy/pasted four times.  Maybe it should be factored out?

It could be.  hasPageWithNavigationEnabled() ?

>> WebCore/page/Page.h:349
>> +        bool m_navigationEnabled;
> 
> This seems slightly sketchy to me, but I'm not sure.  It might be ok.

Since this is very specific to beforeunload event, I'd like to call it m_navigationDisabledInBeforeaUnload but we don't really have a disabled flag in WebKit.  I'm open to a better name / approach.  I too hate the fact I'm adding a new reflection to Page just so that beforeunload can do the right thing.
------- Comment #50 From 2011-01-05 15:44:06 PST -------
(From update of attachment 77955 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=77955&action=review

>>> WebCore/loader/FrameLoader.cpp:2879
>>> +bool FrameLoader::fireBeforeUnloadEvent(Chrome* chrome, Frame* frame)
>>> +{
>> 
>> I'm confused.  Is this a static?  Why isn't this a member function?
> 
> Yes.  This static member function is called by shouldClose on m_frame AND m_frame's descendents.  So it certainly cannot be a member function because it is called on different frames.  However, it can't be a non-member static function either because it sets values of m_pageDismissalEventBeingDispatched, which is a private member variable of FrameLoader.

Why can't you call it like this:

otherFrame->loader()-> fireBeforeUnloadEvent();

?

>>> WebCore/loader/NavigationScheduler.cpp:268
>>> +    if (!m_frame->page() || (!protocolIsJavaScript(url) && !m_frame->page()->navigationEnabled()))
>> 
>> This check is copy/pasted four times.  Maybe it should be factored out?
> 
> It could be.  hasPageWithNavigationEnabled() ?

shouldScheduleNavigation() ?  I'd include the null page check too.

>>> WebCore/page/Page.h:349
>>> +        bool m_navigationEnabled;
>> 
>> This seems slightly sketchy to me, but I'm not sure.  It might be ok.
> 
> Since this is very specific to beforeunload event, I'd like to call it m_navigationDisabledInBeforeaUnload but we don't really have a disabled flag in WebKit.  I'm open to a better name / approach.  I too hate the fact I'm adding a new reflection to Page just so that beforeunload can do the right thing.

Yeah, just adding a bool to Page requires some thought.  Page and Frame are dumping grounds for random state that we're not sure where to put.
------- Comment #51 From 2011-01-05 15:53:35 PST -------
(From update of attachment 77955 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=77955&action=review

>>>> WebCore/loader/FrameLoader.cpp:2879
>>>> +{
>>> 
>>> I'm confused.  Is this a static?  Why isn't this a member function?
>> 
>> Yes.  This static member function is called by shouldClose on m_frame AND m_frame's descendents.  So it certainly cannot be a member function because it is called on different frames.  However, it can't be a non-member static function either because it sets values of m_pageDismissalEventBeingDispatched, which is a private member variable of FrameLoader.
> 
> Why can't you call it like this:
> 
> otherFrame->loader()-> fireBeforeUnloadEvent();
> 
> ?

Ah, why didn't I think of that.  Let me try.

>>>> WebCore/loader/NavigationScheduler.cpp:268
>>>> +    if (!m_frame->page() || (!protocolIsJavaScript(url) && !m_frame->page()->navigationEnabled()))
>>> 
>>> This check is copy/pasted four times.  Maybe it should be factored out?
>> 
>> It could be.  hasPageWithNavigationEnabled() ?
> 
> shouldScheduleNavigation() ?  I'd include the null page check too.

Will do.

>>>> WebCore/page/Page.h:349
>>>> +        bool m_navigationEnabled;
>>> 
>>> This seems slightly sketchy to me, but I'm not sure.  It might be ok.
>> 
>> Since this is very specific to beforeunload event, I'd like to call it m_navigationDisabledInBeforeaUnload but we don't really have a disabled flag in WebKit.  I'm open to a better name / approach.  I too hate the fact I'm adding a new reflection to Page just so that beforeunload can do the right thing.
> 
> Yeah, just adding a bool to Page requires some thought.  Page and Frame are dumping grounds for random state that we're not sure where to put.

I experimented with adding a flag to Frame/FrameLoader (FrameLoader already has shouldAllowNavigation) but it gets ugly because shouldAllowNavigation needs to take a url string and then we need to iterate through frames to figure it out.  The same is true if we were to add something to NavigationScheduler.  I could push lock/unlock logic into fireBeforeUnloadEvent though.  That way we can add a flag to FrameLoader or NavigationScheduler although this would still O(n) assignments but that might be negligible given the existing code.  Any preferences?
------- Comment #52 From 2011-01-05 16:24:19 PST -------
(From update of attachment 77955 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=77955&action=review

>>>>> WebCore/page/Page.h:349
>>>>> +        bool m_navigationEnabled;
>>>> 
>>>> This seems slightly sketchy to me, but I'm not sure.  It might be ok.
>>> 
>>> Since this is very specific to beforeunload event, I'd like to call it m_navigationDisabledInBeforeaUnload but we don't really have a disabled flag in WebKit.  I'm open to a better name / approach.  I too hate the fact I'm adding a new reflection to Page just so that beforeunload can do the right thing.
>> 
>> Yeah, just adding a bool to Page requires some thought.  Page and Frame are dumping grounds for random state that we're not sure where to put.
> 
> I experimented with adding a flag to Frame/FrameLoader (FrameLoader already has shouldAllowNavigation) but it gets ugly because shouldAllowNavigation needs to take a url string and then we need to iterate through frames to figure it out.  The same is true if we were to add something to NavigationScheduler.  I could push lock/unlock logic into fireBeforeUnloadEvent though.  That way we can add a flag to FrameLoader or NavigationScheduler although this would still O(n) assignments but that might be negligible given the existing code.  Any preferences?

I don't really like any of those alternatives.  Maybe someone reading this thread has a clever idea?  If not, we can go this route.

Have you considered the case where the beforeload event in one page triggers a navigation in another page?  Are we supposed to block that navigation too?
------- Comment #53 From 2011-01-05 16:24:51 PST -------
(From update of attachment 77955 [details])
Marking this R- so you can address the comments from the discussion above.
------- Comment #54 From 2011-01-05 16:29:49 PST -------
(From update of attachment 77955 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=77955&action=review

>>>>>> WebCore/page/Page.h:349
>>>>>> +        bool m_navigationEnabled;
>>>>> 
>>>>> This seems slightly sketchy to me, but I'm not sure.  It might be ok.
>>>> 
>>>> Since this is very specific to beforeunload event, I'd like to call it m_navigationDisabledInBeforeaUnload but we don't really have a disabled flag in WebKit.  I'm open to a better name / approach.  I too hate the fact I'm adding a new reflection to Page just so that beforeunload can do the right thing.
>>> 
>>> Yeah, just adding a bool to Page requires some thought.  Page and Frame are dumping grounds for random state that we're not sure where to put.
>> 
>> I experimented with adding a flag to Frame/FrameLoader (FrameLoader already has shouldAllowNavigation) but it gets ugly because shouldAllowNavigation needs to take a url string and then we need to iterate through frames to figure it out.  The same is true if we were to add something to NavigationScheduler.  I could push lock/unlock logic into fireBeforeUnloadEvent though.  That way we can add a flag to FrameLoader or NavigationScheduler although this would still O(n) assignments but that might be negligible given the existing code.  Any preferences?
> 
> I don't really like any of those alternatives.  Maybe someone reading this thread has a clever idea?  If not, we can go this route.
> 
> Have you considered the case where the beforeload event in one page triggers a navigation in another page?  Are we supposed to block that navigation too?

Yes, MSIE blocks such navigation so we should do prevent such navigations as well.
------- Comment #55 From 2011-01-05 17:09:30 PST -------
(From update of attachment 77955 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=77955&action=review

>>>>>>> WebCore/page/Page.h:349
>>>>>>> +        bool m_navigationEnabled;
>>>>>> 
>>>>>> This seems slightly sketchy to me, but I'm not sure.  It might be ok.
>>>>> 
>>>>> Since this is very specific to beforeunload event, I'd like to call it m_navigationDisabledInBeforeaUnload but we don't really have a disabled flag in WebKit.  I'm open to a better name / approach.  I too hate the fact I'm adding a new reflection to Page just so that beforeunload can do the right thing.
>>>> 
>>>> Yeah, just adding a bool to Page requires some thought.  Page and Frame are dumping grounds for random state that we're not sure where to put.
>>> 
>>> I experimented with adding a flag to Frame/FrameLoader (FrameLoader already has shouldAllowNavigation) but it gets ugly because shouldAllowNavigation needs to take a url string and then we need to iterate through frames to figure it out.  The same is true if we were to add something to NavigationScheduler.  I could push lock/unlock logic into fireBeforeUnloadEvent though.  That way we can add a flag to FrameLoader or NavigationScheduler although this would still O(n) assignments but that might be negligible given the existing code.  Any preferences?
>> 
>> I don't really like any of those alternatives.  Maybe someone reading this thread has a clever idea?  If not, we can go this route.
>> 
>> Have you considered the case where the beforeload event in one page triggers a navigation in another page?  Are we supposed to block that navigation too?
> 
> Yes, MSIE blocks such navigation so we should do prevent such navigations as well.

In that case, we should probably use a static variable in a RAII.
------- Comment #56 From 2011-01-05 19:34:34 PST -------
Created an attachment (id=78089) [details]
Updated per Adam's comment
------- Comment #57 From 2011-01-05 19:42:22 PST -------
(From update of attachment 78089 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=78089&action=review

> WebCore/loader/FrameLoader.cpp:2864
> +    {
> +        DisallowNavigationInBeforeUnload disallowNavigation;

I've got an impression that we normally put a RAII object in a new block when it appears in the middle of a function but I can get rid of it if it's not customary.
------- Comment #58 From 2011-01-05 19:47:44 PST -------
(From update of attachment 78089 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=78089&action=review

>> WebCore/loader/FrameLoader.cpp:2864
>> +        DisallowNavigationInBeforeUnload disallowNavigation;
> 
> I've got an impression that we normally put a RAII object in a new block when it appears in the middle of a function but I can get rid of it if it's not customary.

It's might not be necessary here, but it's also nice because it calls attention to the fact that this object has a side-effect.
------- Comment #59 From 2011-01-05 20:24:59 PST -------
(From update of attachment 78089 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=78089&action=review

> WebCore/loader/NavigationScheduler.cpp:275
> +    return m_frame->page() && (protocolIsJavaScript(url) || DisallowNavigationInBeforeUnload::isNavigationAllowed());

I'd call through to shouldScheduleNavigation() here.

> WebCore/loader/NavigationScheduler.h:57
> +    DisallowNavigationInBeforeUnload() { ASSERT(s_navigationIsAllowed); s_navigationIsAllowed = false; }
> +    ~DisallowNavigationInBeforeUnload() { ASSERT(!s_navigationIsAllowed); s_navigationIsAllowed = true; }

Functions with multiple statements shouldn't be all on one line.

> LayoutTests/fast/events/before-unload-forbidden-navigation.html:4
> +<p>This test ensures navigation is forbidden while beforeunload event is being fired. You should see PASS 1/2 and PASS 2/2 blow:</p>

blow => below?

> LayoutTests/fast/events/before-unload-in-multiple-subframes.html:34
> +        log.innerHTML = 'PASS\n';

Is the newline really helpful here?
------- Comment #60 From 2011-01-05 20:26:19 PST -------
(From update of attachment 78089 [details])
i have some slight reservations about blocking the navigation in the NavigationScheduler, but it seems to be what we're supposed to do to match IE.
------- Comment #61 From 2011-01-06 08:48:32 PST -------
Thanks for the review, Adam.

(In reply to comment #59)
> > WebCore/loader/NavigationScheduler.cpp:275
> > +    return m_frame->page() && (protocolIsJavaScript(url) || DisallowNavigationInBeforeUnload::isNavigationAllowed());
> 
> I'd call through to shouldScheduleNavigation() here.

Will do.

> > WebCore/loader/NavigationScheduler.h:57
> > +    DisallowNavigationInBeforeUnload() { ASSERT(s_navigationIsAllowed); s_navigationIsAllowed = false; }
> > +    ~DisallowNavigationInBeforeUnload() { ASSERT(!s_navigationIsAllowed); s_navigationIsAllowed = true; }
> 
> Functions with multiple statements shouldn't be all on one line.

Oops, will fix.

> > LayoutTests/fast/events/before-unload-forbidden-navigation.html:4
> > +<p>This test ensures navigation is forbidden while beforeunload event is being fired. You should see PASS 1/2 and PASS 2/2 blow:</p>
> 
> blow => below?

Yes, will fix (as well as all other tests).

> > LayoutTests/fast/events/before-unload-in-multiple-subframes.html:34
> > +        log.innerHTML = 'PASS\n';
> 
> Is the newline really helpful here?

Nope, fixed.

By the way, do you think of a better name for DisallowNavigationInBeforeUnload?  Classname shouldn't really be a class name but I couldn't come up with a better one.
------- Comment #62 From 2011-01-06 08:49:01 PST -------
Classname shouldn't really be a *verb* but I couldn't come up with a better one.
------- Comment #63 From 2011-01-06 10:46:43 PST -------
(From update of attachment 78089 [details])
It turned out that I need to a little more attention to the case where iframe is moved within beforeunload event handler. Namely, I end up hitting an assert I added in DisallowNavigationInBeforeUnload's c'tor and d'tor now.
------- Comment #64 From 2011-01-06 19:42:52 PST -------
Created an attachment (id=78203) [details]
added more tests
------- Comment #65 From 2011-01-07 13:49:58 PST -------
(From update of attachment 78203 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=78203&action=review

> WebCore/loader/FrameLoader.cpp:2876
> +            while (i < descendentFrames.size() && (!descendentFrames[i]->tree()->isDescendantOf(m_frame)
> +                                                   || descendentFrames[i]->loader()->fireBeforeUnloadEvent(chrome)))

This condition is kind of out of control.  Maybe use some break statements?

I'd at least put fireBeforeUnloadEvent on its own line b/c it has side effects.
------- Comment #66 From 2011-01-07 14:06:15 PST -------
Created an attachment (id=78268) [details]
added one more test per abarth's request
------- Comment #67 From 2011-01-07 14:10:26 PST -------
(From update of attachment 78268 [details])
Ok.  Let's give this a try.
------- Comment #68 From 2011-01-07 14:13:43 PST -------
(In reply to comment #67)
> (From update of attachment 78268 [details] [details])
> Ok.  Let's give this a try.

Thanks for the review!  I'll land after build & running tests locally.
------- Comment #69 From 2011-01-07 18:15:34 PST -------
Committed r75305: <http://trac.webkit.org/changeset/75305>
------- Comment #70 From 2011-01-08 09:11:05 PST -------
Is it possible that this commit broken all these on Qt Release bots?

fast/workers/dedicated-worker-lifecycle.html -> failed
fast/workers/shared-worker-exception.html -> failed
fast/workers/shared-worker-lifecycle.html -> failed
fast/workers/shared-worker-name.html -> failed
fast/workers/shared-worker-script-error.html -> failed
fast/workers/stress-js-execution.html -> failed
fast/workers/termination-early.html -> failed
fast/workers/termination-with-port-messages.html -> failed
fast/workers/worker-cloneport.html -> failed
fast/workers/worker-close-more.html -> failed
fast/workers/worker-close.html -> failed
fast/workers/worker-context-multi-port.html -> failed
fast/workers/worker-gc2.html -> failed
fast/workers/worker-lifecycle.html -> failed
fast/workers/worker-messageport-gc.html -> failed
fast/workers/worker-multi-port.html -> failed
fast/workers/worker-terminate.html -> failed
fast/workers/storage ...
fast/workers/storage/change-version-sync.html -> failed
fast/workers/storage/interrupt-database.html -> failed
fast/xmlhttprequest .
fast/xmlhttprequest/null-document-xmlhttprequest-open.html -> failed
fast/xmlhttprequest/xmlhttprequest-nonexistent-file.html -> failed
------- Comment #71 From 2011-01-08 10:57:37 PST -------
(In reply to comment #70)
> Is it possible that this commit broken all these on Qt Release bots?
> 
> fast/workers/dedicated-worker-lifecycle.html -> failed
> fast/workers/shared-worker-exception.html -> failed
> fast/workers/shared-worker-lifecycle.html -> failed
> fast/workers/shared-worker-name.html -> failed
> fast/workers/shared-worker-script-error.html -> failed
> fast/workers/stress-js-execution.html -> failed
> fast/workers/termination-early.html -> failed
> fast/workers/termination-with-port-messages.html -> failed
> fast/workers/worker-cloneport.html -> failed
> fast/workers/worker-close-more.html -> failed
> fast/workers/worker-close.html -> failed
> fast/workers/worker-context-multi-port.html -> failed
> fast/workers/worker-gc2.html -> failed
> fast/workers/worker-lifecycle.html -> failed
> fast/workers/worker-messageport-gc.html -> failed
> fast/workers/worker-multi-port.html -> failed
> fast/workers/worker-terminate.html -> failed
> fast/workers/storage ...
> fast/workers/storage/change-version-sync.html -> failed
> fast/workers/storage/interrupt-database.html -> failed
> fast/xmlhttprequest .
> fast/xmlhttprequest/null-document-xmlhttprequest-open.html -> failed
> fast/xmlhttprequest/xmlhttprequest-nonexistent-file.html -> failed

I saw the failure but thought it's some flakiness because these tests aren't failing on any other platforms.  But then it has been consistently failing after my patch so I'm less convinced.  Do those tests fail if you ran one by one?
------- Comment #72 From 2011-01-08 11:28:07 PST -------
> I saw the failure but thought it's some flakiness because these tests aren't failing on any other platforms.  But then it has been consistently failing after my patch so I'm less convinced.  Do those tests fail if you ran one by one?

FAIL: Timed out waiting for notifyDone to be called

Maybe they're refusing to close for some reason?  It might have to see with how Qt calls into should close.  I would assume it's related to this patch unless there's evidence otherwise.  You might want to ask dglazkov if he's got a machine that can build / test Qt that you can borrow.
------- Comment #73 From 2011-01-08 11:30:32 PST -------
(In reply to comment #72)
> > I saw the failure but thought it's some flakiness because these tests aren't failing on any other platforms.  But then it has been consistently failing after my patch so I'm less convinced.  Do those tests fail if you ran one by one?
> 
> FAIL: Timed out waiting for notifyDone to be called
> 
> Maybe they're refusing to close for some reason?  It might have to see with how Qt calls into should close.  I would assume it's related to this patch unless there's evidence otherwise.  You might want to ask dglazkov if he's got a machine that can build / test Qt that you can borrow.

I don't have a Qt machine handy. But I am sure our friendly Qt peeps can help us debug this. Right, friendly Qt peeps? :)
------- Comment #74 From 2011-01-08 14:32:38 PST -------
The Qt fails weren't caused by this patch. It is Qt-DRT bug: https://bugs.webkit.org/show_bug.cgi?id=42578

I'll check it and fix will land soon.
------- Comment #75 From 2011-01-08 15:41:14 PST -------
(In reply to comment #74)
> The Qt fails weren't caused by this patch. It is Qt-DRT bug: https://bugs.webkit.org/show_bug.cgi?id=42578
> 
> I'll check it and fix will land soon.

Thanks for the clarification, and thanks for fixing the bug.
------- Comment #76 From 2011-03-11 07:41:35 PST -------
Created an attachment (id=85476) [details]
change framecontent with document.write
------- Comment #77 From 2011-03-11 07:45:03 PST -------
all works fine if i change the frame content by modifying the frame src.

but when i change the frame content with document.open(..., 'replace'), document.write(...), document.close the beforeunload handler is not fired.

tested in chrome 10. and chrome 10 works with all other testcases.
------- Comment #78 From 2011-03-11 09:36:10 PST -------
(In reply to comment #77)
> all works fine if i change the frame content by modifying the frame src.
> 
> but when i change the frame content with document.open(..., 'replace'), document.write(...), document.close the beforeunload handler is not fired.
> 
> tested in chrome 10. and chrome 10 works with all other testcases.

Please file a new bug as this bug has been closed.
------- Comment #79 From 2011-03-11 13:49:36 PST -------
ok. here is the new bug: https://bugs.webkit.org/show_bug.cgi?id=56221