Closed Bug 820630 Opened 7 years ago Closed 7 years ago

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

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

VERIFIED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(2 files, 3 obsolete files)

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
blocking-basecamp: --- → ?
Blocks: 815411
Assignee: nobody → francisco.jordano
Attached patch Gaia patch v1 (obsolete) — Splinter Review
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!
Attachment #692914 - Flags: review?(fabrice)
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.
Attachment #692914 - Flags: review?(fabrice) → review-
Attached patch Gaia patch v2Splinter Review
Attachment #692914 - Attachment is obsolete: true
Attachment #693194 - Flags: review?(fabrice)
Attachment #693194 - Flags: review?(fabrice) → review+
Gaia part landed:

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

Reassigning to Fabrice for the gecko one.

Thanks a lot!
Assignee: francisco.jordano → fabrice
Target Milestone: --- → B2G C3 (12dec-1jan)
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #694185 - Flags: feedback?(ferjmoreno)
Attachment #694185 - Flags: feedback?(felash)
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.
(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.
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.
No problem, we take patches.
Fabrice, your patch doesn't apply cleanly on current central or b2g18 so I can't test now.
and here's your patch
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.
Attached patch patchSplinter Review
Works fine for me, but I welcome other eyeballs to test.
Attachment #694185 - Attachment is obsolete: true
Attachment #694487 - Attachment is obsolete: true
Attachment #694185 - Flags: feedback?(ferjmoreno)
Attachment #694185 - Flags: feedback?(felash)
Attachment #694614 - Flags: review?(ferjmoreno)
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" {
[...]
});
Attachment #694614 - Flags: review?(ferjmoreno) → review+
https://hg.mozilla.org/mozilla-central/rev/cf3f846822fd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
This has caused a general regression with packaged app install - see bug 824695
Depends on: 824695
Proven and tested through partner customizations. Marking as verified.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.