823648 - We should support Last-Modified headers in addition to ETag headers for manifest and package downloads

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[Julien Wajsberg [:julienw]]

The rationale 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.

The Marketplace is generating a sensible ETag 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.

And because that's the way the web works since HTTP 1.1.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: add lastmodified support for manifest downloads

applies on b2g18

We should probably wait for Bug 820630 and add the support for package downloads as well. Or fix this now and support this header in Bug 820630.

Comment 2

[[:fabrice] Fabrice Desré]

What we want to know is if the *content* of the file has changed. This is not what LastModified gives us especially with static files I think.

Comment 3

[Julien Wajsberg [:julienw]]

Most out-of-marketplace app will not have an ETag with this information.

Of course, Last-Modified might change without the content changing, but I think this is still a better state than now where we're not supporting Last-Modified at all.

And it's ivery unlikely that Last-Modified does not change if the content is changing.

Comment 4

[Julien Wajsberg [:julienw]]

As seen in Bug 829853, github servers do not send ETag:

$ curl -I http://snake-app.github.com/snake.webapp
HTTP/1.1 200 OK
Server: GitHub.com
Date: Sat, 12 Jan 2013 12:36:27 GMT
Content-Type: application/x-web-app-manifest+json
Content-Length: 810
Last-Modified: Sat, 12 Jan 2013 03:44:17 GMT
Connection: keep-alive
Expires: Sun, 13 Jan 2013 12:36:27 GMT
Cache-Control: max-age=86400
Accept-Ranges: bytes


Therefore, I'd like that we reconsider to fix this bug for v1. Since I don't know the new stuff in blocking-b2g, I'm nominating for blocking-basecamp, but please choose what's appropriated here.

Comment 5

[Jason Smith [:jsmith]]

Switching flags per new usage for tracking? + tef? after retiring basecamp?

Comment 6

[Andrew Overholt [:overholt]]

Fabrice said this is something we'd like to take for 1.x but not something we'd hold for.

Comment 7

[Honza Bambas (:mayhemer)]

I a bit don't understand why you are handling ETag and Last-Modified (not talking about Expire) "manually" in a JS code when these headers are stored and correctly handled by the Necko cache natively for you.

If you want to check whether the content has changed on the server, then use following channel flags on the xhr:
[1] LOAD_ONLY_IF_MODIFIED
    - this one will skip load from cache and just give OnStartRequest and OnStopRequest (no content load) when the cached content is found valid ; it means for you that there is no update
[2] VALIDATE_ALWAYS
    - even there is no ETag or Last-Modified and the calculated expiration time indicates the resource is valid, we will recheck with the server, always
    - this is the most optimal way of loading always valid resource since when there is ETag or Last-Modified we use those for conditional requests (that you are bustling your self in JS)
    - with this flag you don't need to require ETag/Last-Modified being set by the serve

However, when the resource has not been modified on the server, you need to check whether the manifest (or which ever file you are checking on) has changed.  Thus a hash checker as in case of offline app cache manifest is what you need (bug 829934).


[1] http://hg.mozilla.org/mozilla-central/annotate/56ff556e74d9/netwerk/base/public/nsICachingChannel.idl#l126
[2] http://hg.mozilla.org/mozilla-central/annotate/56ff556e74d9/netwerk/base/public/nsIRequest.idl#l182

Comment 8

[Jason Smith [:jsmith]]

Putting back into the nom queue. If this is the direction we choose to go to fix the bug blocking this bug, then this blocks. If it isn't and the other bug ends up being the choice, then feel free to minus this and + the other bug (bug 829934).

Comment 9

[Julien Wajsberg [:julienw]]

Adding :sicking and :philikon because of the latest conversation on IRC, and I'd like to add some more information about server support.

Apache supports ETag out of the box (however AFAIK nginx support is trickier to get correct).

However all webperf best practices (most notably Yahoo and Google) advise to deactivate support in Apache and to rely only on Last-Modified headers which is sufficient for most cases.

Default configuration in Apache generates an ETag in the following format: INode-ModificationTime-Size. This is wrong in many cases, most notably when using load balancing (-> different INodes -> different ETag -> 200 instead of 304).

So ETag are only useful in specific cases, such as our Marketplace, but we can't expect people to do that, and most of the times it will be disabled because of webperf best practices.

Comment 10

[Jason Smith [:jsmith]]

Jonas - Do we need to block on this one or only block on the other bug for the hash verifier?

Comment 11

[[:fabrice] Fabrice Desré]

I think we spend more time discussing that writing the code. That is silly. I'll happily review and land a patch.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think bug 829934 is enough. Though doing this is a performance improvement which would also save users some bandwidth (in the order of 1kB/app/day)

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.