820630 - Allow preinstalled packaged apps to specify the etag of the update manifest and package file

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[[:fabrice] Fabrice Desré]

Without that, the firsts update check is always positive. On the gaia side, this information will be added in external-apps/APP_ID/metadata.json

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Gaia patch v1

Hi :fabrice, 

I'm adding the gaia patch, modifying the build script to add the etag header for the external apps via the metadata.json.

Also added the etag null for the rest of the apps.

Will it require more work on the gecko part?

Thanks!

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 692914 [details] [diff] [review]
Gaia patch v1

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

Your patch adds support for the manifest's etag, but we also need to add the package etag. Can you add it from the metadata.json?
Let's name this new property "packageEtag".

We'll need some gecko support, but I'll do it since we have no support at all for package etags right now.

::: build/webapp-manifests.js
@@ +74,5 @@
>      installTime:   132333986000,
>      manifestURL:   url + '/manifest.webapp',
>      appStatus:     appStatus,
> +    localId:       id++,
> +    etag:          null

we don't need that, the platform does the right thing already if the etag for the manifest is not present.

Comment 3

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Gaia patch v2

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

Gaia part landed:

https://github.com/mozilla-b2g/gaia/commit/95a47fe895c0136ac042795cfb5b87ae1827498c

Reassigning to Fabrice for the gecko one.

Thanks a lot!

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: wip

This patch adds support for checking the etag of the package, and to not download it when it changes. To test that, change the update manifest but not the package and update a packaged app.

Comment 6

[Julien Wajsberg [:julienw]]

Is there a reason why we don't use the "Last-Modified" (response) and "If-Modified-Since" (request) HTTP headers as well ?

Nowadays, most web performance good practices tutorial recommend to switch off ETag and to relay on these headers for cache instead. Problem is that ETag sometimes uses informations that is local to a server (eg: inode number) and though it causes problems when one file is on several servers for load balancing.

I'll try the patch later today.

Comment 7

[Rob Hudson [:robhudson]]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Is there a reason why we don't use the "Last-Modified" (response) and
> "If-Modified-Since" (request) HTTP headers as well ?

There are a few reasons mentioned in this bug, including this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=772364#c10

> Nowadays, most web performance good practices tutorial recommend to switch
> off ETag and to relay on these headers for cache instead. Problem is that
> ETag sometimes uses informations that is local to a server (eg: inode
> number) and though it causes problems when one file is on several servers
> for load balancing.

For both the packaged app mini-manifests and the manifest for the Marketplace app I'm using the MD5 hash of the contents of the manifest to generate the ETag.

Comment 8

[Julien Wajsberg [:julienw]]

rob> thanks.

I don't understand why we can't do both (that's why I said "as well"). That's the way the web works for a long time.

The reason outlined in Bug 772364 comment 10 do not apply here :
* we don't care about subseconds problems, we definitely won't update less than one second after the download.
* clock warp problems are negligible (same device/same server). I can live with this risk.
* no need to parse, we just need to store the value of l-m as a string, and output the same value in i-m-s, just like ETag.

I'm glad you're generating a sensible ETag in the Marketplace but not all apps will come from the Marketplace and I'm quite sure that we will hit this problem. So let's just store both if they're both present.

Comment 9

[[:fabrice] Fabrice Desré]

No problem, we take patches.

Comment 10

[Julien Wajsberg [:julienw]]

Fabrice, your patch doesn't apply cleanly on current central or b2g18 so I can't test now.

Comment 11

[Julien Wajsberg [:julienw]]

Attachment: add last modified support for manifest checks

and here's your patch

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 694487 [details] [diff] [review]
add last modified support for manifest checks

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

Move this patch to a new bug please and go though nomination. Also, we need to do the same checks for the packages.

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: patch

Works fine for me, but I welcome other eyeballs to test.

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 694614 [details] [diff] [review]
patch

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

Works for me too.
r=me

::: dom/apps/src/Webapps.jsm
@@ +1840,5 @@
> +            app.downloadAvailable = false;
> +            app.downloadSize = 0;
> +            app.installState = "installed";
> +            app.readyToApplyDownload = false;
> +            self.broadcastMessage(

nit: we can save a few lines this way:

self.broadcastMessage("Webapps:PackageEvent" {
[...]
});

Comment 15

[[:fabrice] Fabrice Desré]

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

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b33fd3db8f0b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/574b7b76b920

Comment 18

[Jason Smith [:jsmith]]

This has caused a general regression with packaged app install - see bug 824695

Comment 19

[Jason Smith [:jsmith]]

Proven and tested through partner customizations. Marking as verified.