1415918 - tabbrowser.discardBrowser - allow discarding browsers that have beforeunload handlers

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P5)

Description

[u462496]

I am observing that when trying to discard certain web pages, discarding these pages is inhibited due to browser.frameLoader.tabParent.hasBeforeUnload returning true[1].  However I observe that when closing these pages, I do not get a prompt.

-----------------------------

STR:

1) New profile on latest mc.

2) Open new tabs with the following urls:

   http://forecast.weather.gov/MapClick.php?x=321&y=11&site=abq&zmx=&zmy=&map_x=321&map_y=11#.WgR1H2QkpGP
   https://www.repairclinic.com/RepairHelp/How-To-Fix-A-Refrigerator/51-3-853651-/GE-Refrigerator-not-cooling-GTS17JBSARWW
   https://www.searspartsdirect.com/repair-parts-for-GE-GTS17JBSARWW/0432/0166000.html

3) Select the original first tab (without any of these urls)

4) Open browser console.

5) Run the following code in scratchpad - browser context (devtools.chrome.enabled must be true):

    for (let browser of gBrowser.browsers) {
      if (!browser.selected && browser.isConnected && browser.isRemoteBrowser) {
       console.log("hasBeforeUnload? "+browser.frameLoader.tabParent.hasBeforeUnload+"    "+browser.currentURI.spec);
      }
    }

6) Observe hasBeforeUnload returns true for the above urls.

7) Commence to begin closing same tabs.

8) Observe there is no prompt inhibiting the closing of tabs.

-----------------------------

I don't know what actually sets hasBeforeUnload, nevertheless, it seems that that in any case, we should not be prohibiting discarding tabs which would not normally prompt on closing.  Perhaps we need to check something other than hasBeforeUnload to make this decision?

[1] http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2548

Comment 1

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

hasBeforeUnload means precisely that one or more documents in the tab has added a beforeunload event handler.

A beforeunload event handler does not necessarily mean that a prompt will be shown.

Sometimes, for example, beforeunload is (ab)used so that a sync XHR can be sent out with some payload when the user decides to leave the tab, without ever showing the user a prompt.

The beforeunload handler specifies whether or not a prompt should be shown when it executes, and this is not something we can programmatically determine reliably without just running the handler.

Instead of checking hasBeforeUnload, you _could_ run permitUnload on the browser:

http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/toolkit/content/widgets/remote-browser.xml#287

which does the job of sending the message to the content asking to run the beforeunload handler and gets the result back.

Comment 2

[Dão Gottwald [:dao]]

Comment 1 is spot-on and I don't think there's anything we should change here. Let me know if you disagree.

Comment 3

[u462496]

 (In reply to Dão Gottwald [::dao] from comment #2)
> Comment 1 is spot-on and I don't think there's anything we should change
> here. Let me know if you disagree.

Maybe I am not understanding what the reason for not wanting to discard tabs with unload listeners.  I thought it was because we didn't want prompts popping up from nowhere when trying to discard tabs (bug 1284886 comment 44).  Are you saying that there are other reasons as well?

What I am finding IRL, is that when I try to discard tabs, a fair amount of them are not discardable.  In one of my normal life profiles, only 36 out of 83 tabs are discardable.  In this profile 6 of them are youtube - and no youtube tabs are discardable.

This renders the discardBrowser API pretty ineffectual as it sits now.

Comment 4

[u462496]

...So I don't think this is something we can simply ignore.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Kevin Jones from comment #3)
>  (In reply to Dão Gottwald [::dao] from comment #2)
> > Comment 1 is spot-on and I don't think there's anything we should change
> > here. Let me know if you disagree.
> 
> Maybe I am not understanding what the reason for not wanting to discard tabs
> with unload listeners.  I thought it was because we didn't want prompts
> popping up from nowhere when trying to discard tabs (bug 1284886 comment
> 44).  Are you saying that there are other reasons as well?

No, but as Mike said we don't know beforehand whether a page with an beforeunload handler would prompt.

Comment 6

[u462496]

I got the impression that checking browser.permitUnload().permitUnload would tell us that.  Did I not understand Mike's comment correctly?

Comment 7

[u462496]

Users of tab unload addons are accustomed to having any or all of their tabs unloaded, without care of unload handlers.

The current API will not be satisfying to them.

Comment 8

[u462496]

Am I correct in concluding that even if we weren't de-lazifying browsers, but simply unloading the content, the beforeunload concerns mentioned here would still be an issue?

Comment 9

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

(In reply to Kevin Jones from comment #7)
> Users of tab unload addons are accustomed to having any or all of their tabs
> unloaded, without care of unload handlers.
>

If that's the case, then maybe we shouldn't care if the tabs have hasBeforeUnload set to true, and just unilaterally unload them.

Comment 10

[u462496]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #9)
> (In reply to Kevin Jones from comment #7)
> > Users of tab unload addons are accustomed to having any or all of their tabs
> > unloaded, without care of unload handlers.
> >
> 
> If that's the case, then maybe we shouldn't care if the tabs have
> hasBeforeUnload set to true, and just unilaterally unload them.

+1

Comment 11

[Kris Maglione [:kmag]]

My initial suggestion was to immediately unload any tab without a beforeunload listener, and for tabs with a beforeUnload listener, dispatch the event, and cancel the unload without prompting if their beforeunload event handlers tried to block it.

Comment 12

[Kris Maglione [:kmag]]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #9)
> (In reply to Kevin Jones from comment #7)
> > Users of tab unload addons are accustomed to having any or all of their tabs
> > unloaded, without care of unload handlers.
> 
> If that's the case, then maybe we shouldn't care if the tabs have
> hasBeforeUnload set to true, and just unilaterally unload them.

I don't think this is a good idea. The consequences of failing to discard a tab with a beforeunload handler are much less problematic than discarding a tab with unsaved data.

Comment 13

[u462496]

I was able to discard tabs containing the following urls in Google Chrome, which are not discardable in Firefox:

https://stackoverflow.com/questions/586182/how-to-insert-an-item-into-an-array-at-a-specific-index

https://accounts.google.com/signin/v2/sl/pwd?service=wise&passive=1209600&continue=https%3A%2F%2Fdrive.google.com%2Fdrive%2Fmy-drive&followup=https%3A%2F%2Fdrive.google.com%2Fdrive%2Fmy-drive&flowName=GlifWebSignIn&flowEntry=ServiceLogin

https://www.ghacks.net/2016/11/24/firefox-will-only-support-web-extensions-by-the-end-of-2017/

https://github.com/steeve/python-lz4/issues/20

http://macappstore.org/lz4/

Chrome's feature has the option to:

Discard (safely)
Discard (allow unsafe process shutdown)

I don't yet know how each of these work.

Comment 14

[:Gijs (he/him)]

Sounds like we should at least implement a way of basically running permitUnload but disabling the dialog, and making the docshell interpret any attempt to show the dialog as "abort unload/discard".

From the last few comments, it seems we may also want to offer a chrome-matching API that allows discards on things that *would* prompt iff the webextension opts in. Either way, this bug should probably be open.

Comment 15

[u462496]

(In reply to :Gijs (slow, PTO recovery mode) from comment #14)
> ...it seems we may also want to offer a
> chrome-matching API that allows discards on things that *would* prompt iff
> the webextension opts in. Either way, this bug should probably be open.

I agree.

Comment 16

[u462496]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #12)
> (In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and
> needinfos from comment #9)
> > (In reply to Kevin Jones from comment #7)
> > > Users of tab unload addons are accustomed to having any or all of their tabs
> > > unloaded, without care of unload handlers.
> > 
> > If that's the case, then maybe we shouldn't care if the tabs have
> > hasBeforeUnload set to true, and just unilaterally unload them.
> 
> I don't think this is a good idea. The consequences of failing to discard a
> tab with a beforeunload handler are much less problematic than discarding a
> tab with unsaved data.

Are you referring to eg, form data?  Isn't form data saved by SessionStore?  Can you give an example where data would be lost?

Comment 17

[u462496]

(In reply to :Gijs (slow, PTO recovery mode) from comment #14)
> Sounds like we should at least implement a way of basically running
> permitUnload but disabling the dialog, and making the docshell interpret any
> attempt to show the dialog as "abort unload/discard".

Is this something that would have to be handled with native code?

Comment 18

[:Gijs (he/him)]

(In reply to Kevin Jones from comment #17)
> (In reply to :Gijs (slow, PTO recovery mode) from comment #14)
> > Sounds like we should at least implement a way of basically running
> > permitUnload but disabling the dialog, and making the docshell interpret any
> > attempt to show the dialog as "abort unload/discard".
> 
> Is this something that would have to be handled with native code?

Yes.
(In reply to Kevin Jones from comment #16)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #12)
> > (In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and
> > needinfos from comment #9)
> > > (In reply to Kevin Jones from comment #7)
> > > > Users of tab unload addons are accustomed to having any or all of their tabs
> > > > unloaded, without care of unload handlers.
> > > 
> > > If that's the case, then maybe we shouldn't care if the tabs have
> > > hasBeforeUnload set to true, and just unilaterally unload them.
> > 
> > I don't think this is a good idea. The consequences of failing to discard a
> > tab with a beforeunload handler are much less problematic than discarding a
> > tab with unsaved data.
> 
> Are you referring to eg, form data?  Isn't form data saved by SessionStore? 
> Can you give an example where data would be lost?

You're assuming that form data is all there is to page state, which is not the case. Webpages routinely store internal state outside of forms, purely in their JS context, or even in server/connection-related state.

But even if they didn't, session store's form data restoration is very error-prone when fields are added to the page post-load, or when the page clears them with JS either at or after page load (which we can't very well prevent, of course).

If you are confused about the value of beforeunload and have spare time, bug 578828 has plenty of strongly-held opinions on the matter. (If you don't have a lot of spare time, bug 578828 comment 70 has 2 reasonable examples of cases where the user/site loses important data/state if we discarded the browser without prompting. One has to assume that not all close/hide/discard operations will be user-intentional, at least due to misclicks etc.)

Comment 19

[u462496]

(In reply to :Gijs (slow, PTO recovery mode) from comment #18)
> > Is this something that would have to be handled with native code?
> 
> Yes.

I'm definitely not making any commitments, but I wouldn't mind taking a look into this.  I am not really familiar with C++, but if you could direct me to what files would be involved, and any other prompting which would be helpful, I would appreciate it.

This bug is pretty important to me[1] and it has a P5 :-/  (Though obviously making it into 59 is probably not going to happen anyway)

> You're assuming that form data is all there is to page state, which is not
> the case. Webpages routinely store internal state outside of forms, purely
> in their JS context, or even in server/connection-related state.
> 
> But even if they didn't, session store's form data restoration is very
> error-prone when fields are added to the page post-load, or when the page
> clears them with JS either at or after page load (which we can't very well
> prevent, of course).
> 
> If you are confused about the value of beforeunload and have spare time, bug
> 578828 has plenty of strongly-held opinions on the matter. (If you don't
> have a lot of spare time, bug 578828 comment 70 has 2 reasonable examples of
> cases where the user/site loses important data/state if we discarded the
> browser without prompting. One has to assume that not all close/hide/discard
> operations will be user-intentional, at least due to misclicks etc.)

Thanks very much for the explanations.  Yes, I have not taken much concern over beforeunload, maybe because in my workflow it doesn't seem to matter much to me.  ATH's legacy version just indiscriminately unloaded tabs, and it never created a problem for me (or many others apparently).  Though it probably has more to do with my philosophy about things - I try to be mindful about what I am doing, and if I lose something it is my fault[2].

I will study the examples you provided and __try__ to have a more open paradigm about this[2].

[1] AFAIC, the api in its present state is somewhat useless.  How this bug ends up also has implications on bug 1384515, and could create a lot of frustration for tab groups devs.

Comment 20

[:Gijs (he/him)]

(In reply to Kevin Jones from comment #19)
> (In reply to :Gijs (slow, PTO recovery mode) from comment #18)
> > > Is this something that would have to be handled with native code?
> > 
> > Yes.
> 
> I'm definitely not making any commitments, but I wouldn't mind taking a look
> into this.  I am not really familiar with C++, but if you could direct me to
> what files would be involved, and any other prompting which would be
> helpful, I would appreciate it.
> 
> This bug is pretty important to me[1] and it has a P5 :-/  (Though obviously
> making it into 59 is probably not going to happen anyway)

OK. So for background, there's a (hidden, about:config) pref to disable beforeunload prompts (but not the firing of the event in the webpage!). In bug 1107771 I fixed my initial implementation of it so that we just never prompt (but still fire events on the entire docshell tree). There is already a boolean option that gets passed around internally that controls whether the user has already responded to at least 1 prompt and said they want to continue unloading (this is so we don't show repeated prompts for nested frames that might also respond to their respective beforeunload events). All the relevant code is in nsDocumentViewer's PermitUnload and PermitUnloadInternal methods:

https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/layout/base/nsDocumentViewer.cpp#1146

The corresponding IDL entry is https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsIContentViewer.idl#52-68 . Note that even the "internal" method is exposed on IDL (but as noscript etc.) because it's also called from within permitunload on the content viewers of child docshells.

It seems now that we basically want one of 3 behaviours, and the pref and/or arguments should control the behaviour. The pref should probably override runtime-specified argument behaviour (so if you set the pref saying "never prompt for beforeunload", even if we're discarding tabs that have beforeunload handlers and the webextension/tabbrowser callsite specifies "if the beforeunload handler asks to prompt, just don't unload (return false from PermitUnload)" then we should instead still allow the unload because the user's about:config pref specifies so. Likewise, we have a pref and "interactivity" checks, so pages that the user has never interacted with cannot cause a prompt to be shown (and will normally be unloaded without question). We should do the same even if we would normally take the "don't unload" approach.

One way to do this would be to replace the "aShouldPrompt" option with a flag, which can have 3 values:

- when the web page asks to prompt, we prompt (default) and return true/false depending on what the user does with the prompt
- the web page's request to prompt gets ignored but we stop sending further/nested beforeunload events as soon as a prompt is requested and we do NOT unload (so permitUnload returns false)
- any requests to prompt get ignored and we DO unload (so permitUnload returns true). This would be used for the case where the main 'disable beforeunload prompts' pref is flipped, or if/when we provide a JS API that allows webextensions to force discarding/unloading even tabs that do request prompts.

You can add the flag to the public API as an optional parameter in the IDL (`in optional unsigned aPermitUnloadFlags`). Making it optional means we don't need to update all the consumers.
You can replace `aShouldPrompt` with a non-optional inout param on the internal API IDL: `inout unsigned aPermitUnloadFlags` .

You can define the 3 flag values as constants on nsIContentViewer. You can probably use https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsIContentViewer.idl#113-118 as an example. The default / 0 value should be "prompt". Then existing JS callers to PermitUnload won't need to be modified and will have their current behaviour.

The public PermitUnload method can call the private one with an arg that then gets changed based on the pref and/or prompt responses so that we switch appropriately to "no longer prompt and eventually return true". If we pass in "ignore prompts but don't unload", then at the first point where we would prompt (ie when we enter this if statement: https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/layout/base/nsDocumentViewer.cpp#1239 ) we should just return false immediately (rather than firing the event everywhere else - because we've already decided not to unload).


Instead of the flag with 3 states, we could also have 2 boolean arguments, but that seems less tidy to me and we no longer have to worry about binary or add-on compatibility, so we might as well go with the flag option, AFAICT.

Finally, we will need to make sure that discarding the browser doesn't re-fire beforeunload a second time. It would probably be nice to also take this opportunity to verify that we send the 'unload' event when the browser is discarded (both when the page has and has not set a 'beforeunload' listener as well).

I hope that helps, feel free to ask more questions and/or ping me on IRC/email.

Comment 21

[u462496]

(In reply to :Gijs (slow, PTO recovery mode) from comment #20)
> (In reply to Kevin Jones from comment #19)
> > (In reply to :Gijs (slow, PTO recovery mode) from comment #18)
> > > > Is this something that would have to be handled with native code?
> > > 
> > > Yes.
> > 
> > I'm definitely not making any commitments, but I wouldn't mind taking a look
> > into this.  I am not really familiar with C++, but if you could direct me to
> > what files would be involved, and any other prompting which would be
> > helpful, I would appreciate it.
> > 
> > This bug is pretty important to me[1] and it has a P5 :-/  (Though obviously
> > making it into 59 is probably not going to happen anyway)
> 
> OK. So for background, there's a (hidden, about:config) pref to disable
> beforeunload prompts (but not the firing of the event in the webpage!). In
> bug 1107771 I fixed my initial implementation of it so that we just never
> prompt (but still fire events on the entire docshell tree). There is already
> a boolean option that gets passed around internally that controls whether
> the user has already responded to at least 1 prompt and said they want to
> continue unloading (this is so we don't show repeated prompts for nested
> frames that might also respond to their respective beforeunload events). All
> the relevant code is in nsDocumentViewer's PermitUnload and
> PermitUnloadInternal methods:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 2535bad09d720e71a982f3f70dd6925f66ab8ec7/layout/base/nsDocumentViewer.
> cpp#1146
> 
> The corresponding IDL entry is
> https://dxr.mozilla.org/mozilla-central/source/docshell/base/
> nsIContentViewer.idl#52-68 . Note that even the "internal" method is exposed
> on IDL (but as noscript etc.) because it's also called from within
> permitunload on the content viewers of child docshells.
> 
> It seems now that we basically want one of 3 behaviours, and the pref and/or
> arguments should control the behaviour. The pref should probably override
> runtime-specified argument behaviour (so if you set the pref saying "never
> prompt for beforeunload", even if we're discarding tabs that have
> beforeunload handlers and the webextension/tabbrowser callsite specifies "if
> the beforeunload handler asks to prompt, just don't unload (return false
> from PermitUnload)" then we should instead still allow the unload because
> the user's about:config pref specifies so. Likewise, we have a pref and
> "interactivity" checks, so pages that the user has never interacted with
> cannot cause a prompt to be shown (and will normally be unloaded without
> question). We should do the same even if we would normally take the "don't
> unload" approach.
> 
> One way to do this would be to replace the "aShouldPrompt" option with a
> flag, which can have 3 values:
> 
> - when the web page asks to prompt, we prompt (default) and return
> true/false depending on what the user does with the prompt
> - the web page's request to prompt gets ignored but we stop sending
> further/nested beforeunload events as soon as a prompt is requested and we
> do NOT unload (so permitUnload returns false)
> - any requests to prompt get ignored and we DO unload (so permitUnload
> returns true). This would be used for the case where the main 'disable
> beforeunload prompts' pref is flipped, or if/when we provide a JS API that
> allows webextensions to force discarding/unloading even tabs that do request
> prompts.
> 
> You can add the flag to the public API as an optional parameter in the IDL
> (`in optional unsigned aPermitUnloadFlags`). Making it optional means we
> don't need to update all the consumers.
> You can replace `aShouldPrompt` with a non-optional inout param on the
> internal API IDL: `inout unsigned aPermitUnloadFlags` .
> 
> You can define the 3 flag values as constants on nsIContentViewer. You can
> probably use
> https://dxr.mozilla.org/mozilla-central/source/docshell/base/
> nsIContentViewer.idl#113-118 as an example. The default / 0 value should be
> "prompt". Then existing JS callers to PermitUnload won't need to be modified
> and will have their current behaviour.
> 
> The public PermitUnload method can call the private one with an arg that
> then gets changed based on the pref and/or prompt responses so that we
> switch appropriately to "no longer prompt and eventually return true". If we
> pass in "ignore prompts but don't unload", then at the first point where we
> would prompt (ie when we enter this if statement:
> https://dxr.mozilla.org/mozilla-central/rev/
> 2535bad09d720e71a982f3f70dd6925f66ab8ec7/layout/base/nsDocumentViewer.
> cpp#1239 ) we should just return false immediately (rather than firing the
> event everywhere else - because we've already decided not to unload).
> 
> 
> Instead of the flag with 3 states, we could also have 2 boolean arguments,
> but that seems less tidy to me and we no longer have to worry about binary
> or add-on compatibility, so we might as well go with the flag option, AFAICT.
> 
> Finally, we will need to make sure that discarding the browser doesn't
> re-fire beforeunload a second time. It would probably be nice to also take
> this opportunity to verify that we send the 'unload' event when the browser
> is discarded (both when the page has and has not set a 'beforeunload'
> listener as well).
> 
> I hope that helps, feel free to ask more questions and/or ping me on
> IRC/email.

Thank you Gijs for taking the time to write that all out, it is very detailed and follow-able.  I'll see what I can do.

Comment 22

[u462496]

Attachment: bug_1415918_PART_1_permitUnload_with_options.diff

Comment 23

[u462496]

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

Comment 24

[u462496]

(In reply to Kevin Jones from comment #23)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b9c51d67a2b391c4c70020bbfd7434af62079d18

Try looks good.

Comment 25

[u462496]

Ugh, code handling eDontPromptAndDontUnload option in PermitUnloadInternal is incomplete.  But you're welcome to give feedback on what I've got so far if you like.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8929848 [details] [diff] [review]
bug_1415918_PART_1_permitUnload_with_options.diff

Review of attachment 8929848 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is definitely on the right track. Thanks for looking at this!

::: browser/components/sessionstore/test/browser_1415918_beforeunload_options.js
@@ +9,5 @@
> +      ["dom.require_user_interaction_for_beforeunload", false],
> +    ]
> +  });
> +
> +  let url = "http://example.com/browser/browser/components/sessionstore/test/beforeunload.html";

Nit: I like to use relative paths in tests, so if tests ever get moved it doesn't break the world. See https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/test/browser_readerMode.js#13 for an example.

::: docshell/base/nsIContentViewer.idl
@@ +63,4 @@
>     * Checks if the document wants to prevent unloading by firing beforeunload on
>     * the document, and if it does, prompts the user. The result is returned.
>     */
> +  boolean permitUnload([optional] in unsigned long aPermitUnloadFlags);

Nit: I think we should probably have the constants defined above the methods, and document the new argument here, and then just have an 'as above' comment in front of the internal method much like we do now (but adjust the name of the argument).

@@ +73,5 @@
>    /**
> +   * Flags for permitUnloadInternal.
> +   *
> +   * ePrompt Prompt and return the user's choice (default).
> +   * eDontPromptAndDontUnload Don't prompt and don't unload.

Nit: this should probably say "Don't prompt, and return false (unload not permitted) if the document (or its children) asks us to prompt." to clarify that if the document *doesn't* request a prompt (e.g. youtube) then we do allow the unload (ie return true).

Similar comment for `eDontPromptAndUnload`.

As usual, naming things is one of the harder problems in CS and I don't have a good idea about naming the constants. What you have is probably fine unless an actual docshell peer (bz would be a good reviewer, I think) objects.

::: layout/base/nsDocumentViewer.cpp
@@ +1245,3 @@
>    // NB: we nullcheck mDocument because it might now be dead as a result of
>    // the event being dispatched.
> +  if (!sIsBeforeUnloadDisabled && *aPermitUnloadFlags == ePrompt && dialogsAreEnabled &&

So I think what needs to happen is:

- before this block, if `sIsBeforeUnloadDisabled`, force-set `aPermitUnloadFlags` to `eDontPromptAndUnload`.

- in this existing if condition, check `aPermitUnloadFlags != eDontPromptAndUnload`, instead of the `sIsBeforeUnloadDisabled && *aShouldPrompt` check.

Finally, *inside* the if block, we can handle the `eDontPromptAndDontUnload` case. This would avoid not closing the tab even for beforeunload pages that haven't been interacted with, are sandboxed, don't return any text as the return string or preventDefault() the event (so there would be no need to prompt) etc.

We should probably make sure that last case is included in the unit-test.

Comment 27

[u462496]

Attachment: bug_1415918.diff

Comment 28

[u462496]

(In reply to Kevin Jones from comment #27)
> Created attachment 8930594 [details] [diff] [review]
> bug_1415918.diff

I'm thinking, that in discardBrowser, I wonder if there is a point in keeping the default permitUnload options.

I'm thinking that we should just make the dontPromptAndDontUnload case the default (which I think was Kris' suggestion to begin with), and use a boolean for opting in to the dontPromptAndUnload case.

Comment 29

[:Gijs (he/him)]

(In reply to Kevin Jones from comment #28)
> (In reply to Kevin Jones from comment #27)
> > Created attachment 8930594 [details] [diff] [review]
> > bug_1415918.diff
> 
> I'm thinking, that in discardBrowser, I wonder if there is a point in
> keeping the default permitUnload options.
> 
> I'm thinking that we should just make the dontPromptAndDontUnload case the
> default (which I think was Kris' suggestion to begin with), and use a
> boolean for opting in to the dontPromptAndUnload case.

Yep, this would make sense to me. I don't think anyone wants the 'ePrompt' behaviour when calling discardBrowser.

Comment 30

[:Gijs (he/him)]

Comment on attachment 8930594 [details] [diff] [review]
bug_1415918.diff

Review of attachment 8930594 [details] [diff] [review]:
-----------------------------------------------------------------

On the right track, but I still think the logic of flag checking in PermitUnloadInternal is not quite right yet. Sorry if my previous comments weren't clear - feel free to ping me on IRC if I'm not making any sense. English is hard. :-(

::: browser/components/sessionstore/test/browser_1284886_suspend_tab.js
@@ +35,5 @@
>  
> +  // Test that tab with beforeunload handler which would show a prompt cannot be
> +  // suspended if `dontPromptAndDontUnload` option is passed.
> +  gBrowser.discardBrowser(browser1, browser1.dontPromptAndDontUnload);
> +  ok(tab1.linkedPanel, "cannot suspend a tab with beforeunload handler which would show a propmt");

Nit: "prompt" here and below

::: docshell/base/nsIContentViewer.idl
@@ +49,5 @@
>  
>    [noscript] readonly attribute boolean isStopped;
>  
>    /**
> +   * aPermitUnloadFlags are passed to permitLoad to tell what action to take

Nit: permitUnload :-)

::: layout/base/nsDocumentViewer.cpp
@@ +1236,5 @@
>    nsCOMPtr<nsIDocShell> docShell(mContainer);
>    nsAutoString text;
>    event->GetReturnValue(text);
>  
> +  if (*aPermitUnloadFlags == eDontPromptAndDontUnload && !text.IsEmpty()) {

Hm, so now we will not unload, even if this is a document the user has never interacted with, or if it's sandboxed, or if we would otherwise not show the prompt even if aPermitUnloadFlags was `ePrompt`. Also, I don't think this covers the case where the beforeunload handler calls `event.preventDefault()` (which is the other way they can normally trigger a prompt). This is why I suggested moving this check into the block for the existing big 'if' clause below.

@@ +1241,5 @@
> +    *aPermitUnload = false;
> +    return NS_OK;
> +  }
> +
> +  if (sIsBeforeUnloadDisabled) {

Either way we should do this (ie overriding the flags based on the about:config pref) before checking if `aPermitUnloadFlags` matches anything else. So if you need to keep the above if clause separate for some reason (not entirely sure why) then this one should come before it.

Comment 31

[u462496]

Attachment: 029.SUBMIT.bug_1415918_v2.diff

Updated per Gijs' suggestions.

Comment 32

[u462496]

Attachment: 032.SUBMIT.bug_1415918_v3.diff

Updated per Gijs' recommendations.

In tabbrowser.discardBrowser I changed aPermitUnloadFlags to boolean aForceDiscard, making dontPromptAndDontUnload the default action with the option to dontPromptAndUnload.

Also I removed the test for hasBeforeUnload, since browser.permitUnload tests for that right off anyway:

https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml#292-294

Moved the generic permitUnload tests to docshell.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8931177 [details] [diff] [review]
032.SUBMIT.bug_1415918_v3.diff

>+++ b/browser/components/sessionstore/test/browser_1284886_suspend_tab.js

>+  // Test that tab with beforeunload handler which would show a prompt will be suspended if opted.

"if forced", maybe?

>+  // Test that tab with beforeunload handler which would show a prompt cannot be suspended.
>+  gBrowser.discardBrowser(browser1, true);
>+  ok(!tab1.linkedPanel, "can suspend a tab with beforeunload handler which would not show a prompt");

The comment there is wrong, I think.  This is testing that a tab with a handler which would _not_ show a prompt _can_ be suspended.

And the discardBrowser() call should not pass that second "true" arg here, I would think.

>+++ b/docshell/base/nsIContentViewer.idl
>+   * aPermitUnloadFlags are passed to PermitUnload to tell what action to take

Maybe "indicate" or "say" rather than "tell"?

>+   *                           if the document (or its children) asks us to prompt..

Extra '.' at the end there.

>+      return PermitUnload(0, canUnload);

Maybe pass ePrompt here to make it clearer what this is doing?

>+++ b/docshell/test/browser/browser_bug1415918_beforeunload_options.js
>+    ok(content.window.document.body.hasAttribute("fired"), "parent document beforeunload handler fired");

I think the test description (second arg to ok()) would be clearer as "parent document beforeunload handler should fire".  Same sort of thing for the other test descriptions in this test.

Otherwise if this ok() fails, the failure message will say "parent document beforeunload handler fired" when the whole problem is that it did _not_ do that.

>+  // Prompt is not shown, don't permit unload.

Should buttonId be reset to something falsy before this part?  Otherwise you could just get the expected behavior of this test because we keep pretending the user is clicking "Cancel".

Maybe we should be testing whether there is in fact a prompt happening too?

There are several places in the test (all prefaced with this comment) where I think we should clear out buttonId.

The actual code change looks great.  r=me with the above nits addressed.

Comment 34

[u462496]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #33)

Thank you very much, Boris.

> >+  // Prompt is not shown, don't permit unload.
> 
> Should buttonId be reset to something falsy before this part?  Otherwise you
> could just get the expected behavior of this test because we keep pretending
> the user is clicking "Cancel".
> ....
> There are several places in the test (all prefaced with this comment) where
> I think we should clear out buttonId.

It seems to me that when we aren't showing a prompt, the value of buttonId is irrelevant as there are no buttons to click.  I checked over the code again, and AFAICT whenever we come back to showing the prompt, buttonId is the value we want.  Or maybe I'm missing something here?

> Maybe we should be testing whether there is in fact a prompt happening too?

Okay.  And okay on all the other stuff too.

Comment 35

[u462496]

Attachment: bug_1415918_v4.diff

Updated as recommended by Boris (including immediately clearing buttonId's once they are no longer needed)

Comment 36

[u462496]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9268ce340ff225354ba8477d3d9aa3ce4735dbe9

Comment 37

[Dão Gottwald [:dao]]

applying https://bug1415918.bmoattachments.org/attachment.cgi?id=8931333
patching file docshell/base/nsIContentViewer.idl
Hunk #1 FAILED at 44
Hunk #2 FAILED at 193
2 out of 2 hunks FAILED -- saving rejects to file docshell/base/nsIContentViewer.idl.rej
abort: patch failed to apply

this is on mozilla-inbound tip

Comment 38

[u462496]

(In reply to Dão Gottwald [::dao] from comment #37)
> applying https://bug1415918.bmoattachments.org/attachment.cgi?id=8931333
> patching file docshell/base/nsIContentViewer.idl
> Hunk #1 FAILED at 44
> Hunk #2 FAILED at 193
> 2 out of 2 hunks FAILED -- saving rejects to file
> docshell/base/nsIContentViewer.idl.rej
> abort: patch failed to apply
> 
> this is on mozilla-inbound tip

So should I rebase to mozilla-inbound?

Comment 39

[u462496]

Attachment: bug_1415918_v5.diff

Rebased.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/629bb65f18cb
Allow discarding browsers that have beforeunload handlers in tabbrowser.discardBrowser. r=bz

Comment 41

[Dão Gottwald [:dao]]

FWIW, that patch was messed up and gave me this:

dao@x1:~/mozilla/inbound$ hg import https://bug1415918.bmoattachments.org/attachment.cgi?id=8931529
applying https://bug1415918.bmoattachments.org/attachment.cgi?id=8931529
dao@x1:~/mozilla/inbound$ hg out
comparing with ssh://hg.mozilla.org/integration/mozilla-inbound/
searching for changes
changeset:   393568:db7ace3b7b89
tag:         tip
user:        Dão Gottwald <[e-mail]>
date:        Fri Nov 24 15:13:33 2017 +0100
summary:     user: Kevin Jones <[e-mail]>

Had to fix up the user and commit message manually.

Comment 42

[u462496]

(In reply to Dão Gottwald [::dao] from comment #41)
> FWIW, that patch was messed up and gave me this:
> 
> dao@x1:~/mozilla/inbound$ hg import
> https://bug1415918.bmoattachments.org/attachment.cgi?id=8931529
> applying https://bug1415918.bmoattachments.org/attachment.cgi?id=8931529
> dao@x1:~/mozilla/inbound$ hg out
> comparing with ssh://hg.mozilla.org/integration/mozilla-inbound/
> searching for changes
> changeset:   393568:db7ace3b7b89
> tag:         tip
> user:        Dão Gottwald <[e-mail]>
> date:        Fri Nov 24 15:13:33 2017 +0100
> summary:     user: Kevin Jones <[e-mail]>
> 
> Had to fix up the user and commit message manually.

Hmmm???  I don't what I should do different next time.

Thanks Dao.

Comment 43

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/629bb65f18cb

Comment 45

[u462496]

Can this be uplifted to 58?  WE tabs.discard becomes available in 58, and users would have to wait another cycle to experience the full feature.  The functionality in 58 is currently pretty limited.

Comment 46

[u462496]

^^^^

Revisit comment 3 and comment 13.

Comment 47

[:Gijs (he/him)]

(In reply to Kevin Jones from comment #45)
> Can this be uplifted to 58?  WE tabs.discard becomes available in 58, and
> users would have to wait another cycle to experience the full feature.  The
> functionality in 58 is currently pretty limited.

Boris, how do you feel about uplifting this? We've still got around 7 weeks of beta runway, but that does include the holiday season. I'm not sure how to evaluate the docshell change in terms of uplift. I think it should be fine given testing, but I'd like to get a second opinion.

Kevin: if Boris is fine with this, you can use the "approval-mozilla-beta" flag on the attachment to request uplift.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Sorry for the horrible lag here...  I do think uplifting this is OK from the docshell/viewer point of view.

Comment 50

[:Gijs (he/him)]

Comment on attachment 8931529 [details] [diff] [review]
bug_1415918_v5.diff

Approval Request Comment
[Feature/Bug causing the regression]: this is fixing things for the discard webextensions API which is shipping with 58
[User impact if declined]: some tabs can't be discarded, with no obvious reasoning for the add-on/user to understand why "it didn't work".
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not manually, but the tests are pretty exhaustive
[Needs manual test from QE? If yes, steps to reproduce]: Unsure - there's automated coverage, so that's pretty good... see comment #13 for a few urls to test with. Testing should check that either using gBrowser.discardBrowser (like in bug 1405442 comment #0) or using the webextensions discard API, the tabs get unloaded if they're background/unselected tabs (will reload when selected again)
[List of other uplifts needed for the feature/fix]: n/a. Would be nice to have bug 1415918 as well but that is unfinished.
[Is the change risky?]: not very
[Why is the change risky/not risky?]: we're changing some relatively well-understood beforeunload code, which has effectively been reviewed by both bz and me, where both of us know the code in question. Plus there are already decent tests for beforeunload, and this test adds a lot more.
The only risk I see is to the discard API we expose to webextensions, and given that that's opt-in and the state without this patch isn't really all that great to be shipping, I think that taking that risk is the right choice.
[String changes made/needed]: no

Comment 51

[Gerry Chang [:gchang]]

Comment on attachment 8931529 [details] [diff] [review]
bug_1415918_v5.diff

We won't have any beta builds during the time off and we only have 1.5 weeks before RC after coming back from holidays. It seems this patch is huge and risky to me. Let's let it ride the train on 59. Beta58-.

Comment 52

[Danial Horton]

Is this why none of the discard/unload addons can unload a single Youtube tab?