Last Comment Bug 669001 - Vary: User-Agent + new UA string (i.e. after update) + HTTP 304 = broken cache
: Vary: User-Agent + new UA string (i.e. after update) + HTTP 304 = broken cache
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla11
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-02 10:28 PDT by Doug Wright
Modified: 2011-11-10 03:18 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (877 bytes, patch)
2011-07-04 06:44 PDT, Honza Bambas (:mayhemer)
bzbarsky: review+
Details | Diff | Splinter Review
v1+test (5.41 KB, patch)
2011-10-12 10:55 PDT, Honza Bambas (:mayhemer)
honzab.moz: review+
Details | Diff | Splinter Review

Description Doug Wright 2011-07-02 10:28:31 PDT
For resources that use Vary: User-Agent, the cache stores the UA string that was used in the initial request. When a request is made for that same resource but the UA string has changed since the initial request, Gecko will validate the previously cached data using If-None-Match.

That's fine, except when the server replies with a 304, the cache is apparently not updated with the new UA string and consequently the resource will be continue to be revalidated every single time the browser asks for it which renders the cache largely useless (particularly on a high-latency connection).

STR:

0) Enable HTTP logger of your choice

1) Visit www.pre-school.org.uk
2) Click Home. Again. And Again.
3) Note that the only resource fetched from those clicks is the page itself

4) Change your UA string (e.g. upgrade to new build, or fiddle with about:config)

5) Repeat steps (1) and (2)
6) Note that in addition to the page, requests for the CSS and JS are issued every time.

7) Downgrade to previous build, or revert about:config changes

8) Repeat steps (1) and (2)
9) Note that cache behaves again, and requests for the CSS and JS are not issued.
Comment 1 Boris Zbarsky [:bz] 2011-07-02 21:29:57 PDT
Honza, can you take a look please?
Comment 2 Honza Bambas (:mayhemer) 2011-07-04 06:44:22 PDT
Created attachment 543762 [details] [diff] [review]
v1

According to RFC 2616, section 13.6: "If the entity-tag of the new response matches that of an existing entry, the new response SHOULD be used to update the header fields of the existing entry, and the result MUST be returned to the client."  This applies to 304 responses.

Code that does this update is in nsHttpChannel::AddCacheEntryHeaders.  We seems to lack call to that method from nsHttpChannel::ProcessNotModified.  The patch adds it, and this also fixes this bug.

Only disadvantage is that we overwrite the response head in cache with the cached one - i.e. identical one (a bit wasting).
Comment 3 Honza Bambas (:mayhemer) 2011-07-04 06:52:47 PDT
And yes, we should have a test as well.  And I will also push this to try.
Comment 4 Honza Bambas (:mayhemer) 2011-07-04 12:10:35 PDT
Try is green: http://tbpl.mozilla.org/?tree=Try&rev=8aad3b30b3cb
Comment 5 Boris Zbarsky [:bz] 2011-07-05 14:59:02 PDT
Comment on attachment 543762 [details] [diff] [review]
v1

Please add a test.

r=me with that.
Comment 6 Doug Wright 2011-08-08 12:14:46 PDT
If a test is really tricky, can the patch get checked in anyway in the meantime?
Comment 7 Honza Bambas (:mayhemer) 2011-08-08 12:21:16 PDT
The test is probably not that tricky, just no time to write it now.  Anyway, I think it is too late, we have some 4 days to merge to aurora.  I don't think we want to take this change w/o proper testing on the trunk first.
Comment 8 Honza Bambas (:mayhemer) 2011-10-12 10:55:58 PDT
Created attachment 566591 [details] [diff] [review]
v1+test

Ready to land
Comment 10 Marco Bonardo [::mak] 2011-11-10 03:18:06 PST
https://hg.mozilla.org/mozilla-central/rev/8d10107531b3

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