765380 - Do not remove a running application

Status: RESOLVED INVALID

(Firefox Graveyard :: Web Apps, defect, P2)

Description

[Marco Castelluccio [:marco]]

We shouldn't remove running applications, this could cause some problems.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Lock webapps during installation

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 633693 [details] [diff] [review]
Lock webapps during installation

This seems like an improvement over the current behavior, but it also feels incomplete, since it prevents the installer from trying to reinstall on top of a running application, but it doesn't provide feedback to the user about why the installation failed or help the user reinstall the app.

When a page tries to install an app, we should check to see if the app is already installed; and if it is, we should prompt the user to reinstall it, not just to install it.

And if the app is also running, we should tell the user they need to quit the app to reinstall it, prompt them to do so, and then do it for them if they so indicate.

So the doorhanger for "installed but not running" should say:

    'App Name' is already installed on your computer.  Would you like to
    reinstall it?

                                                             [Reinstall]

And the doorhanger for "installed and running" should say:

    'App Name' is already installed and is running on your computer.  Would
    you like to reinstall it?  You have to quit 'App Name' to reinstall it.

                                                         [Quit & Reinstall]

However, we can do this work in followup bugs.


>+      throw "The application is running, you should close it before re-installation.";

Since this message only appears in the error console, it isn't particularly obvious to users, so it doesn't seem worth making it user-friendly, especially not once we start checking to see if the app is installed/running before prompting the user to install it, and especially at the cost of displaying the original exception's message, which might contain useful information for developers (like the path of the profile that could not be locked).

At the very least, you should append the original exception to the error message you throw:

      throw "The application is running; you should close it before re-installation: " + e;

But I would just let the original exception propagate instead of catching it and throwing a new one.

Comment 3

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #2) 
> When a page tries to install an app, we should check to see if the app is
> already installed; and if it is, we should prompt the user to reinstall it,
> not just to install it.

There's bug 710062.

> And if the app is also running, we should tell the user they need to quit
> the app to reinstall it, prompt them to do so, and then do it for them if
> they so indicate.

Felipe's opinion was to show the error in the marketplace and not directly in the Firefox's UI. Do you agree?
 
> But I would just let the original exception propagate instead of catching it
> and throwing a new one.

Ok.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to Marco Castelluccio from comment #3)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> > And if the app is also running, we should tell the user they need to quit
> > the app to reinstall it, prompt them to do so, and then do it for them if
> > they so indicate.
> 
> Felipe's opinion was to show the error in the marketplace and not directly
> in the Firefox's UI. Do you agree?

That's better than the current behavior, but it isn't as good as it could be, since Firefox can actually quit the app on the user's behalf, while Marketplace can't.  So Marketplace can prompt the user to quit the app, but the user will have to do that themself and then retrigger installation, while Firefox can display a message like this:

    'App Name' is already installed and is running on your computer.  Would
    you like to reinstall it?  You have to quit 'App Name' to reinstall it.

                                                         [Quit & Reinstall]

And then the user only has to click a single button to quit the app and reinstall it.

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Lock webapps during installation

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 636522 [details] [diff] [review]
Lock webapps during installation

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

Note: I'm not a Firefox reviewer, nor have review responsibilities been delegated to me for browser/ (like they have for webapprt/), so I can't review this code, although I can give feedback.  f=myk!  But requesting actual review from Felipe.

::: browser/modules/WebappsInstaller.jsm
@@ +938,5 @@
>               .replace(/</g, "&lt;")
>               .replace(/>/g, "&gt;");
>  }
>  
> +function lockWebAppProfile(profileParentDir) {

Nit: "webapp" is used as a single compound word in this file and elsewhere in the codebase (with a few exceptions), so the "a" in it should not be capitalized when it is used in interCaps-style names, i.e. this should be called "lockWebappProfile".

@@ +945,5 @@
> +
> +  if (profilesINI.exists()) {
> +    let reader = Cc["@mozilla.org/xpcom/ini-processor-factory;1"]
> +                   .getService(Ci.nsIINIParserFactory).createINIParser(profilesINI);
> +    let profileRelativeDir = reader.getString("Profile0", "Path");

The runtime doesn't provide a mechanism for using multiple profiles, so this seems fine, but the platform does support using multiple profiles, so it would be worth noting that this assumes a single profile.

@@ +955,5 @@
> +                    .getService(Ci.nsIToolkitProfileService);
> +    return profSvc.lockProfilePath(profileDir, profileDir);
> +  }
> +
> +  return false;

It'd be better to return `null` in this case, since the success case returns an object rather than a boolean.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Lock webapps during installation

(In reply to Myk Melez [:myk] [@mykmelez] from comment #6)
> The runtime doesn't provide a mechanism for using multiple profiles, so this
> seems fine, but the platform does support using multiple profiles, so it
> would be worth noting that this assumes a single profile.

Sorry, do you mean I should add a comment? Or do you mean I should read profiles.ini like the profile service does?
In the second case, we should modify the profile service API in order not to duplicate code.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to Marco Castelluccio from comment #7)
> Sorry, do you mean I should add a comment? Or do you mean I should read
> profiles.ini like the profile service does?
> In the second case, we should modify the profile service API in order not to
> duplicate code.

I just meant to add a comment, so it's more obvious to others down the road that this needs updating if we ever add support for multiple profiles.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 637811 [details] [diff] [review]
Lock webapps during installation

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

I believe we now have everything in place to support what Myk suggested in comment 2.  The getSelf/getInstalled work will allow the marketplace to differentiate between install and reinstall/launch.  And some refactoring + bug 755934 allows us to display notification messages so now we can show "This app is already running, please quit it before reinstalling" or something like that. So I believe we should do that

::: browser/modules/WebappsInstaller.jsm
@@ +164,5 @@
>     * Install the app in the system by creating the folder structure,
>     *
>     */
>    install: function() {
> +    this.profileLocker = lockWebappProfile(this.installDir);

you're counting here on lockProfilePath throwing and the error propagating up. It's usually bad to have these kind of implicit exception handling because it's very hard to realize that just by reading the code.

Let's do this differently. Since what you want to know is if the app is running, you don't need to keep the lock during the whole process. Just try to lock it and immediately release it, and if an exception was thrown then you can return and set an error value in the `shell` object that webappsUI gets and could use that to show an error notification.
Another advantage is that you should be able to do this in the global install function (after init() was called in the shell), instead of doing it per-platform

Comment 10

[Dan Walkowski]

This issue on OS X is lessened, if not fixed, by my patch in bug 749826, which moves the existing install, if any, to the Trash before installing the new one.  Whether it is running or not doesn't matter, and has no effect.

Comment 11

[Marco Castelluccio [:marco]]

Attachment: wip

The patch is still incomplete as it fails to unlock the profile on reinstallation.
Works as expected in the other cases.

Comment 12

[Marco Castelluccio [:marco]]

Attachment: wip

About the profile lock problem, could you ask to someone expert with that code?

Comment 13

[Marco Castelluccio [:marco]]

Comment on attachment 653014 [details] [diff] [review]
wip

Benjamin, I need your feedback for the change in /profile/dirserviceprovider/src/nsProfileLock.cpp.
Before the re-installation of a webapp, I'm trying to lock (and unlock) the profile of the webapp to check if it's running. The problem is that two locks are created in the Lock function (fcntl and symlink), but only one lock is freed in the Unlock function.
I guess the code doesn't fail in Firefox because when you quit it, mLockFileDesc is automatically closed.

Comment 14

[Benjamin Smedberg]

Comment on attachment 653014 [details] [diff] [review]
wip

r=me for the nsProfileLock.cpp bits only. Please note that this feel vaguely risky to me so I don't want it jumping trains.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 653014 [details] [diff] [review]
wip

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

::: browser/modules/webappsUI.jsm
@@ +110,5 @@
>        label: bundle.getString("webapps.install"),
>        accessKey: bundle.getString("webapps.install.accesskey"),
>        callback: function() {
>          let app = WebappsInstaller.install(aData);
> +        if (!app.error) {

the dry_run pref in WebappsInstaller.jsm still returns only the boolean true, so we'll need to fix that as well

::: toolkit/webapps/WebappsInstaller.jsm
@@ +42,3 @@
>  #endif
>  
> +    if (isRunning(shell.installDir)) {

shell.installDir on Mac is the temp directory, at least before the installation is performed. The way it works will change with bug 749826 so let's deal with this in a follow-up. Can you file it so we don't forget?

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Patch

Carrying r+ from bsmedberg.

Filed bug 789840 for Mac.

I still need to test on Windows, but I think there won't be the locking problems that were on Linux.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 659602 [details] [diff] [review]
Patch

Hmm, this doesn't apply anymore.  I also wonder how much it matters, given that we don't currently allow reinstalls.  Although I suppose that could change in the future.

Comment 18

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #17)
> Comment on attachment 659602 [details] [diff] [review]
> Patch
> 
> Hmm, this doesn't apply anymore.  I also wonder how much it matters, given
> that we don't currently allow reinstalls.  Although I suppose that could
> change in the future.

Heh, it's quite old :D
Anyway there are many bugs about reinstalls, we need to keep track of them and remember we need to fix them before re-enabling reinstalls.

Comment 20

[Marco Castelluccio [:marco]]

This will no longer be necessary after bug 898647.