Closed Bug 765380 Opened 8 years ago Closed 6 years ago

Do not remove a running application

Categories

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

defect

Tracking

(firefox16 wontfix)

RESOLVED INVALID
Firefox 16
Tracking Status
firefox16 --- wontfix

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 5 obsolete files)

We shouldn't remove running applications, this could cause some problems.
Attached patch Lock webapps during installation (obsolete) — Splinter Review
Attachment #633693 - Flags: feedback?(myk)
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.
Attachment #633693 - Flags: feedback?(myk) → feedback+
Priority: -- → P2
Target Milestone: --- → Firefox 16
(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.
(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.
Attached patch Lock webapps during installation (obsolete) — Splinter Review
Attachment #633693 - Attachment is obsolete: true
Attachment #636522 - Flags: review?(myk)
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.
Attachment #636522 - Flags: review?(myk)
Attachment #636522 - Flags: review?(felipc)
Attachment #636522 - Flags: feedback+
Attached patch Lock webapps during installation (obsolete) — Splinter Review
(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.
Attachment #636522 - Attachment is obsolete: true
Attachment #636522 - Flags: review?(felipc)
Attachment #637811 - Flags: review?(felipc)
(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.
QA Contact: jsmith
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
Attachment #637811 - Flags: review?(felipc)
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.
Attached patch wip (obsolete) — Splinter Review
The patch is still incomplete as it fails to unlock the profile on reinstallation.
Works as expected in the other cases.
Attachment #637811 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
About the profile lock problem, could you ask to someone expert with that code?
Attachment #653009 - Attachment is obsolete: true
Attachment #653014 - Flags: feedback?(felipc)
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.
Attachment #653014 - Flags: review?(benjamin)
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.
Attachment #653014 - Flags: review?(benjamin) → review+
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?
Attachment #653014 - Flags: feedback?(felipc) → feedback+
Attached patch PatchSplinter Review
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.
Attachment #653014 - Attachment is obsolete: true
Attachment #659602 - Flags: review?(felipc)
Attachment #659602 - Flags: review?(felipc) → review?(myk)
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.
Attachment #659602 - Flags: review?(myk)
(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.
No longer blocks: 751898
Duplicate of this bug: 751898
Blocks: 751943
This will no longer be necessary after bug 898647.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.