1125285 - Ignore beforeunload on frames when triggered by blur handlers on parent document

Status: RESOLVED WORKSFORME

(Core :: DOM: Navigation, defect)

Description

[:Gijs (he/him)]

Attachment: beforeunload.html

+++ This bug was initially created as a clone of Bug #1116977 +++

I don't know how to fix this, and if it's even reasonably possible because of the amount of indirection (and then what happens if you use another setTimeout in the middle, etc. etc.)

Comment 1

[:Gijs (he/him)]

Attachment: Testcase

Comment 2

[Jesse Ruderman]

What do you mean by "when triggered by blur handlers on parent document"? Should we just disable beforeunload for frames entirely, as a followup to bug 636374?

Comment 3

[:Gijs (he/him)]

(In reply to Jesse Ruderman from comment #2)
> What do you mean by "when triggered by blur handlers on parent document"?

See the testcase? Basically, in a blur handler on the top document, trigger a load on the kid (change the src of the iframe, trigger location.reload, whatever), which triggers onbeforeunload on the frame.

> Should we just disable beforeunload for frames entirely, as a followup to
> bug 636374?

Interesting idea. I'm not the right person to comment on the web-compat side of that. Olli, thoughts?

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Jesse Ruderman from comment #2)
> > What do you mean by "when triggered by blur handlers on parent document"?
> 
> See the testcase? Basically, in a blur handler on the top document, trigger
> a load on the kid (change the src of the iframe, trigger location.reload,
> whatever), which triggers onbeforeunload on the frame.

To be clear, at least on OS X, the consequence is basically that it becomes hard(er) to switch tabs, or focus any of the browser UI - you can do it the second time you try, after the beforeunload dialog has come up, but it's still annoying, causes flickering and generally can leave the browser in a pretty confused state... If you click 'stay on page', then that just means focus gets put back in the document, and the next time you click on a tab/urlbar/whatever, the same dialog pops up again and steals focus again... it's annoying.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Gijs Kruitbosch from comment #3)
> > Should we just disable beforeunload for frames entirely, as a followup to
> > bug 636374?
> 
> Interesting idea. I'm not the right person to comment on the web-compat side
> of that. Olli, thoughts?
We certainly can't remove the event, but maybe the confirmation dialog?


Again, what do other browsers do here?

Comment 6

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #5)
> (In reply to :Gijs Kruitbosch from comment #3)
> > > Should we just disable beforeunload for frames entirely, as a followup to
> > > bug 636374?
> > 
> > Interesting idea. I'm not the right person to comment on the web-compat side
> > of that. Olli, thoughts?
> We certainly can't remove the event, but maybe the confirmation dialog?

Yeah, sorry for not being clear - that's what the hidden pref does (now), too.

> Again, what do other browsers do here?

I'll try to have a look early next week.

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Olli Pettay [:smaug] from comment #5)
> > (In reply to :Gijs Kruitbosch from comment #3)
> > > > Should we just disable beforeunload for frames entirely, as a followup to
> > > > bug 636374?
> > > 
> > > Interesting idea. I'm not the right person to comment on the web-compat side
> > > of that. Olli, thoughts?
> > We certainly can't remove the event, but maybe the confirmation dialog?
> 
> Yeah, sorry for not being clear - that's what the hidden pref does (now),
> too.
> 
> > Again, what do other browsers do here?
> 
> I'll try to have a look early next week.

The DoS in the testcase works even 'better' in Chrome in that the "do you want to reload this page" popup seems to be app-modal there; I can't switch to another tab while it's up. IE11 does the same, but at least after interacting with the popup, the focus is on the tabbar and so I can switch the second time I try.

Both of them thus seem to run beforeunload handlers for frames in the page (and prompt for them). Sounds like we can't outright remove that per the current version of the spec. Equally, I bet other vendors would be interested in fixing the DoS-style annoyances here...

Comment 8

[Jesse Ruderman]

Rather than trying to prevent specific combinations of events and frames, I think we should disable onbeforeunload dialogs:

* When you haven't interacted with a page (bug 636905)
* In frames (bug 1131187)

See also bug 1003967, which proposes disabling all dialogs (not just onbeforeunload) in the intersection of those two cases: (cross-origin) frames the user has not interacted with.

WONTFIX?

Comment 9

[:Gijs (he/him)]

(In reply to Jesse Ruderman from comment #8)
> Rather than trying to prevent specific combinations of events and frames, I
> think we should disable onbeforeunload dialogs:
> 
> * When you haven't interacted with a page (bug 636905)

Can we actually reasonably detect whether you've "interacted" with a page? (what counts? scrolling? For e.g. reading a news article, I might not "interact" with the page at all...)

Comment 10

[Jesse Ruderman]

As I suggested in bug 636905 comment 0: anything that would allow a popup would also allow onbeforeunload. So clicking and typing would allow, but scrolling and pressing modifier keys would not.

There are some edge cases, such as scrolling with arrow keys or spacebar, but I'm not really worried about evil popups doing those things just to make it hard to close the popup.

Comment 11

[:Gijs (he/him)]

(In reply to Jesse Ruderman from comment #10)
> As I suggested in bug 636905 comment 0: anything that would allow a popup
> would also allow onbeforeunload. So clicking and typing would allow, but
> scrolling and pressing modifier keys would not.

Is this already implemented/exposed on the docshell in some way?

Comment 12

[Jesse Ruderman]

Maybe smaug knows

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Well, exposed in docshell...no.
gPopupControlState is in nsGlobalWindow.cpp

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Gijs, we are triaging evil trap bugs at the moment. This one is still reproducible, but also a valid use case, right? Any opinion on the priority of this bug? Asked differently, do you still think that comment 11 is the way to move forward? Thanks for any response.

Comment 15

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> Gijs, we are triaging evil trap bugs at the moment. This one is still
> reproducible, but also a valid use case, right? Any opinion on the priority
> of this bug? Asked differently, do you still think that comment 11 is the
> way to move forward? Thanks for any response.

We have already implemented comment #11, in bug 636905. We no longer show beforeunload if you don't interact (mousedown, keypress, etc.) with the frame which requests it before closing the tab / navigating it. As a result the iframe attack stuff doesn't really work anymore, from what I can tell.

I think we should fix bug 1345830 and then close this as WFM. Does that sound like a reasonable suggestion?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #15)
> I think we should fix bug 1345830 and then close this as WFM. Does that
> sound like a reasonable suggestion?

I think this sounds like a perfectly reasonable idea. I marked this bug to be dependent on Bug 1345830. Thanks!

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Close per the suggestion from comment 16.