Last Comment Bug 748199 - Installation fails if name of app conflicts with native applications
: Installation fails if name of app conflicts with native applications
Status: VERIFIED FIXED
[marketplace-beta+]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: 14 Branch
: All Mac OS X
: -- normal
: Firefox 15
Assigned To: :Felipe Gomes (needinfo me!)
: Jason Smith [:jsmith]
Mentors:
https://marketplace-dev.allizom.org/e...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-23 18:19 PDT by Christopher Van Wiemeersch [:cvan]
Modified: 2016-02-04 15:00 PST (History)
10 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
App installation not allowed (679.27 KB, image/png)
2012-04-23 18:19 PDT, Christopher Van Wiemeersch [:cvan]
no flags Details
WIP (3.15 KB, patch)
2012-04-25 11:49 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Patch (2.94 KB, patch)
2012-04-25 15:03 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Patch v2 (3.80 KB, patch)
2012-04-25 21:12 PDT, :Felipe Gomes (needinfo me!)
mstange: review+
Details | Diff | Splinter Review

Description Christopher Van Wiemeersch [:cvan] 2012-04-23 18:19:39 PDT
Created attachment 617727 [details]
App installation not allowed

User Agent:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20120420 Firefox/14.0a1

Steps to reproduce:
1) Ensure that you have /Applications/Chess.app installed
2) Load https://marketplace-dev.allizom.org/en-US/app/chess-2/
3) Click the "Install" button and click "Install" in the doorhanger installer.
4) Notice the error: "App install not allowed" (error code: DENIED)

Thoughts:
Because I'm on Mac OS X and Chess.app is already an application I have installed (as a native app, not a web app), it thinks the Chess webapp is already installed and rejects installation.
Comment 1 Jason Smith [:jsmith] 2012-04-23 18:27:29 PDT
Confirmed on OS X 10.6.8. Also checked if you could install the app specified generally without a name conflict - That worked. Will want to see what happens on Windows as well to find out if this is OS-specific or not.
Comment 2 Jason Smith [:jsmith] 2012-04-23 18:30:57 PDT
Error Console:

Timestamp: 4/23/12 6:29:30 PM
Error: Error installing app: [Exception... "Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsILocalFile.create]"  nsresult: "0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS)"  location: "JS frame :: resource:///modules/WebappsInstaller.jsm :: <TOP_LEVEL> :: line 190"  data: no]
Source File: resource:///modules/WebappsInstaller.jsm
Line: 36
Comment 3 Jason Smith [:jsmith] 2012-04-23 18:36:50 PDT
This appears to be a Mac-specific issue. Windows also has incorrect behavior, but its a different issue (it overwrites shortcuts, where as Mac stops install).
Comment 4 Jason Smith [:jsmith] 2012-04-24 16:15:50 PDT
Need to talk to BD, Jason needs to figure out how many top apps this affects.
Comment 5 Jason Smith [:jsmith] 2012-04-24 20:17:07 PDT
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. I'd definitely mark this as a marketplace beta blocker.
Comment 6 :Felipe Gomes (needinfo me!) 2012-04-25 11:49:33 PDT
Created attachment 618377 [details] [diff] [review]
WIP

work in progress, untested
Comment 7 :Felipe Gomes (needinfo me!) 2012-04-25 15:03:16 PDT
Created attachment 618445 [details] [diff] [review]
Patch

This seems to work. Dan, can you test?

This patch should fix both the name collision problem and the icons problem by:
 - creating the app bundle in a temporary folder
 - checking for an existing app with the same name and adding "(2)" (or higher) to the name
 - moving it to the /Applications folder or, if the user has no permissions to do it, to the Desktop
Comment 8 :Felipe Gomes (needinfo me!) 2012-04-25 21:12:33 PDT
Created attachment 618548 [details] [diff] [review]
Patch v2

- Creates app in temporary folder
- Finds available name in /Applications folder
- moves app to /Application

I'm not using the nsIFile.createUnique function for two reasons:
 - It uses Filename-1.app style, and we want Filename (2).app (or to be able to tweak this styling later)
 - it would make it harder to do the step in which we create the folder in the tmp dir and then move it to Applications. This is necessary to fix other bugs but I'm doing it here together since I would also need it for this change.

With this change (writing into tmp folder), I don't need to separate the plist file creation done in bug 747601 (as it didn't entirely fix the problem it tried to fix, and the proper way to do it is this one). So I put the .plist creation back to its original function, but I'm still using the getIconForApp callback to do the final move
Comment 9 Markus Stange [:mstange] 2012-04-26 02:55:17 PDT
Comment on attachment 618548 [details] [diff] [review]
Patch v2

>+/**
>+ *  get a unique name for a file on a folder
>+ */
> /* More helpers for handling the app icon */
> #include WebappsIconHelpers.js

stray comment

Why do you switch from Filename (2).app to Filename-12.app if the number is >= 10?
Comment 10 :Felipe Gomes (needinfo me!) 2012-04-26 07:47:08 PDT
I figured if we're not having luck finding an available name up to (9) then changing the filename style made help. 

https://hg.mozilla.org/mozilla-central/rev/8180e2442cd2

Note You need to log in before you can comment on or make changes to this bug.