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

RESOLVED FIXED in Firefox 44

Status

()

defect
--
critical
RESOLVED FIXED
8 years ago
a month ago

People

(Reporter: jruderman, Assigned: Gijs)

Tracking

(Depends on 2 bugs, Blocks 2 bugs, 5 keywords)

Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(5 attachments)

(Reporter)

Description

8 years ago
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

8 years ago
can we also add some minimum time period of interaction, say at least 10 seconds interaction.
This should also extend to other cases like onload, onunload, etc.
This is a great idea. Jesse++!

Comment 4

6 years ago
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 […]
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?
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

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

Comment 8

6 years ago
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.
(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

6 years ago
(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

6 years ago
(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

6 years ago
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.
Flags: needinfo?(fb+mozdev)

Updated

5 years ago
See Also: → 1003967

Comment 13

5 years ago
(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.
Flags: needinfo?(fb+mozdev)
(Assignee)

Comment 14

4 years ago
Bug 636905 - part 1: add a flag that tracks whether the user has interacted with a given document, r?smaug
Attachment #8665372 - Flags: review?(bugs)
(Assignee)

Comment 15

4 years ago
Bug 636905 - part 2: check for document interactivity state when prompting for beforeunload, r?bz
Attachment #8665373 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

4 years ago
Bug 636905 - part 3: flip prefs in tests to keep them working, r?dolske
Attachment #8665374 - Flags: review?(dolske)
(Assignee)

Comment 17

4 years ago
Bug 636905 - part 4: add test for interactivity requirement for onbeforeunload, r?bz
Attachment #8665375 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

4 years ago
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?
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1205678
(Assignee)

Updated

4 years ago
Blocks: 1003967
(Assignee)

Comment 20

4 years ago
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.
I think we should care about that, especially if top level document and iframe are same origin.
But probably even without same origin.
I think we should care about that too, fwiw.
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.
Attachment #8665373 - Flags: review?(bzbarsky) → review+
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).
Attachment #8665372 - Flags: review?(bugs) → review-
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);
  }

}
(Assignee)

Comment 26

4 years ago
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.
(Assignee)

Comment 27

4 years ago
(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...
(Assignee)

Comment 28

4 years ago
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
Attachment #8665372 - Flags: review- → review?(bugs)
(Assignee)

Comment 29

4 years ago
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
(Assignee)

Comment 30

4 years ago
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
(Assignee)

Comment 31

4 years ago
Bug 636905 - tangent: allow using JS to select clickable element on the content side, r?mconley
Attachment #8666129 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Attachment #8665375 - Attachment description: MozReview Request: Bug 636905 - part 4: add test for interactivity requirement for onbeforeunload, r?bz → MozReview Request: Bug 636905 - part 4: add tests for interactivity requirement for onbeforeunload, r?bz
(Assignee)

Comment 32

4 years ago
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
(Assignee)

Comment 33

4 years ago
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.
Attachment #8666129 - Flags: review?(mconley) → review+
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 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++.
Attachment #8665372 - Flags: review?(bugs) → review+
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
Attachment #8665375 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 38

4 years ago
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
(Assignee)

Updated

4 years ago
Attachment #8665372 - Flags: review+ → review?(bugs)
(Assignee)

Comment 39

4 years ago
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
(Assignee)

Comment 40

4 years ago
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
(Assignee)

Comment 41

4 years ago
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
(Assignee)

Comment 42

4 years ago
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
(Assignee)

Comment 43

4 years ago
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
(Assignee)

Comment 44

4 years ago
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+...
Attachment #8665372 - Flags: review?(bugs) → review+
(Assignee)

Comment 46

4 years ago
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.
Attachment #8665374 - Flags: review?(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/#review18495

rs=me for part 3
Attachment #8665374 - Flags: review+
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.
Attachment #8665374 - Flags: review?(mconley) → review+
(Assignee)

Updated

4 years ago
Attachment #8665374 - Flags: review?(dolske)
(Assignee)

Comment 52

4 years ago
(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.
But the earlier comments talk about popup ads. Then the implementation is different from the original purpose?
(Assignee)

Comment 54

4 years ago
(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?
Flags: needinfo?(kohei.yoshino)
Ah, now I got it. Will update the doc shortly. Thanks for your explanation!
Flags: needinfo?(kohei.yoshino)
(Assignee)

Updated

2 years ago
Depends on: 1345830
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.