993011 - Existing Gaia apps fields are not updated when performing OTA

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P3)

Description

[:gerard-majax]

During investigation on bug 989876, we noticed that setting a new value to the installTime field was not being reflected to /data/local/webapps/webapps.json file content.

Comment 1

[:gerard-majax]

The rationale we identified is the following. When we perform an OTA/FOTA update, Webapps code will trigger installSystemApps() code path at http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#507 (if the buildID has been updated).

When this happens, the system will pull infos from /system/b2g/webapps/webapps.json which contains infos from Gaia apps. It will use it to update its /data/local/webapps/webapps.json.
In rest this code, this.webapps is the future to be written /data/local/webapps/webapps.json while data is /system/b2g/webapps/webapps.json.

(1) The first for loop at http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#525 will remove the entry from this.webapps only in two cases: if the id is not in data or if it's a removable app. The first case matches the removal of a gaia app. 

This means that when we are updating gaia, i.e., when an app is still present in data, we will go to the next iteration, and thus not remove the entry from this.webapps.

(2) Then comes the second for loop at http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#540. This one will iterate over all id of the newly pushed apps. And we will populate fields by means of |this.webapps[id] = data[id];| only if |(!(id in this.webapps))|. This condition will be hit only in two cases: installation of a new gaia app, or when we went through the |delete| call in the previous for loop.

Now, in our case, we were updating icons of current Gaia apps. Homescreen cache update is triggered on updateTime value. If the webapps.json entry does not contains any updateTime value, then installTime is used. To be able to trigger the upgrade path code, installTime or updateTime of the updated Gaia apps must be set from /system/b2g/webapps/webapps.json.

Because of (1) and (2), we never enter the |this.webapps[id] = data[id];|:
 - (1) will never delete because the condition |id in data| will always match for existing gaia apps
 - (2) will never enter the if branch because the condition |!(id in this.webapps)| will never be true, since the id is effectively already there

Comment 2

[:gerard-majax]

Attachment: Hackish patch

The attached patch is a way to circumvent the issue. With this one applied, installTime field was correctly updated and reflected from /system/b2g/webapps/webapps.json to /data/local/webapps/webapps.json, and the icon upgrade performed correctly.

Comment 3

[:gerard-majax]

Fabrice, you know this code very well, and checking with Vivien, we feel there is probably a lot more fields we would like to update this way. I'm going to address the updateTime one first in bug 989876. We need your input on how to deal with it for all others fields, and more specifically, which fields.

Comment 4

[:gerard-majax]

De-assigning myself from this for now.

Comment 5

[Jason Smith [:jsmith]]

Is this a regression?

Comment 6

[:gerard-majax]

(In reply to Jason Smith [:jsmith] from comment #5)
> Is this a regression?

Nope, it's more like we never did it at all.

Comment 7

[Jason Smith [:jsmith]]

In that case, I don't think we should block on this if it's already been present on shipped releases.

Comment 8

[Kevin Grandon :kgrandon]

I think this is something we now need with the new homescreen. If we don't have this I believe the 'role' field will not be updated during upgrade, causing two homescreen options to appear in the settings app. Noming to block.

Comment 9

[:gerard-majax]

(In reply to Kevin Grandon :kgrandon from comment #8)
> I think this is something we now need with the new homescreen. If we don't
> have this I believe the 'role' field will not be updated during upgrade,
> causing two homescreen options to appear in the settings app. Noming to
> block.

We can easily add just updating the role field, but it would be better that we fix all fields at once.

Fabrice, do you know if there are fields we must not touch?

Comment 10

[[:fabrice] Fabrice Desré]

(In reply to Alexandre LISSY :gerard-majax from comment #9)
> (In reply to Kevin Grandon :kgrandon from comment #8)
> > I think this is something we now need with the new homescreen. If we don't
> > have this I believe the 'role' field will not be updated during upgrade,
> > causing two homescreen options to appear in the settings app. Noming to
> > block.
> 
> We can easily add just updating the role field, but it would be better that
> we fix all fields at once.
> 
> Fabrice, do you know if there are fields we must not touch?

Yes: localId, basePath, receipts, id. I don't see any good reason to touch installerAppId, installerIsBrowser, storeId and storeVersion either on an OTA, as this may mess up subsequent non-OTA app updates.

Comment 11

[:gerard-majax]

Thanks :). I'll prepare a patch that does copy all new fields but those.

Comment 12

[:gerard-majax]

I checked, restoring an old profile of mine:
> alex@portable-alex:~/codaz/B2G/backups/backup_20140605$ adb shell cat /data/local/webapps/webapps.json | grep -c '"role": "homescreen"'
> 2

Since we have verticalhome and homescreen app with the |"role": "homescreen"| role.

Then I boot an uptodate Gecko master:
> alex@portable-alex:~/codaz/B2G/backups/backup_20140605$ adb shell start b2g
> alex@portable-alex:~/codaz/B2G/backups/backup_20140605$ adb shell cat /data/local/webapps/webapps.json | grep -c '"role": "homescreen"'
> 1

Then once Gecko has started, the local webapps registry is updated fine and we only have verticalhome app with the homescreen role.

I'm doubtful this should block 2.0, as it seems we are updating the role field as expected.

I'd like QA to test updating a 1.4 to 2.0 and check whether there is a "Homescreens" entry within the Settings app. If there is, it means we are not properly updating the role app. If not, then this should not block vertical homescreen nor 2.0.

Comment 13

[Jason Smith [:jsmith]]

Okay, sounds fine.

Comment 14

[Kevin Grandon :kgrandon]

Thanks for checking on this Alex, that's super helpful. As you said on IRC this was probably due to the fact that we were not properly updating the role field for a while. This regressed after a backout, but we now have a test for it. Sorry about the confusion.

Comment 15

[Kevin Grandon :kgrandon]

Going to leave qawanted on this to verify upgrade paths for the vertical homescreen, but all signs are pointing to the fact that this no longer needs to block the vertical homescreen. We have had multiple people confirm the expected behavior (not having two homescreen options in settings). Unblocking and removing 2.0+ for now as it's not needed.

Comment 16

[Gregor Wagner [:gwagner]]

Alex, can we close this bug now?

Comment 17

[:gerard-majax]

I'm working on a patch, sorry for the delay on this ! :(

Comment 18

[:gerard-majax]

Attachment: Update existing Gaia apps fields

When performing an update, whether OTA or FOTA, some already existing
Gaia apps field may have changed. This happened in the past with bug
989876 where we lacked a proper updateTime value, and this may bite us
later. So we update all field, except some that we now must not be
changing.

Comment 19

[:gerard-majax]

Attachment: Update existing Gaia apps fields r=fabrice

When performing an update, whether OTA or FOTA, some already existing
Gaia apps field may have changed. This happened in the past with bug
989876 where we lacked a proper updateTime value, and this may bite us
later. So we update all field, except some that we now must not be
changing.

Comment 20

[:gerard-majax]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=331d27733b29

Comment 21

[:gerard-majax]

Attachment: Update existing Gaia apps fields r=fabrice

When performing an update, whether OTA or FOTA, some already existing
Gaia apps field may have changed. This happened in the past with bug
989876 where we lacked a proper updateTime value, and this may bite us
later. So we update all field, except some that we now must not be
changing.

Comment 22

[:gerard-majax]

Comment on attachment 8498223 [details] [diff] [review]
Update existing Gaia apps fields r=fabrice

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

r+ from fabrice, fixing just a nit because of tab instead of spaces

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0d1cc20298

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb0d1cc20298

Comment 25

[Joshua Mitchell (Inactive)]

issue resolved / cleaning old tags