Bug 19418 – onbeforeunload is broken for framesets

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[William J. Edney]

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

[William J. Edney]

Created attachment 21535 [details]
The frameset file

[William J. Edney]

Created attachment 21536 [details]
The frame content file

[Ted Rust]

This issue has stricken us as well.  Can someone mark this confirmed?

[Alexey Proskuryakov]

Confirmed with r38387.

See also: bug 15652.

[Alexey Proskuryakov]

...and bug 21699.

[Alexey Proskuryakov]

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.

[William J. Edney]

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

[Alexey Proskuryakov]

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.

[Ted Rust]

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.

[pablo]

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?

[Oliver Hunt]

*** Bug 21699 has been marked as a duplicate of this bug. ***

[Kyle Fox]

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.

[Ted Rust]

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.

[Sam Weinig]

<rdar://problem/7177160>

[Eric]

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.

[William J. Edney]

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

[William J. Edney]

Created attachment 43664 [details]
The frameset file v2

[William J. Edney]

Created attachment 43665 [details]
The frame content file v2

[Mike Slegeir]

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.

Bump. Please fix.

Please fix.

[pablo]

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

[pablo]

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.

[Ryosuke Niwa]

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

[Ojan Vafai]

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.

[Ryosuke Niwa]

Created attachment 75730 [details]
initial patch

[Ojan Vafai]

Comment on attachment 75730 [details]
initial patch

This patch looks good to me. Alexey, you OK with this?

[Ryosuke Niwa]

(In reply to comment #26)
> Created an attachment (id=75730) [details]
> initial patch

I know there are some controversy around this feature so this is more like a starting point.

[Darin Adler]

Comment on attachment 75730 [details]
initial patch

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?

[William J. Edney]

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

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 76910 [details]
Added more tests per darin's comment and to address ap's concern

[Ryosuke Niwa]

Comment on attachment 76910 [details]
Added more tests per darin's comment and to address ap's concern

I think I can eliminate one file from this patch.

[Ryosuke Niwa]

Created attachment 76911 [details]
Removed before-unload-in-subframe-fail.html

[Ryosuke Niwa]

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

[Eric Seidel (no email)]

Comment on attachment 76911 [details]
Removed before-unload-in-subframe-fail.html

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
> [phone]  Ryosuke Niwa  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).

Double entry in your ChangeLog.

[Alexey Proskuryakov]

I think Ryosuke was going to do some more testing based on an IRC discussion.

[Ryosuke Niwa]

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

[Jonathan Hurshman]

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.

[Eric]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 77903 [details]
test case (zip)

[Ryosuke Niwa]

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.

[William J. Edney]

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

[Ryosuke Niwa]

Created attachment 77955 [details]
fixes the bug

[Ryosuke Niwa]

(In reply to comment #46)
> Created an attachment (id=77955) [details]
> fixes the bug

Unfortunately, I could not figure out how to test location.reload and history.back/history.forward.

[Adam Barth]

Comment on attachment 77955 [details]
fixes the bug

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.

[Ryosuke Niwa]

Comment on attachment 77955 [details]
fixes the bug

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.

[Adam Barth]

Comment on attachment 77955 [details]
fixes the bug

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.

[Ryosuke Niwa]

Comment on attachment 77955 [details]
fixes the bug

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?

[Adam Barth]

Comment on attachment 77955 [details]
fixes the bug

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?

[Adam Barth]

Comment on attachment 77955 [details]
fixes the bug

Marking this R- so you can address the comments from the discussion above.

[Ryosuke Niwa]

Comment on attachment 77955 [details]
fixes the bug

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.

[Adam Barth]

Comment on attachment 77955 [details]
fixes the bug

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.

[Ryosuke Niwa]

Created attachment 78089 [details]
Updated per Adam's comment

[Ryosuke Niwa]

Comment on attachment 78089 [details]
Updated per Adam's comment

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.

[Adam Barth]

Comment on attachment 78089 [details]
Updated per Adam's comment

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.

[Adam Barth]

Comment on attachment 78089 [details]
Updated per Adam's comment

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?

[Adam Barth]

Comment on attachment 78089 [details]
Updated per Adam's comment

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Classname shouldn't really be a *verb* but I couldn't come up with a better one.

[Ryosuke Niwa]

Comment on attachment 78089 [details]
Updated per Adam's comment

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.

[Ryosuke Niwa]

Created attachment 78203 [details]
added more tests

[Adam Barth]

Comment on attachment 78203 [details]
added more tests

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.

[Ryosuke Niwa]

Created attachment 78268 [details]
added one more test per abarth's request

[Adam Barth]

Comment on attachment 78268 [details]
added one more test per abarth's request

Ok.  Let's give this a try.

[Ryosuke Niwa]

(In reply to comment #67)
> (From update of attachment 78268 [details])
> Ok.  Let's give this a try.

Thanks for the review!  I'll land after build & running tests locally.

[Ryosuke Niwa]

Committed r75305: <http://trac.webkit.org/changeset/75305>

[Antonio Gomes]

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

[Ryosuke Niwa]

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

[Adam Barth]

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

[Dimitri Glazkov (Google)]

(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? :)

[Csaba Osztrogonác]

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.

[Ryosuke Niwa]

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

[Holger Hees]

Created attachment 85476 [details]
change framecontent with document.write

[Holger Hees]

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.

[Ryosuke Niwa]

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

[Holger Hees]

ok. here is the new bug: https://bugs.webkit.org/show_bug.cgi?id=56221