Closed
Bug 993011
Opened 10 years ago
Closed 10 years ago
Existing Gaia apps fields are not updated when performing OTA
Categories
(Core Graveyard :: DOM: Apps, defect, P3)
Tracking
(tracking-b2g:backlog)
People
(Reporter: gerard-majax, Assigned: gerard-majax)
Details
(Keywords: dataloss, Whiteboard: [systemsfe])
Attachments
(1 file, 3 obsolete files)
1.83 KB,
patch
|
gerard-majax
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Comment 2•10 years ago
|
||
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
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
De-assigning myself from this for now.
Assignee: lissyx+mozillians → nobody
Assignee | ||
Comment 6•10 years ago
|
||
(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)
Comment 7•10 years ago
|
||
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
Updated•10 years ago
|
Whiteboard: [systemsfe]
Comment 8•10 years ago
|
||
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?
Updated•10 years ago
|
Blocks: vertical-homescreen
blocking-b2g: 2.0? → 2.0+
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+]
Assignee | ||
Comment 9•10 years ago
|
||
(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)
Comment 10•10 years ago
|
||
(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.
Assignee | ||
Comment 11•10 years ago
|
||
Thanks :). I'll prepare a patch that does copy all new fields but those.
Assignee | ||
Comment 12•10 years ago
|
||
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 14•10 years ago
|
||
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)
Updated•10 years ago
|
Assignee: nobody → lissyx+mozillians
Target Milestone: --- → 2.0 S4 (20june)
Updated•10 years ago
|
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Comment 15•10 years ago
|
||
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+ → ---
Updated•10 years ago
|
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+]
Updated•10 years ago
|
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Updated•10 years ago
|
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Updated•10 years ago
|
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Updated•10 years ago
|
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Updated•10 years ago
|
blocking-b2g: --- → backlog
Target Milestone: 2.1 S3 (29aug) → ---
Comment 16•10 years ago
|
||
Alex, can we close this bug now?
Flags: needinfo?(lissyx+mozillians)
Priority: -- → P3
Assignee | ||
Comment 17•10 years ago
|
||
I'm working on a patch, sorry for the delay on this ! :(
Flags: needinfo?(lissyx+mozillians)
Assignee | ||
Comment 18•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8402768 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8498012 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8498044 -
Flags: review?(fabrice)
Assignee | ||
Comment 20•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=331d27733b29
Updated•10 years ago
|
Attachment #8498044 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 21•10 years ago
|
||
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.
Assignee | ||
Comment 22•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #8498044 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 23•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0d1cc20298
Keywords: checkin-needed
Comment 24•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fb0d1cc20298
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•