1325527 - Style tabs with a pending close request (but that aren't closing and don't have a beforeunload dialog up) differently to give user feedback when closing is slow

Status: RESOLVED DUPLICATE of bug 1336763

(Firefox :: Tabbed Browser, enhancement)

Description

[sworddragon2]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161213225349

Steps to reproduce:

While the browser is busy (for example loading a complex site like https://html.spec.whatwg.org/) close the tab in an instance that has e10s enabled and another instance that has e10s disabled.


Actual results:

If e10s is disabled Firefox appears to be frozen for a short time on trying to close the tab and the tab will close as soon as Firefox unfreezes. With e10s enabled on trying to close the tab for example the click animation for the x is shown but the tab does not close immediately.


Expected results:

As the user may expect that the tab closes delayed due to not giving feedback with e10s disabled all is fine here. But with e10s enabled the tab gives feedback in the case on clicking the x but it does not close immediately which can cause problems (like the user trying to close the tab again and hiting a race where he closes another tab as the old tab closed at the exact time where the user clicked). I think with e10s enabled a tab should just be immediately removed from the tab bar on closing it to avoid such issues.

Comment 1

[:Gijs (out for now; he/him)]

Even in e10s (some) webpages rely on beforeunload events (the things that can cause "Are you sure you want to leave this page" dialogs). Without breaking those events (and the workflows of some users), we're required to make roundtrips to the child process to execute those before closing the tab. This is the case even in e10s mode, though at least in e10s there are limits to the time we'll wait for an 'answer'.

I don't think we should spend time on this independent of killing beforeunload events altogether (which is kind of what this implies), which is bug 578828, so I'm marking this WONTFIX.

Comment 2

[YF (Yang)]

I think it may not be entirely due to beforeunload, when user tries to close a tab while it is busy (e.g. through bug 1323800), this operation will wait about 5 seconds (we may have a pref). If this waits has a looks on tabs bar (for example, the tab is grayed; like no response for window), this would be a better user experience.

Comment 3

[:Gijs (out for now; he/him)]

(In reply to YF (Yang) from comment #2)
> I think it may not be entirely due to beforeunload, when user tries to close
> a tab while it is busy (e.g. through bug 1323800), this operation will wait
> about 5 seconds (we may have a pref).

The 5 second wait *is* beforeunload, it's the time limit I mentioned in comment #1. IIRC it's not in a pref. It could be, but it's likely people will shoot themselves in the foot with it.

> If this waits has a looks on tabs bar
> (for example, the tab is grayed; like no response for window), this would be
> a better user experience.

In non-e10s we would simply wait ~forever, so the current state is already better. I don't think pretending the tabstrip is nonfunctional is the right way to go, especially as within the next few months we're likely to increase the number of content processes, meaning that one of them could be hung but the others wouldn't be, and so it would no longer be accurate to suggest the entire tabstrip is unusable.

Comment 4

[YF (Yang)]

(In reply to :Gijs (gone 24 dec - 3 jan) from comment #3)
> (In reply to YF (Yang) from comment #2)
> > I think it may not be entirely due to beforeunload, when user tries to close
> > a tab while it is busy (e.g. through bug 1323800), this operation will wait
> > about 5 seconds (we may have a pref).
> 
> The 5 second wait *is* beforeunload, it's the time limit I mentioned in
> comment #1. IIRC it's not in a pref. It could be, but it's likely people
> will shoot themselves in the foot with it.

Sorry, I confuse the beforeunload with onunload and onbeforeunload event.

Comment 5

[sworddragon2]

(In reply to :Gijs (gone 24 dec - 3 jan) from comment #1)
> Even in e10s (some) webpages rely on beforeunload events (the things that
> can cause "Are you sure you want to leave this page" dialogs).

But if the page has not setup a custom beforeunload event handler the tab could be closed. Or are there another issues left?


(In reply to :Gijs (gone 24 dec - 3 jan) from comment #1)
> This is the case even in e10s mode, though at least in e10s there are
> limits to the time we'll wait for an 'answer'.

There is a timeout forcing the tab/page getting closed even if it shows a message from a beforeunload event? I wanted to check this with a testcase but for some reason I can't get Firefox to show a dialog on returning a string when window.onbeforeunload executes (while the browser console confirms from a console.log message that the function is being executed - no idea what went wrong as I'm normally not working with this event).


(In reply to YF (Yang) from comment #2)
> If this waits has a looks on tabs bar
> (for example, the tab is grayed; like no response for window), this would be
> a better user experience.

This would be a good idea in cases where the tab can't get removed immediately.


(In reply to :Gijs (gone 24 dec - 3 jan) from comment #3)
> In non-e10s we would simply wait ~forever, so the current state is already
> better.

But without e10s the user is aware that the tab is being closed as he got feedback due to the freeze while this is not the case with e10s - so feedback is needed here (like the suggestion from Yang).

Comment 6

[sworddragon2]

(In reply to sworddragon2 from comment #5)
> (In reply to :Gijs (gone 24 dec - 3 jan) from comment #1)
> > This is the case even in e10s mode, though at least in e10s there are
> > limits to the time we'll wait for an 'answer'.
> 
> There is a timeout forcing the tab/page getting closed even if it shows a
> message from a beforeunload event? I wanted to check this with a testcase
> but for some reason I can't get Firefox to show a dialog on returning a
> string when window.onbeforeunload executes (while the browser console
> confirms from a console.log message that the function is being executed - no
> idea what went wrong as I'm normally not working with this event).

On giving it another try I was able to create a testcase. The dialog itself seems to never timeout and on causing an infinite loop in the window.onbeforeunload function I got just the common message that the web page is slowing down the browser (after dom.max_script_run_time seconds) and if I want to wait or stop it which I think should also appear in a similar form without e10s. No idea which other timeout triggers at which condition.

Comment 7

[:Gijs (out for now; he/him)]

(In reply to sworddragon2 from comment #5)
> (In reply to :Gijs (gone 24 dec - 3 jan) from comment #1)
> > Even in e10s (some) webpages rely on beforeunload events (the things that
> > can cause "Are you sure you want to leave this page" dialogs).
> 
> But if the page has not setup a custom beforeunload event handler the tab
> could be closed. Or are there another issues left?

That would require the parent process knowing there are no beforeunload handlers. It doesn't right now, and it can't find out without talking to the child process, which in your hypothetical is somehow hung, so that doesn't work.

> (In reply to :Gijs (gone 24 dec - 3 jan) from comment #1)
> > This is the case even in e10s mode, though at least in e10s there are
> > limits to the time we'll wait for an 'answer'.
> 
> There is a timeout forcing the tab/page getting closed even if it shows a
> message from a beforeunload event? 

No. There is a timeout for the actual sending of messages from the parent to the child process to prompt for beforeunload. If we don't hear *anything* from the child process (because it's hung on another tab or whatever), we will time out. If the dialog shows up successfully and we tell the parent about it, of course we don't time out - that would stop the user interacting with the prompt after the website has explicitly said the user might not want to close it...

> (In reply to :Gijs (gone 24 dec - 3 jan) from comment #3)
> > In non-e10s we would simply wait ~forever, so the current state is already
> > better.
> 
> But without e10s the user is aware that the tab is being closed as he got
> feedback due to the freeze while this is not the case with e10s - so
> feedback is needed here (like the suggestion from Yang).

I don't understand this point. If my browser is frozen in non-e10s, I often don't even get visual feedback on clicking the [x] on a tab (or sometimes I do, but the tab doesn't close). How would "the browser is frozen" ever be 'feedback' ?

I already explained why Yang's suggestion seems unworkable to me. Do you have another suggestion for potential UI feedback?

Note that this seems like an edgecase to me, and like it'd be working around / wallpapering over a bigger problem (the content process being hung, beforeunload being a cluster.... in general), so I'm still not inclined to think we should spend significant time on this problem.
This is in addition to the general "close tabs or windows" logic already being very hairy (because there are a lot of edgecases where closing a tab can automatically close other tabs that the tab content controls, and might close the window at the same time, etc. etc.). Adding some kind of ephemeral UI state here while we wait for the beforeunload message to come in is likely to complicate the code significantly and lead to regressions (e.g. where tabs get left in this state indefinitely, or don't enter it in some circumstances but are still hung, etc.).


(In reply to sworddragon2 from comment #6)
> on causing an infinite loop in the
> window.onbeforeunload function I got just the common message that the web
> page is slowing down the browser (after dom.max_script_run_time seconds) and
> if I want to wait or stop it which I think should also appear in a similar
> form without e10s. No idea which other timeout triggers at which condition.

I've explained this several times now. In e10s, try hanging a tab and then closing another tab, and you'll see a delay before the other tab closes (IIRC it's 5 seconds). Without this timeout the tab would never close (forever trying to talk to the content process, which isn't responding). Without seeing the testcase I can't really say anything else.

Note that the slow script warning appears as a notification bar in e10s mode and a window-modal dialog in non-e10s, so I don't think the two situations are really comparable...

Comment 8

[sworddragon2]

(In reply to :Gijs (gone until 3 jan) from comment #7)
> That would require the parent process knowing there are no beforeunload
> handlers. It doesn't right now, and it can't find out without talking to the
> child process, which in your hypothetical is somehow hung, so that doesn't
> work.

A potential solution could be that the content process tells the main process as soon as he is using a custom beforeunload handler and the main process keeps track of this information for all tabs.


(In reply to :Gijs (gone until 3 jan) from comment #7)
> > (In reply to :Gijs (gone 24 dec - 3 jan) from comment #1)
> > > This is the case even in e10s mode, though at least in e10s there are
> > > limits to the time we'll wait for an 'answer'.
> > 
> > There is a timeout forcing the tab/page getting closed even if it shows a
> > message from a beforeunload event? 
> 
> No. There is a timeout for the actual sending of messages from the parent to
> the child process to prompt for beforeunload. If we don't hear *anything*
> from the child process (because it's hung on another tab or whatever), we
> will time out. If the dialog shows up successfully and we tell the parent
> about it, of course we don't time out - that would stop the user interacting
> with the prompt after the website has explicitly said the user might not
> want to close it...

Thanks for the explanation and I was able to reproduce this. After 5 seconds the tab will simply be closed but the code execution from the tab (for example an infinite loop) will not be stopped as after dom.max_script_run_time seconds the "A web page is slowing down your browser. What would you like to do?" dialog still appears and any content process is unuseable as long as I don't stop the closed tab.

Also I'm not sure what I should think about the tab autoclosing after 5 seconds. Maybe it should have been bound to dom.max_script_run_time and showing a custom dialog like "A tab does not respond to your closing request. What would you like to do?". But the idea I have mentioned at the top might be better from the view of this feature request ticket. Also while the mentioned idea could increase the code complexity maybe a bit it would eventually reduce the code complexity for the timeout code as we would not to have to talk and wait for a response from the content process of the tab.


(In reply to :Gijs (gone until 3 jan) from comment #7)
> > (In reply to :Gijs (gone 24 dec - 3 jan) from comment #3)
> > > In non-e10s we would simply wait ~forever, so the current state is already
> > > better.
> > 
> > But without e10s the user is aware that the tab is being closed as he got
> > feedback due to the freeze while this is not the case with e10s - so
> > feedback is needed here (like the suggestion from Yang).
> 
> I don't understand this point. If my browser is frozen in non-e10s, I often
> don't even get visual feedback on clicking the [x] on a tab (or sometimes I
> do, but the tab doesn't close). How would "the browser is frozen" ever be
> 'feedback' ?

To explain this and finding a solution I should explain my use case a bit more: With e10s if I hit the x on a tab to close it it is not guaranteed that it does get immediately closed. This is also the case with non-e10s but while Firefox would now freeze but give immediate feedback by either closing the tab or showing a dialog on unfreezing I'm able with e10s to continue to interact with Firefox like scrolling down in the current non-closed tab to continue reading. Now I might lose track of the state of the tab especially if I have several tabs opened and if the tab was between tabs of the same domain (for example if I have several Twitch or YouTube tabs opened). If I want now to move the mouse from the current document to the tab bar to interact with the tabs it is possible that I'm getting clickjacked from Firefox causing unexpected behavior (like closing a tab by accident). This is not possible in non-e10s because I'm noticing that Firefox got frozen and of course I'm then not doing clicks anymore that might end up in a race between the click and the unfreeze as I'm aware that this could do an action I do not want. This is why the browser freeze in non-e10s is the visual feedback here. And also I got not annoyed only once by this use case described here.


(In reply to :Gijs (gone until 3 jan) from comment #7)
> I already explained why Yang's suggestion seems unworkable to me. Do you
> have another suggestion for potential UI feedback?

I'm thinking if my mentioned idea at the top of this post would solve all cases:

- Since the main process already knows if there is a custom beforeunload handler even if the tab hangs we have not to talk to it to request this information and wait up to the current 5 seconds for a response.
- In case there is no beforeunload handler the tab could get immediately closed even if it hangs.
- In case it has a beforeunload handler either the user would see the dialog if the tab is also the visible one or I would currently assume that the font of the tab would get bold otherwise to inform the user that a dialog poped up (if I'm not wrong this happens with alert messages so I would assume that this is the case with any dialog type).
- But if the beforeunload handler needs some seconds to return to show the dialog I think this could be an issue that might need a solution.

Comment 9

[:Gijs (out for now; he/him)]

(Sorry for the delay, I was out and this needed a lengthy response. I hope the below is helpful.)

(In reply to sworddragon2 from comment #8)
> (In reply to :Gijs (gone until 3 jan) from comment #7)
> > That would require the parent process knowing there are no beforeunload
> > handlers. It doesn't right now, and it can't find out without talking to the
> > child process, which in your hypothetical is somehow hung, so that doesn't
> > work.
> 
> A potential solution could be that the content process tells the main
> process as soon as he is using a custom beforeunload handler and the main
> process keeps track of this information for all tabs.

This only helps for tabs without a beforeunload handler, so it doesn't solve the basic "the behaviour is not predictable" problem.

> (In reply to :Gijs (gone until 3 jan) from comment #7)
> > > (In reply to :Gijs (gone 24 dec - 3 jan) from comment #1)
> > > > This is the case even in e10s mode, though at least in e10s there are
> > > > limits to the time we'll wait for an 'answer'.
> > > 
> > > There is a timeout forcing the tab/page getting closed even if it shows a
> > > message from a beforeunload event? 
> > 
> > No. There is a timeout for the actual sending of messages from the parent to
> > the child process to prompt for beforeunload. If we don't hear *anything*
> > from the child process (because it's hung on another tab or whatever), we
> > will time out. If the dialog shows up successfully and we tell the parent
> > about it, of course we don't time out - that would stop the user interacting
> > with the prompt after the website has explicitly said the user might not
> > want to close it...
> 
> Thanks for the explanation and I was able to reproduce this. After 5 seconds
> the tab will simply be closed but the code execution from the tab (for
> example an infinite loop) will not be stopped as after
> dom.max_script_run_time seconds the "A web page is slowing down your
> browser. What would you like to do?" dialog still appears and any content
> process is unuseable as long as I don't stop the closed tab.

Can you file a separate bug with a testcase for this? I'm not sure if it's easily fixable, but in theory at the point where we stop because of the slow script dialog, we should be able to know that we're trying to kill the docshell and we should just be doing that instead, without prompting.


> Also I'm not sure what I should think about the tab autoclosing after 5
> seconds. Maybe it should have been bound to dom.max_script_run_time and
> showing a custom dialog like "A tab does not respond to your closing
> request. What would you like to do?".

The user clearly indicated that they wanted to close the tab by clicking the close button, and the lack of response might be caused by another tab, as multiple tabs could be running in the same content process. It's pretty difficult to come up with a better process here, as hangs will not normally be as 'permanent' as the infinite loop testcase that you're presumably using. There could be just about anything going on which limits the response time of the tab. Reliably and correctly telling the user that, and giving them an actual meaningful choice, is a difficult problem, and if they intentionally clicked the close button, closing the tab will be the right thing to do in most cases. Just punting and saying "hey, it looks like this part of the browser UI isn't working right now, sorry about that" isn't good UI/UX.

> But the idea I have mentioned at the
> top might be better from the view of this feature request ticket. Also while
> the mentioned idea could increase the code complexity maybe a bit it would
> eventually reduce the code complexity for the timeout code as we would not
> to have to talk and wait for a response from the content process of the tab.

Well no, because we still have to do the timeout dance if there is a beforeunload handler (which unfortunately is more common than one would like...). Having two codepaths instead of 1 is more complex.

> (In reply to :Gijs (gone until 3 jan) from comment #7)
> > > (In reply to :Gijs (gone 24 dec - 3 jan) from comment #3)
> > > > In non-e10s we would simply wait ~forever, so the current state is already
> > > > better.
> > > 
> > > But without e10s the user is aware that the tab is being closed as he got
> > > feedback due to the freeze while this is not the case with e10s - so
> > > feedback is needed here (like the suggestion from Yang).
> > 
> > I don't understand this point. If my browser is frozen in non-e10s, I often
> > don't even get visual feedback on clicking the [x] on a tab (or sometimes I
> > do, but the tab doesn't close). How would "the browser is frozen" ever be
> > 'feedback' ?
> 
> To explain this and finding a solution I should explain my use case a bit
> more: With e10s if I hit the x on a tab to close it it is not guaranteed
> that it does get immediately closed. This is also the case with non-e10s but
> while Firefox would now freeze but give immediate feedback by either closing
> the tab or showing a dialog on unfreezing I'm able with e10s to continue to
> interact with Firefox like scrolling down in the current non-closed tab to
> continue reading. Now I might lose track of the state of the tab especially
> if I have several tabs opened and if the tab was between tabs of the same
> domain (for example if I have several Twitch or YouTube tabs opened). If I
> want now to move the mouse from the current document to the tab bar to
> interact with the tabs it is possible that I'm getting clickjacked from
> Firefox causing unexpected behavior (like closing a tab by accident). This
> is not possible in non-e10s because I'm noticing that Firefox got frozen and
> of course I'm then not doing clicks anymore that might end up in a race
> between the click and the unfreeze as I'm aware that this could do an action
> I do not want. This is why the browser freeze in non-e10s is the visual
> feedback here. And also I got not annoyed only once by this use case
> described here.

OK, I have morphed this bug to provide UI feedback for this case, which I think is more feasible than "close me immediately", given that that won't always solve the problem - and reopened the bug, of course.

If you feel strongly, file a separate bug to request propagating beforeunload state to the parent process, and CC me and :billm. It's true that that may improve the experience in some cases. I'm just not entirely convinced it's worth the complexity. I know for a fact that trackers/ads are likely to register unload handlers, and maybe beforeunload handlers, to do tracking, and we won't be able to know whether a beforeunload handler is going to return a result that causes the dialog to come up without solving the halting problem. If the ad case is very prevalent, which is possible, it may well end up being a lot of work for very little benefit - of course, the relative merit will always depend on what sites users browse, and I could be wrong about how prevalent this abuse of the beforeunload mechanism is... The complicated thing will be that there can be e.g. frames in the page that have a beforeunload handler (or several) and so after each subframe unload or addeventlistener call, we'd have to re-count the number of listeners...

> (In reply to :Gijs (gone until 3 jan) from comment #7)
> > I already explained why Yang's suggestion seems unworkable to me. Do you
> > have another suggestion for potential UI feedback?
> 
> I'm thinking if my mentioned idea at the top of this post would solve all
> cases:
> 
> - Since the main process already knows if there is a custom beforeunload
> handler even if the tab hangs we have not to talk to it to request this
> information and wait up to the current 5 seconds for a response.
> - In case there is no beforeunload handler the tab could get immediately
> closed even if it hangs.
> - In case it has a beforeunload handler either the user would see the dialog
> if the tab is also the visible one or I would currently assume that the font
> of the tab would get bold otherwise to inform the user that a dialog poped
> up (if I'm not wrong this happens with alert messages so I would assume that
> this is the case with any dialog type).

Almost. We always switch to the tab in the beforeunload case (unlike alert()) because of... reasons. See bug 1234936 and others.

I don't think we should add a dialog basically asking the user "something is wrong, would you like us to do what you already told us to do? [yes] [no]", which is what I think is what your suggestion unfortunately devolves into. If the tab is not responding, in a sense, that's the browser's fault, and we should be dealing with it rather than delegating to the user.

> - But if the beforeunload handler needs some seconds to return to show the
> dialog I think this could be an issue that might need a solution.

Right, so, we call (all) the beforeunload handler(s) and if any of them return a non-falsy value, we show a dialog, to which the user can then respond (or not). That process is synchronous in the child process, although it calls into web content and so web content can do stupid evil things like firing off synchronous XHR requests. So we can't block on it, we have to wait to be told whether to show the dialog, which we then do in the parent, so from the parent's perspective the whole process is async.


Can you clarify if the above makes sense? If so we probably need to get some UX ideas about how to signal that the tab is waiting to close. Maybe just greying it out would work, or strike-through through the tab title or something, but I guess UX might have better ideas?

Comment 10

[sworddragon2]

(In reply to :Gijs from comment #9)
> Can you file a separate bug with a testcase for this? I'm not sure if it's
> easily fixable, but in theory at the point where we stop because of the slow
> script dialog, we should be able to know that we're trying to kill the
> docshell and we should just be doing that instead, without prompting.

It is now reported at bug #1328264 .


(In reply to :Gijs from comment #9)
> Right, so, we call (all) the beforeunload handler(s) and if any of them
> return a non-falsy value, we show a dialog, to which the user can then
> respond (or not). That process is synchronous in the child process, although
> it calls into web content and so web content can do stupid evil things like
> firing off synchronous XHR requests. So we can't block on it, we have to
> wait to be told whether to show the dialog, which we then do in the parent,
> so from the parent's perspective the whole process is async.
> 
> 
> Can you clarify if the above makes sense? If so we probably need to get some
> UX ideas about how to signal that the tab is waiting to close. Maybe just
> greying it out would work, or strike-through through the tab title or
> something, but I guess UX might have better ideas?

So in the worst case it could need up to 5 seconds until the site responds to have a beforeunload handler and then it could need up to dom.max_script_run_time (default 10) seconds until the site shows the dialog. That the tab can close in an unpretictable way during this long time can cause the race I have mentioned in the use case so signaling that the tab might close would be indeed useful here. I would say that we could replace the x on the tab during the "shutdown" phase with a spinning circle but this would be probably confusing with the loading circle on the left of the tab. Another idea could be to fade out the tab icon and text during the first 2-3 seconds (with maybe a small starting delay of ~100ms) up to maybe 50% transparency and stop there in case the site needs longer to respond as I think that a transparency effect here is a very obvious hint to the user that the tab is going about to vanish. In case the site should then show the dialog from the beforeunload handler the transparency effect could then be removed.

Comment 11

[:Gijs (out for now; he/him)]

(In reply to sworddragon2 from comment #10)
> (In reply to :Gijs from comment #9)
> > Can you clarify if the above makes sense? If so we probably need to get some
> > UX ideas about how to signal that the tab is waiting to close. Maybe just
> > greying it out would work, or strike-through through the tab title or
> > something, but I guess UX might have better ideas?
> 
> So in the worst case it could need up to 5 seconds until the site responds
> to have a beforeunload handler and then it could need up to
> dom.max_script_run_time (default 10) seconds until the site shows the
> dialog. That the tab can close in an unpretictable way during this long time
> can cause the race I have mentioned in the use case so signaling that the
> tab might close would be indeed useful here. I would say that we could
> replace the x on the tab during the "shutdown" phase with a spinning circle
> but this would be probably confusing with the loading circle on the left of
> the tab. Another idea could be to fade out the tab icon and text during the
> first 2-3 seconds (with maybe a small starting delay of ~100ms) up to maybe
> 50% transparency and stop there in case the site needs longer to respond as
> I think that a transparency effect here is a very obvious hint to the user
> that the tab is going about to vanish. In case the site should then show the
> dialog from the beforeunload handler the transparency effect could then be
> removed.

Stephen, thoughts about what kind of UI we could use for "tabs that are about to close as soon as we hear from them"? :-)

Comment 12

[sworddragon2]

While the feedback signals the user that the tab might close soon I'm seeing a minor issue: The user is now required to not close/click any tab right of it or otherwise he might click at the same time the tab closes (as it is not known to the user at which exact time the "fading away" tab might close). Eventually this would require that tabs right of the semi-autoclosed tab would not automatically fill up if the mouse is still hovering over the tab bar (with maybe exceptions to do this on safe interactions like closing/clicking or sorting tabs left of the gap).

Comment 13

[:Gijs (out for now; he/him)]

I think given bug 1336763 we shouldn't need to do anything else here.