Closed
Bug 406869
Opened 17 years ago
Closed 17 years ago
[FIX]SetCacheAsFile(PR_TRUE) on memory-cached loads is broken (doesn't throw)
Categories
(Core :: Networking: HTTP, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.9beta2
People
(Reporter: jruderman, Assigned: bzbarsky)
References
()
Details
(Keywords: regression)
Attachments
(2 files)
(deleted),
application/java-archive
|
Details | |
(deleted),
patch
|
Biesinger
:
review+
Biesinger
:
superreview+
mtschrep
:
approvalM10+
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007120401 Minefield/3.0b2pre Steps to reproduce: 1. Load part of the testcase from bug 406740: jar:https://bugzilla.mozilla.org/attachment.cgi?id=291410!/diagnostic.html 2. View Source Result: the View Source window appears, but it's empty. "Save As" seems to be broken too. Regression from bug 369814?
Reporter | ||
Comment 1•17 years ago
|
||
Oops, the testcase is from bug 406744, not bug 406740.
Assignee | ||
Comment 2•17 years ago
|
||
Assignee | ||
Comment 3•17 years ago
|
||
This regressed between 2007-11-30-02 and 2007-12-01-02. That corresponds to the checkins for bug 262116 and bug 345181. What happens is that nsDownloader::OnStopRequest QIs the channel (which is an HTTP channel) to nsICachingChannel. This succeeds. Then it calls GetCacheFile(), which fails with NS_ERROR_NOT_IMPLEMENTED because the channel is using the memory cache device. This error status is then propagated to OnDownloadComplete, etc.
Assignee | ||
Comment 4•17 years ago
|
||
So the problem is that the HTTP channel is using the memory cache device but does NOT have the INHIBIT_PERSISTENT_CACHING load flag set. Which means that SetCacheAsFile() returns success (incorrectly) and nsDownloader::OnStartRequest doesn't set up a temp file (which in this case it should). Looks like a regression from bug 345181.
Blocks: 345181
Component: Networking: JAR → Networking: HTTP
Flags: blocking1.9?
Keywords: regression
QA Contact: networking.jar → networking.http
Assignee | ||
Comment 5•17 years ago
|
||
nsHttpChannel::InitCacheEntry is never called in this case, since we never _write_ to cache. All we do is read from it.
Assignee | ||
Updated•17 years ago
|
Summary: View Source on jar: URL is broken → SetCacheAsFile(PR_TRUE) on memory-cached loads is broken (doesn't throw)
Comment 6•17 years ago
|
||
Bz do we need to fix this for b2 you think?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Assignee | ||
Comment 7•17 years ago
|
||
The users of SetCacheAsFile() in our code are nsDownloader and the plugin host. The former is only an issue for from-cache loads of things like jar:. The latter will be a problem for from-cache loads over ssl of all sorts of plug-in stuff. I'm just not sure how much this will bite in practice. I don't think this should be a particularly hard fix, in any case, so I think we should definitely try to fix it for b2...
Assignee | ||
Comment 8•17 years ago
|
||
So I see two main options: 1) In OpenCacheEntry() (or the async callback, as relevant), set the INHIBIT_PERSISTENT_CACHING flag if the cache entry storage policy is STORE_IN_MEMORY 2) Make SetStoragePolicy(STORE_ON_DISK_AS_FILE) fail if the current policy is STORE_IN_MEMORY or the cache entry is opened readonly (or both). In either case, we should fix GetStoragePolicy() (which has never worked; luckily no one uses it). I sort of prefer #2, I think. Christian, what do you think?
Updated•17 years ago
|
Priority: P2 → P1
Comment 9•17 years ago
|
||
I prefer 2) as well. This was broken for no-store HTTP loads too, right?
Assignee | ||
Comment 10•17 years ago
|
||
Yeah, any time we cache in memory this would be broken. I guess I should write a testcase with a no-store header or something, eh? Sounds painful, since I have to guarantee a cache hit... :(
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #291826 -
Flags: superreview?(cbiesinger)
Attachment #291826 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•17 years ago
|
Summary: SetCacheAsFile(PR_TRUE) on memory-cached loads is broken (doesn't throw) → [FIX]SetCacheAsFile(PR_TRUE) on memory-cached loads is broken (doesn't throw)
Target Milestone: --- → mozilla1.9 M10
Updated•17 years ago
|
Attachment #291826 -
Flags: superreview?(cbiesinger)
Attachment #291826 -
Flags: superreview+
Attachment #291826 -
Flags: review?(cbiesinger)
Attachment #291826 -
Flags: review+
Updated•17 years ago
|
Attachment #291826 -
Flags: approvalM10+
Checked in as requested for b2!
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 13•17 years ago
|
||
Yeah, we wanted this. I just didn't have a chance to watch the tree until tonight. vlad, thanks for checking this in!
Flags: in-testsuite?
Updated•16 years ago
|
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Updated•16 years ago
|
Flags: blocking1.8.1.15? → blocking1.8.1.16?
Updated•16 years ago
|
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.17?
You need to log in
before you can comment on or make changes to this bug.
Description
•