Closed Bug 993011 Opened 6 years ago Closed 5 years ago

Existing Gaia apps fields are not updated when performing OTA

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
mozilla35
tracking-b2g backlog

People

(Reporter: gerard-majax, Assigned: gerard-majax)

Details

(Keywords: dataloss, Whiteboard: [systemsfe])

Attachments

(1 file, 3 obsolete files)

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.
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
Attached patch Hackish patch (obsolete) — Splinter Review
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.
Assignee: nobody → lissyx+mozillians
Status: NEW → ASSIGNED
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.
Flags: needinfo?(fabrice)
De-assigning myself from this for now.
Assignee: lissyx+mozillians → nobody
No longer blocks: 989876
Is this a regression?
Flags: needinfo?(lissyx+mozillians)
(In reply to Jason Smith [:jsmith] from comment #5)
> Is this a regression?

Nope, it's more like we never did it at all.
Flags: needinfo?(lissyx+mozillians)
In that case, I don't think we should block on this if it's already been present on shipped releases.
blocking-b2g: 2.0? → backlog
Whiteboard: [systemsfe]
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.
blocking-b2g: backlog → 2.0?
blocking-b2g: 2.0? → 2.0+
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+]
(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?
Flags: needinfo?(fabrice)
(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.
Thanks :). I'll prepare a patch that does copy all new fields but those.
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.
Flags: needinfo?(kgrandon)
Flags: needinfo?(jsmith)
Keywords: qawanted
Okay, sounds fine.
Flags: needinfo?(jsmith)
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.
Flags: needinfo?(kgrandon)
Assignee: nobody → lissyx+mozillians
Target Milestone: --- → 2.0 S4 (20june)
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
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.
No longer blocks: vertical-homescreen
blocking-b2g: 2.0+ → ---
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+]
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
blocking-b2g: --- → backlog
Target Milestone: 2.1 S3 (29aug) → ---
Alex, can we close this bug now?
Flags: needinfo?(lissyx+mozillians)
Priority: -- → P3
I'm working on a patch, sorry for the delay on this ! :(
Flags: needinfo?(lissyx+mozillians)
Attached patch Update existing Gaia apps fields (obsolete) — Splinter Review
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.
Attachment #8402768 - Attachment is obsolete: true
Attachment #8498012 - Attachment is obsolete: true
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.
Attachment #8498044 - Flags: review?(fabrice)
Attachment #8498044 - Flags: review?(fabrice) → review+
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 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
Attachment #8498223 - Flags: review+
Attachment #8498044 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fb0d1cc20298
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
issue resolved / cleaning old tags
Keywords: qawanted
blocking-b2g: backlog → ---
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.