710786 - Installing a web app natively can overwrite all the shortcuts of a previously installed native non-web app

Status: RESOLVED FIXED

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

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

If two apps have the same name, or names that resolve to the same name after sanitizing for Windows filenames, the app that is installed second will have its shortcuts replace the shortcuts of the previously installed app.  This makes the first app essentially inaccessible to users who do not know where to look in the filesystem for the app.

Comment 1

[Jason Smith [:jsmith]]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/24808745

Comment 2

[Jason Smith [:jsmith]]

Confirmed this is still happening on Win 7 64-bit. I created a test application called Evernote with my Evernote native application installed. Installed the web app, and my Evernote native app shortcut was overwritten.

Comment 3

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

Note: I think we do want to overwrite when reinstalling a webapp (f.e. installing the Evernote webapp twice).  We only want to avoid overwriting when installing a webapp that has the same name as a native app (or a different webapp).

Comment 4

[Jason Smith [:jsmith]]

Look at tier 1 apps against this bug.

Comment 5

[Jason Smith [:jsmith]]

These apps are known to risk conflicts (ones known so far, haven't gone through the whole list):

	* Springpad
	* Timer
	* Checkers
	* Sandglaz
	* Chess
	* Calculator
	* Soundcloud
	* Kicksend

These are possible. The issue probably is going to more heavily prominent on Mac, given that install is blocked if there's a name conflict and since mac has a built-in app store. For windows, the name conflicts will probably less likely. Also, this bug does not block install - It's just embarrassing behavior show to the user (which isn't good). I'd call this a soft blocker for the marketplace beta release.

Comment 6

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

Felipe: are you going to be able to work on this?

Comment 7

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

Marco: can you take this on?

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Patch

Looks a bit overkill, but, hey, we're good citizens and we don't want to overwrite other apps!

We'll also need to read the "${UninstallDir}/uninstall.log" file on updates, otherwise we end up creating a new shortcut every time.

Comment 9

[Marco Castelluccio [:marco]]

Attachment: Patch v2

This is maybe more beautiful.

Comment 10

[Marco Castelluccio [:marco]]

Attachment: Patch v3

I forgot to handle reinstalls.

Comment 11

[Marco Castelluccio [:marco]]

Comment on attachment 783374 [details] [diff] [review]
Patch v3

No, I need to read the shortcuts from the shortcuts_log.ini file

Comment 12

[Marco Castelluccio [:marco]]

Attachment: Patch v4

This is actually tricky.

The shortcuts contain info that could change after an update (there's the description). Should we overwrite them on updates/reinstalls? If we overwrite them we need to remember the shortcut names we used, luckily we have shortcuts_log.ini, but what if an user renames the shortcuts? We can't keep track of shortcut names, so in that case we'd create new ones.

Comment 13

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

Comment on attachment 783395 [details] [diff] [review]
Patch v4

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

(In reply to Marco Castelluccio [:marco] from comment #12)
> The shortcuts contain info that could change after an update (there's the
> description). Should we overwrite them on updates/reinstalls?

I think so, although we can tackle that in a different bug.


> If we
> overwrite them we need to remember the shortcut names we used, luckily we
> have shortcuts_log.ini, but what if an user renames the shortcuts? We can't
> keep track of shortcut names, so in that case we'd create new ones.

Indeed, we can't reasonably handle shortcut renames, but that's an edge case, and it seems reasonable to create new shortcuts in that case.


Otherwise this looks good, r=myk!

::: toolkit/webapps/WebappsInstaller.jsm
@@ +290,5 @@
> +      let factory = Cc["@mozilla.org/xpcom/ini-processor-factory;1"]
> +                      .getService(Ci.nsIINIParserFactory);
> +      let parser = factory.createINIParser(this.shortcutLogsINI);
> +
> +      this.shortcutName = parser.getString("STARTMENU", "Shortcut0");

I'm unfamiliar with the format of this file, so I'd like tabraldes to take a look at this patch.

@@ +302,5 @@
> +                                                 this.appNameAsFilename,
> +                                                 ".lnk");
> +      let [ _, shortcutNum2 ] = getAvailableFile(desktop,
> +                                                 this.appNameAsFilename,
> +                                                 ".lnk");

Nit: there's no need to define an underscore variable just to throw away part of a deconstruction; just leave it out:

  let [ , shortcutNum1 ] = ...

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 783395 [details] [diff] [review]
Patch v4

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

I feel like this patch would be cleaner if we replaced |this.shortcutName| with two variables: |this.startMenuShortcutName| and |this.desktopShortcutName|.

In the case where "shortcuts_log.ini" exists, we would initialize them as:
    this.startMenuShortcutName = parser.getString("STARTMENU", "Shortcut0");
    this.desktopShortcutName = parser.getString("DESKTOP", "Shortcut0");

In the case where the INI file doesn't exist, we would initialize them using the unmodified implementation of |getAvailableFile|:
    this.startMenuShortcutName = getAvailableFile(startMenu, this.appNameAsFilename, ".lnk");
    ...
    this.desktopShortcutName = getAvailableFile(desktop, this.appNameAsFilename, ".lnk");

Then elsewhere we would replace |this.shortcutName| as it makes sense, e.g.:
    shortcut.copyTo(desktop, this.desktopShortcutName);
    shortcut.copyTo(startMenu, this.startMenuShortcutName);

If we decide not to go with that approach, we still have to fix the issue in WebappsInstaller.jsm (see file comment).

Someday it would be nice to get rid of |getAvailableFile| entirely and use |nsIFile.createUnique| to get unique file names. We wouldn't be able to use |nsIFile.copyTo| in that case, so when we copy the shortcuts we'd have to actually open them and copy the bytes of the shortcut directly into the open files.

::: toolkit/webapps/WebappsInstaller.jsm
@@ +308,5 @@
> +      this.shortcutName = this.appNameAsFilename;
> +      if (shortcutNum1 > 1 || shortcutNum2 > 1) {
> +        this.shortcutName += " (";
> +        this.shortcutName += (shortcutNum1 > shortcutNum2) ? shortcutNum1 : shortcutNum2;
> +        this.shortcutName += ")";

This code is going to run into trouble if the shortcuts in the Start Menu aren't the same as the ones on the Desktop.

e.g...

Desktop:
 notepad.lnk

Start Menu:
 notepad (2).lnk

Comment 15

[Marco Castelluccio [:marco]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #14)
> Comment on attachment 783395 [details] [diff] [review]
> Patch v4
> 
> Review of attachment 783395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel like this patch would be cleaner if we replaced |this.shortcutName|
> with two variables: |this.startMenuShortcutName| and
> |this.desktopShortcutName|.
> 
> In the case where "shortcuts_log.ini" exists, we would initialize them as:
>     this.startMenuShortcutName = parser.getString("STARTMENU", "Shortcut0");
>     this.desktopShortcutName = parser.getString("DESKTOP", "Shortcut0");
> 
> In the case where the INI file doesn't exist, we would initialize them using
> the unmodified implementation of |getAvailableFile|:
>     this.startMenuShortcutName = getAvailableFile(startMenu,
> this.appNameAsFilename, ".lnk");
>     ...
>     this.desktopShortcutName = getAvailableFile(desktop,
> this.appNameAsFilename, ".lnk");
> 
> Then elsewhere we would replace |this.shortcutName| as it makes sense, e.g.:
>     shortcut.copyTo(desktop, this.desktopShortcutName);
>     shortcut.copyTo(startMenu, this.startMenuShortcutName);
> 
> If we decide not to go with that approach, we still have to fix the issue in
> WebappsInstaller.jsm (see file comment).
> 
> Someday it would be nice to get rid of |getAvailableFile| entirely and use
> |nsIFile.createUnique| to get unique file names. We wouldn't be able to use
> |nsIFile.copyTo| in that case, so when we copy the shortcuts we'd have to
> actually open them and copy the bytes of the shortcut directly into the open
> files.
> 
> ::: toolkit/webapps/WebappsInstaller.jsm
> @@ +308,5 @@
> > +      this.shortcutName = this.appNameAsFilename;
> > +      if (shortcutNum1 > 1 || shortcutNum2 > 1) {
> > +        this.shortcutName += " (";
> > +        this.shortcutName += (shortcutNum1 > shortcutNum2) ? shortcutNum1 : shortcutNum2;
> > +        this.shortcutName += ")";
> 
> This code is going to run into trouble if the shortcuts in the Start Menu
> aren't the same as the ones on the Desktop.
> 
> e.g...
> 
> Desktop:
>  notepad.lnk
> 
> Start Menu:
>  notepad (2).lnk

Actually I wrote the code like this to take care of this situation. I think the webapp shortcuts on the desktop and in the start menu should have the same name.
In your example, with my approach this.shortcutName would be equal to "notepad (3).lnk", with your approach this.desktopShortcutName would be "notepad (2).lnk" and this.startMenuShortcutName would be "notepad (3).lnk".

What that code is supposed to do is take the max available number and use it for both shortcuts.

Comment 16

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Marco Castelluccio [:marco] from comment #15)
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #14)
[...]
> > Desktop:
> >  notepad.lnk
> > 
> > Start Menu:
> >  notepad (2).lnk
> 
> Actually I wrote the code like this to take care of this situation. I think
> the webapp shortcuts on the desktop and in the start menu should have the
> same name.

I would find the logic easier to follow if we didn't have the restriction of keeping the names the same, but keeping the names the same might be less confusing for the user. When in doubt, I suppose we should opt to make things easier for the user :)

> In your example, with my approach this.shortcutName would be equal to
> "notepad (3).lnk", with your approach this.desktopShortcutName would be
> "notepad (2).lnk" and this.startMenuShortcutName would be "notepad (3).lnk".
> 
> What that code is supposed to do is take the max available number and use it
> for both shortcuts.

The intent of the code is clear, but there's a bug.

shortcutNum1 will be set by a call to |getAvailableFile| that looks like this:
    getAvailableFile(startMenu,
                     this.appNameAsFilename,
                     ".lnk");

There is no file in the Start Menu called "notepad.lnk" so shortcutNum1 will be 1.

shortcutNum2 will be set by a call to |getAvailableFile| that looks like this:
    getAvailableFile(desktop,
                     this.appNameAsFilename,
                     ".lnk");

Since there is a file called "notepad.lnk" shortcutNum2 will be 2.

Later on, when we're creating |this.shortcutName|, we put a number in parentheses:
    this.shortcutName += (shortcutNum1 > shortcutNum2) ? shortcutNum1 : shortcutNum2;

Note that the number in parentheses will be equal to |shortcutNum2| in this example, which is 2.

Later, when we copy our shortcut to the Start Menu we will overwrite the file "notepad (2).lnk" which already existed there.

Comment 17

[Marco Castelluccio [:marco]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #16)
> Note that the number in parentheses will be equal to |shortcutNum2| in this
> example, which is 2.
> 
> Later, when we copy our shortcut to the Start Menu we will overwrite the
> file "notepad (2).lnk" which already existed there.

Oh, now I see what you meant! Looks like a remote possibility, should we consider it anyway?

Comment 18

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I'd prefer not to knowingly land this bug, but I'll defer to myk's judgement.

Comment 19

[Marco Castelluccio [:marco]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #18)
> I'd prefer not to knowingly land this bug, but I'll defer to myk's judgement.

I don't want to make you unhappy, I'll write a new patch to take care of this problem! It won't take so long.

Comment 20

[Marco Castelluccio [:marco]]

Attachment: Patch v5

Comment 21

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 785460 [details] [diff] [review]
Patch v5

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

Nice. This patch looks a lot cleaner to me.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ac6eacb47d55

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac6eacb47d55