1277747 - Firefox freezes before restarting when I clicked the [Restart to Update...] button in about dialog

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

On the Aurora channel (which updates daily), Firefox takes sometime to close up for update. That itself is a problem, but per comment here [1] we are supposedly to open up the updater.xul dialog and show an "Updating..." label before attempting to close the program?

[1] https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/browser/base/content/aboutDialog-appUpdater.js#248-249

I blamed the code and found bug 596813, but I couldn't find a design spec related to the behavior above. Robert, do you remember what this is supposed to happen here? It's pretty old, sorry about that.

I should probably compare the behavior on Windows too. Let me check...

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I've tried to test Aurora on Windows, but since it's a new profile, it did not freeze so I couldn't see if it has opened a new window or not.

Comment 2

[Robert Strong (they/them - no direct email)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on leave until July) from comment #0)
> On the Aurora channel (which updates daily), Firefox takes sometime to close
> up for update. That itself is a problem, but per comment here [1] we are
> supposedly to open up the updater.xul dialog and show an "Updating..." label
> before attempting to close the program?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 4d63dde701b47b8661ab7990f197b6b60e543839/browser/base/content/aboutDialog-
> appUpdater.js#248-249
> 
> I blamed the code and found bug 596813, but I couldn't find a design spec
> related to the behavior above. Robert, do you remember what this is supposed
> to happen here? It's pretty old, sorry about that.
Clicking restart is supposed to restart the application after which the updater application (it is native and not xul). This sounds like a restart bug and not an updater bug. What happens when you install an add-on that requires a restart and you restart?

Comment 3

[Robert Strong (they/them - no direct email)]

spohl, could you help figure this out with Tim? Keep in mind it might be the restart code.

Comment 4

[Robert Strong (they/them - no direct email)]

Definitely not bug 596813 which landed well over 5 years ago. :)

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #2)
> Clicking restart is supposed to restart the application after which the
> updater application (it is native and not xul). This sounds like a restart
> bug and not an updater bug. What happens when you install an add-on that
> requires a restart and you restart?

I tried this by disabling one of the add-ons. Nothing happened for ~500ms after I clicked [Restart Now] and the browser window closes. Arguable not a good experience either.

What I am trying to solve here is to improve the UX of Firefox, following the principle of "user must receives a feedback within 100ms of an action". In the updater case, Firefox indeed freezes. If that's expected, we should at least replace the button with something like "Applying update..." label for the user.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

... so, why did the code openWindow the updater.xul there but the dialog never show on either Windows nor Linux? If the dialog is shown before Firefox restarts, we would have satisfied the mentioned UX requirement...

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

s/Linux/Mac/

Comment 8

[Robert Strong (they/them - no direct email)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on leave until July) from comment #6)
> ... so, why did the code openWindow the updater.xul there but the dialog
> never show on either Windows nor Linux? If the dialog is shown before
> Firefox restarts, we would have satisfied the mentioned UX requirement...
There is no updater.xul

http://mxr.mozilla.org/mozilla-central/find?string=updater.xul

There is a timeout after Firefox finishes closing and after the updater binary is launched before the updater binary displays a user interface. The updater binary is native code and not xul.

It can't be shown since it is intentionally launched by Firefox extremely early during startup which then quickly exits. This way if you exit Firefox with an update ready to apply it will apply on next launch and if you restart Firefox it does the same except that the restart code (not app update) does the restart itself. Note that when the restart happens the Firefox user interface is not displayed. So, the problem here based on what you have described is entirely outside of app update. Firefox could display something during a restart but it would be better to understand why your system takes so long to shutdown first. If the slow shutdown can't be avoided then you might consider adding a Firefox is restarting message for restarts, change the button text in the about dialog (not app update code), or perhaps something else.

Comment 9

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on
> leave until July) from comment #6)
> > ... so, why did the code openWindow the updater.xul there but the dialog
> > never show on either Windows nor Linux? If the dialog is shown before
> > Firefox restarts, we would have satisfied the mentioned UX requirement...
> There is no updater.xul
> 
> http://mxr.mozilla.org/mozilla-central/find?string=updater.xul
> 
> There is a timeout after Firefox finishes closing and after the updater
> binary is launched before the updater binary displays a user interface. The
> updater binary is native code and not xul.
Forgot to mention that this is likely why you didn't see the updater binary user interface on Linux or Windows.

> 
> It can't be shown since it is intentionally launched by Firefox extremely
> early during startup which then quickly exits. This way if you exit Firefox
> with an update ready to apply it will apply on next launch and if you
> restart Firefox it does the same except that the restart code (not app
> update) does the restart itself. Note that when the restart happens the
> Firefox user interface is not displayed. So, the problem here based on what
> you have described is entirely outside of app update. Firefox could display
> something during a restart but it would be better to understand why your
> system takes so long to shutdown first. If the slow shutdown can't be
> avoided then you might consider adding a Firefox is restarting message for
> restarts, change the button text in the about dialog (not app update code),
> or perhaps something else.

Comment 10

[Robert Strong (they/them - no direct email)]

FYI though I'm not going to paste links to all of the startup or other code

Code that launches the updater when Firefox starts
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsUpdateDriver.cpp

Code that is called when the restart button in the about dialog is clicked
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutDialog-appUpdater.js#209

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #10)
> Code that is called when the restart button in the about dialog is clicked
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> aboutDialog-appUpdater.js#209

I see. I was looking at the wrong function here:

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on leave until July) from comment #0)
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 4d63dde701b47b8661ab7990f197b6b60e543839/browser/base/content/aboutDialog-
> appUpdater.js#248-249

and I assume updates.xul (not *updater.xul*) will be opened at the restart stage :-/ ... So yeah, what I am looking for is simply update the button text when the user hits restart...

Sorry about the confusion!

Comment 12

[Robert Strong (they/them - no direct email)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on leave until July) from comment #11)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #10)
> > Code that is called when the restart button in the about dialog is clicked
> > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> > aboutDialog-appUpdater.js#209
> 
> I see. I was looking at the wrong function here:
> 
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on
> leave until July) from comment #0)
> > [1]
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 4d63dde701b47b8661ab7990f197b6b60e543839/browser/base/content/aboutDialog-
> > appUpdater.js#248-249
> 
> and I assume updates.xul (not *updater.xul*) will be opened at the restart
> stage :-/ ... So yeah, what I am looking for is simply update the button
> text when the user hits restart...
> 
> Sorry about the confusion!
You are still confused and making incorrect assumptions.

updates.xul is only displayed when we have to display a billboard, license (recently removed), or add-on incompatibilities (recently removed). It is never displayed when you click restart and clicking restart does exactly that... restart. As I have also stated the user interface displayed by app update is not xul and it is native code.

I know you really want to change the text and I am fine with the text changing in the Firefox about dialog but you really should file a bug about Firefox taking so long to restart that it appears to freeze. I know you think the solution is to just change the text but Firefox should not be taking that long to restart. Without that bug as well I will push back on a bug to just change the text.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #12)
> I know you really want to change the text and I am fine with the text
> changing in the Firefox about dialog but you really should file a bug about
> Firefox taking so long to restart that it appears to freeze. I know you
> think the solution is to just change the text but Firefox should not be
> taking that long to restart. Without that bug as well I will push back on a
> bug to just change the text.

Agreed, filed bug1277761. Will proposed a patch on label here.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1277747 - Show "Restarting..." label when we restart from about dialog updater,

Review commit: https://reviewboard.mozilla.org/r/57530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57530/

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 8759525 [details]
Bug 1277747 - Show "Restarting..." label when we restart from about dialog updater,

Tim, it would be better to get one of the Firefox front end developers to review this since I am not terribly familiar with panels, etc.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8759525 [details]
Bug 1277747 - Show "Restarting..." label when we restart from about dialog updater,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57530/diff/1-2/

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8759525 [details]
Bug 1277747 - Show "Restarting..." label when we restart from about dialog updater,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57530/diff/2-3/

Comment 18

[Mike de Boer [:mikedeboer]]

Tim, how did you test this?

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

I can see the same shutdown lag in between the update on my Macbook while running our Firefox ui update tests. It takes up to 25s before Firefox restarts. What I noticed is that I do not see an "Applying update" label anymore. Is that something we got rid of?

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Henrik Skupin (:whimboo) from comment #19)
> I can see the same shutdown lag in between the update on my Macbook while
> running our Firefox ui update tests. It takes up to 25s before Firefox
> restarts. What I noticed is that I do not see an "Applying update" label
> anymore. Is that something we got rid of?

It's still in the code, although I don't think I have seem it. According to the code it should show briefly after the update has downloaded and before we show the restart button.

The id of the hbox containing the restart button is "apply", which is a bit confusing :-/, if I read the code correctly, the naming should be

The "Applying update..." label: id=applying -> id=extracting/verifying (as in extracting/verifying download)
The restart button: id=apply -> id=restart
(and I added "restarting" in this patch)

(In reply to Mike de Boer [:mikedeboer] from comment #18)
> Tim, how did you test this?

It's not tested :-/ I was reluctant to spend more time one this. Do you know how I should test this?

Comment 21

[Mike de Boer [:mikedeboer]]

I don't have a clue, TBH. That's why I asked :-P
Alright, let's ask Robert if he knows of something we can play with in the Browser Console. Thing is, I'm reluctant to rubberstamp something that is part of quite an important flow.

Comment 22

[Robert Strong (they/them - no direct email)]

I don't know much about the current about dialog code but from code inspection you should be able to change the following and compile to test it.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutDialog-appUpdater.js#62

//   if (this.isPending || this.isApplied) {
       this.selectPanel("apply");
       return;
//   }

This will put it into the state where the about dialog thinks there is an update ready to apply so it will display the restart button which when clicked will run the restart code.

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Here is my manual test:

1. Open chrome://browser/content/aboutDialog.xul
2. Open DevTools console
3. `gAppUpdater.selectPanel('apply'); Object.defineProperties(gAppUpdater, { "isPending": { value: true }, "isApplied": { value: true }})`
5. Click on [Restart Nightly to Update] button

Expected:

1. Nightly restarts

Mike, how about this?

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on leave until July) from comment #23)
> 1. Open chrome://browser/content/aboutDialog.xul

Please load this page in a new window because this tab will resize the window.

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8759525 [details]
Bug 1277747 - Show "Restarting..." label when we restart from about dialog updater,

https://reviewboard.mozilla.org/r/57530/#review54770

LGTM! Thanks for the good STR, Tim!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/878ecb7bb6f9
Show "Restarting..." label when we restart from about dialog updater, r=mikedeboer

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/878ecb7bb6f9

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/878ecb7bb6f9

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8759525 [details]
Bug 1277747 - Show "Restarting..." label when we restart from about dialog updater,

I would love to get this small change landed into Aurora, but there is a new string there ...

Approval Request Comment
[Feature/regressing bug #]: feature
[User impact if declined]: see bug, confusing interaction when the user clicks [Restart to Update]
[Describe test coverage new/current, TreeHerder]: manually tested and has lived on Nightly for a few days.
[Risks and why]: very small change. should not cause problem since Nightly user updates normally.
[String/UUID change made/needed]: New string added. Not sure if it is ok at this point?

Comment 30

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

flod, new string in this patch, is this ok with you to change in aurora? I have hopes we can fix the underlying issue, but until we can, this may help to correctly convey what's happening to users.

Comment 31

[Francesco Lodolo [:flod]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #30)
> flod, new string in this patch, is this ok with you to change in aurora? I
> have hopes we can fix the underlying issue, but until we can, this may help
> to correctly convey what's happening to users.

To be honest I'm quite reluctant to accept any string change at this point in the cycle. My suggestion would be to let it ride the trains (I also checked and we don't have any similar reusable string in the tree).

If I'm reading this patch correctly, it will display the string to all users updating from the About dialog, it will just be a very short moment for most users. If that's the case, one more good reason to not uplift or hardcode the label in mozilla-aurora, given the potential high visibility.

P.S. I'm on PTO this week, if there's something urgent that can't wait next week, please direct the request to Pike (sorry).

Comment 32

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

That sounds reasonable. Thanks flod.  
rstrong or tim, do you have any sense of how common this is for users to encounter? Are you ok with this change staying on 50? Maybe we can fix the underlying hang issue, instead.

Comment 33

[Axel Hecht [:Pike]]

Note, I'm pretty sure that the problems with shutdown timing are e10s-related. There's been a couple of bugs on nightly around that, depending largely on profile size etc.

To which extent they actually affect the population of 49 by the time it hits release is TBD, and I strongly hope that we'll have our shutdown code paths lined up by then.

Comment 34

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Tim, can you take flod's comment 31 into consideration and rewrite your patch so as not to break the string freeze?

Comment 35

[Francesco Lodolo [:flod]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #34)
> Tim, can you take flod's comment 31 into consideration and rewrite your
> patch so as not to break the string freeze?

I'm not sure it's possible in this case, and before hardcoding the message I would really like a clarification on the visibility of this string, and how many users will see it.

Comment 36

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Francesco Lodolo [:flod] from comment #35)
> I'm not sure it's possible in this case, and before hardcoding the message I
> would really like a clarification on the visibility of this string, and how
> many users will see it.

Let's not hardcoding this message. I was simply looking for an opinion on whether or not we should uplift this. Let me retract my uplift request.

Comment 37

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

I don't see any duplicates of this issue, so let's just call this fix-optional for 49 and move on, now that it's fixed in 50.