Jesse Ruderman Reporter
	Description • 13 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.

	Biju
	Comment 1 • 13 years ago

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

	David Vo [:auscompgeek]
	Comment 2 • 13 years ago

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

	Alex Limi (:limi) — Firefox UX Team
	Comment 3 • 11 years ago

This is a great idea. Jesse++!

	Florian Bender
	Comment 4 • 11 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 […]

	Tom S [:evilpie]
	Comment 5 • 11 years ago

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?

	Tom S [:evilpie]
	Comment 6 • 11 years ago

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.

	Potch [:potch]
	Comment 7 • 11 years ago

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

	Jay
	Comment 8 • 11 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.

	John Drinkwater (:beta)
	Comment 9 • 11 years ago

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

	Jay
	Comment 10 • 11 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.

	James Hartig
	Comment 11 • 11 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.

	Florian Bender
	Comment 12 • 11 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.

	Florian Bender
	Comment 13 • 10 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.

	:Gijs (he/him) Assignee
	Comment 14 • 9 years ago

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

	:Gijs (he/him) Assignee
	Comment 15 • 9 years ago

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

	:Gijs (he/him) Assignee
	Comment 16 • 9 years ago

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

	:Gijs (he/him) Assignee
	Comment 17 • 9 years ago

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

	:Gijs (he/him) Assignee
	Comment 18 • 9 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?

	:Gijs (he/him) Assignee
	Comment 20 • 9 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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 21 • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 9 years ago

I think we should care about that too, fwiw.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 9 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 24 • 9 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

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 25 • 9 years ago

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);
  }

}

	:Gijs (he/him) Assignee
	Comment 26 • 9 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.

	:Gijs (he/him) Assignee
	Comment 27 • 9 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...

	:Gijs (he/him) Assignee
	Comment 28 • 9 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

	:Gijs (he/him) Assignee
	Comment 29 • 9 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

	:Gijs (he/him) Assignee
	Comment 30 • 9 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

	:Gijs (he/him) Assignee
	Comment 31 • 9 years ago

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

	:Gijs (he/him) Assignee
	Comment 32 • 9 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

	:Gijs (he/him) Assignee
	Comment 33 • 9 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.

	Mike Conley (:mconley) (:⚙️)
	Comment 34 • 9 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

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 35 • 9 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

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

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 9 years ago

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

	:Gijs (he/him) Assignee
	Comment 38 • 9 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

	:Gijs (he/him) Assignee
	Comment 39 • 9 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

	:Gijs (he/him) Assignee
	Comment 40 • 9 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

	:Gijs (he/him) Assignee
	Comment 41 • 9 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

	:Gijs (he/him) Assignee
	Comment 42 • 9 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

	:Gijs (he/him) Assignee
	Comment 43 • 9 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

	:Gijs (he/him) Assignee
	Comment 44 • 9 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+...

	:Gijs (he/him) Assignee
	Comment 46 • 9 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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 47 • 9 years ago

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

	Pulsebot
	Comment 48 • 9 years ago

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

	Mike Conley (:mconley) (:⚙️)
	Comment 49 • 9 years ago

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.

	Carsten Book [:Tomcat]
	Comment 50 • 9 years ago

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

	Kohei Yoshino [:kohei]
	Comment 51 • 9 years ago

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/

	:Gijs (he/him) Assignee
	Comment 52 • 9 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.

	Kohei Yoshino [:kohei]
	Comment 53 • 9 years ago

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

	:Gijs (he/him) Assignee
	Comment 54 • 9 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?

	Kohei Yoshino [:kohei]
	Comment 55 • 9 years ago

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

	Kohei Yoshino [:kohei]
	Comment 56 • 9 years ago

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

	Kohei Yoshino [:kohei]
	Comment 57 • 9 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 58 • 9 years ago

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

	:Gijs (he/him) Assignee
	Comment 59 • 7 years ago

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 .