Last Comment Bug 714299 - Manifest fetched on install shouldn't be cached
: Manifest fetched on install shouldn't be cached
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: [:fabrice] Fabrice Desré
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-30 10:01 PST by Ian Bicking (:ianb)
Modified: 2012-08-06 01:55 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
patch (688 bytes, patch)
2012-08-02 13:47 PDT, [:fabrice] Fabrice Desré
honzab.moz: review+
Details | Diff | Splinter Review

Description Ian Bicking (:ianb) 2011-12-30 10:01:44 PST
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] 2012-04-09 17:14:17 PDT
Is this a mozapps api bug? If so, migrate this bug to Core --> DOM/Mozilla Extensions.
Comment 2 Honza Bambas (:mayhemer) 2012-07-26 06:00:15 PDT
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) PTO Until July 5th 2012-07-30 10:27:34 PDT
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) 2012-07-31 13:08:54 PDT
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) 2012-08-01 05:00:03 PDT
(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) PTO Until July 5th 2012-08-01 23:26:42 PDT
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é 2012-08-02 09:15:29 PDT
We load the manifests using an xhr request. Is setting a |Cache-control: no-cache| enough?
Comment 8 Honza Bambas (:mayhemer) 2012-08-02 11:29:11 PDT
(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é 2012-08-02 13:47:46 PDT
Created attachment 648470 [details] [diff] [review]
patch

Setting the flag on the xhr once it is opened.
Comment 10 Honza Bambas (:mayhemer) 2012-08-02 13:53:01 PDT
Are there also some tests for this?
Comment 11 [:fabrice] Fabrice Desré 2012-08-02 14:24:33 PDT
Not yet - I filed bug 779991 on this.
Comment 12 Honza Bambas (:mayhemer) 2012-08-03 09:04:01 PDT
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é 2012-08-03 09:28:09 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/169186616cd2
Comment 14 Ed Morley [:emorley] 2012-08-04 11:20:02 PDT
https://hg.mozilla.org/mozilla-central/rev/169186616cd2
Comment 15 Nikolaus Filus 2012-08-06 01:55:29 PDT
Will this be included in FF 10esr? If not, please consider adding/backporting this. Thank you

Note You need to log in before you can comment on or make changes to this bug.