836016 - Cannot reinstall an app after uninstalling it

Status: VERIFIED FIXED

(Firefox Graveyard :: Web Apps, defect)

Description

[krupa raj[:krupa]]

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

Comment 1

[Christopher Van Wiemeersch [:cvan]]

See test case: http://people.mozilla.org/~cwiemeersch/install.html

Reproducible on Firefox Nightly 21.0a1 (2013-01-29).

Comment 2

[Jason Smith [: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)

Comment 3

[Jason Smith [:jsmith]]

Yeah, this looks to be desktop specific. Can't reproduce on B2G.

Comment 4

[Alex Keybl [:akeybl]]

(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.

Comment 5

[Jason Smith [:jsmith]]

(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.

Comment 6

[Jason Smith [:jsmith]]

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.

Comment 7

[Bill Walker [:bwalker] [@wfwalker]]

(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.

Comment 8

[Jason Smith [:jsmith]]

Bill says he'll take ownership of this to find someone to fix this.

Comment 9

[Alex Keybl [:akeybl]]

Given comment 5, we'll track.

Comment 10

[Marco Castelluccio [:marco]]

Attachment: Patch

Myk, what do you think of this approach?

Comment 11

[Marco Castelluccio [:marco]]

Attachment: Second opportunity

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.

Comment 12

[Julien Wajsberg [:julienw]]

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 ?

Comment 13

[Marco Castelluccio [:marco]]

(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.

Comment 14

[Julien Wajsberg [:julienw]]

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.

Comment 15

[Marco Castelluccio [:marco]]

(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?

Comment 16

[Jason Smith [:jsmith]]

(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 17

[[:fabrice] Fabrice Desré]

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.

Comment 18

[[:fabrice] Fabrice Desré]

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.

Comment 19

[Marco Castelluccio [:marco]]

(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?

Comment 20

[Julien Wajsberg [:julienw]]

Provide an uninstall program that would correctly clean up things in our Webapps implementation ?

Comment 21

[Marco Castelluccio [:marco]]

(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.

Comment 22

[Julien Wajsberg [:julienw]]

Yep I admit that would not be perfect and we'd still need to guard against that.

maybe fabrice will habve a better idea.

Comment 23

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

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.

Comment 24

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

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 25

[Marco Castelluccio [:marco]]

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.

Comment 26

[[:fabrice] Fabrice Desré]

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 27

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

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b5ccfcda7e7

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b5ccfcda7e7

Comment 30

[Mihai Morar, (:MihaiMorar)]

I confirm the fix is verified on FF 21.b3 on Mac OS 10.8 and Windows 7 x64.
Build id: 20130416200523