468594 - Fix heuristic query freshness

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Bjarne (:bjarne)]

This is a spin-off from bug #462243 addressing the problem of heuristic query-freshness.

Comment 1

[Bjarne (:bjarne)]

Attachment: V1.0 patch w/ xpcshell testcase

Fixes this particular issue on mnot's webpage.

Comment 2

[Bjarne (:bjarne)]

Comment on attachment 352298 [details] [diff] [review]
V1.0 patch w/ xpcshell testcase

Changed reviewer

Comment 3

[Bjarne (:bjarne)]

Comment on attachment 352298 [details] [diff] [review]
V1.0 patch w/ xpcshell testcase

Changed reviewer again...

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Hrm, I'd suggest fixing this by making MustValidate() return true if the URI contains a query but no explicit expiration info, except that that function doesn't have access to the query.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 352298 [details] [diff] [review]
V1.0 patch w/ xpcshell testcase

+        nsresult res = mCachedResponseHead->GetExpiresValue(&tmp);

why not use the "rv" variable that exists already?

+    if (!doValidation && mRequestHead.Method() == nsHttp::Get && index(mSpec.get(),'?'))

This is wrong, mSpec can contain #? in which case you don't want to invalidate the cache entry.

Use mURI->QueryInterface(nsIURL)->GetQuery?

Also it looks like not all lines are below 80 characters, please fix (also in the test)

some of my style comments in bug 203271 apply to the test in this bug as well

Comment 6

[Bjarne (:bjarne)]

Attachment: V1.1 honoring comments from reviewer

Same approach, but using GetQuery() instead which is most likely more robust.

(In reply to comment #4)
> Hrm, I'd suggest fixing this by making MustValidate() return true if the URI
> contains a query but no explicit expiration info, except that that function
> doesn't have access to the query.

I agree that this logic belongs to MustValidate(). It is only called from nsHttpChannel and we could parametrize the method to get access to the query. Your call...  :)

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 366655 [details] [diff] [review]
V1.1 honoring comments from reviewer

+        nsCAutoString tmpString;

query or queryString would be a more descriptive name for this variable :-)

Comment 8

[Bjarne (:bjarne)]

Attachment: V1.2 re-naming string-variable [Checkin: Comment 9]

(In reply to comment #7)
> (From update of attachment 366655 [details] [diff] [review])
> +        nsCAutoString tmpString;
> 
> query or queryString would be a more descriptive name for this variable :-)

I considered calling it "grwwwmPZPX" but decided against it. I guess your suggestion is better...  ;)

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 366852 [details] [diff] [review]
V1.2 re-naming string-variable
[Checkin: Comment 9]


http://hg.mozilla.org/mozilla-central/rev/01987cfc569b

Comment 10

[Sylvain Pasche]

An annoying side effect since this change: bug 490095

Comment 11

[Sylvain Pasche]

The trouble with this change (causing bug 490095) is that it overrides the VALIDATE_NEVER flag (i.e. an HTTP request with validation could still be issued even when the flag is set).
That flag is set when you go back (http://www.google.com/codesearch/p?hl=en#e_ObwTAVPyo/docshell/base/nsDocShell.cpp&q=VALIDATE_NEVER&exact_package=http://hg.mozilla.org/index.cgi/mozilla-central&l=7590).

Putting that logic in MustValidate() is one way to fix that issue.

Comment 12

[(no longer active)]

The fix to this bug has caused the regression in bug 538892.