Closed Bug 468594 Opened 14 years ago Closed 13 years ago

Fix heuristic query freshness

Categories

(Core :: Networking: Cache, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: bjarne, Assigned: bjarne)

References

Details

Attachments

(1 file, 2 obsolete files)

This is a spin-off from bug #462243 addressing the problem of heuristic query-freshness.
Attached patch V1.0 patch w/ xpcshell testcase (obsolete) — Splinter Review
Fixes this particular issue on mnot's webpage.
Attachment #352298 - Flags: review?(cbiesinger)
Blocks: 462243
Comment on attachment 352298 [details] [diff] [review]
V1.0 patch w/ xpcshell testcase

Changed reviewer
Attachment #352298 - Flags: review?(cbiesinger) → review?(jonas)
Flags: wanted1.9.1?
Comment on attachment 352298 [details] [diff] [review]
V1.0 patch w/ xpcshell testcase

Changed reviewer again...
Attachment #352298 - Flags: review?(jonas) → review?(cbiesinger)
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 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
Attachment #352298 - Flags: review?(cbiesinger) → review-
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...  :)
Attachment #352298 - Attachment is obsolete: true
Attachment #366655 - Flags: review?(cbiesinger)
Attachment #366655 - Flags: review?(cbiesinger) → review+
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 :-)
(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...  ;)
Attachment #366655 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 366852 [details] [diff] [review]
V1.2 re-naming string-variable
[Checkin: Comment 9]


http://hg.mozilla.org/mozilla-central/rev/01987cfc569b
Attachment #366852 - Attachment description: V1.2 re-naming string-variable → V1.2 re-naming string-variable [Checkin: Comment 9]
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Depends on: 490095
An annoying side effect since this change: bug 490095
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.
Flags: wanted1.9.1?
Depends on: 538892
The fix to this bug has caused the regression in bug 538892.
You need to log in before you can comment on or make changes to this bug.