616853 - The onbeforeunload dialog should be tab-modal

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Roman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 ( .NET CLR 3.5.30729; .NET4.0E)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 ( .NET CLR 3.5.30729; .NET4.0E)

(Filing this bug as part of an effort to get the window-modal dialogs listed in bug 562258 fixed. The obvious fix is to make them tab-modal instead, but if a better idea has been suggested, please close as duplicate of the better idea.)

The onbeforeunload, onunload, onpagehide dialogs are currently window-modal, blocking the entire Firefox window. This is undesirable.

Reproducible: Always



Expected Results:  
The tab should be focused if the prompt is displayed as a result of user action (e.g. closing a background tab). However the dialog itself should be tab-modal, allowing the user to use other tabs while it is displayed.

Comment 1

[Biju]

only onbeforeunload
onunload and onpagehide dont have dialog feature

Comment 2

[Biju]

testcase attachment 492119 [details]

Comment 3

[Nochum Sossonko [:Natch]]

Doesn't block bug 613800 in any way I can see.

Comment 4

[Felix Fung (:felix)]

Attachment: Use Tab-Modal Dialogs During 'onbeforeunload'

Summary of Changes

Switch to tab-modal prompts where possible during onbeforeunload. (Checking user preference of course.)

Comment 5

[Felix Fung (:felix)]

Im not so sure we can do this for the onunload and onpagehide events. As it is, if you have an alert during either of those events it's pretty confusing because the tab WILL close and then the alert will show up on whatever tab we've switched to. In other words, there is no tab left when those events get called so we can't really make it tab modal. Making it tab-modal on the shown tab will just make it even more confusing than it already is.

Comment 6

[Nochum Sossonko [:Natch]]

I don't think this can be done. See the end of bug 613800 for the history behind this exception.

Comment 7

[Felix Fung (:felix)]

I think comment 6 is probably right =\. bug 613800 comment 5 can be repro'd.

Comment 8

[Roman]

Do you guys mean it can't be done with a reasonable amount of effort? I don't see any reason why this can't be done *at all*; there's no logical contradiction at the root of this request...

Or do you mean that the specific approach taken in the patch is not viable?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Felix, do you still need my review here?

Comment 10

[Felix Fung (:felix)]

Comment on attachment 563588 [details] [diff] [review]
Use Tab-Modal Dialogs During 'onbeforeunload'

See comment 7

Comment 11

[Felix Fung (:felix)]

@Roman: I think the answer is something like, this will require more effort and the path taken by the patch is not viable. Looking over the issues in bug 613800, we probably need a good amount of refactoring the code to avoid the race conditions. Someone else might be able to take a look at that and I'm not sure I'm the right person.

Comment 12

[Justin Dolske [:Dolske]]

The basic approach needed here has a few parts:
  1. set some flag when we're showing the onbeforeunload dialog
  2. check for that flag when closing a document, and if set skip showing the usual
     prompt / calling onbeforeunload handlers (sort of a "force close")
  3. make sure that nothing in the callstack up to this point is unhappy with
     reentrancy by the second attempt to close the tab
  4. make sure we abort the 1st prompt and it cleans up properly after a later
     attempt forces the document to close.

#3 is probably the trickiest part. It's possible we might need to support async prompts with callback and refactor a bunch of code, but I hope not. :)

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1

(In reply to Justin Dolske [:Dolske] from comment #12)
> The basic approach needed here has a few parts:
>   1. set some flag when we're showing the onbeforeunload dialog
>   2. check for that flag when closing a document, and if set skip showing
> the usual
>      prompt / calling onbeforeunload handlers (sort of a "force close")

Now that bug 864247 has landed, I believe that parts 1 and 2 are complete.

>   3. make sure that nothing in the callstack up to this point is unhappy with
>      reentrancy by the second attempt to close the tab
>   4. make sure we abort the 1st prompt and it cleans up properly after a
> later
>      attempt forces the document to close.

From my limited testing with the attached patch and on Firefox Metro, things seem to mostly "Just Work". The behavior from comment 5 no longer occurs, and the prompts do what I expect (which is really impressive, since I have no idea what to expect in some of these wacky cases).

This patch does cause some pretty serious issues if you follow the steps from bug 613800 comment 5, so I'm not going to request review just yet.

Perhaps, if someone is feeling generous, he/she could give me a small history lesson. It seems that at one point we wanted to prevent popups from ever being created in a beforeunload handler [1][2]. However, some simple testing shows that it is absolutely possible to create a popup from a beforeunload handler (just add an alert). Additionally, we seem to be explicitly trying to support this case [3][4]. I'm wondering when we changed our minds about this issue.

[1] bug 391834
[2] https://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?rev=6e2d977529e4#1133
[3] comment 5
[4] bug 613800 comment 5 (the test case creates an alert in its beforeunload handler)

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #13)
> Perhaps, if someone is feeling generous, he/she could give me a small
> history lesson. It seems that at one point we wanted to prevent popups from
> ever being created in a beforeunload handler [1][2]. However, some simple
> testing shows that it is absolutely possible to create a popup from a
> beforeunload handler (just add an alert). Additionally, we seem to be
> explicitly trying to support this case [3][4]. I'm wondering when we changed
> our minds about this issue.

We didn't change out minds, we do want to prevent them. We fail in some cases (e.g. bug 875986, perhaps other cases not yet filed). Your [3][4] don't seem to be indications to the contrary to me, just bugs.

Comment 15

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #14)
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #13)
> > Perhaps, if someone is feeling generous, he/she could give me a small
> > history lesson. It seems that at one point we wanted to prevent popups from
> > ever being created in a beforeunload handler [1][2]. However, some simple
> > testing shows that it is absolutely possible to create a popup from a
> > beforeunload handler (just add an alert). Additionally, we seem to be
> > explicitly trying to support this case [3][4]. I'm wondering when we changed
> > our minds about this issue.
> 
> We didn't change out minds, we do want to prevent them. We fail in some
> cases (e.g. bug 875986, perhaps other cases not yet filed). Your [3][4]
> don't seem to be indications to the contrary to me, just bugs.

Perhaps I was reading too much into those comments.

I went to file a bug about the fact that beforeunload handlers are capable of creating prompts, but discovered that it was already filed! Updating this bug to depend on bug 856977.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #15)
> I went to file a bug about the fact that beforeunload handlers are capable
> of creating prompts, but discovered that it was already filed! Updating this
> bug to depend on bug 856977.

Heh, clipboard fail - that's the one I meant to refer to in comment 15! Glad you found it.

Comment 17

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

bug 856977 will be landing soon. When it does (and assuming it sticks), I'll rebase and re-test the patch in this bug, then r?

Comment 19

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I'm not working on this ATM. Anyone is welcome to take the patch and run with it. It works well in most cases, but causes the browser to be unusable if you do the following:
  1. Open a page that has a beforeunload handler
  2. Using panorama, close the tab group that the page in 1 is part of.

I imagine that fixing this is relatively straightforward front-end work. If no one picks this up I might be able to work on it in the coming weeks.

Comment 24

[spamscud2003]

bug https://bugzilla.mozilla.org/show_bug.cgi?id=950336 was marked as a duplicate of this bug, erroneously. The entire "onUnload" event within javascript is a bug and a security threat. There needs to be a way to DISABLE IT COMPLETELY. I don't appreciate how my bug report was completely dismissed and then closed as a duplicate.

Comment 27

[Dave Garrett]

According to bug 953147 we've got in the wild scam pages exploiting this to make pseudo-ransomware:
http://techfruit.com/2013/12/12/europol-ec3-scam-targeting-unsuspecting-internet-users/

This should probably be bumped up in priority. Who needs to get poked to get someone to focus on this now? CCing and needinfoing Gijs or bzbarsky due to working on bug 950336 and Jesse due to filing bug 578828 (the alternate route to dealing with this). (not sure who to ask, so please needinfo someone more appropriate if needed)

Comment 28

[stavstav2]

This really needs to be fixed like right now... These pages are popping up all over... I am not dumb enough to fall for it but it is really annoying.  Most pages you can click cancel like 3-10 times and eventually get off the page, that actually took me a few times to figure out... there is one page in particular that I could not get off of it was a fake fbi page have you seen this, although I did not try clicking cancel a ton of times, I tried at least 5 times in a row.  That is what made me submit this "bug".

And then then the only way to get off the page, you have to ctrl-alt-del close firefox but then when you restart it it automatically "restores" your tabs and then the page has you locked f***ing again... you better close that tab really fast or unplug your internet...

I can see most people who are not of the highest intelligence or computer literacy, or maybe of the older generation totally falling for this, paying the scammer, giving up their personal info, and/or installing malware by clicking OK thinking they might be able to get off the page that way....

I'm suprised this was not fixed months ago and it is probably fixed in IE by now...

Problem with firefox is they have nobody listening to these type of problems...

Then some guy marks my bug as a "duplicate" when it is not, I do not care about tab-modal... you will still not be able to close that tab. What needs to happen here is simple, you allow only one "confirm you want to leave page" to pop up per web address, and you do not restore tabs.

But probably no one from firefox dev is even listening

Comment 29

[Stephan Sokolow]

(In reply to stavstav2 from comment #28)
> Then some guy marks my bug as a "duplicate" when it is not, I do not care
> about tab-modal... you will still not be able to close that tab. What needs
> to happen here is simple, you allow only one "confirm you want to leave
> page" to pop up per web address, and you do not restore tabs.

I think the idea is that, if it's tab-modal, you can close the tab without having to close the popup first, which means that the site can't re-trigger.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> But probably no one from firefox dev is even listening

They were, until the signal-to-noise ratio for this bug dropped through the floor...  But I'm stopping now, sadly.

Dave, I don't really deal with front-end bits, so I can't tell you much about this bug.  But Gijs is working on a patch to only put up one beforeunload dialog once per navigation, which should help.  I'd link to that bug, but...

Comment 32

[Florian Bender]

(In reply to Vacation Dec 19 to Jan 1 from comment #30)
> But Gijs is working on a patch to only put up one beforeunload dialog 
> once per navigation, which should help.  I'd link to that bug, but...

That is Bug 636374.

Comment 33

[FredMcD]

Good day, and Happy Holidays to all. Bug # 953147 had been reported to you after I placed
a warning at;<BR><BR>

https://support.mozilla.org/en-US/questions/981475#answer-515850<BR><BR>

At the time the FBI message appeared, Firefox v26 was totally locked. NOTHING responded.
My warning message, copied below, tells how I got out of it. I have since added FBI.GOV
to my '''''Blocked Sites''''' add-on. Hears hoping it works.<BR><BR>

I was surfing the web, when somehow I landed on the web page below. The web page displayed a so-called message from the FBI (you know the one), and Firefox was locked. I shut down FF via the Windows Task Manager. When I tried to restart FF, FBI was back. I shut FF down. Then, using the Open New Window option, I was able to get FF up. Here is the web address. If you have site blocking or pop-up add-ons, add this in.


http://fbi.gov.====REMOVE===


id561073976-7652854433.===REMOVE===


v886341'.'com

Comment 34

[:Gijs (he/him)]

Attachment: bug 903613. Make the onbeforeunload prompt tab-modal,

This rebases the patch on top of the one for bug 636374. I've fixed the panorama issue, so asking Tim (Taubert) to review that bit. Otherwise, this seems solid to me... thanks Tim (Abraldes) for doing all the hard work. :-)

Comment 35

[Benjamin Smedberg]

What does this do when Firefox is shutting down and shutdown is paused because of an onbeforeunload dialog? Previously that would steal focus so that the user was aware that their attention was requested.

Comment 36

[:Gijs (he/him)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #35)
> What does this do when Firefox is shutting down and shutdown is paused
> because of an onbeforeunload dialog? Previously that would steal focus so
> that the user was aware that their attention was requested.

It does that. I tested it. I was surprised, too. Not really sure how it works, but it does (at least on OS X).

Comment 37

[Benjamin Smedberg]

On Windows too? That's where we see the most problems with Firefox blocking Windows shutdown and being force-killed due to this sort of thing.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8356096 [details] [diff] [review]
bug 903613. Make the onbeforeunload prompt tab-modal,

r=me on the documentviewer changes if you've checked that this does not regress bug 613800.  That's if we're purposefully allowing tab-modal prompts here even if we're hidden?

You may want to have dolske take a look at this too...

Comment 39

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Boris Zbarsky [:bz] from comment #38)
[...]
> r=me on the documentviewer changes if you've checked that this does not
> regress bug 613800.

I might be misunderstanding bug 613800 but it looks like it should be impossible to regress that bug now that bug 856977 is fixed.

> That's if we're purposefully allowing tab-modal prompts
> here even if we're hidden?

From the diff, I see this:
 NS_IMETHODIMP
 nsDocumentViewer::GetIsTabModalPromptAllowed(bool *aAllowed)
 {
-  *aAllowed = !(mInPermitUnload || mHidden);
+  *aAllowed = !mHidden;
   return NS_OK;
 }

Is this section of code what you're referring to? It looks to me like we still don't allow tab-modal prompts while we're hidden. If you're referring to something else, please let me know.



(In reply to Benjamin Smedberg  [:bsmedberg] from comment #37)
> On Windows too? That's where we see the most problems with Firefox blocking
> Windows shutdown and being force-killed due to this sort of thing.

I applied this patch and tested on Windows with multiple tabs that have beforeunload handlers across multiple panorama groups. When you try to exit Firefox, you are brought to one of the tabs that has a beforeunload handler and it shows you a "Leave page" prompt. I waited for what I thought to be a reasonable amount of time to determine whether Windows was going to force-quite Firefox. Nothing seemed to be happening along those lines. Afterward, I tested various types of user response:
  A) If you select "Stay on page," nothing further happens: No further tabs are focused and no further tabs are closed.
  B) If you attempt to close Firefox again by pressing the 'X,' the behavior is identical to what happens if you press the "Leave page" button - see C
  C) If you select "Leave page," the tab does not close (this is the same behavior as in release Firefox) and you are taken to another tab that has a beforeunload handler. If there are no more tabs with beforeunload handlers, the browser shuts down.
  D) If you navigate away or close the tab, Firefox shuts down (or maybe crashes) - this is definitely a bug and should be fixed before landing

The following behaviors are kind of weird and might confuse users:
  1) After trying to close Firefox, and while the "Leave Page" prompt is displayed, you can do other things (open new tabs, interact with other pages, etc). At some point later you might come back to the tab displaying the "Leave page" prompt and select "Leave page." When you do that, you might not expect to then be taken to another tab with a beforeunload handler or for the browser to quit, but either of those things might happen. One way to approach this problem is to make it so that switching to another tab implicitly selects "Stay on page" - we could also make navigation implicitly select "Stay on page" to alleviate one of the issues in D above. I don't mind if we file this as a follow-up rather than block the landing of tabmodal beforeunload prompts, but I understand if others disagree.
  2) When closing a tab group from panorama, you don't see the beforeunload handler immediately (because panorama apparently doesn't actually close the tab until the "undo close group" text is no longer visible). This could use some UX love, but I don't think it needs to be fixed in the near future (my browsing experience would not be impacted if we never fix this).

Comment 40

[Boris Zbarsky [:bzbarsky]]

> Is this section of code what you're referring to?

No, I'm referring to this bit:

>+        promptBag->SetPropertyAsBool(NS_LITERAL_STRING("allowTabModal"), true);

Most other consumers pass the return value of GetIsTabModalPromptAllowed as the second argument there, which would take mHidden into account.  But this code is explicitly passing true no matter what.

Comment 41

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Boris Zbarsky [:bz] from comment #40)
> > Is this section of code what you're referring to?
> 
> No, I'm referring to this bit:
> 
> >+        promptBag->SetPropertyAsBool(NS_LITERAL_STRING("allowTabModal"), true);
> 
> Most other consumers pass the return value of GetIsTabModalPromptAllowed as
> the second argument there, which would take mHidden into account.  But this
> code is explicitly passing true no matter what.

My mistake! I don't recall from when I wrote that section whether there was a reason to ignore mHidden. If I get a free moment today I'll build a patch that passes in the result of GetIsTabModalPromptAllowed to see if it achieves the behavior we're looking for.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #39)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #37)
> > On Windows too? That's where we see the most problems with Firefox blocking
> > Windows shutdown and being force-killed due to this sort of thing.
> 
> I applied this patch and tested on Windows with multiple tabs that have
> beforeunload handlers across multiple panorama groups. When you try to exit
> Firefox, you are brought to one of the tabs that has a beforeunload handler
> and it shows you a "Leave page" prompt. I waited for what I thought to be a
> reasonable amount of time to determine whether Windows was going to
> force-quite Firefox. Nothing seemed to be happening along those lines.

I think the scenario Benjamin is talking about is shutting down _Windows_ while Firefox is open. It's more aggressive about shutting down apps in that scenario, IIRC.

> we could also make navigation implicitly
> select "Stay on page" to alleviate one of the issues in D above.

Doesn't the tab-modal prompting code already handle the page navigation case? http://hg.mozilla.org/mozilla-central/annotate/8f1c9cdedba5/toolkit/components/prompts/content/tabprompts.xml#l152

Comment 43

[:Gijs (he/him)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #42)
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #39)
> > we could also make navigation implicitly
> > select "Stay on page" to alleviate one of the issues in D above.
> 
> Doesn't the tab-modal prompting code already handle the page navigation
> case?
> http://hg.mozilla.org/mozilla-central/annotate/8f1c9cdedba5/toolkit/
> components/prompts/content/tabprompts.xml#l152

It certainly seems to want to, but for whatever reason, I've also seen this not work (see point D in comment 39). I don't know why.

It's a delicate thing to auto-handle though - if I'm a user who closes a window, and then a tab pops up an onbeforeunload dialog, and then I close that tab (rather than interacting with the dialog), I would assume that ought to mean "close this tab" (not "stay on this page, so leave the tab open"). Whether it should then close the entire window immediately as well... probably not. Not sure how to solve for that case, though.

I've also seen a case where if there are two tabs with an onbeforeunload handler, on OS X, hitting cmd-q gets me a dialog for one of the tabs, but if I select "Leave Page" there, I don't get a dialog for the other tab, and the browser just quits... (this happens without this patch already)

Comment 44

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #42)
> I think the scenario Benjamin is talking about is shutting down _Windows_
> while Firefox is open. It's more aggressive about shutting down apps in that
> scenario, IIRC.

Tested this on my Windows 8.1 device: both a local build with this patch applied and current release Firefox were closed without noticeable incident when shutting down Windows (even with open tabs that had beforeunload handlers). Not sure if other versions of Windows might produce different results.

(In reply to :Gijs Kruitbosch from comment #43)
> I've also seen a case where if there are two tabs with an onbeforeunload
> handler, on OS X, hitting cmd-q gets me a dialog for one of the tabs, but if
> I select "Leave Page" there, I don't get a dialog for the other tab, and the
> browser just quits... (this happens without this patch already)

This can happen if the other tab hasn't actually loaded (e.g. if it's a background tab and you haven't visited it since launching the browser). I think I've also seen this behavior occur with background tabs that should have been loaded, but I'm unaware of a way to reproduce consistently.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #44)
> Tested this on my Windows 8.1 device: both a local build with this patch
> applied and current release Firefox were closed without noticeable incident
> when shutting down Windows (even with open tabs that had beforeunload
> handlers). Not sure if other versions of Windows might produce different
> results.

I thought the scenario to test was:
- have a page loaded in Firefox with an onbeforeunload dialog
- shut down Windows
- when the onbeforeunload dialog appears in Firefox because Windows requested that it exit, ignore it and then try to interact with Firefox for a while

I'm not sure why this scenario would behave differently with a tab-modal prompt rather than a normal one, but that seemed to be Benjamin's concern. I vaguely recall Windows does weird things when determining whether to force-kill an app on shutdown, like monitoring system events being processed or some such.

Comment 46

[Benjamin Smedberg]

The primary issue is that the tab-modal prompts

1) may not steal focus: this means that if you're not looking at that particular tab, you may not realize that Firefox is waiting for your input before closing
2) aren't actually a new window, which may lead Windows to assume that we're just hung and not actually waiting for user input

The scenario I'm worried about in particular is:

* Have a Firefox window with two tabs: tab A has an onbeforeunload dialog, and tab B doesn't.
* Have tab B in the foreground
* Shut down Windows

Comment 47

[Dão Gottwald [:dao]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #46)
> The primary issue is that the tab-modal prompts
> 
> 1) may not steal focus: this means that if you're not looking at that
> particular tab, you may not realize that Firefox is waiting for your input
> before closing

Tab-modal prompts steal focus, but that's undesirable legacy behavior as far as alert() is concerned (bug 332195).

Comment 48

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #46)
> 1) may not steal focus: this means that if you're not looking at that
> particular tab, you may not realize that Firefox is waiting for your input
> before closing

As mentioned elsewhere, tab-modal dialogs do steal focus. I'm not aware of anything that would cause them not to do so when shutting down Windows, but I'm unable to test that (see below).

> 2) aren't actually a new window, which may lead Windows to assume that we're
> just hung and not actually waiting for user input
> 
> The scenario I'm worried about in particular is:
> 
> * Have a Firefox window with two tabs: tab A has an onbeforeunload dialog,
> and tab B doesn't.
> * Have tab B in the foreground
> * Shut down Windows

This is the scenario I tested. Windows 8.1 seems not to care whether Firefox has a dialog open (tab-modal or traditional) and shuts down without even notifying me that Firefox was blocking the shutdown. We'll want to test what happens on previous versions of Windows, specifically XP, Vista, and 7. I don't have these OSes so hopefully someone else can test on them.

Comment 49

[Roman]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #48)
> We'll want to test what
> happens on previous versions of Windows, specifically XP, Vista, and 7. I
> don't have these OSes so hopefully someone else can test on them.

The same thing happens on Windows 7. In fact, Windows 7 closes Firefox without blocking the shutdown in any way in every scenario I tested, including having this modal dialog actually visible and waiting for the user to select one of the buttons.

Comment 50

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3

(In reply to Boris Zbarsky [:bz] from comment #40)
> > Is this section of code what you're referring to?
> 
> No, I'm referring to this bit:
> 
> >+        promptBag->SetPropertyAsBool(NS_LITERAL_STRING("allowTabModal"), true);
> 
> Most other consumers pass the return value of GetIsTabModalPromptAllowed as
> the second argument there, which would take mHidden into account.  But this
> code is explicitly passing true no matter what.

Tested passing the result of GetIsTabModalPromptAllowed and didn't notice any ill effects. The attached patch passes the result of GetIsTabModalPromptAllowed instead of always passing true.

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #39)
>   D) If you navigate away or close the tab, Firefox shuts down (or maybe
> crashes) - this is definitely a bug and should be fixed before landing

From my testing so far, the attached patch fixes this issue.

Additionally, the attached patch fixes this other issue that I recently discovered with the v2 patch:
  1) Open a page with a beforeunload handler
  2) Attempt to close the tab (so that the beforeunload prompt appears)
  3) Navigate to a new page
At this point the prompt is correctly dismissed but the tab is now uncloseable (pressing the 'x' has no effect).

Finally, the attached patch also makes it possible to close a tab (using the 'x' or ctrl+w or other shortcuts) that already has a beforeunload prompt showing. nsDocumentViewer::PermitUnload is supposed to allow this behavior already [1][2], and this is currently the behavior in metroFx.


Soliciting feedback - this patch makes nsDocumentViewer::PermitUnload treat the "dialog aborted" case identically to the way it treats the "stay on page" case. This seems to me like it shouldn't cause any issues, but maybe one of you can think of issues that I'm missing?

Still to address:
  1) I think that selecting a different tab should have the effect of pressing "Stay on page" on the tab that is being de-selected. Until we fix this, we can get weird user flows where the user tries to close Firefox (causing a beforeunload prompt to appear), then performs other interactions with the browser, and at a later time responds to the beforeunload prompt. I'm not sure how common those cases will be, and this sounds like it could be a non-trivial amount of work, so it might be better to file as a follow-up bug.
  2) We still need to verify that we correctly handle cases where Windows shuts down while a page is open that has a beforeunload handler.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=864247#c2
[2] https://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?rev=cf2d1bd796ea#1066

Comment 51

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v4

Attached the wrong patch in the previous comment. This should be the correct one.

Comment 52

[Boris Zbarsky [:bzbarsky]]

I suspect we want "dialog aborted" to be "leave page", but maybe I'm overreacting to all the malicious sites using beforeunload...

Comment 53

[:Gijs (he/him)]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #50)
> Finally, the attached patch also makes it possible to close a tab (using the
> 'x' or ctrl+w or other shortcuts) that already has a beforeunload prompt
> showing. nsDocumentViewer::PermitUnload is supposed to allow this behavior
> already [1][2], and this is currently the behavior in metroFx.
> 
> Soliciting feedback - this patch makes nsDocumentViewer::PermitUnload treat
> the "dialog aborted" case identically to the way it treats the "stay on
> page" case.

(In reply to Boris Zbarsky [:bz] from comment #52)
> I suspect we want "dialog aborted" to be "leave page", but maybe I'm
> overreacting to all the malicious sites using beforeunload...

Yeah, these three things together don't make sense to me. I would have thought that closing the page (by navigating or closing the tab) would cause "dialog aborted", and Tim wrote that that is treated as "stay on page" but also that it allows closing the tab (ie "leave page").

Tim, can you elaborate?

Comment 54

[:Gijs (he/him)]

(Tim, I'm assigning to you as you seem to be working on this right now - let me know if that's not OK :-) )

Comment 55

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v5

1. The first time that a consumer calls nsDocumentViewer::PermitUnload (as a result of user action like closing a tab, navigating, closing the browser, etc), we block on this call [1] while the prompt is active.
2. The second time that a consumer calls nsDocumentViewer::PermitUnload (i.e. the user performed an action like navigating, closing tab, etc while the prompt was already active), we early return [2] and allow whatever action the user was trying to perform.

The action in 2 is going to be performed without prompting. That action will cause the prompt created in 1 to abort [3], so the confirmEx call in [1] will get a failure nsresult, causing PermitUnload to throw. One of the consumers of PermitUnload is broken in that it doesn't handle the case where PermitUnload throws [4]. Two possible fixes are to make PermitUnload return false (stay on page) instead of throwing when its prompt is aborted, or to fix the consumers of PermitUnload. The attached patch does both.

I had originally made PermitUnload continue its execution instead of just returning false immediately because I thought there was more logic in PermitUnload that we wanted to run. After looking at the code again, I'm not so sure that we care whether the remainder of the function runs if our prompt aborted.

[1] https://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?rev=cf2d1bd796ea#1192
[2] https://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?rev=cf2d1bd796ea#1071
[3] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/content/tabprompts.xml?rev=80e4ab0d24bc#198
[4] https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=98660dab432f#5894

Comment 56

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8358615 [details] [diff] [review]
Patch v5

r? :gavin for the browser pieces
r? :bz for the layout pieces

Also requesting feedback from :gijs since he worked on the patch as well and might have ideas about something I may have missed.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8358615 [details] [diff] [review]
Patch v5

r=me given the comment, though I'd be happier if it were "always", not "usually"....

Comment 58

[:Gijs (he/him)]

Comment on attachment 8358615 [details] [diff] [review]
Patch v5

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

This looks good generally, but for the browser.js comment below.

The other thing is that I think we really should fix the "shutdown, onbeforeunload dialog, navigate/ignore" case to no longer shutdown when the tab goes away. I don't know if we want to do that as a followup, but it seems really important not to shut down unexpectedly on our users. That might need a frontend-exposed way of killswitch-ing the pending dialog / shutdown. Although I wonder if/why the pagehide listener doesn't already deal with this case...

::: browser/base/content/browser.js
@@ +5894,5 @@
> +    try {
> +      if (ds.contentViewer && !ds.contentViewer.permitUnload()) {
> +        return false;
> +      }
> +    } catch(e) {

This ends up meaning return true, but your comment 55 says we want to be doing the same as when permitUnload returns false, so I think we need a return false; in the catch block.

Comment 59

[Dão Gottwald [:dao]]

Comment on attachment 8358615 [details] [diff] [review]
Patch v5

>                 // We need to block while calling permitUnload() because it
>                 // processes the event queue and may lead to another removeTab()
>                 // call before permitUnload() even returned.
>-                aTab._pendingPermitUnload = true;
>                 let permitUnload = ds.contentViewer.permitUnload();
>-                delete aTab._pendingPermitUnload;
> 
>                 if (!permitUnload)
>                   return false;

That comment doesn't apply anymore. You could also get rid of the permitUnload variable.

Comment 60

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v6

(In reply to Dão Gottwald [:dao] from comment #59)
> Comment on attachment 8358615 [details] [diff] [review]
> Patch v5
> 
> >                 // We need to block while calling permitUnload() because it
> >                 // processes the event queue and may lead to another removeTab()
> >                 // call before permitUnload() even returned.
> >-                aTab._pendingPermitUnload = true;
> >                 let permitUnload = ds.contentViewer.permitUnload();
> >-                delete aTab._pendingPermitUnload;
> > 
> >                 if (!permitUnload)
> >                   return false;
> 
> That comment doesn't apply anymore. You could also get rid of the
> permitUnload variable.

Done.


(In reply to :Gijs Kruitbosch from comment #58)
> The other thing is that I think we really should fix the "shutdown,
> onbeforeunload dialog, navigate/ignore" case to no longer shutdown when the
> tab goes away. I don't know if we want to do that as a followup, but it
> seems really important not to shut down unexpectedly on our users.

I agree. At first I wanted to fix this as a follow-up but it's a common enough case and is a confusing enough user experience that I think we should fix this now. I have at least one idea to go about implementing this; I'll post about that separately.
 
> ::: browser/base/content/browser.js
> @@ +5894,5 @@
> > +    try {
> > +      if (ds.contentViewer && !ds.contentViewer.permitUnload()) {
> > +        return false;
> > +      }
> > +    } catch(e) {
> 
> This ends up meaning return true, but your comment 55 says we want to be
> doing the same as when permitUnload returns false, so I think we need a
> return false; in the catch block.

Good point. I've removed the changes to consumers of permitUnload in this new patch: That function no longer throws when the dialog aborts, and instead returns false. If we want to update consumers to deal with cases where permitUnload throws (I'm not sure what can cause those cases, but it's not really related to the issue in this bug) we can do that in other bugs.


(In reply to Boris Zbarsky [:bz] from comment #57)
> Comment on attachment 8358615 [details] [diff] [review]
> Patch v5
> 
> r=me given the comment, though I'd be happier if it were "always", not
> "usually"....

Updated the comment to more accurately reflect my understanding of the situation.

Comment 61

[Dão Gottwald [:dao]]

Comment on attachment 8360150 [details] [diff] [review]
Patch v6

>               if (ds && ds.contentViewer) {
>-                // We need to block while calling permitUnload() because it
>-                // processes the event queue and may lead to another removeTab()
>-                // call before permitUnload() even returned.
>-                aTab._pendingPermitUnload = true;
>-                let permitUnload = ds.contentViewer.permitUnload();
>-                delete aTab._pendingPermitUnload;
>-
>-                if (!permitUnload)
>+                if (!ds.contentViewer.permitUnload()) {
>                   return false;

if (ds &&
    ds.contentViewer &&
    !ds.contentViewer.permitUnload())
  return false;

Comment 62

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v7

The attached patch makes it so that selecting a tab causes the no-longer-selected tab to dismiss its beforeunload prompt if it had one active. This resolves the issue of users confusing themselves by initiating a browser shutdown and then navigating away from a beforeunload prompt that appears.

I've tested locally as much as is feasible, but there are lots of potential scenarios that users could encounter. The one situation I tested where behavior didn't match my expectation was this:
  1) Open at least one tab with a beforeunload handler
  2) Go to Panorama's "zoomed out" view (ctrl+shift+e)
  3) Press the "X" on the browser's window
At this point, the the tab with the beforeunload handler is focused but the beforeunload prompt does not actually appear. Even though this behavior is strange, it's actually better than what the current Nightly does when you go through the same STR (no prompt appears but additionally the tab is left in a broken state).

:bz - sorry to keep asking you to review iterations of this patch!

Comment 63

[Boris Zbarsky [:bzbarsky]]

> :bz - sorry to keep asking you to review iterations of this patch!

Can you just post an interdiff from the last thing I reviewed?

Comment 64

[Dão Gottwald [:dao]]

Comment on attachment 8364091 [details] [diff] [review]
Patch v7

>               // We've selected the new tab, so go ahead and notify listeners.
>               let event = document.createEvent("Events");
>               event.initEvent("TabSelect", true, false);
>               this.mCurrentTab.dispatchEvent(event);
> 
>               this._tabAttrModified(oldTab);
>               this._tabAttrModified(this.mCurrentTab);
> 
>+              if (oldBrowser != newBrowser) {
>+                let cv = oldBrowser.docShell.contentViewer;
>+                if (cv.inPermitUnload) {
>+                  let tabBrowser = oldBrowser.getTabBrowser();
>+                  let promptBox = tabBrowser.getTabModalPromptBox(oldBrowser);
>+                  let prompts = promptBox.listPrompts();
>+                  if (prompts.length) {
>+                    promptBox.removePrompt(prompts[prompts.length - 1]);
>+                  }

How could prompts.length be 0 in the case of inPermitUnload?

'oldBrowser.getTabBrowser()' should be the same as 'this'.

>               let ds = browser.docShell;
>               if (ds && ds.contentViewer) {
>-                // We need to block while calling permitUnload() because it
>-                // processes the event queue and may lead to another removeTab()
>-                // call before permitUnload() even returned.
>-                aTab._pendingPermitUnload = true;
>                 let permitUnload = ds.contentViewer.permitUnload();
>-                delete aTab._pendingPermitUnload;
>-
>                 if (!permitUnload)
>                   return false;
>               }

See comment 61.

Comment 65

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v8

Review comments addressed.

(In reply to Boris Zbarsky [:bz] from comment #63)
> > :bz - sorry to keep asking you to review iterations of this patch!
> 
> Can you just post an interdiff from the last thing I reviewed?

I don't think there's an interdiff port for Windows and I don't have a Linux box to use. Bugzilla's interdiff seems to be working correctly if you compare against patch v6. Do you mind using that? The major difference between the patch you previously reviewed and the current patch is that this one adds a new attribute, "inPermitUnload," to nsIContentViewer and implements it in nsDocumentViewer. That attribute exposes whether the content viewer is currently blocked in a call to "PermitUnload".

Comment 66

[Boris Zbarsky [:bzbarsky]]

> I don't think there's an interdiff port for Windows

You should be using hg diff to produce interdiffs...  Just diff your working tree against what you had before before you qref.  Or better yet put the new work in a new patch in your mq and fold before pushing.

> Bugzilla's interdiff seems to be working correctly if you compare against patch v6.

Thanks.  I never trust it, esp when it has the big red letters.  ;)

Comment 67

[Dão Gottwald [:dao]]

Comment on attachment 8364467 [details] [diff] [review]
Patch v8

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -1094,16 +1094,25 @@
>               // We've selected the new tab, so go ahead and notify listeners.
>               let event = document.createEvent("Events");
>               event.initEvent("TabSelect", true, false);
>               this.mCurrentTab.dispatchEvent(event);
> 
>               this._tabAttrModified(oldTab);
>               this._tabAttrModified(this.mCurrentTab);
> 
>+              if (oldBrowser != newBrowser) {
>+                let cv = oldBrowser.docShell.contentViewer;
>+                if (cv.inPermitUnload) {
>+                  let promptBox = this.getTabModalPromptBox(oldBrowser);
>+                  let prompts = promptBox.listPrompts();
>+                  promptBox.removePrompt(prompts[prompts.length - 1]);
>+                }
>+              }

This could use a comment, saying what you're doing here and why you're doing it.

>+              if (ds
>+               && ds.contentViewer
>+               && !ds.contentViewer.permitUnload()) {

Please format this like I did in comment 61.

Comment 68

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Boris Zbarsky [:bz] from comment #66)
> > I don't think there's an interdiff port for Windows
> 
> You should be using hg diff to produce interdiffs...  Just diff your working
> tree against what you had before before you qref.  Or better yet put the new
> work in a new patch in your mq and fold before pushing.

In the future, I'll modify my workflow.

Instead of doing this:
  1. Write/modify patch and hg qref
  2. Upload patch directly from ".hg/patches" directory
  3. If not r+, goto 1
  4. Push reviewed patch

I'll do this:
  1. Write new patch (on top of any existing patches) and hg qnew
  2. Create patch for attaching to bug by "hg diff --rev qparent" and create interdiff patches by "hg diff --rev previouslyReviewedPatch"
  3. If not r+, goto 1
  4. hg qfold and push folded patch

For this particular patch, though, I can't produce an interdiff. Please accept my apologies and review the docshell and layout bits (or delegate).

Comment 69

[Boris Zbarsky [:bzbarsky]]

Yeah, I was assuming I need to do that.  Working on it; it just takes longer because I need a bigger free chunk of time.  :(

Comment 70

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Boris Zbarsky [:bz] from comment #69)
> Yeah, I was assuming I need to do that.  Working on it; it just takes longer
> because I need a bigger free chunk of time.  :(

In that case, thanks! And sorry for pestering you :)

Comment 71

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8364467 [details] [diff] [review]
Patch v8

r=me on the Gecko part.  Thank you!

Comment 72

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v9

Review comments addressed. Carrying forward r+

Still to do:
  1. Run tests, clean up any that are broken by this patch
  2. Test the effects of this patch on the Windows shutdown case in comment 46 on Windows XP and Windows Vista

Comment 73

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Try run:
  https://tbpl.mozilla.org/?tree=Try&rev=57550f0499cb

Comment 74

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Just a quick status update - the patch in this bug breaks a bunch of tests so I'm working on cleaning them up and will post a separate patch that fixes tests.

Comment 75

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

As discussed in #fx-team, :smacleod will take over fixing the tests that are broken by the current patch

Comment 76

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

There are some UI specs for tab modal dialogs in bug 977037. Those should also apply here.

Comment 77

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-616853-Make-the-beforeunload-prompt-tab-modal-r-.patch

This is the same as patch v9, rebased. Carrying over r=dao,bz.

Comment 78

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-616853-Fix-tabview-tests-r-smacleod.patch

Stealing after talking to Steven :)

This patch fixes two Panorama tests that relied on the old modal dialog, that should be pretty straight forward. The tabbrowser.xml change actually only adds "oldBrowser.docShell" to the condition as there was a tabview test that hit a case where the oldBrowser was probably already destructed and didn't have a docShell anymore.

I'm planning to squash those commits before landing.

https://tbpl.mozilla.org/?tree=Try&rev=d9256c383a58

Comment 79

[Dão Gottwald [:dao]]

Comment on attachment 8400507 [details] [diff] [review]
0001-Bug-616853-Make-the-beforeunload-prompt-tab-modal-r-.patch

>+              if (ds &&
>+                  ds.contentViewer &&
>+                  !ds.contentViewer.permitUnload()) {
>+                 return false;
>               }

return false; is indented too far

Comment 80

[Dão Gottwald [:dao]]

Comment on attachment 8400509 [details] [diff] [review]
0002-Bug-616853-Fix-tabview-tests-r-smacleod.patch

>+              if (oldBrowser != newBrowser &&
>+                  oldBrowser && oldBrowser.docShell &&
>+                  oldBrowser.docShell.contentViewer.inPermitUnload) {

What's the point of null-checking oldBrowser?

Comment 81

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #79)
> >+              if (ds &&
> >+                  ds.contentViewer &&
> >+                  !ds.contentViewer.permitUnload()) {
> >+                 return false;
> >               }
> 
> return false; is indented too far

Thanks for catching that, will fix locally.

(In reply to Dão Gottwald [:dao] from comment #80)
> >+              if (oldBrowser != newBrowser &&
> >+                  oldBrowser && oldBrowser.docShell &&
> >+                  oldBrowser.docShell.contentViewer.inPermitUnload) {
> 
> What's the point of null-checking oldBrowser?

We do that a few lines above as well. That's why I assumed it could be null?

Comment 82

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #81)
> > What's the point of null-checking oldBrowser?
> 
> We do that a few lines above as well. That's why I assumed it could be null?

It should never be null... but since you didn't introduce this, that's probably something for another bug...

Comment 83

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-616853-Fix-toolkit-startup-tests-r-smacleod.patch

There's more tests failing in toolkit/components/startup/. Those need the same changes as the tabview tests.

https://tbpl.mozilla.org/?tree=Try&rev=8c9bb5f938a5

Comment 84

[Steven MacLeod [:smacleod]]

Comment on attachment 8400509 [details] [diff] [review]
0002-Bug-616853-Fix-tabview-tests-r-smacleod.patch

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

Looks good for the most part.

It's not obvious to me how you came to select the |browsers[1]| index. Could you please comment with an explanation?

Assuming this isn't an off-by-one, r+ with a comment explaining.

::: browser/components/tabview/test/browser_tabview_bug626455.js
@@ +31,5 @@
>    });
>  }
>  
>  function testStayOnPage() {
> +  let browser = gBrowser.browsers[1];

Why 1 not 0 here? It's not immediately obvious to me what the reason for the index here is. Worth explaining?

Comment 85

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-616853-Fix-tabview-tests-r-smacleod.patch, v2

Seized the chance to clean up the test a little more. Removed the onpagehide and onunload alerts as I don't know why they're in there at all and they threw errors when closing the tab. I hope it's a little clearer now what tab we're using to wait for the onbeforeunload dialog.

Comment 86

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-616853-Fix-newPrompt.abortPrompt-is-not-a-functi.patch

This patch fixes the following failure that occurs when running tabview tests:

[Exception... "[JavaScript Error: "newPrompt.abortPrompt is not a function" {file: "objdir-ff-release/dist/Nightly.app/Contents/MacOS/components/nsPrompter.js" line: 402}]'[JavaScript Error: "newPrompt.abortPrompt is not a function" {file: "objdir-ff-release/dist/Nightly.app/Contents/MacOS/components/nsPrompter.js" line: 402}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/tabbrowser.xml :: _endRemoveTab :: line 2077"  data: yes]

Comment 87

[Dão Gottwald [:dao]]

Comment on attachment 8403157 [details] [diff] [review]
0004-Bug-616853-Fix-newPrompt.abortPrompt-is-not-a-functi.patch

>         winUtils.leaveModalState();
>+        domWin.removeEventListener("pagehide", pagehide);
> 
>         PromptUtils.fireDialogEvent(domWin, "DOMModalDialogClosed");

nit: please move this up and add a blank line, i.e.:

>         domWin.removeEventListener("pagehide", pagehide);
>
>         winUtils.leaveModalState();

Comment 88

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-616853-Fix-newPrompt.abortPrompt-is-not-a-functi.patch, v2

Addressed feedback and carrying over r=dao.

Comment 89

[Steven MacLeod [:smacleod]]

Comment on attachment 8403152 [details] [diff] [review]
0002-Bug-616853-Fix-tabview-tests-r-smacleod.patch, v2

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

LGTM

::: browser/components/tabview/test/browser_tabview_bug599626.js
@@ +21,5 @@
>    });
>  }
>  
>  function testStayOnPage(contentWindow, groupItemOne, groupItemTwo) {
> +  let browser = gBrowser.browsers[1];

As discussed in IRC, a comment explaining the magic [1] wouldn't hurt.

Comment 90

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/8a7368027aad
https://hg.mozilla.org/integration/fx-team/rev/fdaa22262cc0

Comment 91

[Tim Taubert [:ttaubert] (inactive)]

Crap, I actually wanted to change the patch author before pushing because I really didn't do the main work here. Sorry, Tim (Abraldes)! :/

Comment 92

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a7368027aad
https://hg.mozilla.org/mozilla-central/rev/fdaa22262cc0

Comment 93

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Tim Taubert [:ttaubert] from comment #91)
> ****, I actually wanted to change the patch author before pushing because I
> really didn't do the main work here. Sorry, Tim (Abraldes)! :/

No worries! In my mind, fixing the tests is the hard part ;)

I'm really excited to see this in a release. It gets us a little closer to the dream of (almost) no modal prompts

Comment 94

[:Gijs (he/him)]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #93)
> (In reply to Tim Taubert [:ttaubert] from comment #91)
> > Crap, I actually wanted to change the patch author before pushing because I
> > really didn't do the main work here. Sorry, Tim (Abraldes)! :/
> 
> No worries! In my mind, fixing the tests is the hard part ;)
> 
> I'm really excited to see this in a release. It gets us a little closer to
> the dream of (almost) no modal prompts

Speaking of which... how risky is this for uplifting to 30? We're quite close to the end of the cycle, sooo...

Comment 95

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8400507 [details] [diff] [review]
0001-Bug-616853-Make-the-beforeunload-prompt-tab-modal-r-.patch

This a request to upload all of the patches attached to Aurora.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None.
Testing completed (on m-c, etc.): Included in today's Nightly.
Risk to taking this patch (and alternatives if risky): Medium to low risk.
String or IDL/UUID changes made by this patch: Yes.

User impact if declined:
This is not your average approval request because I'm actually requesting to uplift a "feature". Talking to Gijs yesterday I feel like it would be great to have this in even Firefox 30 because there currently are a number of pages that hold people hostage by not allowing them to leave their page easily abusing the onbeforeunload dialog. We have worked around this in a different bug but the ultimate fix would be to make the onbeforeunload dialog tab-modal.

Comment 96

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #95)
> String or IDL/UUID changes made by this patch: Yes.

These patches actually have *no* string changes. Only IDL/UUID changes. (Thanks, Gijs!)

Comment 97

[u279076]

Camelia, will you be able to complete testing of this before the end of this iteration on Monday, April 14? If not please let me know so I can make sure it gets carried over. Thanks.

Comment 98

[Camelia Badau [:cbadau], Desktop QA]

I tested with the latest Nightly 31.0a1 (buildID: 20140413030203) on Windows 7 X64, on Windows 8 x86, on Ubuntu 13.10 x86, on Mac OSX 10.9 and it works as expected. 

I found a new potential scenario and I logged bug 995953 for it.

Comment 99

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Tim Taubert [:ttaubert] from comment #95)
> > String or IDL/UUID changes made by this patch: Yes.
> 
> These patches actually have *no* string changes. Only IDL/UUID changes.
> (Thanks, Gijs!)


Great, cause otherwise this would be a non-starter.   So with bug 995953 can you talk about the risk/reward here?  It looks like we might be introducing a new regression but bug 995953 comment #4 seems to suggest it might not be.  I'd like to get a sense of whether or not this is a bigger win for the users before making a call.  We might want to hold off and get more stable on 31 with this new feature.

Comment 100

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Lukas Blakk [:lsblakk] from comment #99)
> (In reply to Tim Taubert [:ttaubert] from comment #95)
> > > String or IDL/UUID changes made by this patch: Yes.
> > 
> > These patches actually have *no* string changes. Only IDL/UUID changes.
> > (Thanks, Gijs!)
> 
> 
> Great, cause otherwise this would be a non-starter.   So with bug 995953 can
> you talk about the risk/reward here?  It looks like we might be introducing
> a new regression but bug 995953 comment #4 seems to suggest it might not be.
> I'd like to get a sense of whether or not this is a bigger win for the users
> before making a call.  We might want to hold off and get more stable on 31
> with this new feature.

Risk:
  There is an infinite number of scenarios that the patch in this bug could affect: Panorama, multiple tabs with beforeunload handlers, multiple iframes with beforeunload handlers, closing tabs/windows/browsers through each of our existing mechanisms, script actions vs user actions, etc. can all interact with a beforeunload prompt and potentially make it misbehave. I don't think we could possibly test every scenario, so there's definitely a chance that the patch in this bug breaks some cases.
  That being said, we already have plenty of beforeunload edge case bugs (bugzilla search for "beforeunload") and a handful of them should either be fixed (bug 895467, bug 895492) or alleviated (bug 636905) by the patch in this bug.
  I don't believe that bug 995953 is a bug/regression. The behavior described in that bug is consistent with the current behavior of release versions of Firefox. The behavior might not align with user expectations (I can see an argument in either direction), in which case we might want to change the behavior in the future, but the patch in this bug did not cause the specified behavior to change.

Reward:
  My understanding is that sites are actively making hostages out of our users by taking advantage of the app-modal nature of our beforeunload dialogs. The patch in this bug should make our users less susceptible to such attacks.
  App-modal dialogs provide an infuriating user experience. This might not be as convincing as the item mentioned above, but it is the reason I started working on this in the first place.

Both (risk and reward):
  I don't believe that most sites have beforeunload handlers, and I believe that complex interactions between beforeunload handlers and other parts of the browser are even more rare. On the plus side, that means that any regressions/bugs introduced would be less impactful. On the minus side, we're unlikely to get much testing of this new functionality before release.

My gut tells me that there is going to be a handful of regressions to weed out but that we're probably not going to find out about those issues until this patch is exposed to a large audience. I imagine that most users will be unaffected by the bugs and that more users will be affected by the benefits of having tab-modal beforeunload dialogs.

Not sure that I can provide a clear suggestion one way or the other, but I hope these thoughts are helpful!

Comment 101

[Benjamin Smedberg]

I do not believe we should uplift this. The risk of regressions seems pretty high to me.

That said, if we do uplift, what are the options for turning it off if it causes regressions? Since a straight backout would cause IDL changes, that's not really possible once we get to beta.

Comment 102

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #101)
> That said, if we do uplift, what are the options for turning it off if it
> causes regressions? Since a straight backout would cause IDL changes, that's
> not really possible once we get to beta.

That's indeed a valid concern. If we would uplift this I guess we should have an easy way to turn it off, which we don't. I'm leaning towards just letting it ride the trains.

Comment 103

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8400507 [details] [diff] [review]
0001-Bug-616853-Make-the-beforeunload-prompt-tab-modal-r-.patch

Thanks for the additional info Tim, and for pointing out the IDL changes, Ben.  I agree we should wait on this and give more time to uncover potential regressions while on Aurora.

Comment 104

[Sylvestre Ledru [:Sylvestre]]

Added in the release notes with the wording "onbeforeunload dialog now tab-modal" 
We don't want sentences in the release notes but a better wording is welcome.

Comment 105

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

"Dialogs spawned from the onbeforeunload event no longer block access to the rest of the browser"

I think this is better as it gets at the root of the previous problem, and people may not know what "tab-modal" means.

Comment 106

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

Sylvestre, can we adjust the release notes to use the wording from comment #105?

Comment 107

[Lukas Blakk [:lsblakk] use ?needinfo]

Updated.