1144689 - Apps preinstalled with a preloaded appcache are not updated

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Antonio Manuel Amaya Calvo (:amac)]

STR:
1. Generate a build that includes a preloaded hosted app that uses appcache. Use the preload.py script at [1] to preload the app on the build
2. Flash the build, boot the phone, access the app without network. Check that the app works correctly.
3. Modify something on the hosted app (doesn't matter what, a CSS, an HTML, something). You can actually do the step 3 *before* doing step 2 (and the phone could have been flashed half a year ago!)
4. Enable the network on the phone and access the app again

EXPECTED:
The phone should detect the changes and offer to update the app when the user launches the app (or once per day when the updates are checked, whatever happens sooner).

ACTUAL:

The update is not offered ever.

CAUSE:
When we generate the app cache from the preloaded files included in system/b2g/webapps/whatever/cache we're setting the lastFetched and lastModified date to whatever the date on the phone is, and expirationDate to the end of time (which apparently will be at some time in 2106). 

We should store with the build the actual time the files were fetched and whatever the modified time was at that time, so when we hit the network updates are offered correctly.


[1] https://github.com/mozilla-b2g/preload-app-toolkit

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V1. Allow setting manually a fetch and last modified time for cache entries

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] V1. Add the resource_metadata.json file to the build

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8579392 [details] [review]
[gaia] V1. Add the resource_metadata.json file to the build

Dunno who can review this since Yurenju seems to not be available.

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Fix the preload script so it adds the resource metadata (times)

Same as with the gaia patch, dunno who can review this, Fabrice?

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8579388 [details] [diff] [review]
V1. Allow setting manually a fetch and last modified time for cache entries

I don't understand the motivation here.  Why do you need these metadata to update manually?  

If there is a problem the offline cache update doesn't see/recognize updated resources on the server I don't think it's because of these metadata being set to something you don't want.  It might be a different bug we should find and fix.

Also, it's against meaning of the lastFetched metadata to be set to something *before* the entry has actually been fetched.

Comment 6

[Honza Bambas (:mayhemer)]

I'm doing some more tests.  In nsHttpChannel::OnOfflineCacheEntryForWritingAvailable we read LastModified property of entries we want to write to (stored at mOfflineCacheLastModifiedTime).  These entries are tho always new and the value is always 0.  Reading the property is probably just a legacy code that should be removed anyway.  In nsHttpChannel::ShouldUpdateOfflineCacheEntry (during normal offline cache updates) we never reach the code that works with mOfflineCacheLastModifiedTime member.


BTW, in your step 3 you don't say anything about modifying the manifest byte content - a precondition to start an offline cache update.  Are you doing that?

Also, what exactly leads you to a conclusion that LastModified and LastFetched are the properties you need to update?

Comment 7

[Honza Bambas (:mayhemer)]

Please also don't mix lastModified metadata on the entry with Last-Modified response headers.  Those two are completely different thing.

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Comment on attachment 8579388 [details] [diff] [review]
> V1. Allow setting manually a fetch and last modified time for cache entries
> 
> I don't understand the motivation here.  Why do you need these metadata to
> update manually?  

So the way AppCache preloading works for preinstalled apps is:

* At some point T in time, the build is generated. At that point, the resources are fetched from the network (so last fetched should be T). And the resources fetched have a last modified time each of them (let's call them LMTi). 
* The build is burned on the phones.
* At a later point in time T1, the phone is started up for the first time. The phone has a configured time T2 which might be earlier than T1, or later, or whatever.
* The entries are added to the cache with:

  Last Modified = T2
  Last Fetched = T2
  Expires = 0xFFFFFFFF

With that data if the resources where modified between T and T1 (now in other words), they are *not* refreshed on the phone when the phone gets the network data back.

What the patch does is to store T and LMTi for each of the resources with the correct values (when they were *actually* fetched and modified) and then at the first run time it set's those values manually to the correct ones. Doing that, if the resources are modified then the update is offered correctly to the user and the new version is downloaded.

> 
> If there is a problem the offline cache update doesn't see/recognize updated
> resources on the server I don't think it's because of these metadata being
> set to something you don't want.  It might be a different bug we should find
> and fix.

I don't know. I made the change this way because I noticed that the only difference between manually installing the application and preloading it was precisely the metadata that was being stored (correct dates and expiration time against incorrect ones). And with the patch applied, the resource changes are recognized and processed correctly (changing only those data)

> Also, it's against meaning of the lastFetched metadata to be set to
> something *before* the entry has actually been fetched.

And that's exactly what the patch does, set it to the *actual* date when the entry was fetched, as opposed to the current phone date which might be earlier or later (we don't actually know unless we store that data with the build which what the other patches do).

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> I'm doing some more tests.  In
> nsHttpChannel::OnOfflineCacheEntryForWritingAvailable we read LastModified
> property of entries we want to write to (stored at
> mOfflineCacheLastModifiedTime).  These entries are tho always new and the
> value is always 0.  Reading the property is probably just a legacy code that
> should be removed anyway.  In nsHttpChannel::ShouldUpdateOfflineCacheEntry
> (during normal offline cache updates) we never reach the code that works
> with mOfflineCacheLastModifiedTime member.
> 
> 
> BTW, in your step 3 you don't say anything about modifying the manifest byte
> content - a precondition to start an offline cache update.  Are you doing
> that?

Yep, sorry. The test was:

1. Generate a build and flash it
2. Modify the app on the server
3. See if I got an update notification

With the incorrect metadata, I don't. If I set the metadata to the correct values, I do.

> 
> Also, what exactly leads you to a conclusion that LastModified and
> LastFetched are the properties you need to update?

Manual tests (stopping B2G, modifying index.sqlite, restarting it and testing). When I set LastModified and LastFetched to the values that they would have had if the content had been downloaded from the network when it was actually fetched, then the device recognized that there was new content. If I didn't, it didn't.

Is LastModified the last time the entry was modified on the database then?

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #9)
> (In reply to Honza Bambas (:mayhemer) from comment #6)
> > I'm doing some more tests.  In
> > nsHttpChannel::OnOfflineCacheEntryForWritingAvailable we read LastModified
> > property of entries we want to write to (stored at
> > mOfflineCacheLastModifiedTime).  These entries are tho always new and the
> > value is always 0.  Reading the property is probably just a legacy code that
> > should be removed anyway.  In nsHttpChannel::ShouldUpdateOfflineCacheEntry
> > (during normal offline cache updates) we never reach the code that works
> > with mOfflineCacheLastModifiedTime member.
> > 
> > 
> > BTW, in your step 3 you don't say anything about modifying the manifest byte
> > content - a precondition to start an offline cache update.  Are you doing
> > that?
> 
> Yep, sorry. The test was:
> 
> 1. Generate a build and flash it
> 2. Modify the app on the server

Meaning to also update the manifest, I presume.

> 3. See if I got an update notification
> 
> With the incorrect metadata, I don't. If I set the metadata to the correct
> values, I do.

Does it work when "incorrect metadata" values are in the past against time you attempting the update or in the future?  

What are the values after the manual preload you see in the database w/o your patch?

> 
> > 
> > Also, what exactly leads you to a conclusion that LastModified and
> > LastFetched are the properties you need to update?
> 
> Manual tests (stopping B2G, modifying index.sqlite, restarting it and
> testing). When I set LastModified and LastFetched to the values that they
> would have had if the content had been downloaded from the network when it
> was actually fetched, then the device recognized that there was new content.
> If I didn't, it didn't.
> 
> Is LastModified the last time the entry was modified on the database then?

LastModified is time when the entry has been modified via:
- any metadata element has been updated, also during the first population
- data size has changed, which also happens during the first population

On all those circumstances it's set to NOW.


It would be great for me to have a test case I could run on desktop.  At least the appcache preload script to run for my custom content I can build easily on my local server.  Can you help me how to do that?

Then, what are the values of metadata.lastFetched and lastModified in your patch (OfflineCacheInstaller.jsm)?  Is it now?  Is it something else?

Comment 11

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #9)
> > (In reply to Honza Bambas (:mayhemer) from comment #6)
> > > I'm doing some more tests.  In
> > > nsHttpChannel::OnOfflineCacheEntryForWritingAvailable we read LastModified
> > > property of entries we want to write to (stored at
> > > mOfflineCacheLastModifiedTime).  These entries are tho always new and the
> > > value is always 0.  Reading the property is probably just a legacy code that
> > > should be removed anyway.  In nsHttpChannel::ShouldUpdateOfflineCacheEntry
> > > (during normal offline cache updates) we never reach the code that works
> > > with mOfflineCacheLastModifiedTime member.
> > > 
> > > 
> > > BTW, in your step 3 you don't say anything about modifying the manifest byte
> > > content - a precondition to start an offline cache update.  Are you doing
> > > that?
> > 
> > Yep, sorry. The test was:
> > 
> > 1. Generate a build and flash it
> > 2. Modify the app on the server
> 
> Meaning to also update the manifest, I presume.

Yes, although I thought it should work if any of the files that had been cached are modified. In any case, the modification done *is* detected correctly when the application has been installed by using navigator.mozApps.install. It is *not* detected if it has been preinstalled/preloaded with the build.

> 
> > 3. See if I got an update notification
> > 
> > With the incorrect metadata, I don't. If I set the metadata to the correct
> > values, I do.
> 
> Does it work when "incorrect metadata" values are in the past against time
> you attempting the update or in the future?  

Yes and no. If the incorrect data is on the past, but expiration date is still 0xFFFFFFFF then the update doesn't work. If the incorrect data is on the past, and expiration date is set to 0 (manually) then it works. If the incorrect data is on the future (of the update, not necessarily of now) then it doesn't work. So graphically (for some value of graphic :P)

F: Time when the data was actually fetched
T: Time set on index.sqlite
M: Time of the modification
N: Now

F <= T <= M <= N, Expiration Date 0xFFFFFFFF => FAIL

F <= T <= M <= N, Expiration Date 0x0        => SUCCESS

T <= F <= M <= N, Expiration Date 0xFFFFFFFF => FAIL

T <= F <= M <= N, Expiration Date 0x0        => SUCCESS

F <= T <= M <= N, Expiration Date 0xFFFFFFFF => FAIL

F <= T <= M <= N, Expiration Date 0x0        => SUCESS

F <= M <= T <= N, Expiration Date 0xFFFFFFFF => FAIL

F <= M <= T <= N, Expiration Date 0x0        => FAIL

As you can see, it always works correctly if T <= M and Expiration Date is 0. If ED is 0 but the T is incorrectly > M then it doesn't work.

> 
> What are the values after the manual preload you see in the database w/o
> your patch?

Without the patch, LastFetch and last modified are set to whatever the date was on the device when the first run was executed, and expiration date is set to 0xFFFFFFFF.
p
> 
> > 
> > > 
> > > Also, what exactly leads you to a conclusion that LastModified and
> > > LastFetched are the properties you need to update?
> > 
> > Manual tests (stopping B2G, modifying index.sqlite, restarting it and
> > testing). When I set LastModified and LastFetched to the values that they
> > would have had if the content had been downloaded from the network when it
> > was actually fetched, then the device recognized that there was new content.
> > If I didn't, it didn't.
> > 
> > Is LastModified the last time the entry was modified on the database then?
> 
> LastModified is time when the entry has been modified via:
> - any metadata element has been updated, also during the first population
> - data size has changed, which also happens during the first population
> 
> On all those circumstances it's set to NOW.
> 
> 
> It would be great for me to have a test case I could run on desktop.  At
> least the appcache preload script to run for my custom content I can build
> easily on my local server.  Can you help me how to do that?
> 
> Then, what are the values of metadata.lastFetched and lastModified in your
> patch (OfflineCacheInstaller.jsm)?  Is it now?  Is it something else?

In my patch, the values are read from a new file generated at build time, that has lastFetched to now (now meaning when the build was built, NOT when the OfflineCacheInstaller is run!), and lastModified to the last-modified header on the answer (I can fix that though, it's on one of the other patches).

I will upload some test files to github later and put the branch here.

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

Ok, https://github.com/AntonioMA/gaia/tree/cache_test should create a build with a preloaded app with an app cache. Without my patch it'll set the dates to whatever the device has when it boots. With my patch, it should set them to what's on the resources_metadata.json file.

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #11)
> (In reply to Honza Bambas (:mayhemer) from comment #10)
> > (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #9)
> > > (In reply to Honza Bambas (:mayhemer) from comment #6)
> > > > I'm doing some more tests.  In
> > > > nsHttpChannel::OnOfflineCacheEntryForWritingAvailable we read LastModified
> > > > property of entries we want to write to (stored at
> > > > mOfflineCacheLastModifiedTime).  These entries are tho always new and the
> > > > value is always 0.  Reading the property is probably just a legacy code that
> > > > should be removed anyway.  In nsHttpChannel::ShouldUpdateOfflineCacheEntry
> > > > (during normal offline cache updates) we never reach the code that works
> > > > with mOfflineCacheLastModifiedTime member.
> > > > 
> > > > 
> > > > BTW, in your step 3 you don't say anything about modifying the manifest byte
> > > > content - a precondition to start an offline cache update.  Are you doing
> > > > that?
> > > 
> > > Yep, sorry. The test was:
> > > 
> > > 1. Generate a build and flash it
> > > 2. Modify the app on the server
> > 
> > Meaning to also update the manifest, I presume.
> 
> Yes, although I thought it should work if any of the files that had been
> cached are modified.

No, you MUST binary modify the manifest.  Also, the manifest MUST be served with caching headers that prevent heuristic caching.  Simply said, Cache-control: no-cache is a must.

> In any case, the modification done *is* detected
> correctly when the application has been installed by using
> navigator.mozApps.install. It is *not* detected if it has been
> preinstalled/preloaded with the build.
> 
> > 
> > > 3. See if I got an update notification
> > > 
> > > With the incorrect metadata, I don't. If I set the metadata to the correct
> > > values, I do.
> > 
> > Does it work when "incorrect metadata" values are in the past against time
> > you attempting the update or in the future?  
> 
> Yes and no. If the incorrect data is on the past, but expiration date is
> still 0xFFFFFFFF then the update doesn't work. If the incorrect data is on
> the past, and expiration date is set to 0 (manually) then it works. 

I would expect that.

> If the
> incorrect data is on the future (of the update, not necessarily of now) then
> it doesn't work. So graphically (for some value of graphic :P)
> 
> F: Time when the data was actually fetched
> T: Time set on index.sqlite
> M: Time of the modification
> N: Now
> 
> F <= T <= M <= N, Expiration Date 0xFFFFFFFF => FAIL
> 
> F <= T <= M <= N, Expiration Date 0x0        => SUCCESS
> 
> T <= F <= M <= N, Expiration Date 0xFFFFFFFF => FAIL
> 
> T <= F <= M <= N, Expiration Date 0x0        => SUCCESS
> 
> F <= T <= M <= N, Expiration Date 0xFFFFFFFF => FAIL
> 
> F <= T <= M <= N, Expiration Date 0x0        => SUCESS
> 
> F <= M <= T <= N, Expiration Date 0xFFFFFFFF => FAIL
> 
> F <= M <= T <= N, Expiration Date 0x0        => FAIL
> 
> As you can see, it always works correctly if T <= M and Expiration Date is
> 0. If ED is 0 but the T is incorrectly > M then it doesn't work.

Good, I'll try to find out something from this.

> 
> > 
> > What are the values after the manual preload you see in the database w/o
> > your patch?
> 
> Without the patch, LastFetch and last modified are set to whatever the date
> was on the device when the first run was executed, and expiration date is
> set to 0xFFFFFFFF.

Hmm.. so those are not set to the time you've been doing the script preload?  That would be odd.  If I undersntand the "first run" time.  But see bellow..

> p
> > 
> > > 
> > > > 
> > > > Also, what exactly leads you to a conclusion that LastModified and
> > > > LastFetched are the properties you need to update?
> > > 
> > > Manual tests (stopping B2G, modifying index.sqlite, restarting it and
> > > testing). When I set LastModified and LastFetched to the values that they
> > > would have had if the content had been downloaded from the network when it
> > > was actually fetched, then the device recognized that there was new content.
> > > If I didn't, it didn't.
> > > 
> > > Is LastModified the last time the entry was modified on the database then?
> > 
> > LastModified is time when the entry has been modified via:
> > - any metadata element has been updated, also during the first population
> > - data size has changed, which also happens during the first population
> > 
> > On all those circumstances it's set to NOW.
> > 
> > 
> > It would be great for me to have a test case I could run on desktop.  At
> > least the appcache preload script to run for my custom content I can build
> > easily on my local server.  Can you help me how to do that?
> > 
> > Then, what are the values of metadata.lastFetched and lastModified in your
> > patch (OfflineCacheInstaller.jsm)?  Is it now?  Is it something else?
> 
> In my patch, the values are read from a new file generated at build time,
> that has lastFetched to now (now meaning when the build was built, NOT when
> the OfflineCacheInstaller is run!), and lastModified to the last-modified
> header on the answer

That's really not the way as I explained earlier.  Maybe start with that and setting the exp time to 0 all the time.

From what I read it seems the update is made when the phone (device)'s time is in the past - so not correctly set.  I assume you reset the time from carrier or NTP later.  Maybe do the update then?


The thing is that what the OfflineCacheInstaller script does is not that different from what we do internally.  There is no API to set the metadata directly, so it must work w/o exposing that API.

>  (I can fix that though, it's on one of the other
> patches).
> 
> I will upload some test files to github later and put the branch here.

Thanks.

Comment 14

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> > 
> > In my patch, the values are read from a new file generated at build time,
> > that has lastFetched to now (now meaning when the build was built, NOT when
> > the OfflineCacheInstaller is run!), and lastModified to the last-modified
> > header on the answer
> 
> That's really not the way as I explained earlier.  Maybe start with that and
> setting the exp time to 0 all the time.
> 
> From what I read it seems the update is made when the phone (device)'s time
> is in the past - so not correctly set.  I assume you reset the time from
> carrier or NTP later.  Maybe do the update then?

That's not that easily done... the user can boot the phone without a SIM card and without network and he still might want to use the music player (for example). We could re-run the process the first time we have network, but it still wouldn't work. Because then T would be === N (in my diagrams before) and if the content have been modified between the preload time and now, it won't work no matter what we do with Expiration Time. We have to set the date to something that's *before* the modification time but *after* the preload time. And without my other patches we don0t know the preload time. And without this patch, we cannot set the time manually (since as you said, it's always set to 'now').

> 
> 
> The thing is that what the OfflineCacheInstaller script does is not that
> different from what we do internally.  There is no API to set the metadata
> directly, so it must work w/o exposing that API.
> 
> >  (I can fix that though, it's on one of the other
> > patches).
> > 
> > I will upload some test files to github later and put the branch here.
> 
> Thanks.

Comment 15

[Honza Bambas (:mayhemer)]

The problem is that you don't get the "update is available" notification?  The notification coming from nsIOfflineCacheUpdateService.checkForUpdate (called from dom/apps/Webapps.jsm, updateHostedApp).  Please confirm that.

Before I start investigating this (really not easy..) I'll ask for few experiments on your side first.

BTW, I can see few bugs in OfflineCacheInstaller.jsm.  Might not be related tho, but apparently that code has not been well written or reviewed.

Anyway, there is a major omission: there are no cache headers in the artificially created response head for each resource, try adding Cache-control: no-cache header (at |cacheEntry.setMetaDataElement('response-head', 'HTTP/1.1 200 OK\r\n');|).

Also set the expiration time to 0.

Then retry.

Comment 16

[Honza Bambas (:mayhemer)]

BTW, instead of "T: Time set on index.sqlite" I would be more interested on values of LastModified and LastFetched stored in the database.

Comment 17

[Honza Bambas (:mayhemer)]

index.sqlite is modified any time we somehow touch the appcache.  So, not really a pointer for anything.

Comment 18

[Honza Bambas (:mayhemer)]

Also, there is another missing response header (actually mandatory) - Date.  Set it to some point in the past the phone device can never go before.

Comment 19

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Honza Bambas (:mayhemer) from comment #15)
> The problem is that you don't get the "update is available" notification? 
> The notification coming from nsIOfflineCacheUpdateService.checkForUpdate
> (called from dom/apps/Webapps.jsm, updateHostedApp).  Please confirm that.

I don't get the update is available and I don't see the new version of the modified files, yeah.

> 
> Before I start investigating this (really not easy..) I'll ask for few
> experiments on your side first.
> 
> BTW, I can see few bugs in OfflineCacheInstaller.jsm.  Might not be related
> tho, but apparently that code has not been well written or reviewed.
> 
> Anyway, there is a major omission: there are no cache headers in the
> artificially created response head for each resource, try adding
> Cache-control: no-cache header (at
> |cacheEntry.setMetaDataElement('response-head', 'HTTP/1.1 200 OK\r\n');|).
> 
> Also set the expiration time to 0.
> 
> Then retry.

Ok, I'll try but I'm traveling now (till tomorrow night, European time) so I don't think I'll be able to try this till Monday morning.

Comment 20

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> BTW, instead of "T: Time set on index.sqlite" I would be more interested on
> values of LastModified and LastFetched stored in the database.

Aren't those the same? When I say time set in index.sqlite I'm referring to the lastFetched and lastModified fields on the moz_cache table on index.sqlite (for each entry of the cache), sorry if I wasn't clear about that. 

About the date header, should it be a string as returned by the server, or a "number of seconds from the epoch" integer? (Or something else). I can actually set the date to the *actual* date where the fetch happened (since with the other two patches I have that on the resources_metadata.json, that's where I get the lastfetched value there).

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #19)
> (In reply to Honza Bambas (:mayhemer) from comment #15)
> > The problem is that you don't get the "update is available" notification? 
> > The notification coming from nsIOfflineCacheUpdateService.checkForUpdate
> > (called from dom/apps/Webapps.jsm, updateHostedApp).  Please confirm that.
> 
> I don't get the update is available and I don't see the new version of the
> modified files, yeah.

What I was asking was if you used nsIOfflineCacheUpdateService.checkForUpdate to check the update availability status.  That was the thing to confirm from you.


(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #20)
> (In reply to Honza Bambas (:mayhemer) from comment #16)
> > BTW, instead of "T: Time set on index.sqlite" I would be more interested on
> > values of LastModified and LastFetched stored in the database.
> 
> Aren't those the same? 

I would not count on that.  index.sqlite may be modified whenever the profile is used.  So, unless you are 100% sure the profile was intact, I would not count index.sqlite time == lastmodified time of the cache entries.

> When I say time set in index.sqlite I'm referring to
> the lastFetched and lastModified fields on the moz_cache table on
> index.sqlite (for each entry of the cache), sorry if I wasn't clear about
> that. 

Aha!  Now it's clear.

> 
> About the date header, should it be a string as returned by the server, or a
> "number of seconds from the epoch" integer? (Or something else). 

String as returned by the server.

> I can
> actually set the date to the *actual* date where the fetch happened (since
> with the other two patches I have that on the resources_metadata.json,
> that's where I get the lastfetched value there).

That would do.

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #19)
> > (In reply to Honza Bambas (:mayhemer) from comment #15)
> > > The problem is that you don't get the "update is available" notification? 
> > > The notification coming from nsIOfflineCacheUpdateService.checkForUpdate
> > > (called from dom/apps/Webapps.jsm, updateHostedApp).  Please confirm that.
> > 
> > I don't get the update is available and I don't see the new version of the
> > modified files, yeah.
> 
> What I was asking was if you used
> nsIOfflineCacheUpdateService.checkForUpdate to check the update availability
> status.  That was the thing to confirm from you.

Hmm dunno. I didn't actually made the call. I used whatever Gaia/FFOS uses. Which is:

http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#2214

So yeah, it's nsIOfflineCacheUpdateService.checkForUpdate.

> 
> 
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #20)
> > (In reply to Honza Bambas (:mayhemer) from comment #16)
> > > BTW, instead of "T: Time set on index.sqlite" I would be more interested on
> > > values of LastModified and LastFetched stored in the database.
> > 
> > Aren't those the same? 
> 
> I would not count on that.  index.sqlite may be modified whenever the
> profile is used.  So, unless you are 100% sure the profile was intact, I
> would not count index.sqlite time == lastmodified time of the cache entries.
> 
> > When I say time set in index.sqlite I'm referring to
> > the lastFetched and lastModified fields on the moz_cache table on
> > index.sqlite (for each entry of the cache), sorry if I wasn't clear about
> > that. 
> 
> Aha!  Now it's clear.

Cool, sorry about the misunderstanding :) I realized it sounded like I was talking of the last modified time of the whole file.

> 
> > 
> > About the date header, should it be a string as returned by the server, or a
> > "number of seconds from the epoch" integer? (Or something else). 
> 
> String as returned by the server.
> 
> > I can
> > actually set the date to the *actual* date where the fetch happened (since
> > with the other two patches I have that on the resources_metadata.json,
> > that's where I get the lastfetched value there).
> 
> That would do.

Comment 23

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V2. Just set the metadata for date and last-modified

As per Honza suggestion, I just added the date (and for good measure, last-modified since I had that one available also) header to the metadata, and changed the expiration time to 0. That seems to work and updates are offered correctly.

Comment 24

[Antonio Manuel Amaya Calvo (:amac)]

I updated the preload.py patch also to just add the last-modified and date headers as got from the network instead of calculating the seconds from epoch.

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8585376 [details] [diff] [review]
V2. Just set the metadata for date and last-modified

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

nit: maybe provide a patch with -w option (ignoring white spaces) for simpler review

::: dom/apps/OfflineCacheInstaller.jsm
@@ +54,5 @@
> +  metadata = metadata || {};
> +  metadata.lastFetched = metadata.lastFetched || nowGMT;
> +  metadata.lastModified = metadata.lastModified || nowGMT;
> +  let lastFetched = metadata.lastFetched;
> +  debug("CJC lastFetched:"+lastFetched);

nit: spaces around +

@@ +62,5 @@
>          cacheEntry.setMetaDataElement('request-method', 'GET');
>          cacheEntry.setMetaDataElement('response-head', 'HTTP/1.1 200 OK\r\n');
> +        cacheEntry.setMetaDataElement('date', metadata.lastFetched);
> +        cacheEntry.setMetaDataElement('last-modified', metadata.lastModified);
> +        cacheEntry.setMetaDataElement('cache-control', 'no-cache');

no.  I apparently wasn't clear enough.  The code has to look like:

cacheEntry.setMetaDataElement('response-head', 
  'HTTP/1.1 200 OK\r\n' + 
  'Date: ' + metadata.lastFetched + "\r\n" +
  'Last-Modified: ' + metadata.lastModified + "\r\n" +
  'Cache-Control: no-cache' + "\r\n");

Anyway, I think you can easily omit Last-Modified header at all.

@@ +78,2 @@
>          bufferedOutputStream.init(outputStream, 1024);
>          bufferedOutputStream.writeFrom(inputStream, inputStream.available());

btw, if this runs on the device, then you are doing a main thread IO here.

Comment 26

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V3. Add all the fields to response-head

Assuming the Last-Modified field is used, and since we have it, I would prefer leaving it in.

By the way, why are we setting no-cache for something we're actually caching?

I'll try to upload a -w diff in a few.

Comment 27

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V3. Patch ignoring whitespace changes

This is the same patch ignoring the whitespace changes. About the IO on the main thread, I didn't write nor change that. Also this runs only at boot time, the first time the phone boots.

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #26)
> Created attachment 8585952 [details] [diff] [review]
> V3. Add all the fields to response-head
> 
> Assuming the Last-Modified field is used, and since we have it, I would
> prefer leaving it in.

No, it is not used at all.

> 
> By the way, why are we setting no-cache for something we're actually caching?

Please read RFC2616 on meaning of the Cache-control header.

> 
> I'll try to upload a -w diff in a few.

Thanks.

Comment 29

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #28)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #26)
> > Created attachment 8585952 [details] [diff] [review]
> > V3. Add all the fields to response-head
> > 
> > Assuming the Last-Modified field is used, and since we have it, I would
> > prefer leaving it in.
> 
> No, it is not used at all.

Ah, as a header it is, but not as a metadata element (if you just set it as setMetaDataElement("Last-Modified", something...) on the entry.)

Comment 30

[Honza Bambas (:mayhemer)]

Comment on attachment 8585954 [details] [diff] [review]
V3. Patch ignoring whitespace changes

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

r+'ing the full patch.

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 8585952 [details] [diff] [review]
V3. Add all the fields to response-head

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

With the nits fixed, r=honzab

::: dom/apps/OfflineCacheInstaller.jsm
@@ +61,5 @@
> +        cacheEntry.setMetaDataElement('response-head',
> +        'HTTP/1.1 200 OK\r\n' +
> +        'Date: ' + metadata.lastFetched + "\r\n" +
> +        'Last-Modified: ' + metadata.lastModified + "\r\n" +
> +        'Cache-Control: no-cache' + "\r\n");

please indent as:
cacheEntry.setMetaDataElement('response-head',
  'HTTP ...' + 
  'Date: ...'

s/"/'/ (simply be consistent)

'Cache-Control: no-cache\r\n'

@@ +246,5 @@
> +    metadataLoaded = Promise.resolve({});
> +  } else {
> +    metadataLoaded = new Promise(
> +      (resolve, reject) =>
> +        readFile(resourcesMetadata, principal, content => resolve(JSON.parse(content))));

should it be as (content) => resolve(...) ?

Comment 32

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V4. With niths fixed

r=mayhemer, r=fabrice

Fixed the nits, except for:


> 
> @@ +246,5 @@
> > +    metadataLoaded = Promise.resolve({});
> > +  } else {
> > +    metadataLoaded = new Promise(
> > +      (resolve, reject) =>
> > +        readFile(resourcesMetadata, principal, content => resolve(JSON.parse(content))));
> 
> should it be as (content) => resolve(...) ?

I don't really mind one way or the other... but this way it follows the style of the examples at [1]

Try run is at: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ee03c3cf612



[1] https://github.com/mozilla/addon-sdk/wiki/Coding-style-guide

Comment 33

[Antonio Manuel Amaya Calvo (:amac)]

Try run looks mostly green, requesting checkin. Note that when the gaia patch lands any app that's preinstalled with app cache (there's none on the standard repository) will have to be refetched/regenerated using the patched preload.py or the build will fail.

Comment 34

[Antonio Manuel Amaya Calvo (:amac)]

Requesting 2.2 blocking. Without this patch, preinstalled apps with app cache (and the OEMs/operators are including some of those on the builds) will never be updated (so the only way to update them is via OTA/FOTA).

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20d5299e3b57

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20d5299e3b57

Comment 37

[Antonio Manuel Amaya Calvo (:amac)]

The two git patches (gaia and preload) weren't landed by the autolander for some reason. Do I need to request checkin again?

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Dunno, autolander is kgrandon's creation and Tomcat landed the gecko patch.

Comment 39

[Antonio Manuel Amaya Calvo (:amac)]

Hi Kevin, do you known why the Gaia and preload patches didn't land?

Comment 40

[Kevin Grandon :kgrandon]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #39)
> Hi Kevin, do you known why the Gaia and preload patches didn't land?

Autolander currently processes attachments that it creates, and currently only on the FirefoxOS component. In the future I recommend filing an additional bug for the FirefoxOS gaia work, for now let's manually land this.

In master: https://github.com/mozilla-b2g/gaia/commit/318ded377534ea39c903ddb98ae7c68f63c117e7

Comment 41

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:reland_bug_1144689 > mozilla-b2g:master

Comment 42

[Kevin Grandon :kgrandon]

(In reply to Autolander from comment #41)
> Created attachment 8589723 [details] [review]
> [gaia] KevinGrandon:reland_bug_1144689 > mozilla-b2g:master

Sorry, I accidentally landed this without noticing the lint error on the try run (autolander is smarter than I am apparantly),  reverted: https://github.com/mozilla-b2g/gaia/commit/ecdbd488d0080e3c10f80d92a38201d552542d96

Here is a new pull request with your commit and a fix for the lint error. Will re-land this assuming try is green. Sorry about the churn here.

Comment 43

[Kevin Grandon :kgrandon]

Re-landed in master: https://github.com/mozilla-b2g/gaia/commit/e5e42aa03941795da09fb5fa290d047c9a2b540b

Comment 44

[howie [:howie]]

Triage: blocking. Hi Antonio, can you request 2.2 patch uplift approval? Thanks.

Comment 45

[Loli (:lolimartinezcr)]

Tested and working:
flame
eng
master
Gecko-6febe17
Gaia-cdf5276

Comment 46

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8588574 [details] [diff] [review]
V4. With niths fixed

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 #): AFAIK this hasn't worked ever... we just noticed now
User impact if declined: Preinstalled apps that use appcache (Wikipedia for once) are *never* updated
Testing completed: Manually
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: Nonw

Comment 47

[Antonio Manuel Amaya Calvo (:amac)]

I cannot request approval for the Gaia patch (preload.py is not part of the build) because the approval-2.2 for gaia is not shown (maybe because of the component of the bug?).

Comment 48

[Antonio Manuel Amaya Calvo (:amac)]

The preload.py patch hasn't landed either... dunno if that repository is controlled by the autolander or not, TBH. Can you land it?

Comment 49

[Kevin Grandon :kgrandon]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #47)
> I cannot request approval for the Gaia patch (preload.py is not part of the
> build) because the approval-2.2 for gaia is not shown (maybe because of the
> component of the bug?).

Maybe we need a new bug for the gaia changes, or maybe mozilla‑b2g37 will do it?

Landed preload.py patch in master: https://github.com/mozilla-b2g/preload-app-toolkit/commit/feb47748534864d86f7cd63415e5ed1ce5896616

Comment 50

[howie [:howie]]

Seems like due to component so there's no Gaia request option.
I've opened a Gaia bug 1153762.
Hi Antonio, can you help with the rest? Thanks!

Comment 51

[Antonio Manuel Amaya Calvo (:amac)]

Done, thank you

Comment 52

[Ryan VanderMeulen [:RyanVM]]

The Gecko patch needs rebasing for b2g37 uplift.

Comment 53

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Gecko patch, rebased for 2.2 (b2g37)

This is the rebased patch for B2G37

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/dd394cdc4976

FYI, you don't need to ni? me in the future. The bugs will continue to show up on the needs-uplift radar until their status changes to fixed.