Closed Bug 836016 Opened 12 years ago Closed 12 years ago

Cannot reinstall an app after uninstalling it

Categories

(Firefox Graveyard :: Web Apps, defect)

defect
Not set
normal

Tracking

(firefox21+ verified)

VERIFIED FIXED
Firefox 21
Tracking Status
firefox21 + verified

People

(Reporter: krupa.mozbugs, Assigned: marco)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

steps to reproduce: 1. Install an app from https://marketplace-dev.allizom.org/search/?q= 2. Uninstall the app from your Applications folder 3. reinstall the app expected behavior: user is allowed to reinstall an app they have uninstalled. observed behavior: On trying to install an uninstalled app, we get REINSTALL_ERROR testcase: http://people.mozilla.org/~cwiemeersch/install.html
Component: Consumer Pages → DOM: Apps
Product: Marketplace → Core
Version: 1.0 → Trunk
See test case: http://people.mozilla.org/~cwiemeersch/install.html Reproducible on Firefox Nightly 21.0a1 (2013-01-29).
Component: DOM: Apps → Web Apps
Product: Core → Firefox
QA Contact: jsmith
I know what happened. I'm guessing this only reproduces on desktop, right? What's going wrong here is desktop follows a different path than using the webapps registry. So there's likely a collision of the requirement we put in place for REINSTALL_FORBIDDEN and desktop's definition of if the app is installed or not. Timestamp: 1/29/2013 2:03:41 PM Error: Error installing app from: https://marketplace-dev.allizom.org: REINSTALL_FORBIDDEN Source File: resource://gre/modules/Webapps.jsm Line: (null)
Yeah, this looks to be desktop specific. Can't reproduce on B2G.
Keywords: regression
(In reply to Jason Smith [:jsmith] from comment #3) > Yeah, this looks to be desktop specific. Can't reproduce on B2G. Are there any desktop web app pushes coming in the FF21 timeframe? If not, this likely doesn't need to track.
(In reply to Alex Keybl [:akeybl] from comment #4) > (In reply to Jason Smith [:jsmith] from comment #3) > > Yeah, this looks to be desktop specific. Can't reproduce on B2G. > > Are there any desktop web app pushes coming in the FF21 timeframe? If not, > this likely doesn't need to track. We're actively advertising installation of web apps on desktop on marketplace. See for yourself - http://marketplace.firefox.com/. Although we aren't doing any desktop web app pushes to advertise new features anytime soon, we actually do maintain this code to make sure it doesn't fall apart. Which in the case of this bug, it has.
Basically if we don't go after fixing this in the FF 21 timeframe we're likely going to have make a decision to turn off web app installation on desktop through marketplace.
(In reply to Jason Smith [:jsmith] from comment #6) > Basically if we don't go after fixing this in the FF 21 timeframe we're > likely going to have make a decision to turn off web app installation on > desktop through marketplace. We should fix, I do not want to turn off Apps support on desktop.
Bill says he'll take ownership of this to find someone to fix this.
Assignee: nobody → bwalker
Given comment 5, we'll track.
Blocks: 821288
Assignee: bwalker → mar.castelluccio
Attached patch Patch (obsolete) — Splinter Review
Myk, what do you think of this approach?
Attachment #711637 - Flags: feedback?(myk)
I forgot we had the _isLaunchable function in webapps.jsm. This patch is a lot simpler, but in the future I think we should move the OS specific logic to webapposutils.js.
Attachment #711654 - Flags: feedback?(myk)
Before adding another test like that, I'd really like to understand why |this._appId(app.origin) !== null)| is true. In |uninstall| we're supposed to delete the app from this.webapps so that should not happen. What is different here in the desktop ?
(In reply to Julien Wajsberg [:julienw] from comment #12) > Before adding another test like that, I'd really like to understand why > |this._appId(app.origin) !== null)| is true. > > In |uninstall| we're supposed to delete the app from this.webapps so that > should not happen. What is different here in the desktop ? On desktop you can uninstall the application through other means.
You mean, without using any Firefox UI at all (like the windows add/remove panel) ? ok I get it. If you add this test, then please add a comment explaining that. You should also ask feedback from :fabrice.
(In reply to Julien Wajsberg [:julienw] from comment #14) > You mean, without using any Firefox UI at all (like the windows add/remove > panel) ? Exactly :) > If you add this test, then please add a comment explaining that. You should > also ask feedback from :fabrice. Ok. Fabrice, what do you prefer between the two proposed solutions?
Flags: needinfo?(fabrice)
(In reply to Julien Wajsberg [:julienw] from comment #14) > You mean, without using any Firefox UI at all (like the windows add/remove > panel) ? > > ok I get it. > > If you add this test, then please add a comment explaining that. You should > also ask feedback from :fabrice. To summarize - desktop's concept of whether an app is installed is very different than Android and B2G. Android and B2G uses the webapps registry. Desktop relies on the fact that it's "natively" installed on the machine (e.g. windows executable for example is installed on the machine in app data). That's why there's differences here to determine if the app is installed or not.
Comment on attachment 711637 [details] [diff] [review] Patch Review of attachment 711637 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +1574,5 @@ > Cu.reportError("Error installing app from: " + app.installOrigin + > ": " + aError); > }.bind(this); > > +#ifdef MOZ_WIDGET_GONK That should not be MOZ_WIDGET_GONK, but MOZ_B2G, and we need to check how the android webRT behaves.
Flags: needinfo?(fabrice)
I much prefer the second patch, but I'm really annoyed that the desktop webRT leaves orphaned entries in the webapps registry. That also needs to be fixed.
(In reply to Fabrice Desré [:fabrice] from comment #18) > I much prefer the second patch, but I'm really annoyed that the desktop > webRT leaves orphaned entries in the webapps registry. That also needs to be > fixed. I think the only way we can fix that is by checking every time we access the registry if the application is locally installed and, if it isn't, remove it. But I don't know how much I like this solution. Do you have anything else in mind?
Provide an uninstall program that would correctly clean up things in our Webapps implementation ?
(In reply to Julien Wajsberg [:julienw] from comment #20) > Provide an uninstall program that would correctly clean up things in our > Webapps implementation ? What if an user removes the installation manually? And I don't know how things work in Mac OS, but I think there isn't any uninstaller on that platform.
Yep I admit that would not be perfect and we'd still need to guard against that. maybe fabrice will habve a better idea.
Attachment #711637 - Flags: feedback?(myk) → feedback-
Comment on attachment 711654 [details] [diff] [review] Second opportunity I too prefer this version. And I agree with Fabrice that it's an issue that the webapps registry gets out-of-sync with the OS registry. But that isn't an easy problem to solve, and we should do so in a separate bug.
Attachment #711654 - Flags: feedback?(myk) → feedback+
On Windows and Linux, we provide an uninstaller program, and it could update the webapp registry. We wouldn't want it to edit the datastore directly, however. At least not while they're implemented as flat files. Rather we'd want the registry to expose an interface that the uninstaller process can use to tell the registry to remove the app record. Which needs to work whether or not Firefox is running. @bsmedberg previously suggested having Firefox register a system service that can provide this interface, which we can do on Windows (not sure about Linux or Mac). On Mac, although there is no uninstaller program, the OS does mark the app uninstalled when its .app package is moved to the trash, and perhaps there's a way for Firefox to get notified in that case. Alternately, we could implement a command-line flag for Firefox that starts Firefox (if it isn't already running), removes the entry, and then quits Firefox again (if it wasn't running when we called it). That should work on all three systems. Or we could change the registry datastore format from a flat file to SQLite, have the uninstaller process change the datastore directly, and define a trigger that tells Firefox to update its in-memory cache of the registry, if it's running when an app is uninstalled. But this is all out-of-scope for this particular bug.
Comment on attachment 711654 [details] [diff] [review] Second opportunity (In reply to Myk Melez [:myk] [@mykmelez] from comment #24) > On Windows and Linux, we provide an uninstaller program, and it could update > the webapp registry. We wouldn't want it to edit the datastore directly, > however. At least not while they're implemented as flat files. Rather we'd > want the registry to expose an interface that the uninstaller process can > use to tell the registry to remove the app record. Which needs to work > whether or not Firefox is running. > > @bsmedberg previously suggested having Firefox register a system service > that can provide this interface, which we can do on Windows (not sure about > Linux or Mac). On Mac, although there is no uninstaller program, the OS > does mark the app uninstalled when its .app package is moved to the trash, > and perhaps there's a way for Firefox to get notified in that case. > > Alternately, we could implement a command-line flag for Firefox that starts > Firefox (if it isn't already running), removes the entry, and then quits > Firefox again (if it wasn't running when we called it). That should work on > all three systems. > > Or we could change the registry datastore format from a flat file to SQLite, > have the uninstaller process change the datastore directly, and define a > trigger that tells Firefox to update its in-memory cache of the registry, if > it's running when an app is uninstalled. > > But this is all out-of-scope for this particular bug. The problem would be that an user could remove the application manually, without an uninstaller. But we can decide not to support this use case. We could do something like this: 1) When Firefox starts, check if the registry is up-to-date by checking if the applications are locally installed. 2) While Firefox is running, we could "watch" the installation directories and get notified when they are removed. I think there are APIs to do that on Windows, Mac and Linux. Myk, I think you can review the patch, because I think it doesn't need other changes.
Attachment #711654 - Flags: review?(myk)
Myk, that's fine, I didn't expected the out-of-sync issue to be fixed in this bug! And I'm not sure what the best solution is. I kind of like the sqlite trigger idea, but that conflicts with my planned refactoring using indexedDB ;)
Comment on attachment 711654 [details] [diff] [review] Second opportunity (In reply to Marco Castelluccio [:marco] from comment #25) > The problem would be that an user could remove the application manually, > without an uninstaller. But we can decide not to support this use case. Indeed, we should not support this use case. Once the user starts mucking about with internals, there are too many things that can go wrong for us to effectively sync our state with their changes. > We could do something like this: > 1) When Firefox starts, check if the registry is up-to-date by checking if > the applications are locally installed. > 2) While Firefox is running, we could "watch" the installation directories > and get notified when they are removed. I think there are APIs to do that on > Windows, Mac and Linux. Yes, that's an option, althoughu we need to be careful about the performance implications of this approach. > Myk, I think you can review the patch, because I think it doesn't need other > changes. I'm not a DOM peer, so I can't review this. (Fabrice isn't a DOM peer either, but sicking has delegated review rights to him for the apps-specific DOM code, so he can do the review.) (In reply to Fabrice Desré [:fabrice] from comment #26) > Myk, that's fine, I didn't expected the out-of-sync issue to be fixed in > this bug! And I'm not sure what the best solution is. I kind of like the > sqlite trigger idea, but that conflicts with my planned refactoring using > indexedDB ;) It's conceivable to have different implementations on desktop and mobile, although I would very much like to avoid that situation. In any case, let's cross this bridge when we come to it.
Attachment #711654 - Flags: review?(myk) → review?(fabrice)
Attachment #711654 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
Attachment #711637 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
I confirm the fix is verified on FF 21.b3 on Mac OS 10.8 and Windows 7 x64. Build id: 20130416200523
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: