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

RESOLVED FIXED in Firefox 25

Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jsmith, Assigned: amac)

Tracking

({regression})

Trunk
mozilla25
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [LeoVB+])

Attachments

(5 attachments, 2 obsolete attachments)

Reporter

Description

6 years ago
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.
Reporter

Comment 1

6 years ago
Posted file Old Privileged App
Reporter

Comment 2

6 years ago
Posted file New Privileged App
Reporter

Comment 3

6 years ago
Build: B2G 18 7/18 build
Device: Unagi
Reporter

Comment 4

6 years ago
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?
Flags: needinfo?(amac)
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.
Reporter

Comment 6

6 years ago
Looks like this is a regression from bug 887118.

Antonio - Can you take a look into this?
Blocks: 887118
blocking-b2g: --- → leo?
Keywords: regression
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?
Flags: needinfo?(amac)
Reporter

Comment 8

6 years ago
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.
Reporter

Comment 9

6 years ago
Posted file Webapps JSON
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...
TRIAGE- leo+ given the user impact of failing to update an app. 
Please mark accordingly when confirmed if this is non-client side bug.
blocking-b2g: leo? → leo+
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
Assignee: nobody → amac
Blocks: 852720
I'll have a patch for this later today.
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?
Flags: needinfo?(fabrice)
Reporter

Comment 15

6 years ago
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?
(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 :)
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.
Attachment #778557 - Attachment is obsolete: true
Attachment #778558 - Flags: review?(fabrice)
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 :).
Flags: needinfo?(fabrice)
No longer blocks: 887118
Reporter

Comment 21

6 years ago
Bonus points btw for including a mochitest here.
(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'?
Reporter

Comment 23

6 years ago
Okay, makes sense. We have bug 880043 tracking the work there.
(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 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.
Attachment #778558 - Flags: review?(fabrice)
(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/
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).
Attachment #778558 - Attachment is obsolete: true
Attachment #779100 - Flags: review?(fabrice)
Attachment #779100 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/43d127288aa7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Reporter

Updated

6 years ago
Keywords: verifyme
QA Contact: jsmith
Reporter

Comment 32

6 years ago
Verified on b2g18 on 7/30/2013 - I was successfully able to update a privileged packaged app with an origin specified.
Whiteboard: [LeoVB+]

Updated

2 years ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.