526212 - Would be great to restart firefox after update by third-party installer (like yum/rpm)

Status: RESOLVED INCOMPLETE

(Firefox :: Shell Integration, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

Attachment: WIP

Firefox has to be restarted after update. Unfortunately we can't restart a running instance from the update software (like yum/rpm) so users experience weird application behavior after the update.

Downstream bug - https://bugzilla.redhat.com/show_bug.cgi?id=489306

A solution could be to restart firefox remotely by mozilla-xremote-client, like 
mozilla-xremote-client -a firefox 'restart()' - it's implemented in an attached patch. We may add some confirmation dialog here like or so...

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: adds xfeDoCommand(restartUpdate) command

with this patch, firefox can be restarted by simple command like:

mozilla-xremote-client -a firefox 'xfeDoCommand(restartUpdate)'

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 418838 [details] [diff] [review]
adds xfeDoCommand(restartUpdate) command

Can you please check this one?

Comment 3

[Mike Hommey [:glandium]]

The problem I can see is that there are chances that the locale file fails to be read properly because of the upgrade, leading to an unreadable message. Likewise, are we sure all the components used are going to work as expected after the upgrade (depending on where they use file from).

Comment 4

[Mike Hommey [:glandium]]

Another thought: this also means the upgrade process has to go through all running mozilla instances and trigger the restart for those that are impacted, which will probably be painful to implement. A solution that involves a component checking itself whether the application or the gre directory have been changed would probably be better, though polling is not exactly nice.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Thanks for the comments. 

As for the running instances - I have a version which uses an extension and the restart command is send via. D-bus. But I think this one is much smaller change and have a better chance to be accepted. 

The location of running application is not so big problem because we care only about instances which is installed by rpm and they are placed in one fixed location (like /usr/lib/firefox).

Comment 6

[Mike Hommey [:glandium]]

(In reply to comment #5)
> The location of running application is not so big problem because we care only
> about instances which is installed by rpm and they are placed in one fixed
> location (like /usr/lib/firefox).

The problem I that you will have to
- find all corresponding firefox instances and only them
- run each x-remote client with the appropriate user and the appropriate command line
This will make the upgrade script in your rpm package bloaty.

You will also hit the fact that you can only run x-remote on a single instance for any given user, so if users are running 2 instances with a different profile, you can't get them to restart it.

On the other hand, if you had firefox polling the update of a given file (.autoreg, maybe ?), any instance could be handled. On Linux, inotify could probably be used ; or for compatibility with other platforms (in Debian we would interested in solutions that would also work on freebsd kernels, that don't have inotify but kqueue), one could think of FAM or something similar. Just my ¢2

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 418838 [details] [diff] [review]
adds xfeDoCommand(restartUpdate) command

Hmm, I'm not a good reviewer for this -- you probably want bsmedberg?

Comment 8

[Martin Stránský [:stransky] (ni? me)]

> On the other hand, if you had firefox polling the update of a given file
> (.autoreg, maybe ?), any instance could be handled. On Linux, inotify could
> probably be used ; or for compatibility with other platforms (in Debian we
> would interested in solutions that would also work on freebsd kernels, that
> don't have inotify but kqueue), one could think of FAM or something similar.
> Just my ¢2

Unfortunately, the inotify solutions requires instant pooling/reading changes from an inotify device. I don't think we want to instantly read a directory status because of monthly update.

Comment 9

[Mike Hommey [:glandium]]

> Unfortunately, the inotify solutions requires instant pooling/reading changes
> from an inotify device. I don't think we want to instantly read a directory
> status because of monthly update.

Isn't it exactly what we want ? to have notification about the upgrade so that the user can be notified before their browser collapses ?

Comment 10

[Martin Stránský [:stransky] (ni? me)]

I think the problem has two parts:

- former - how to delivery an update message to firefox? (x-remote, dbus, inotify, something else).

- the later - how to handle it? (restart browser after the update, wait to users, just raise some dialog box...)

We can notify users about the upcoming update, but there are some scenarios:

- we'll wait until all users finish and then update the browser - it's bad because one user can block whole update, and I'm not sure it's even possible to hold the update by rpm script.

- users will be notified before the update - not sure what happens if they restart it during the update when the browser is in an inconsistent state.

- users will be notified after the update - should be safe (firefox is completely updated) but the restart may not be successful.

- we will kill all running browser instances (before update) and restart it (after the update). Should be safe if we manage to quit all instances but does not look very user friendly.

btw. the browser does not collapses right after the update because it still lives in memory, but it slowly dies when you try to open a new window instance or reload some existing pages :)

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Okay, let's try another solution. I tested how Ubuntu handles it, they have a special extension which periodically checks some file and throw the restart dialog  if the file changes. I'm going to make such extension which is more universal and suitable for other distributions.

Comment 12

[Mike Hommey [:glandium]]

I took a quick look at the Ubuntu extension, and have a few comments:
- It monitors files that are unrelated to firefox itself, while it is really not necessary: installing a new package *will* modify either the application.ini file in the app directory or the platform.ini file in the GRE directory (or both), which means there is only a need to monitor these, not some special file.
- It seems to fire a notification. I'm unsure this is the proper way to notify users, especially considering firefox is most likely going to break badly in a near future. I think a popup would be better, possibly asking the user if she wants to delay the restart.

Also, as I really don't like polling, I'm starting to implement a file monitoring (xpcom) service, based on GIO's GFileMonitor. It could be extended to support the OSX and Windows equivalent. If more people are interested in such a service, maybe I should file a separate bug and we should find some common ground for a "proper" interface.

Comment 13

[Martin Stránský [:stransky] (ni? me)]

(In reply to comment #12)
> I took a quick look at the Ubuntu extension, and have a few comments:
> - It monitors files that are unrelated to firefox itself, while it is really
> not necessary: installing a new package *will* modify either the
> application.ini file in the app directory or the platform.ini file in the GRE
> directory (or both), which means there is only a need to monitor these, not
> some special file.

I'm going to handle it by some prefs value, like "extensions.updatecheck.check_file". 

It allows distributors to configure it as they need, because it depends how firefox is installed on your system (xulrunner/firefox separation, /usr/lib/firefox vs. /usr/lib/firefox-VERSION and so on). 

And it should catch a situation when only one package is updated (xulrunner or firefox).

> - It seems to fire a notification. I'm unsure this is the proper way to notify
> users, especially considering firefox is most likely going to break badly in a
> near future. I think a popup would be better, possibly asking the user if she
> wants to delay the restart.

Yeah, definitely a popup window.

Comment 14

[Mike Hommey [:glandium]]

(In reply to comment #13)
> I'm going to handle it by some prefs value, like
> "extensions.updatecheck.check_file". 
> 
> It allows distributors to configure it as they need, because it depends how
> firefox is installed on your system (xulrunner/firefox separation,
> /usr/lib/firefox vs. /usr/lib/firefox-VERSION and so on). 

You already know that location with the directory service.

> And it should catch a situation when only one package is updated (xulrunner or
> firefox).

You can do that with platform.ini which is in xulrunner and application.ini which is in firefox.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

(In reply to comment #14)
> You can do that with platform.ini which is in xulrunner and application.ini
> which is in firefox.

platform.ini and application.ini can catch firefox/xulrunner update but not new/removed extensions. I'd like to create it as universal as possible.

Comment 16

[Mike Hommey [:glandium]]

> platform.ini and application.ini can catch firefox/xulrunner update but not
> new/removed extensions. I'd like to create it as universal as possible.

You can also enumerate extensions directories with the extensions manager. Likewise for plugins with the plugin host.

But with so many directories and files to monitor, you really ought to be using a notification api instead of polling. As I said in comment #12, I'm already working on that.

Comment 17

[Martin Stránský [:stransky] (ni? me)]

Attachment: updatecheck extension

There is the watch extension here. It watches given files and notify user if any of them is changed.

Comment 18

[Martin Stránský [:stransky] (ni? me)]

(In reply to comment #16)
> But with so many directories and files to monitor, you really ought to be using
> a notification api instead of polling. As I said in comment #12, I'm already
> working on that.

I'll update it when your API is done.

Comment 19

[Benjamin Smedberg]

Ideally, we would shut down Firefox *before* the update happens, because changing files while an update is happening is a common cause of instability.

Do you really want this in our tree? It is not something we'd want in the mozilla.org builds, which don't update while the app is running, and it's not something we'd want as a user-visible extension in any case. I think that this functionality would be better implemented as a single JS component, if it's needed at all.

Comment 20

[Mike Hommey [:glandium]]

(In reply to comment #19)
> Ideally, we would shut down Firefox *before* the update happens

Well, that is unfortunately not possible, except if we were to force such a shutdown. That would mean possibly killing Firefox under the user's fingers. 
That's definitely a no-go. But...

>, because
> changing files while an update is happening is a common cause of instability.

Package managers (at least, dpkg), don't change files. It replaces them with new ones. This means that the old files are still on the filesystem, and kept as long as they are still open by a process. This also means that all binary files are safe. The only real problem we are having with upgrades is when loading chrome that hadn't been loaded already.

> Do you really want this in our tree? It is not something we'd want in the
> mozilla.org builds, which don't update while the app is running, and it's not
> something we'd want as a user-visible extension in any case. I think that this
> functionality would be better implemented as a single JS component, if it's
> needed at all.

I, for one, don't plan to put that in an extension. As you say, this is too visible, and we don't want 1. to expose it, 2. to allow users to disable it.
Anyways, maybe the file monitoring part I'm currently working on (but which is still GIO-centric) could be of interest to others. That could be put in extensions/ and enableable at build time. It would need to be made cross-platform, though.

Comment 21

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

Not a shell integration bug and no activity in years so resolving -> incomplete. If you still think this bug is applicable please file a toolkit -> startup and profile system since that handles the code mentioned in this bug.