Closed Bug 989876 Opened 10 years ago Closed 10 years ago

Generate an updateTime value for Gaia apps

Categories

(Firefox OS Graveyard :: Gaia::Build, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::Build
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.0+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Status:
RESOLVED FIXED
Milestone:
1.4 S6 (25apr)
Project Flags:
blocking-b2g 2.0+
Tracking Flags:
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

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

References

Details

(Keywords: dataloss, Whiteboard: [systemsfe])

Attachments

(2 files, 2 obsolete files)

Link to Github https://github.com/mozilla-b2g/gaia/pull/18075
46 bytes, text/x-github-pull-request
yurenju
: review+
bajaj
: approval-gaia-v1.4+
Details | Review
0001-Bug-989876-Keep-updateTime-field-uptodate-for-gaia-a.patch
1.15 KB, patch
gerard-majax
: review+
bajaj
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
Updating my device (Nexus S) to current Gecko/Gaia as of march 29th, I don't get the refreshed icons on the homescreen. The new icons are shown at some moments:
 - when launching an app
 - when selecting an app from activity chooser

But on the homescreen grid, icons remains the old ones.

STR:
 0. Build a Gecko/Gaia MAR update package
 1. Push and apply the update

Expected:
 Icons on the homescreen grid are new ones

Actual:
 Icons are still the old look.
It works if you do |make reset-gaia| so it seems that is because of the icon path is the same than before and also the updateTime has not been updated when the homescreen receives the app object

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/page.js#L435

The homescreen has the icon blobs stored on indexedDB and it refreshes them when the icon path or the updateTime prop changes.
(In reply to Cristian Rodriguez (:crdlc) from comment #1)
> It works if you do |make reset-gaia| so it seems that is because of the icon
> path is the same than before and also the updateTime has not been updated
> when the homescreen receives the app object
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/page.
> js#L435
> 
> The homescreen has the icon blobs stored on indexedDB and it refreshes them
> when the icon path or the updateTime prop changes.

I suspected so. Still, it means we have an upgrade bug there.
Keywords: dataloss
Whiteboard: [systemsfe]
What was the bug that implemented these new icons? We need to mark this bug as blocking that one. And maybe back it out if it's not a trivial patch.
Blocks: 975535
Thanks!

Sam: Cristian: Any of you plan to work on this soon? The feature is not properly implemented until we have this fixed. Right now, we see a mix of old icons and new icons.
Flags: needinfo?(sjochimek)
Flags: needinfo?(crdlc)
I don't exactly follow this bug. Are you saying that the update package has a different set of homescreen icons than the current homescreen apps? Trying to figure out how we can test this via a system update.
(In reply to Anthony Ricaud (:rik) from comment #5)
> Thanks!
> 
> Sam: Cristian: Any of you plan to work on this soon? The feature is not
> properly implemented until we have this fixed. Right now, we see a mix of
> old icons and new icons.

I cannot do anything per comment 1. If the path of the icons in the manifest are the same and the updateTime has not changed by apps, the issue is not on the homescreen.
Flags: needinfo?(crdlc)
(In reply to Jason Smith [:jsmith] from comment #6)
> I don't exactly follow this bug. Are you saying that the update package has
> a different set of homescreen icons than the current homescreen apps? Trying
> to figure out how we can test this via a system update.

The icons was modified in this commit: 

https://github.com/mozilla-b2g/gaia/commit/16eac40bf31853f1d0d56aa04d158762609f45ef

The homescreen stores icons in order to be faster when it is rendered. The homescreen will update the icons if the updateTime flag in app object changes when the mozApps returns it or the path in the manifest changes. No changes in those conditions so the homescreen remains the old ones.
How can we change the updatimeFlag ?
Flags: needinfo?(crdlc)
(In reply to Jason Smith [:jsmith] from comment #6)
> I don't exactly follow this bug. Are you saying that the update package has
> a different set of homescreen icons than the current homescreen apps? Trying
> to figure out how we can test this via a system update.

Jason, just install a b2g from before the landing of those new icons, then perform an upgrade applying a MAR update package.
No idea Anthony, it is provided by Gecko by means of 

navigator.mozApps.mgmt.getAll().onsuccess = function onsuccess(event) {
    var apps = event.target.result;
    apps.forEach(function eachApp(app) {
       console.log(app.updateTime);
    });
}
Forcing updateTime to -1 in WebappsApplication, and the icons are updated. This is clearly a Gecko issue then.
Component: Gaia::Homescreen → DOM: Apps
Product: Firefox OS → Core
I'm wondering if the bug is really in Gecko. Why is Homescreen caching that icon but the notification code is not? Or the "launch app" screen?
I don't know why notifications or "launch app" screen don't work in this sense (maybe it is not needed as requirement). Homescreen stores it because we create canvases to generate shadows and other effects. Also, mozApps is slow and storing these blobs, the homescreen is rendered very fast after FTE finishes or after crashing. The bug is NOT on the homescreen.
Flags: needinfo?(crdlc)
I have some fancy STR and confirmation in bug 992777.

This bug is indeed a Homescreen bug because the app keep the icons in it's own storage. Removing the storage will force it to reload from app package. Homescreen app need to update the icons when the app(s) is/are updated.
Component: DOM: Apps → Gaia::Homescreen
Product: Core → Firefox OS
1.5? because it's an incomplete implementation prevents OTA upgrade.
blocking-b2g: --- → 1.5?
Flags: needinfo?(sjochimek) → needinfo?(crdlc)
I think Cristan's point was that the homescreen app was already checking for the timestamp, and that the issue is this timestamp is not updated in this case, for those apps. Hence, no upgrade is triggered.
Ouch, I didn't comment 12 to comment 14.

I wonder what's the timestamp come from? We do have a constant timestamp in Gaia build script.
https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-manifests.js#L6

Do we need to bump that number? When should we bump that number? Can we bump that number automatically on every build?
Component: Gaia::Homescreen → DOM: Apps
Flags: needinfo?(crdlc)
Product: Firefox OS → Core
The feature was implemented in bug 810518.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #19)
> Ouch, I didn't comment 12 to comment 14.
> 
> I wonder what's the timestamp come from? We do have a constant timestamp in
> Gaia build script.
> https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-manifests.js#L6
> 
> Do we need to bump that number? When should we bump that number? Can we bump
> that number automatically on every build?

Nice finding. This is indeed the value I see on my dogfooding Nexus S that exposes the upgrade bug. Both in /system/b2g/webapps/webapps.json and /data/local/webapps/webapps.json
Vivien, please read comment 18. Do you know why we keep this timestamp a constant in the build script? Would anything bad happen if I change it to |Date.now()|?
Flags: needinfo?(21)
This comes from bug 883510
The value is also hardcoded without INSTALL_TIME at https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-manifests.js#L278
(In reply to Alexandre LISSY :gerard-majax from comment #25)
> This comes from bug 883510

No. This come from https://github.com/vingtetun/gaia/commit/b6fbc787b2d4e2c3d2464d8c936595df1d02e970

So back in the days in March 2012 I added that in order to remove the static version of webapps.json and generate it automatically based on the available apps/.

I'm pretty sure there is no reasons to have a static value. At that point in time it was pretty early in our moves to real webapps, and I probably didn't think at all about this flag beeing used for updates.
Flags: needinfo?(21)
Great, that means we can solve this bug by replace the value with |Date.now()|.

This is now a Gaia::Build bug, but everyone is welcome to steal it before office hours tomorrow in Taipei :)
Component: DOM: Apps → Gaia::Build
Product: Core → Firefox OS
I think we have two bugs there:
 - gaia build hardcoded installTime
 - gecko not correctly updating webapps.json

I did a MAR update on my Nexus S after hacking the Gaia side: changing |const INSTALL_TIME| to Date.now(), removing the 132333986000 hard coded value from anywhere.

The values in /system/b2g/webapps/webapps.json are okay and reflect the changes. But the values from /data/local/webapps/webapps.json are not and installTime, for the Gaia apps, is still at 132333986000.
So there is indeed at least two bugs after investigating with Vivien's help:
 (1) we do not set an updateTime value for the Gaia apps

Then, since we do not set this value, the API relies on the installTime value to populate the updateTime value. Which is the other bug:
 (2) Existing Gaia apps fields are not updated when performing OTA

I'm morphing the current bug into (1), and filing a bug for (2).
Summary: New icons are not updated on homescreen grid → Generate an updateTime value for Gaia apps
Assignee: nobody → lissyx+mozillians
Depends on: 993011
Once bug 993011 is fixed, we can fix this one. I changed the name because after discussing with Vivien, we felt that installTime was not the correct semantic to express correctly the updates. The plan would be to:
 - set installTime to some hard coded value, whether the current hardcoded value or something like 0, or another one
 - introduce an updateTime field as for other webapps, that would express the date of the build (taking its value from Date.now() for example).
(In reply to Alexandre LISSY :gerard-majax from comment #31)
> Once bug 993011 is fixed, we can fix this one. I changed the name because
> after discussing with Vivien, we felt that installTime was not the correct
> semantic to express correctly the updates. The plan would be to:
>  - set installTime to some hard coded value, whether the current hardcoded
> value or something like 0, or another one
>  - introduce an updateTime field as for other webapps, that would express
> the date of the build (taking its value from Date.now() for example).

This seems overly complicated IMHO. It makes sense for the updateTime of the preload app to be the last OTA time, AND to be the build time if it's never updated. Why do we need a hardcode value and a installTime property?

0 might make a little bit sense, but it actually means Unix Epoch time and you installed an app in 1970.
After talking to :gerard-majax on IRC I realized I was confused with installTime and updateTime.

So here is my intention:

- updateTime of the preload app: The last OTA time, or |Date.now()| time generated by Gaia build script if it's never updated.
- installTime of the preload app: |Date.now()| time generated by Gaia build script.

From comment 1 I can see the homescreen app is indeed checking |updateTime|, not |installTime|. No patch needed in Gaia application code, as we previous find out.

So if I understand this bug correctly, what we are asking is to have Gaia build script to populate BOTH |installTime| and |updateTime| in the webapps.json file with |Date.now()| timestamp? If so this can be a ready happy good first bug.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #33)
> After talking to :gerard-majax on IRC I realized I was confused with
> installTime and updateTime.
> 
> So here is my intention:
> 
> - updateTime of the preload app: The last OTA time, or |Date.now()| time
> generated by Gaia build script if it's never updated.
> - installTime of the preload app: |Date.now()| time generated by Gaia build
> script.
> 
> From comment 1 I can see the homescreen app is indeed checking |updateTime|,
> not |installTime|. No patch needed in Gaia application code, as we previous
> find out.
> 
> So if I understand this bug correctly, what we are asking is to have Gaia
> build script to populate BOTH |installTime| and |updateTime| in the
> webapps.json file with |Date.now()| timestamp? If so this can be a ready
> happy good first bug.

Yes, I think this is what we want.
Flags: needinfo?(21)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #32)
> (In reply to Alexandre LISSY :gerard-majax from comment #31)
> > Once bug 993011 is fixed, we can fix this one. I changed the name because
> > after discussing with Vivien, we felt that installTime was not the correct
> > semantic to express correctly the updates. The plan would be to:
> >  - set installTime to some hard coded value, whether the current hardcoded
> > value or something like 0, or another one
> >  - introduce an updateTime field as for other webapps, that would express
> > the date of the build (taking its value from Date.now() for example).
> 
> This seems overly complicated IMHO. It makes sense for the updateTime of the
> preload app to be the last OTA time, AND to be the build time if it's never
> updated. Why do we need a hardcode value and a installTime property?
> 
> 0 might make a little bit sense, but it actually means Unix Epoch time and
> you installed an app in 1970.

I don't mind that much about the value of the installTime as long it is not updated when there is an update, and the app was previously installed.

My rationale is that installTime really means when the app has been installed, and I can imagine a feature on a third party homescreen where apps that has been installed in the last 20 minutes or so are move on a special page, to make it easy to retrieve them, instead of having to found it inside the potential many pages of apps on the homescreeen.
Flags: needinfo?(21)
Please find attached the Gecko part of this fix, which makes sure that at least updateTime is updated.
Attachment #8403244 - Flags: review?(fabrice)
Please find attached a link to the github pull request for the gaia part
Attachment #8403254 - Flags: review?(yurenju.mozilla)
Comment on attachment 8403244 [details] [diff] [review]
0001-Bug-989876-Keep-updateTime-field-uptodate-for-gaia-a.patch

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

::: dom/apps/src/Webapps.jsm
@@ +556,5 @@
> +        } else {
> +	  // we fall into this case if the app is present in /system/b2g/webapps/webapps.json
> +	  // and in /data/local/webapps/webapps.json: this happens when updating gaia apps
> +	  // Confere bug 989876
> +          this.webapps[id].updateTime = data[id].updateTime;

can you also set lastUpdateCheck ? We added that for the homescreen at some point...
setting it to updateTime or Date.Now() is fine for me.
Attachment #8403244 - Flags: review?(fabrice) → review+
Comment on attachment 8403254 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/18075

looks good, thank you!
Attachment #8403254 - Flags: review?(yurenju.mozilla) → review+
Carrying r+ from fabrice, adding lastUpdateCheck.
Attachment #8403244 - Attachment is obsolete: true
Attachment #8403829 - Flags: review+
Applying both those patches, making sure to rebuild a MAR OTA package from scratch, installing it on my Nexus S, and homescreen icons are migrated. Be aware it seems there might be some latency for the upgrade process to take place: you may get the old icon for a couple of seconds/minutes.
Carrying r+, reformatting to HG.
Attachment #8403829 - Attachment is obsolete: true
Attachment #8403846 - Flags: review+
Keywords: checkin-needed
This bug still needs the Gecko part to be merged. Once the gecko part is merged, it can be marked as fixed.
That should now be fixed. I've updated my Peak and Keon and they now have the new icons.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
blocking-b2g: 2.0? → 2.0+
Per discussion with Gregor - this is a required followup to uplift bug 975535.
blocking-b2g: 2.0+ → 1.4+
:gwagner just pinged me that this needs to be uplifted to 1.4 as its blocking Bug 975535 which was approved to get landed. Gregor/:gerard although we have passed the time where we may be granting approvals given this should have been uplifted a week back we can consider this.

Please request approval-mozilla-aurora on the attachment that needs to get landed on gecko side. Depending on the user impact and risk we can a+ this or do an entire backout of Bug 975535 and its dependencies keeping in mind the timeline and since these seem to be nice-to-have's for 1.4 and are not blockers.
:gwagner is also investigating if  993011 is a needed dependency here.
blocking-b2g: 1.4+ → 2.0+
Flags: needinfo?(lissyx+mozillians)
(In reply to bhavana bajaj [:bajaj] from comment #50)
> :gwagner just pinged me that this needs to be uplifted to 1.4 as its
> blocking Bug 975535 which was approved to get landed. Gregor/:gerard
> although we have passed the time where we may be granting approvals given
> this should have been uplifted a week back we can consider this.
> 
> Please request approval-mozilla-aurora on the attachment that needs to get
> landed on gecko side. Depending on the user impact and risk we can a+ this
> or do an entire backout of Bug 975535 and its dependencies keeping in mind
> the timeline and since these seem to be nice-to-have's for 1.4 and are not
> blockers.
> :gwagner is also investigating if  993011 is a needed dependency here.

Also see a gaia part here which may require a gaia approval.
I just confirmed with Alex. bug 993011 is not blocking this bug. We are good to go.
No longer depends on: 993011
Comment on attachment 8403254 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/18075

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): existing bug
[User impact] if declined: updated apps won't get a new icon.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): close to zero.
[String changes made]: none
Attachment #8403254 - Flags: approval-gaia-v1.4?
Comment on attachment 8403846 [details] [diff] [review]
0001-Bug-989876-Keep-updateTime-field-uptodate-for-gaia-a.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): existing bug
User impact if declined: updated apps can't get new icon
Testing completed (on m-c, etc.):  yes
Risk to taking this patch (and alternatives if risky): almost zero
String or IDL/UUID changes made by this patch: none
Attachment #8403846 - Flags: approval-mozilla-aurora?
(In reply to Gregor Wagner [:gwagner] from comment #53)
> Comment on attachment 8403254 [details] [review]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/18075
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): existing bug
> [User impact] if declined: updated apps won't get a new icon.
> [Testing completed]: yes
> [Risk to taking this patch] (and alternatives if risky): close to zero.
> [String changes made]: none

Given the bake time here on m-c/master and since no fallouts were seen and keeping the risk/user impact in mind (dataloss for user) if not uplifted, approving this. Also there should be no more dependencies left here as :gwagner confirmed.
Attachment #8403254 - Flags: approval-gaia-v1.4? → approval-gaia-v1.4+
Attachment #8403846 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(lissyx+mozillians)
https://hg.mozilla.org/releases/mozilla-aurora/rev/93904cc2ad7f

Leaving status-b2g-v1.4 as affected for the Gaia uplift.
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → fixed
status-firefox29: --- → wontfix
status-firefox30: --- → fixed
status-firefox31: --- → fixed
Keywords: leave-open
Target Milestone: --- → 1.4 S6 (25apr)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: