837776 - First-time apps installation is buggy

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[Julien Wajsberg [:julienw]]

Fixing Bug 834618 I found several things that seems strange for me. It may be necessary to split this in several bugs but I'd let Fabrice decide.

1. in [1] we set |app.removable = true| _only_ for packaged app. As far as I understand, our core packaged app don't get this because they don't have an |update.webapp| file and as a result because of [2] and [3] they get an early return. I believe that we should have a more reliable way to decide which app should be copied in /data/local/webapps or not.

Here Maps has been copied and got |app.removable = true|.

2. In the same function ([4]) we still copy all hosted apps to /data/local/webapps, but they keep |app.removable = false|. As a result, in [5], we checkForUpdate on all apps, including these one, onlyUpdateAppCache in [6] stays false because baseDir was rewritten in [4], and we end up automatically changing the manifest on hosted app without app cache (like the marketplace). However, this new manifest is never read because removable stayed false, and therefore we always load the manifest in /system/b2g.

3. BTW, when "onlyUpdateAppCache" is true, we end up sending the error "NOT_UPDATABLE" when there is no update available, I believe we'd rather send an event "downloadapplied".

So, I believe that 1. is not for v1, but that 2. and 3. should be fixed.

Fabrice, what is your opinion on this ?

[1] http://hg.mozilla.org/mozilla-central/file/2a8e243711a9/dom/apps/src/Webapps.jsm#l285
[2] http://hg.mozilla.org/mozilla-central/file/2a8e243711a9/dom/apps/src/Webapps.jsm#l247
[3] http://hg.mozilla.org/mozilla-central/file/2a8e243711a9/dom/apps/src/Webapps.jsm#l252
[4] http://hg.mozilla.org/mozilla-central/file/2a8e243711a9/dom/apps/src/Webapps.jsm#l270
[5] http://hg.mozilla.org/mozilla-central/file/2a8e243711a9/b2g/components/UpdatePrompt.js#l454
[6] http://hg.mozilla.org/mozilla-central/file/2a8e243711a9/dom/apps/src/Webapps.jsm#l1385

Comment 1

[[:fabrice] Fabrice Desré]

(In reply to Julien Wajsberg [:julienw] from comment #0)
> Fixing Bug 834618 I found several things that seems strange for me. It may
> be necessary to split this in several bugs but I'd let Fabrice decide.
> 
> 1. in [1] we set |app.removable = true| _only_ for packaged app. As far as I
> understand, our core packaged app don't get this because they don't have an
> |update.webapp| file and as a result because of [2] and [3] they get an
> early return. I believe that we should have a more reliable way to decide
> which app should be copied in /data/local/webapps or not.
> 
> Here Maps has been copied and got |app.removable = true|.

That's what we want. We don't want to promote the possibility for 3rd parties to bundle lots of uninstallable apps.

> 2. In the same function ([4]) we still copy all hosted apps to
> /data/local/webapps, but they keep |app.removable = false|. As a result, in
> [5], we checkForUpdate on all apps, including these one, onlyUpdateAppCache
> in [6] stays false because baseDir was rewritten in [4], and we end up
> automatically changing the manifest on hosted app without app cache (like
> the marketplace). However, this new manifest is never read because removable
> stayed false, and therefore we always load the manifest in /system/b2g.

I would prefer hosted ones to also be unremovable, but the Marketplace is the elephant in the room here.

> 3. BTW, when "onlyUpdateAppCache" is true, we end up sending the error
> "NOT_UPDATABLE" when there is no update available, I believe we'd rather
> send an event "downloadapplied".

I don't think that claiming we applied an update when we did nothing is appropriate, but this is a very minor point anyway. I can buy that not having anything to update is not an update per-se, but this is something that should be fixed when we get a proper spec defining what the behavior should be.

Comment 2

[Julien Wajsberg [:julienw]]

(In reply to Fabrice Desré [:fabrice] from comment #1)
> 
> I don't think that claiming we applied an update when we did nothing is
> appropriate, but this is a very minor point anyway. I can buy that not
> having anything to update is not an update per-se, but this is something
> that should be fixed when we get a proper spec defining what the behavior
> should be.

Filed Bug 838137. I agree it's minor and it's really not related to the other points anyway.

Comment 3

[Julien Wajsberg [:julienw]]

(In reply to Fabrice Desré [:fabrice] from comment #1)
> (In reply to Julien Wajsberg [:julienw] from comment #0)

> > 1. in [1] we set |app.removable = true| _only_ for packaged app. As far as I
> > understand, our core packaged app don't get this because they don't have an
> > |update.webapp| file and as a result because of [2] and [3] they get an
> > early return. I believe that we should have a more reliable way to decide
> > which app should be copied in /data/local/webapps or not.
> > 
> > Here Maps has been copied and got |app.removable = true|.
> 
> That's what we want. We don't want to promote the possibility for 3rd
> parties to bundle lots of uninstallable apps.

I do agree that for Here Maps this is what we want, but I disagree with the heuristic. I don't get why packaged apps with a update manifest gets |app.removable = true| and not the hosted apps.

Here we get a situation where the Marketplace gets copied in /data/local, it's basePath is set to /data/local/webapps, and |app.removable| is still false. This is what looks buggy to me, and that triggers my point 2, and that's why I put both of them in the same bug.

> 
> > 2. In the same function ([4]) we still copy all hosted apps to
> > /data/local/webapps, but they keep |app.removable = false|. As a result, in
> > [5], we checkForUpdate on all apps, including these one, onlyUpdateAppCache
> > in [6] stays false because baseDir was rewritten in [4], and we end up
> > automatically changing the manifest on hosted app without app cache (like
> > the marketplace). However, this new manifest is never read because removable
> > stayed false, and therefore we always load the manifest in /system/b2g.
> 
> I would prefer hosted ones to also be unremovable, but the Marketplace is
> the elephant in the room here.

Soooo the main question is : should the Marketplace be updatable ?

Right now, the manifest for the marketplace is downloaded, stored but not used (because removable is false, then it reads it in /system/b2g even if basePath is set to anything else). If we don't want to update the Marketplace then I'm ok with leaving it like this for now.

Comment 4

[[:fabrice] Fabrice Desré]

We don't want to update the marketplace manifest. We only want to update the appcache if any.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking based on #2 & #3 in comment 0

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.