714299 - Manifest fetched on install shouldn't be cached

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Ian Bicking (:ianbicking)]

The manifests are fetched with no special attention to caching, which means Expires headers and the like can cause a manifest to be cached and make it difficult to undo (and there's nothing like a hard reload for this request).

It seems to me that we should avoid caching this particular resource.  Adding a no-cache request header should be sufficient.

Comment 1

[Jason Smith [:jsmith]]

Is this a mozapps api bug? If so, migrate this bug to Core --> DOM/Mozilla Extensions.

Comment 2

[Honza Bambas (:mayhemer)]

True is that people have reported issues with publishing offline cache manifests too.  Since most of them omitted to add no-cache they were reporting bugs that we didn't update offline cache even though they updated (i.e. changed) their manifest on the server.  All browsers, including Gecko-based behave this way what is according the offline cache spec, though.

Based on this experience, we shouldn't repeat the same mistake for web apps and always refetch the manifest (add e.g. no-cache header artificially) if it hasn't been fetched with ETag.

Comment 3

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

Honza: Are you saying that we should or should not always refetch the manifest? I.e. are you recommending that we ignore cache headers?

Comment 4

[Dietrich Ayala (:dietrich)]

In triage we didn't have the right people in the room to make a blocking call on this. Can you clarify why this is blocking basecamp?

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #3)
> Honza: Are you saying that we should or should not always refetch the
> manifest? I.e. are you recommending that we ignore cache headers?

We should load app manifests with nsIRequest::VALIDATE_ALWAYS flag.  This is the optimal way.  When there is an ETag header available we use it to revalidate.  When there is none, we just always refetch.

Comment 6

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

Sounds good to me. And it indeed sounds like something that could cause authors a lot of confusion and frustration.

Comment 7

[[:fabrice] Fabrice Desré]

We load the manifests using an xhr request. Is setting a |Cache-control: no-cache| enough?

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> We load the manifests using an xhr request. Is setting a |Cache-control:
> no-cache| enough?

Correct is to set nsIRequest::VALIDATE_ALWAYS on its channel.

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: patch

Setting the flag on the xhr once it is opened.

Comment 10

[Honza Bambas (:mayhemer)]

Are there also some tests for this?

Comment 11

[[:fabrice] Fabrice Desré]

Not yet - I filed bug 779991 on this.

Comment 12

[Honza Bambas (:mayhemer)]

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

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

Next time please with

[defaults]
diff=-U 8 -p
qdiff=-U 8

[diff]
git=true
showfunc=true
unified=8 

in your hgrc.

::: dom/apps/src/Webapps.js
@@ +125,4 @@
>      let requestID = this.getRequestId(request);
>      let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
>      xhr.open("GET", aURL, true);
> +    xhr.channel.loadFlags |= Ci.nsIChannel.VALIDATE_ALWAYS;

nsIRequest.VALIDATE_ALWAYS

Comment 13

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/169186616cd2

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/169186616cd2

Comment 15

[Nikolaus Filus]

Will this be included in FF 10esr? If not, please consider adding/backporting this. Thank you