1257391 - Tab browser should manually ensure permitUnload succeeds (fire beforeunload [and unload?] event) when switching remoteness

Status: RESOLVED INCOMPLETE

(Firefox :: Tabbed Browser, defect, P4)

Description

[radu_91]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160316004053

Steps to reproduce:

in chrome:// 
window.addEventListener("beforeunload", function beforeunload(event){
event.preventDefault();
});




Actual results:

Does not work in e10s, but works in non-e10s window

Comment 1

[radu_91]

I want newtab(newtabtools) to fade out on page exit with:

window.addEventListener("beforeunload", function beforeunload(event){
event.preventDefault();
var element = document.getElementsByTagName("html");
element.length; // show number of items
alert(element.length + element + '!');
element[0].classList.add("fadeout");
});

and , does not work in e10s, but works in non-e10s window.

Comment 2

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

So which window you're adding the event listener? And which beforeunload you're trying to prevent.
This sounds like the generic "events don't propagate from child process to parent", which is a design decision.

Comment 3

[radu_91]

(In reply to Olli Pettay [:smaug] from comment #2)
> So which window you're adding the event listener? And which beforeunload
> you're trying to prevent.
> This sounds like the generic "events don't propagate from child process to
> parent", which is a design decision.

What I want is to add fade out beforeunload on "New Tab"(chrome://) and this does not work in e10s, but works in non-e10s window.
I use preventDefault to observe more clearly what is happening.

https://www.youtube.com/watch?v=QOMkNFaZumw&feature=youtu.be

The primary thing in the video: on e10s New Tab, following links is not stopped by preventDefault(fading out does not work).In non-e10s New Tab, following links is stopped by preventDefault( fading out does work)



And on a side note, before I recorded the video I found that the pages must be active/selected so beforeunload can work, I assume maybe this is a security thing.

Comment 4

[Matthew N. [:MattN]]

(In reply to radu_91 from comment #3)
> (In reply to Olli Pettay [:smaug] from comment #2)
> > So which window you're adding the event listener? And which beforeunload
> > you're trying to prevent.
> > This sounds like the generic "events don't propagate from child process to
> > parent", which is a design decision.

Note that you didn't answer the question. Which `window` are you adding the event listener to? The ChromeWindow, the content window of newTab.xul? 

If you can attach a testcase XPI or some code to run in the Browser Console or Web Console I think it will be easier to understand the problem.

Comment 5

[radu_91]

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #4)
> (In reply to radu_91 from comment #3)
> > (In reply to Olli Pettay [:smaug] from comment #2)
> > > So which window you're adding the event listener? And which beforeunload
> > > you're trying to prevent.
> > > This sounds like the generic "events don't propagate from child process to
> > > parent", which is a design decision.
> 
> Note that you didn't answer the question. Which `window` are you adding the
> event listener to? The ChromeWindow, the content window of newTab.xul? 
> 
> If you can attach a testcase XPI or some code to run in the Browser Console
> or Web Console I think it will be easier to understand the problem.

I think on NewTab.xul, I tested with Web Console and I get the same thing:

in e10s open a New Tab, Open the Web Console and run this:


window.addEventListener("beforeunload", function beforeunload(event){
event.preventDefault()
});



Clicking on sites and accessing sites from the address bar, will NOT give me: "This page is asking you to confirm..."


In non-e10s New Tab, clicking on sites and accessing sites from the address bar WILL give me: "This page is asking you to confirm..."

Comment 6

[Andrew Overholt [:overholt]]

Is comment 5 enough info?

Comment 7

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

That sounds like an issue outside event handling. We seem to use non-e10s tab for the new tab and then switch it to e10s window and apparently don't care about the beforeunload handling there.

Comment 8

[:Gijs (he/him)]

We don't fire beforeunload at all in the case where we switch remoteness, AFAICT.

Let's see if we do it in the other direction...

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> Let's see if we do it in the other direction...

Nope.

Re-nomming in case we want to up-prio this for e10s.

Comment 10

[Matthew N. [:MattN]]

Why wouldn't we want to track a regression? My understanding is that tracking doesn't imply blocking an initial release.

Comment 11

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #10)
> Why wouldn't we want to track a regression? My understanding is that
> tracking doesn't imply blocking an initial release.

I'm not saying we shouldn't want to track it. e10s+ doesn't mean very much apart from it going into a big bucket. Only some of the other values for the field actually mean blocking releases. I'm renomming to get it *more* attention, not less.

Comment 12

[Jim Mathies [:jimm]]

Andy, this is a request for a feature request needed by addons. Can your team take a look at this and see if there's something in place they can rely on, or if not, whether or not this needs a browser fix.

Comment 13

[Jorge Villalobos [:jorgev] (he/him)]

This would affect "new tab" type add-ons that use onbeforeunload. It sounds like it's a very small group, and the loss in functionality would be minor. It's still a bug, but I wouldn't consider it a priority.

Comment 14

[Kris Maglione [:kmag]]

I'm not sure this is something that needs to be fixed. Non-remote tabs are either built-in, in which case we know they don't need beforeunload, or are provided by extensions, in which case they have other ways to override navigation if they need to. WebExtensions can't currently override about:newtab, but if we ever decide to allow it, the tab will have to be remote, and this won't apply.

In any case, adding a beforeunload handler to the new tab page sounds like a bug to me. If a new tab page wants to do anything that might require users to confirm that they want to leave, it should navigate to a new URL first.

I think this should be WONTFIX.

Comment 15

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #14)
> I'm not sure this is something that needs to be fixed. Non-remote tabs are
> either built-in, in which case we know they don't need beforeunload, or are
> provided by extensions, in which case they have other ways to override
> navigation if they need to. WebExtensions can't currently override
> about:newtab, but if we ever decide to allow it, the tab will have to be
> remote, and this won't apply.
> 
> In any case, adding a beforeunload handler to the new tab page sounds like a
> bug to me. If a new tab page wants to do anything that might require users
> to confirm that they want to leave, it should navigate to a new URL first.

When I tested in comment #8 / comment #9, it didn't work in either direction. Which means if you're a web page with a beforeunload handler and the user then loads about:preferences, the web page's beforeunload handler does not get called. I don't think that should be WONTFIX, though it's enough of an edgecase that perhaps we don't need to make it too high-prio. OTOH, it's also not super-hard to fix.

Comment 16

[Kris Maglione [:kmag]]

(In reply to :Gijs Kruitbosch from comment #15)
> When I tested in comment #8 / comment #9, it didn't work in either
> direction. Which means if you're a web page with a beforeunload handler and
> the user then loads about:preferences, the web page's beforeunload handler
> does not get called.

OK, that's a good point. I would expect that to work. It seems more likely to come up when hitting the back button, but either way, not super likely.

Comment 17

[radu_91]

(In reply to Kris Maglione [:kmag] from comment #14)
> I'm not sure this is something that needs to be fixed. Non-remote tabs are
> either built-in, in which case we know they don't need beforeunload, or are
> provided by extensions, in which case they have other ways to override
> navigation if they need to. WebExtensions can't currently override
> about:newtab, but if we ever decide to allow it, the tab will have to be
> remote, and this won't apply.
> 
> In any case, adding a beforeunload handler to the new tab page sounds like a
> bug to me. If a new tab page wants to do anything that might require users
> to confirm that they want to leave, it should navigate to a new URL first.
> 
> I think this should be WONTFIX.

The reason I want beforeunload to work on new:tab, is to add fadeout effects(animations) and not "users
 to confirm that they want to leave".

Comment 18

[Firefox Bug Husbandry Bot]

With Firefox 57 only WebExtensions are permitted and are, by default, e10s compatible.