Closed Bug 749826 Opened 12 years ago Closed 11 years ago

Installed app can be reinstalled as a duplicate

Categories

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

15 Branch
All
macOS
defect

Tracking

(blocking-kilimanjaro:+, firefox16 wontfix)

RESOLVED FIXED
Firefox 27
blocking-kilimanjaro +
Tracking Status
firefox16 --- wontfix

People

(Reporter: igarcia, Assigned: marco)

References

Details

(Keywords: regression)

Attachments

(1 file, 7 obsolete files)

Clicking on the "Installed" button of an app that has been already installed in a device generates a copy of the app.

We should disable this or alert the user that a duplication will happen.
I get the feeling this is an issue as a result of the patch in bug 748199.
Version: 11 Branch → 15 Branch
Isn't this a marketplace issue? why should the Installed button be clickable and trigger another install?
(In reply to Felipe Gomes (:felipe) from comment #2)
> Isn't this a marketplace issue? why should the Installed button be clickable
> and trigger another install?

Probably a separate issue of the marketplace on the UI design. This bug is talking about the case where a user installs the same application twice on OS X.
(In reply to Jason Smith [:jsmith] from comment #3)
> (In reply to Felipe Gomes (:felipe) from comment #2)
> > Isn't this a marketplace issue? why should the Installed button be clickable
> > and trigger another install?
> 
> Probably a separate issue of the marketplace on the UI design. This bug is
> talking about the case where a user installs the same application twice on
> OS X.

The underlying issue also that this affects is to you cannot reinstall an application, only create a new application.
I thought reinstalling an app is supposed to update the receipts?
Keywords: regression
Whiteboard: [marketplace-beta=]
Blocks: 731054
Why exactly is this a bug?
There is nothing fundamentally wrong with having two copies of the app.
What should happen instead?
If you reinstall an app that is already installed, reinstallation should update the existing copy of the app, so after reinstallation you continue to have a single copy of the app.
Or if we decide to allow duplications (I don't see any good reason to allow it to be honest) we should at least prompt the user and ask them for confirmation "You already have this app installed in this computer, do you really want to create another copy?" or similar.
Reinstalling assumes you can find the copy the user installed the first time. (you can, by asking the OS) We cannot assume that it's still in the Applications folder, that it has the same name, or that there's only one.  It's likely, but not guaranteed.
Sure, there may be edge cases that are prohibitively difficult to account for.  But we should do whatever we can reasonably do to locate an existing installation before installing a fresh copy.
(In reply to Felipe Gomes (:felipe) from comment #2)
> Isn't this a marketplace issue? why should the Installed button be clickable
> and trigger another install?

The Installed button stays installed once the app has been installed and uninstalled so if it wasn't clickable you wouldn't be able to install an app you'd installed and uninstalled previously.
Would fixing bug 749033, help this bug? At least the Marketplace UI part?
(In reply to Jennifer Arguello :ticachica from comment #12)
> Would fixing bug 749033, help this bug? At least the Marketplace UI part?

Don't think so, as this is not a UX issue. This has to do with reinstalling any app that is already installed on OS X creates a duplicate application. So install same app 10 times in a row on OS X and you get 10 copies of the app.
(In reply to Jason Smith [:jsmith] from comment #13)
> Don't think so, as this is not a UX issue. This has to do with reinstalling
> any app that is already installed on OS X creates a duplicate application.
> So install same app 10 times in a row on OS X and you get 10 copies of the
> app.

Oh ok. I was just keying off of comment #11, where if the install button in the marketplace knew the app was already locally installed it would be unclickable and not allow another installation to occur. This would prevent multiple copies from being installed from the marketplace at least. But maybe I read the comment wrong.
A better visual indication of what happen during the installation will also mitigate this bug but not resolve it completely.

With the actual implementation it's fairly easy to press the Install-Installed button because there's no confirmation at any point. I actually clicked the Installed button hoping to launch the app.
(In reply to Jennifer Arguello :ticachica from comment #12)
> Would fixing bug 749033, help this bug? At least the Marketplace UI part?

(In reply to Jennifer Arguello :ticachica from comment #14)
> (In reply to Jason Smith [:jsmith] from comment #13)
> > Don't think so, as this is not a UX issue. This has to do with reinstalling
> > any app that is already installed on OS X creates a duplicate application.
> > So install same app 10 times in a row on OS X and you get 10 copies of the
> > app.
> 
> Oh ok. I was just keying off of comment #11, where if the install button in
> the marketplace knew the app was already locally installed it would be
> unclickable and not allow another installation to occur. This would prevent
> multiple copies from being installed from the marketplace at least. But
> maybe I read the comment wrong.

Yeah, I was just highlighting why we need the installed button to install again at the moment - once bug 749033 is fixed (assuming it covers all edge cases) then it shouldn't be necessary.
(In reply to Andrew Williamson [:eviljeff] from comment #16)
> Yeah, I was just highlighting why we need the installed button to install
> again at the moment - once bug 749033 is fixed (assuming it covers all edge
> cases) then it shouldn't be necessary.

Unfortunately, we will never be able to cover all edge cases, and there will always be circumstances under which users will want to reinstall an app they had previously installed, even though as far as we can tell it is correctly installed.

So we should always have a mechanism for users to reinstall an app.  Now, we could make that mechanism be "uninstall it first, then reinstall it."  But while we allow reinstallation without prior uninstallation, and perhaps even after that, we must fix this bug.

And note that no native app ever behaves in this way.  When you install a native app twice, you don't get two copies of that native app (unless you intentionally install one to a different location, for the subset of native apps that allow you to do that).
There are very few native apps that even -have- an installer, forcing you to install them to a specific location.  (Microsoft and Adobe products, for example)  Installing on the Mac means copying.  You copy a folder from one disk to another.  They often helpfully include an alias to your Applications folder, so that you can easily put it in the typical spot.  Anecdotally, I rarely do, until I am certain I'm going to keep the app.

Here is an app I happened to 'install' today:
http://dl.dropbox.com/u/169445/Mac%20App%20%27Install%27.png

If I dragged it again, to a different location, I'd have two.  If I dragged it to the same location, it would ask if I wanted to replace or keep both.

So, I disagree with your statement that 'no native app behaves this way'.  In fact almost all of them do, since very few have explicit installers.
To be clear, I am not trying to suggest we -must- behave the same way.  
Simply that on the Mac, Apps are not the one-and-only-one, install-to-blessed-location, users-cannot-rename-or-move-them entities that they are on other platforms.  They don't get installed, in the normal sense of the word, and trying to implement a rigid install strategy on them is tricky, error-prone, and non-standard.
Priority: -- → P2
Target Milestone: --- → Firefox 15
Whiteboard: [marketplace-beta=]
In a Mac the default behavior is to "install" an app into the Application folder, if you are trying to install it again in that folder the OS will prompt you with an Alert. You can duplicate it, but is a conscious decision.

If you install it elsewhere after the package suggest you to drag-and-drop it to the Applications folder and this causes a duplication, you are making a conscious decision.

In our case you can install it a million times and not be a conscious decision because the "Installed" button is misleading.

That said, I'm not even sure if the MacOS behavior is the one we should be looking at. Windows installation flows are way more rigid and that's what 90% of users know. Newly MacOs users struggle with the OS X installation process (this is more of a personal observation).
No longer blocks: 731054
Nominating for k9o - this a basic user flow for desktop webRT (reinstall flow).
blocking-kilimanjaro: --- → ?
Seems like an obvious blocker -- users typically do "hmm something is broken let me try reinstalling", and having duplicate apps result would be quite awful.
blocking-kilimanjaro: ? → +
Target Milestone: Firefox 15 → ---
QA Contact: jsmith
Assignee: nobody → dwalkowski
Attached patch patch to prevcent duplicates (obsolete) — Splinter Review
This patch significantly improves on the update/reinstall behavior on OS X.  If a previous webapp version or install is found, the new update/install replaces it, and the old copy is moved to the Trash.  This is true even if the user has moved and/or renamed the webapp.  If, instead, the webapp collides in name with a -native- app, then the webapp is renamed with a numerical suffix, leaving the native app alone.
Attachment #649862 - Flags: review?(felipc)
> try {
>   this._createDirectoryStructure();
>   this._copyPrebuiltFiles();
>   this._createConfigFiles();
>   this._createAppProfile();
>   } catch (ex) {
>-    this._removeInstallation(false);
>     throw(ex);
>   }

This way if there was an error during the installation, the directories created wouldn't be removed.
Assignee: dwalkowski → nobody
Priority: P2 → P1
Myk this shouldn't be a high priority bug, at least while we don't allow reinstalls.
Blocks: 898647
Attached patch Patch (obsolete) — Splinter Review
I think this could be a good first approach, then we can improve things in other bugs (for example on Mac moving the app to the trash instead of removing it).

I've modified the Win and Linux installers to install the application in a temporary directory and then moving it to the right place at the end of the installation process. I think this way the installer is more robust.

The update process would be basically like a reinstall, we can later specialize the two flows (for example, why would an user reinstall an application? Maybe because it's broken, so maybe we should remove the old profile too).
Assignee: nobody → mcastelluccio
Attachment #649862 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #649862 - Flags: review?(felipc)
Attachment #804773 - Flags: feedback?(myk)
Comment on attachment 804773 [details] [diff] [review]
Patch

Overall, this seems reasonable.  A few thoughts:

* We should continue to forbid reinstalls, since that seems to be part of our public API, although we still have to account for this possibility given that apps can get out of sync with the registry.

* Nice removal of the _init methods, which don't provide any benefit over simply putting the initialization code into the constructors.

* It seems like we shouldn't call _createDirectoryStructure() until install().

* In general, we should *not* remove data on uninstall/reinstall, as the convention on all three OSes is for data to persist across that operation.  However, it's a good point that users might sometimes want to remove data, and we should consider ways to make that easier.

* Can more of moveDirectory(), like entry iteration, be moved off the main thread via OS.File?

* Why is it necessary to recursively move the new installation dir if we remove the old installation dir first?  Can't we simply move the new dir into place?
Attachment #804773 - Flags: feedback?(myk) → feedback+
(In reply to Myk Melez [:myk] [@mykmelez] from comment #29)
> Overall, this seems reasonable.  A few thoughts:
> 
> * We should continue to forbid reinstalls, since that seems to be part of
> our public API, although we still have to account for this possibility given
> that apps can get out of sync with the registry.

Yes, I commented that code out just to be able to test the patch :D

> * Can more of moveDirectory(), like entry iteration, be moved off the main
> thread via OS.File?

I've two versions of moveDirectory (the one commented out is using OS.File). The problem with OS.File was that OS.File.move doesn't preserve file permissions.

> * Why is it necessary to recursively move the new installation dir if we
> remove the old installation dir first?  Can't we simply move the new dir
> into place?

I tried:
|this.tmpInstallDir.moveTo(this.installDir.parent, this.installDir.leafName)|
but it didn't work.
I was told moveTo doesn't support that.
Doesn't really depend on bug 919771, but it'd be better to use OS.File if we could.
Depends on: 919771
(In reply to Marco Castelluccio [:marco] from comment #30)
> > * Why is it necessary to recursively move the new installation dir if we
> > remove the old installation dir first?  Can't we simply move the new dir
> > into place?
> 
> I tried:
> |this.tmpInstallDir.moveTo(this.installDir.parent, this.installDir.leafName)|
> but it didn't work.
> I was told moveTo doesn't support that.

After some testing, I've discovered that it doesn't work when the destination directory already exists.
(We don't remove the old installation directory on Windows and Linux, because it contains the profile)
Attached patch Patch (obsolete) — Splinter Review
I've tested the patch in different configurations and it looks like it solves the problems we had with reinstallations (also the problems with reinstallations while the application is running).
I haven't tested yet on Unity, but if bug 763375 is still there, it's a Unity bug.

On Windows, when we want to move the directory, we can't replace the stub executable if the app is running. So I've added a try-catch that allows us to continue the installation anyway (because the stub executable takes care of updating itself).
If you want we can make the check more specific, something like:

  try {
    ...
  } catch (ex if ex.result == Cr.NS_ERROR_FILE_ACCESS_DENIED) {
    if (!fileName == stubName) {
      throw(ex);
    }
  }

At the moment, we also can't replace the webapp.json file (I'm opening a bug to fix this, the runtime should read it at startup and then close it).
Attachment #804773 - Attachment is obsolete: true
Attachment #810669 - Flags: review?(myk)
Comment on attachment 810669 [details] [diff] [review]
Patch

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

> On Windows, when we want to move the directory, we can't replace the stub
> executable if the app is running. So I've added a try-catch that allows us
> to continue the installation anyway (because the stub executable takes care
> of updating itself).
> If you want we can make the check more specific, something like:
> 
>   try {
>     ...
>   } catch (ex if ex.result == Cr.NS_ERROR_FILE_ACCESS_DENIED) {
>     if (!fileName == stubName) {
>       throw(ex);
>     }
>   }

Yes, I think that's a good idea.

::: dom/apps/src/Webapps.jsm
@@ +1905,5 @@
>                this._isLaunchable(this.webapps[id])) {
>              sendError("MULTIPLE_APPS_PER_ORIGIN_FORBIDDEN");
>              return;
>            }
> +        }*/

Understood about the earlier patch being a work-in-progress for testing purposes, but now that this patch is up for review, this change definitely needs removing! :-)

::: toolkit/webapps/WebappsInstaller.jsm
@@ +23,5 @@
> +                        OS.Constants.libc.S_IROTH | OS.Constants.libc.S_IXOTH;
> +
> +const PERMS_FILE = OS.Constants.libc.S_IRUSR | OS.Constants.libc.S_IWUSR |
> +                   OS.Constants.libc.S_IRGRP |
> +                   OS.Constants.libc.S_IROTH;

Nit: it took me a while to figure out that the columns are intended to group different kinds of perms (for read vs. execute/write permissions).  These'd be easier to read if they were simply all lined up vertically along with a comment expressing the computed value as a Unix-style octal value (i.e. 0755 and 0644), so one can determine the computed values at a glance.

@@ +374,5 @@
> +        // Remove previously installed app
> +        this._removeInstallation(true);
> +      } catch (ex) {
> +        throw(ex);
> +      }

There's no point in catching an exception only to throw it again; better to simply remove the try block and let the exception propagate directly.

@@ +393,3 @@
>     * Remove the current installation
>     */
> +  _removeInstallation: function(keepProfile) {

Nice. :-)

@@ +449,1 @@
>      }

Is there a bug on recursive makeDir?  If not, we should file one!  Either way, it'd be nice to have a comment here explaining that this is recursively creating the icon path's directory structure.  It took me a bit of reading through the code to realize what it's doing.

@@ +746,5 @@
>  
> +    let writer = Cc["@mozilla.org/xpcom/ini-processor-factory;1"]
> +                   .getService(Ci.nsIINIParserFactory)
> +                   .createINIParser(applicationINI)
> +                   .QueryInterface(Ci.nsIINIParserWriter);

Nice simplification.

@@ +884,5 @@
> +
> +        // Remove previously installed app
> +        this._removeInstallation(true);
> +      } catch (ex) {
> +        throw(ex);

Nit: as above, it's better to remove the try block than leave it in and merely rethrow the exception.

@@ +924,5 @@
>    _copyPrebuiltFiles: function() {
> +    let destDir = getFile(this.tmpInstallDir);
> +    let webapprtPre = getFile(this.runtimeFolder, this.webapprt);
> +
> +    webapprtPre.copyTo(destDir, null);

Nit: while you're here, it'd be nice to rename webapprtPre to something clearer, like webapprtStub.

@@ +1218,5 @@
>               .replace(/>/g, ">");
>  }
>  
> +// Helper to create a nsIFile from a set of path components
> +function getFile(pathComponents) {

Nit: pathComponents is unnecessary.

@@ +1219,5 @@
>  }
>  
> +// Helper to create a nsIFile from a set of path components
> +function getFile(pathComponents) {
> +  let args = Array.prototype.slice.call(arguments, 0);

Nit: this can now be simpler:

  let args = Array.slice(arguments, 0);

@@ +1224,5 @@
> +
> +  let path = "";
> +  for (let component of args) {
> +    path = OS.Path.join(path, component);
> +  }

Nit: you should be able to do all this with:

  let path = OS.Path.join.apply(args.unshift(""));

Which means this whole thing can be:

  let path = OS.Path.join.apply(Array.slice(arguments, 0).unshift(""));

However, do you really need to pass the empty string to OS.Path.join?  The function needs at least one argument, but *arguments* should contain one.  And if it doesn't, wouldn't it be better for OS.Path.join to throw than to return successfully, masking the error?
Attachment #810669 - Flags: review?(myk) → review-
(In reply to Myk Melez [:myk] [@mykmelez] from comment #34)
> Nit: this can now be simpler:
> 
>   let args = Array.slice(arguments, 0);

More info:
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array#Array_generic_methods

Also note that the *begin* parameter defaults to 0 if absent, so it can be omitted.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #34)
> ::: dom/apps/src/Webapps.jsm
> @@ +1905,5 @@
> >                this._isLaunchable(this.webapps[id])) {
> >              sendError("MULTIPLE_APPS_PER_ORIGIN_FORBIDDEN");
> >              return;
> >            }
> > +        }*/
> 
> Understood about the earlier patch being a work-in-progress for testing
> purposes, but now that this patch is up for review, this change definitely
> needs removing! :-)

Oops, I forgot it :D

> 
> ::: toolkit/webapps/WebappsInstaller.jsm
> @@ +23,5 @@
> > +                        OS.Constants.libc.S_IROTH | OS.Constants.libc.S_IXOTH;
> > +
> > +const PERMS_FILE = OS.Constants.libc.S_IRUSR | OS.Constants.libc.S_IWUSR |
> > +                   OS.Constants.libc.S_IRGRP |
> > +                   OS.Constants.libc.S_IROTH;
> 
> Nit: it took me a while to figure out that the columns are intended to group
> different kinds of perms (for read vs. execute/write permissions).  These'd
> be easier to read if they were simply all lined up vertically along with a
> comment expressing the computed value as a Unix-style octal value (i.e. 0755
> and 0644), so one can determine the computed values at a glance.

Actually, I intended to group them by row (user, group, other users). I didn't notice I ended up grouping them also by kind of permission :D

> @@ +374,5 @@
> > +        // Remove previously installed app
> > +        this._removeInstallation(true);
> > +      } catch (ex) {
> > +        throw(ex);
> > +      }
> 
> There's no point in catching an exception only to throw it again; better to
> simply remove the try block and let the exception propagate directly.

Actually, here I wanted to remove the temp install directory, to avoid polluting the user's temp directory.

> @@ +449,1 @@
> >      }
> 
> Is there a bug on recursive makeDir?  If not, we should file one!  Either
> way, it'd be nice to have a comment here explaining that this is recursively
> creating the icon path's directory structure.  It took me a bit of reading
> through the code to realize what it's doing.

There isn't a bug, I'll file it. I'm considering directly working on recursive remove, recursive move and recursive make, so that we can use them here.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #810669 - Attachment is obsolete: true
Attachment #810891 - Flags: review?(myk)
Attached patch Patch v2 (obsolete) — Splinter Review
I forgot to remove the debugging stuff again :D
Attachment #810891 - Attachment is obsolete: true
Attachment #810891 - Flags: review?(myk)
Attachment #810893 - Flags: review?(myk)
Attached patch Patch v2 (obsolete) — Splinter Review
Bug 920686 has landed, so we can use OS.Constants.Path everywhere instead of the directory service.
Attachment #810893 - Attachment is obsolete: true
Attachment #810893 - Flags: review?(myk)
Attachment #812219 - Flags: review?(myk)
Comment on attachment 812219 [details] [diff] [review]
Patch v2

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

::: toolkit/webapps/WebappsInstaller.jsm
@@ +340,5 @@
> +    this.installDir = OS.Path.join(OS.Constants.Path.winAppDataDir,
> +                                   this.uniqueName);
> +
> +    let desktop = OS.Constants.Path.desktopDir;
> +    let startMenu = OS.Constants.Path.winStartMenuProgsDir;

Nit: it would be better to declare these as *const*.

(For that matter, it might make sense to define global constants for the various constant paths used throughout this file, to shorten the strings we use to refer to them and make the code easier to read.)
Attachment #812219 - Flags: review?(myk) → review+
Attached patch Patch v3 (obsolete) — Splinter Review
(There were tests failures on Mac with the latest patch, because the /Applications directory isn't writable on test slaves.)
Attachment #812219 - Attachment is obsolete: true
Attachment #812913 - Flags: review?(myk)
Comment on attachment 812913 [details] [diff] [review]
Patch v3

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

::: toolkit/webapps/WebappsInstaller.jsm
@@ +29,5 @@
> +                   OS.Constants.libc.S_IROTH;
> +
> +const desktopDir = OS.Constants.Path.desktopDir;
> +const homeDir = OS.Constants.Path.homeDir;
> +const tmpDir = OS.Constants.Path.tmpDir;

Nit: we should use ALL_CAPS consistently for constants.
Attachment #812913 - Flags: review?(myk) → review+
Attached patch Patch v3Splinter Review
Carrying r+.
Attachment #812913 - Attachment is obsolete: true
Attachment #813568 - Flags: review+
Keywords: checkin-needed
No longer depends on: 919771
https://hg.mozilla.org/mozilla-central/rev/852536d5bf68
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Blocks: 751898
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.