636905 - Don't allow onbeforeunload dialog unless I have interacted with the page

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Many popup ads are abusing onbeforeunload nowadays. We should disable them for tabs that haven't been interacted with.  Perhaps clicking or doing something else that allows popups should also allow the page to use onbeforeunload when closed.

This would be less drastic than bug 578828 but take care of most of the abuse cases.

Comment 1

[Biju]

can we also add some minimum time period of interaction, say at least 10 seconds interaction.

Comment 2

[David Vo [:auscompgeek]]

This should also extend to other cases like onload, onunload, etc.

Comment 3

[Alex Limi (:limi) — Firefox UX Team]

This is a great idea. Jesse++!

Comment 4

[Florian Bender]

I like this idea, too, and I'd like to see it with a 10 sec timer attached. 

The other Bug mentioned a refactoring of the shutdown code including handling prompts from multiple tabs at once which should be handled with this Bug, too. 

(In reply to Florian Bender from Bug 578828 comment #61)
> I'd like to see this Bug marked as duplicate of Bug 636905 […]

Comment 5

[Tom S]

From what I noticed one very strong indicator is popups. The downside is I can think of compose popups for online mail clients that we would alienate with that.

Maybe we already have a good notion of "used page" for something else?

Comment 6

[Tom S]

Oh scratch that. The compose thing is obviously a non-issue, because we would detect if you already had text entered.

So that leaves us at other cases were sites could show legitimate content, which shouldn't not closed. Maybe something like transactions details, new Wifi password on the router? I actually can't remember anything where onbeforeunload was used for something like that.

Comment 7

[Potch [:potch]]

I'd think the same set of actions that would green-light a popup would be good to green-light onbeforeunload.

Comment 8

[Jay]

At first this sounded like an awesome idea because I have encountered popups abusing onbeforeunload, however I realized that this would affect our site in a negative way. We show network banner ads on our home page, and found that some bad advertisers were redirecting users away from the site as soon as the page loads. Users complained that we were redirecting them to malware sites etc., so we use onbeforeunload to ask if they're sure they want to leave if they try to leave in the first few seconds of loading the site.

During that time they obviously would not have had a chance to interact with the site yet, so we'd presumably be blocked from using onbeforeunload, but the malvertisers would not be blocked from doing a redirect.

Perhaps there is some other indicator you could look at (is a popup?) to determine that onbeforeunload should be blocked besides direct interaction.

Comment 9

[John Drinkwater (:beta)]

(In reply to Jay from comment #8)
> We show network banner ads on our home page, and found
> that some bad advertisers were redirecting users away from the site as soon
> as the page loads. Users complained that we were redirecting them to malware
> sites etc., so we use onbeforeunload to ask if they're sure they want to
> leave if they try to leave in the first few seconds of loading the site.

You may wish to get better advertisers rather than hacking around them.

Comment 10

[Jay]

(In reply to John Drinkwater (:beta) from comment #9)
> You may wish to get better advertisers rather than hacking around them.

Of course! We have a team of people dedicated to tracking down and blocking bad ads, but the nature of networks is that occasionally, bad ads get through, especially in countries where advertising usually pays poorly. We want to employ every means possible to try to stop these.

Comment 11

[James Hartig]

(In reply to Jay from comment #8)
> At first this sounded like an awesome idea because I have encountered popups
> abusing onbeforeunload, however I realized that this would affect our site
> in a negative way. We show network banner ads on our home page, and found
> that some bad advertisers were redirecting users away from the site as soon
> as the page loads. Users complained that we were redirecting them to malware
> sites etc., so we use onbeforeunload to ask if they're sure they want to
> leave if they try to leave in the first few seconds of loading the site.
> 
> During that time they obviously would not have had a chance to interact with
> the site yet, so we'd presumably be blocked from using onbeforeunload, but
> the malvertisers would not be blocked from doing a redirect.
> 
> Perhaps there is some other indicator you could look at (is a popup?) to
> determine that onbeforeunload should be blocked besides direct interaction.

It sounds like this bug should be blocked by Bug 785310 ;) If iFrame's can't perform top navigation then we don't need the onbeforeunload to prompt users when they try to navigate away.

Comment 12

[Florian Bender]

A patch for this bug should check for and remember site interaction per origin and/or frame, so if you have a good site with an evil iFrame (loading from a different origin), and you are interacting with the page but not the iFrame, the iFrame should not be able to open a dialog in an onbeforeunload / onload / onunload / etc. handler. In fact, it may be a good idea to block all dialogs for foreign-origin iFrames when you did not interact with it … (ni? me to file a bug)


(In reply to James Hartig from comment #11)
> It sounds like this bug should be blocked by Bug 785310 ;) If iFrame's can't
> perform top navigation then we don't need the onbeforeunload to prompt users
> when they try to navigate away.

Bug 785310 should solve Jay's issue, but has nothing to do with this bug.

Comment 13

[Florian Bender]

(In reply to Florian Bender from comment #12)
> A patch for this bug should check for and remember site interaction per
> origin and/or frame, so if you have a good site with an evil iFrame (loading
> from a different origin), and you are interacting with the page but not the
> iFrame, the iFrame should not be able to open a dialog in an onbeforeunload
> / onload / onunload / etc. handler. In fact, it may be a good idea to block
> all dialogs for foreign-origin iFrames when you did not interact with it …
> (ni? me to file a bug)

Took me quite some time, but here it is: Bug 1003967.

Comment 14

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 636905 - part 1: add a flag that tracks whether the user has interacted with a given document, r?smaug

Bug 636905 - part 1: add a flag that tracks whether the user has interacted with a given document, r?smaug

Comment 15

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 636905 - part 2: check for document interactivity state when prompting for beforeunload, r?bz

Bug 636905 - part 2: check for document interactivity state when prompting for beforeunload, r?bz

Comment 16

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 636905 - part 3: flip prefs in tests to keep them working, r?dolske

Bug 636905 - part 3: flip prefs in tests to keep them working, r?dolske

Comment 17

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 636905 - part 4: add tests for interactivity requirement for onbeforeunload, r?bz

Bug 636905 - part 4: add test for interactivity requirement for onbeforeunload, r?bz

Comment 18

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=a22f2d88ecbb


Earlier try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e43b3cc9f70 - I've fixed a nullcheck (see comment in part 2 above the if block) and added some more pref sets to more tests (2x tabview-related, and browser_double_close_tabs.js).

Not sure if the earlier rc and m9 android failures are related or not, so they should be included in this new push.


Some design considerations when I wrote this:

0) I stuck the bool on the document, rather than (say) on the window or docshell or contentviewer, because this seemed (a) simplest and (b) there's no risk of it remaining set from a previous page or whatever (for the window case - I don't actually remember offhand if contentviewers get destroyed + recreated or not)

1) I think saving things per-domain is too complex and not necessary - I can't think of a good reason why you'd want to have an onbeforeunload handler on a document the user hasn't interacted with, even if they interacted with it on a previous visit;

2) I used the list of events we default to for allowing popups, but I did not create a pref with those events, only a boolean pref (mostly so we can turn this off easily and quickly if we find it breaks $highprofilesite 2 weeks before shipping, or something). I think that pref hasn't been an amazing success considering the bugs on file about our popup blocker being imperfect - people use the pref as a means of trying to make it better, then it starts breaking e.g. the browse... button on file inputs because the click is no longer allowed to open a new window, etc. etc. For similar reasons, I didn't want to reuse that pref, either. In general, I think a complex pref would be overhead and I hope that it won't be necessary for people to alter the list in some way - if there are shortcomings in the implementation (like for the popup blocker) then I'd prefer to deal with them by improving that implementation for everybody.

3) I don't actually know if the eventdispatcher is the best place to catch the events that will hit the document - Olli, if not, can you suggest where else this should go when you review?

Comment 20

[:Gijs (he/him)]

Something I just realized: technically, with this patch, in the following situation:

1) load document that has onbeforeunload handler, that has iframe without onbeforeunload handler
2) interact only with iframe
3) close/navigate toplevel document

we will no longer show a dialog.

I don't know if that's something that happens frequently enough in the wild that we should care about it, but I figured I should flag it up, at least.

Comment 21

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

I think we should care about that, especially if top level document and iframe are same origin.
But probably even without same origin.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I think we should care about that too, fwiw.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8665373 [details]
MozReview Request: Bug 636905 - part 2: check for document interactivity state when prompting for beforeunload, r?bz

https://reviewboard.mozilla.org/r/20217/#review18269

This seems fine as long as we mark the "user interacted" state all the way up the document tree as needed.

Comment 24

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

Comment on attachment 8665372 [details]
MozReview Request: Bug 636905 - part 1: add a flag that tracks whether the user has interacted with a given document, r?smaug

Please put the user interaction detection to EventStateManager::PreHandleEvent.
That is where we deal with UpdateUserActivityTimer anyway.
And I don't know why submit or reset are user interaction. 
Surely user needs to use touch/mouse/keyboard to interact with the page
(well, or speech).
It would be great if we used the same set of events for UpdateUserActivityTimer and for SetUserHasInteracted(true); 
What EventStateManager does now can be changed (like, it is missing touch and pointer events atm).

Comment 25

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

Hmm, perhaps the events can't be really the same set, since here we want clicks and keypresses and such, just moving mouse isn't enough.
But perhaps put the code inside the same if ()
and then inside that explicitly filter out the cases we don't want to deal here.
so
if (<user-interacted>) {
  if (gMouseOrKeyboardEventCounter == 0) {
    ...
  }
  if (mDocument && aEvent->mMessage != eMouseMove && aEvent->mMessage != eTouchMove && ...) {
    mDocument->SetUserHasInteracted(true);
  }

}

Comment 26

[:Gijs (he/him)]

So the changes implied by comments 25 & 24 were not too hard to write after Olli's help on IRC.

However, if you look at the try push, you'll see the test I wrote is failing on infra, when it worked in my local testing.

(I need to expand it anyway to test the frame case at least a bit, but never mind that for now)

Running it by dir fails for me locally, too.

One thing that I think I've figured out is that pref caches are prefix-based (personally, I think that should be fixed or the API should come with a warning...).

Because the pref name I picked for the new pref is essentially the same as the disable_beforeunload pref plus a suffix, changing that pref triggers the pref observer to run there, which breaks stuff (rather, assigns the wrong value into the disable_beforeunload pref cache).

It also seems like the code gets stuck thinking dialogs are disabled, potentially as a result of the earlier test running... which seems to be an addition from bug 1188665. :-\

Going to keep poking.

Comment 27

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #26)
> It also seems like the code gets stuck thinking dialogs are disabled,
> potentially as a result of the earlier test running... which seems to be an
> addition from bug 1188665. :-\

Having renamed the pref, the tests pass, so I now suspect this was XCode and my opt build lying to me...

Comment 28

[:Gijs (he/him)]

Comment on attachment 8665372 [details]
MozReview Request: Bug 636905 - part 1: add a flag that tracks whether the user has interacted with a given document, r?smaug

Bug 636905 - part 1: add a flag that tracks whether the user has interacted with a given document, r?smaug

Comment 29

[:Gijs (he/him)]

Comment on attachment 8665373 [details]
MozReview Request: Bug 636905 - part 2: check for document interactivity state when prompting for beforeunload, r?bz

Bug 636905 - part 2: check for document interactivity state when prompting for beforeunload, r?bz

Comment 30

[:Gijs (he/him)]

Comment on attachment 8665374 [details]
MozReview Request: Bug 636905 - part 3: flip prefs in tests to keep them working, r?dolske

Bug 636905 - part 3: flip prefs in tests to keep them working, r?dolske

Comment 31

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 636905 - tangent: allow using JS to select clickable element on the content side, r?mconley

Bug 636905 - tangent: allow using JS to select clickable element on the content side, r?mconley

Comment 32

[:Gijs (he/him)]

Comment on attachment 8665375 [details]
MozReview Request: Bug 636905 - part 4: add tests for interactivity requirement for onbeforeunload, r?bz

Bug 636905 - part 4: add tests for interactivity requirement for onbeforeunload, r?bz

Comment 33

[:Gijs (he/him)]

New trypush:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=836d3db87c27

There is now a test for the frame case in part 4 as well. I also ended up needing to modify browsertestutils a little so I could click on something in an iframe in content... r?mconley for that bit.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8666129 [details]
MozReview Request: Bug 636905 - tangent: allow using JS to select clickable element on the content side, r?mconley

https://reviewboard.mozilla.org/r/20445/#review18357

Thanks Gijs!

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:375
(Diff revision 1)
>     * @param {string} target
>     *        A selector that identifies the element to target. The syntax is as
>     *        for querySelector. This may also be a CPOW element for easier
>     *        test-conversion. If this is null, then the offset is from the
>     *        content document's edge.

This is fine, but we should update the documentation here to illustrate what this thing can do now.

Comment 35

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

Comment on attachment 8665372 [details]
MozReview Request: Bug 636905 - part 1: add a flag that tracks whether the user has interacted with a given document, r?smaug

+    nsCOMPtr<nsINode> node = do_QueryInterface(aTargetContent);
aTargetContent _is_an_nsINode_, so useless variable and QI. Just use aTargetContent

+    if (node &&
+        (aEvent->mMessage == eKeyUp || aEvent->mMessage == eMouseUp ||
+         aEvent->mMessage == eMouseClick || aEvent->mMessage == eMouseDoubleClick ||
+         aEvent->mMessage == eTouchEnd || aEvent->mMessage == ePointerUp)) {
eMouseClick should never be handled here, since this is lower level code.
EventStateManager dispatches click when mousedown+mouseup happen on the same element.
So, drop eMouseClick and eMouseDoubleClick.
And I think eWheel should be added here.

+      while (doc) {
+        doc->SetUserHasInteracted(true);
+        doc = nsContentUtils::IsChildOfSameType(doc) ?
+          doc->GetParentDocument() : nullptr;
+      }
Please change while (doc) to while (doc && !doc->UserHasInteracted())

"Extension to give Chrome JS..." I'd use lowercase chrome js and drop 'and C++'. 
.webidl isn't really about C++.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8665375 [details]
MozReview Request: Bug 636905 - part 4: add tests for interactivity requirement for onbeforeunload, r?bz

https://reviewboard.mozilla.org/r/20221/#review18271

Comment 38

[:Gijs (he/him)]

Filed bug 1208744 for the prefcache issue.

New trypush with yet another test to flip this pref off to avoid issues:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=91e75d03b472

Comment 39

[:Gijs (he/him)]

Comment on attachment 8665372 [details]
MozReview Request: Bug 636905 - part 1: add a flag that tracks whether the user has interacted with a given document, r?smaug

Bug 636905 - part 1: add a flag that tracks whether the user has interacted with a given document, r?smaug

Comment 40

[:Gijs (he/him)]

Comment on attachment 8665373 [details]
MozReview Request: Bug 636905 - part 2: check for document interactivity state when prompting for beforeunload, r?bz

Bug 636905 - part 2: check for document interactivity state when prompting for beforeunload, r?bz

Comment 41

[:Gijs (he/him)]

Comment on attachment 8665374 [details]
MozReview Request: Bug 636905 - part 3: flip prefs in tests to keep them working, r?dolske

Bug 636905 - part 3: flip prefs in tests to keep them working, r?dolske

Comment 42

[:Gijs (he/him)]

Comment on attachment 8666129 [details]
MozReview Request: Bug 636905 - tangent: allow using JS to select clickable element on the content side, r?mconley

Bug 636905 - tangent: allow using JS to select clickable element on the content side, r?mconley

Comment 43

[:Gijs (he/him)]

Comment on attachment 8665375 [details]
MozReview Request: Bug 636905 - part 4: add tests for interactivity requirement for onbeforeunload, r?bz

Bug 636905 - part 4: add tests for interactivity requirement for onbeforeunload, r?bz

Comment 44

[:Gijs (he/him)]

Comment on attachment 8665372 [details]
MozReview Request: Bug 636905 - part 1: add a flag that tracks whether the user has interacted with a given document, r?smaug

Dunno why reviewboard decided to re-set that to r?, but carrying over r+...

Comment 46

[:Gijs (he/him)]

Comment on attachment 8665374 [details]
MozReview Request: Bug 636905 - part 3: flip prefs in tests to keep them working, r?dolske

Branching out for the reviewers in order to get this landed.

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8665374 [details]
MozReview Request: Bug 636905 - part 3: flip prefs in tests to keep them working, r?dolske

https://reviewboard.mozilla.org/r/20219/#review18495

rs=me for part 3

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f686be0d161e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b34c3a94663d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef432f51a591
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f468caa96a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4df0d1a777ea

Comment 49

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8665374 [details]
MozReview Request: Bug 636905 - part 3: flip prefs in tests to keep them working, r?dolske

https://reviewboard.mozilla.org/r/20219/#review18499

r=me as well.

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f686be0d161e
https://hg.mozilla.org/mozilla-central/rev/b34c3a94663d
https://hg.mozilla.org/mozilla-central/rev/ef432f51a591
https://hg.mozilla.org/mozilla-central/rev/c6f468caa96a
https://hg.mozilla.org/mozilla-central/rev/4df0d1a777ea

Comment 51

[Kohei Yoshino]

Posted the site compatibility document. Reviews are welcome.

https://www.fxsitecompat.com/en-US/docs/2015/beforeunload-event-will-no-longer-be-fired-unless-user-has-interacted-with-the-page/

Comment 52

[:Gijs (he/him)]

(In reply to Kohei Yoshino [:kohei] from comment #51)
> Posted the site compatibility document. Reviews are welcome.
> 
> https://www.fxsitecompat.com/en-US/docs/2015/beforeunload-event-will-no-
> longer-be-fired-unless-user-has-interacted-with-the-page/

Whoa. This is very misleading. The event will still be fired - we will just not show the user a popup warning even if the site requests one.

Comment 53

[Kohei Yoshino]

But the earlier comments talk about popup ads. Then the implementation is different from the original purpose?

Comment 54

[:Gijs (he/him)]

(In reply to Kohei Yoshino [:kohei] from comment #53)
> But the earlier comments talk about popup ads. Then the implementation is
> different from the original purpose?

No. The problem is that pages (including popup ads *themselves*) try to use beforeunload (which pops up a "Are you sure you want to leave this page" dialog) in order to make it harder to close pages the user actually is not interested in ("hostage" taking / eviltraps (see blocking bug)). That is the problem we're attacking here - if the user hasn't interacted with a page, that page should not get an opportunity to force the user to do so.

So what's happening is, if the user hasn't interacted with the page, and the page is unloaded, the beforeunload event will fire but we will ignore the return value and not show a dialog (and simply allow the unload to happen).

Does that make more sense?

Comment 55

[Kohei Yoshino]

Ah, now I got it. Will update the doc shortly. Thanks for your explanation!

Comment 56

[Kohei Yoshino]

Fixed the site compat doc: https://github.com/fxsitecompat/www.fxsitecompat.com/commit/1a45a54

Comment 57

[Kohei Yoshino]

Sorry for spamming, this is the right commit: https://github.com/fxsitecompat/www.fxsitecompat.com/commit/3984fa1

Comment 58

[Jean-Yves Perrier [:teoli]]

Added a note in https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload
and in https://developer.mozilla.org/en-US/Firefox/Releases/44#DOM_HTML_DOM

Comment 59

[:Gijs (he/him)]

Blink is now planning to do the same thing: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/beforeunload%7Csort:relevance/blink-dev/MRWIaGY4Txg/XlWqXel9CwAJ .

Comment 60

[Xeos]

I believe the heuristic needs to include an active Media Capture session. When the user entrusts the site with "permanent" access to the microphone and/or camera, the site no longer requires an interaction before it can launch a Media Capture session (e.g., WebRTC call).

Under the current definition of activity a click or a keypress is required, which consequently means that if the person is participating in a WebRTC call, they can accidentally leave an active call by pressing F5 key, or closing the wrong tab, for example. Furthermore, the presence of onbeforeunload prompt will be inconsistent between sessions (the first time, user might see it because they have muted their audio prior to accidentally closing the tab, then the second time they would not see the prompt, as they haven't interacted with the page, leading them to believe that the site is at fault for not showing the prompt).

If the check for an active Media Capture session doesn't seem sufficient, session duration check may also be added. For example, a minimum session duration of 10 seconds may be used. This would allow users to quickly close the tab if they see content that they don't expect to see, while still preventing accidental reloads that may lead to loss of data. An example of such data loss is a file transfer via RTC Data Channel, when a reload would reset the download progress, potentially leading to irrecoverable loss of data, if for example the transfer was initiated by sender pasting a screenshot from the clipboard.

This proposal is cross-posted on WebKit and Chromium bug trackers.

Comment 61

[:Gijs (he/him)]

(In reply to Xeos from comment #60)

I believe the heuristic needs to include an active Media Capture session. When the user entrusts the site with "permanent" access to the microphone and/or camera, the site no longer requires an interaction before it can launch a Media Capture session (e.g., WebRTC call).

Under the current definition of activity a click or a keypress is required, which consequently means that if the person is participating in a WebRTC call, they can accidentally leave an active call by pressing F5 key, or closing the wrong tab, for example. Furthermore, the presence of onbeforeunload prompt will be inconsistent between sessions (the first time, user might see it because they have muted their audio prior to accidentally closing the tab, then the second time they would not see the prompt, as they haven't interacted with the page, leading them to believe that the site is at fault for not showing the prompt).

If the check for an active Media Capture session doesn't seem sufficient, session duration check may also be added. For example, a minimum session duration of 10 seconds may be used. This would allow users to quickly close the tab if they see content that they don't expect to see, while still preventing accidental reloads that may lead to loss of data. An example of such data loss is a file transfer via RTC Data Channel, when a reload would reset the download progress, potentially leading to irrecoverable loss of data, if for example the transfer was initiated by sender pasting a screenshot from the clipboard.

This proposal is cross-posted on WebKit and Chromium bug trackers.

This is best handled as a new bug instead of posting here. Can you file a new bug and also link to the tickets you filed with chromium and webkit?