895708 - Trying to update a privileged packaged app with the same app origin fails with APP_STORE_VERSION_ROLLBACK error

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

STR

1. Install a privileged packaged app from marketplace
2. Update that privileged packaged app to a new version
3. Check for updates on device
4. Try to apply the update

Expected

The update should be successfully applied.

Actual

The update fails to be applied with an error stating APP_STORE_VERSION_ROLLBACK error.

07-18 17:35:49.957: E/GeckoConsole(108): Content JS INFO at app://system.gaiamobile.org/js/updatable.js:103 in anonymous: downloadError event, error code is APP_STORE_VERSION_ROLLBACK

The old privileged app & new privileged app are being attached shortly.

Comment 1

[Jason Smith [:jsmith]]

Attachment: Old Privileged App

Comment 2

[Jason Smith [:jsmith]]

Attachment: New Privileged App

Comment 3

[Jason Smith [:jsmith]]

Build: B2G 18 7/18 build
Device: Unagi

Comment 4

[Jason Smith [:jsmith]]

We need to figure out if this is a marketplace bug or client-side bug.

Antonio - Do you know if it's a client-side bug or a marketplace bug?

Comment 5

[Rob Hudson [:robhudson]]

It looks like APP_STORE_VERSION_ROLLBACK gets thrown when the old version is > the new version. But this doesn't seem to be the case in the zip files you attached.

Comment 6

[Jason Smith [:jsmith]]

Looks like this is a regression from bug 887118.

Antonio - Can you take a look into this?

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

Hi, I'll take a look but... I don't understand the STR:

1. Install a privileged packaged app from marketplace
2. Update that privileged packaged app to a new version
3. Check for updates on device
4. Try to apply the update

How many updates did you did? One at step 2 that worked and one at step 4 that failed? Do you still have the device that's failing? If so, can you pull /data/local/webapps/webapps.json from that device?

Comment 8

[Jason Smith [:jsmith]]

Badly worded. It was a single update. Should clarify STR:

1. Upload a packaged app with version 1 to marketplace dev
2. Publish the app
3. Install the app (with certs & prefs included on device)
4. On the app's profile on marketplace, upload a new packaged app with version 2
5. Publish the app
6. Check for updates on your device
7. Find an update available from version 1 --> version 2
8. Try applying the update

Result - Update fails with APP_STORE_VERSION_ROLLBACK error.

I'll pull a webapps.json file in a second.

Comment 9

[Jason Smith [:jsmith]]

Attachment: Webapps JSON

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Hmm according to the json you already have the latest version... can you pull out the /data/local/webapps/test.webapi.permissions.com/application.zip file also?

I'm going to upload both apps on our test store to see what happens also...

Comment 11

[Wayne Chang [:wchang]]

TRIAGE- leo+ given the user impact of failing to update an app. 
Please mark accordingly when confirmed if this is non-client side bug.

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

Sorry for the delay, I had an interesting morning.

It seems there are two different problems here. The first time you try to update the app, a 

E/GeckoConsole(  822): Content JS INFO at app://system.gaiamobile.org/js/updatable.js:103 in anonymous: downloadError event, error code is DUPLICATE_ORIGIN

from [1] (bug 852720) is thrown. But for some reason, when that fails the app version has already been updated with the new one, and so the next time you try it fails with:

E/GeckoConsole(  822): Content JS INFO at app://system.gaiamobile.org/js/updatable.js:103 in anonymous: downloadError event, error code is APP_STORE_VERSION_ROLLBACK

[1] https://mxr.mozilla.org/mozilla-b2g18/source/dom/apps/src/Webapps.jsm#2524

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

I'll have a patch for this later today.

Comment 14

[Antonio Manuel Amaya Calvo (:amac)]

The reason for part of this bug (the update failing with a different reason) is that currently when an app update fails the app object is half updated (it's updated till the part where the throw happens). That might leave an inconsistent object stored at webapps.json. 

Want me to fix that also (so the app object isn't modified if there's an error thrown) or do I file another bug for that?

Comment 15

[Jason Smith [:jsmith]]

Why would we be getting DUPLICATE_ORIGIN here though? The origin from old version of the app vs. new version of the app is the same. I thought that was allowed?

Comment 16

[Jason Smith [:jsmith]]

Attachment: App Pulled from Device Post Update

Comment 17

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jason Smith [:jsmith] from comment #15)
> Why would we be getting DUPLICATE_ORIGIN here though? The origin from old
> version of the app vs. new version of the app is the same. I thought that
> was allowed?

Yep, it should be. That's the other part of the bug and the one I'll fix on this bug for sure :)

Comment 18

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Allow updating an app with the same custom origin than it had previously

Comment 19

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Allow updating an app with the same custom origin than it had previously

This does two things:

1. Fixes the logic for updating apps with an origin. The way it's now is:

 * If it's an update, just check than the new origin is the same than the old one
 * If it's not an update, check than the origin doesn't already exist, set the newID and move the files.

2. Moves the version and id checking to be the last verification so we don't update the version if the origin validation failed.

Comment 20

[Antonio Manuel Amaya Calvo (:amac)]

In the end I just fixed the two problems. I don't think the rest of the partially updated data (in case of a thrown exception) forbids trying to reinstall the same update again as the version does, so it should be safe this way. And if not, I think it's better to do that on another bug anyway :).

Comment 21

[Jason Smith [:jsmith]]

Bonus points btw for including a mochitest here.

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jason Smith [:jsmith] from comment #21)
> Bonus points btw for including a mochitest here.

Hmm... I don't think I can do that without adding some way to skip or fake the signature verification... which would be dangerous. Or maybe... maybe I could generate a certificate on the fly and sign the app with that, although the packaged app tests don't actually use a packaged app. I would have to check it out.

In any case the packaged tests apps haven't even landed on 1.1 so... maybe file another no leo+ bug for 'Installing signed apps doesn't have any automated tests at the DOM level'?

Comment 23

[Jason Smith [:jsmith]]

Okay, makes sense. We have bug 880043 tracking the work there.

Comment 24

[[:fabrice] Fabrice Desré]

(In reply to Antonio Manuel Amaya Calvo from comment #22)
> (In reply to Jason Smith [:jsmith] from comment #21)
> > Bonus points btw for including a mochitest here.
> 
> Hmm... I don't think I can do that without adding some way to skip or fake
> the signature verification... which would be dangerous. Or maybe... maybe I
> could generate a certificate on the fly and sign the app with that, although
> the packaged app tests don't actually use a packaged app. I would have to
> check it out.

Packaged apps tests use packages, see file_packaged_app.sjs

> In any case the packaged tests apps haven't even landed on 1.1 so... maybe
> file another no leo+ bug for 'Installing signed apps doesn't have any
> automated tests at the DOM level'?

I don't care about having tests on the 1.1 branch. Having them on m-c is good enough, and we should really have one here.

Comment 25

[[:fabrice] Fabrice Desré]

Comment on attachment 778558 [details] [diff] [review]
Allow updating an app with the same custom origin than it had previously

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

That looks good to me, but we really need a test there.

::: dom/apps/src/Webapps.jsm
@@ +2587,5 @@
> +                    if (uri.prePath != app.origin) {
> +                      throw "INVALID_ORIGIN_CHANGE";
> +                    }
> +                    // Nothing else to do for an update... since the
> +                    // origin can't change we don't need to move the 

nit: trailing white space.

Comment 26

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #24)
> (In reply to Antonio Manuel Amaya Calvo from comment #22)
> > (In reply to Jason Smith [:jsmith] from comment #21)
> > > Bonus points btw for including a mochitest here.
> > 
> > Hmm... I don't think I can do that without adding some way to skip or fake
> > the signature verification... which would be dangerous. Or maybe... maybe I
> > could generate a certificate on the fly and sign the app with that, although
> > the packaged app tests don't actually use a packaged app. I would have to
> > check it out.
> 
> Packaged apps tests use packages, see file_packaged_app.sjs

I see it now, thanks.

> 
> > In any case the packaged tests apps haven't even landed on 1.1 so... maybe
> > file another no leo+ bug for 'Installing signed apps doesn't have any
> > automated tests at the DOM level'?
> 
> I don't care about having tests on the 1.1 branch. Having them on m-c is
> good enough, and we should really have one here.

In fact we should have tests for installing/uninstalling/whatever signed apps, not just for updating signed apps with an origin. And as Jason said, that's bug 880043. I think we could do that by taking a cue from [1] and from the current packaged apps tests on m-c.

But that's not here nor now. This bug is leo+ which means it blocks shipping the 1.1 devices, which are already undergoing certification. Which is why I finished writing the bug on a Friday late evening and why I'm answering this on a Sunday morning, cause I was under the impression that not only is this important, it is also urgent.

I took bug 880043 and will start working on it, but I can't make any promises on when it'll be done so I don't think we should block this one waiting on that. Bug 821589, when the current packaged apps tests (which don't include signed apps tests) were written cooked for 3 months. I don't think it'll take that much to add the signed app tests, but it's not going to be a day or two either. Which I guess is the reason why they weren't added originally when the code I modified was written ;).

Still, it's your call. I can fix the trailing space and resubmit the patch, or we can wait for the tests and I think then we should accept the risk that maybe will ship without this working.

[1] https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/

Comment 27

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Allow updating an app with the same custom origin than it had previously, v2 (nit changes)

This just includes the nit changes from the previous review.
Unit tests will be done on bug 880043 along with the rest of the signed packaged apps tests (there are none currently).
Please note that bug 880043 isn't leo (and at this moment it isn't even on the 1.2 backlog).

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/43d127288aa7

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43d127288aa7

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2d2d9c09d358

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/2d2d9c09d358

Comment 32

[Jason Smith [:jsmith]]

Verified on b2g18 on 7/30/2013 - I was successfully able to update a privileged packaged app with an origin specified.