Closed
Bug 205921
Opened 22 years ago
Closed 21 years ago
Preference browser.cache.disk_cache_ssl does nothing
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla1.6alpha
People
(Reporter: ttudor, Assigned: darin.moz)
References
Details
(Whiteboard: [ready to land])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
darin.moz
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
darin.moz
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030512 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030512 Preference "browser.cache.disk_cache_ssl" has no impact on persitently caching https pages. Reproducible: Always Steps to Reproduce: 1. Open about:configure and change browser.cache.disk_cache_ssl to "true" 2. Load a https://... page 3. Check your disk cache (about:cache?device=disk) Actual Results: The https page should(?) be in the disk cache. Expected Results: The https page is only in the memory-cache. Is this the intented behaviour ? A search for "disk_cache_ssl" on lxr.mozilla.org gives no results and no mention of such an option is made in the nsHttpChannel class. It would be, however, nice to have, especially for embedding purposes. IE has a similar, working option (which is inadvertently set on "true" by default). Implementing it would require some simple changes in the nsHttpChannel class (am I wrong ?) that I would gladly make. The default setting browser.cache.disk_cache_ssl = false is surely the right thing to do, but some embedding applications might be interested in caching https urls, which is impossible at present.
Updated•22 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 1•22 years ago
|
||
benc is doing pref cleanup.. maybe he already has this on his list.
Reporter | ||
Comment 3•22 years ago
|
||
Reporter | ||
Comment 6•22 years ago
|
||
Why implementing this preference would be nice ? My company uses mozilla as an application platform : our application is contained in a web page with js, images and html templates. Once loaded all data is retrieved by xmlHttpRequest from the originating server. In order not to require special privileges, both the xmlHttp request and the "static" application (html + img + js) should have the same base uri - for security reasons, https://smth. In order to optimize network usage the static part should be cached. As all this static part (around 1Mb) is addressed by https:// (for the above- mentioned reason), caching it is impossible without this flag. One final argument : Other browsers do persistently cache ssl documents (or at least can do it) ... Would anyone have the time to have a look at the proposed patch ?
Since this is really more of an HTTP issue than a cache issue, I defer to darin, who should be back next week. Changing component.
Assignee: gordon → darin
Component: Networking: Cache → Networking: HTTP
QA Contact: cacheqa → httpqa
Comment 8•21 years ago
|
||
By all means implement the feature. It doesn't need to be advertised, though, because it's dangerous; but I've had people complaining that their bank's site is too slow under Mozilla compared to IE, and if they're willing to take the risk of caching SSL objects on disk, it's their decision. It would be very much of a shame not to apply this patch now that Tudor wrote it (and it's obviously correct).
Assignee | ||
Comment 9•21 years ago
|
||
ok, i can see the utility of this.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.5alpha
Assignee | ||
Updated•21 years ago
|
Attachment #123700 -
Flags: review?(darin)
Assignee | ||
Comment 10•21 years ago
|
||
Comment on attachment 123700 [details] [diff] [review] browser.cache.disk_cache_ssl implementation >+ // check the disk_cache_ssl preference (only for https requests) >+ if (usingSSL) { >+ nsCOMPtr<nsIPrefService> prefService( do_GetService(NS_PREFSERVICE_CONTRACTID) ); >+ if (prefService) { >+ nsCOMPtr<nsIPrefBranch> prefBranch; >+ prefService->GetBranch(nsnull, getter_AddRefs(prefBranch)); >+ if (prefBranch){ >+ PRBool shouldCache = PR_FALSE; >+ prefBranch->GetBoolPref(PERSISTENT_CACHE_SSL_PREF,&shouldCache); >+ if (shouldCache) mDiskCacheSsl = PR_TRUE; this is all wrong. http prefs are managed by nsHttpHandler. move pref logic there.
Attachment #123700 -
Flags: review?(darin) → review-
Reporter | ||
Comment 11•21 years ago
|
||
Sorry for the pref mess. I'm a total beginner in mozilla, I needed it quickly and I did it wrong. Darin, do you want me to rewrite the patch, or do you have the time to put the pref code at the right place ?
Assignee | ||
Comment 12•21 years ago
|
||
i'm not going to have time to write this patch myself. if you want to revise the patch, i'll gladly review it and help get it into the tree :)
Target Milestone: mozilla1.5alpha → Future
Comment 13•21 years ago
|
||
Implementation of browser.cache.disk_cache_ssl parameter. Regards -Pawan
Comment 14•21 years ago
|
||
Thanks pkjoshi. Darin, could you review this new patch? Is there hope of getting it in for 1.5? I am being thwarted in my Mozilla advocacy efforts because of this bug ("my online banking site loads so sloooowly compared to IE", they tell me).
Updated•21 years ago
|
Attachment #129015 -
Flags: review?(darin)
Assignee | ||
Comment 15•21 years ago
|
||
Comment on attachment 129015 [details] [diff] [review] Persistent HTTPS caching implementation >Index: nsHttpChannel.cpp >+ if (mConnectionInfo && mConnectionInfo->UsingSSL() >+ && !gHttpHandler->IsPersistentHttpsCachingEnabled()) > mLoadFlags |= INHIBIT_PERSISTENT_CACHING; >- >+ nit: don't add whitespace on otherwise empty line. >Index: nsHttpHandler.cpp > #define INTL_ACCEPT_CHARSET "intl.charset.default" > #define NETWORK_ENABLEIDN "network.enableIDN" > >+#define BROWSER_PREF_PREFIX "browser.cache." >+ nit: move this #define next to the other XXX_PREF_PREFIX #define's >+ // enable Persistent caching for HTTPS - bug#205921 >+ cVar = PR_FALSE; >+ if (PREF_CHANGED(BROWSER_PREF("disk_cache_ssl"))) { >+ rv = prefs->GetBoolPref(BROWSER_PREF("disk_cache_ssl"), &cVar); >+ if (NS_SUCCEEDED(rv)) { >+ mEnablePersistentHttpsCaching = cVar; >+ } >+ } >+ LOG(("nsHttpHandler::PrefsChanged [disk_cache_ssl=%s]\n", (cVar ? "Enabled" : "Disabled"))); move LOG statement inside |if (PREF_CHANGED(...))| block. likewise, move cVar initialization in there as well. don't use {}'s if not needed. >Index: nsHttpHandler.h >@@ -213,6 +215,9 @@ > PRUint8 mMaxPipelinedRequests; >+ >+ // Persitent HTTPS caching flag >+ PRPackedBool mEnablePersistentHttpsCaching; > > PRUint8 mRedirectionLimit; move this declaration down to the end just after mSendSecureXSiteReferrer.
Attachment #129015 -
Flags: review?(darin) → review-
Comment 16•21 years ago
|
||
I have incorporated all the review comments excpet the 2nd.
> nit: move this #define next to the other XXX_PREF_PREFIX #define's
There are 2 XXX_PREF_PREIX defined in the nsHttpHandler.cpp file. These 2
(UA_PREF_PREFIX and HTTP_PREF_PREFIX) are not defined next to each other. In
that case where should I place the new #define (for BROWSER_PREF_PREFIX), next
to UA_PREF_PREFIX or HTTP_PREF_PREFIX ?
Regards
-Pawan
Attachment #129015 -
Attachment is obsolete: true
Assignee | ||
Comment 17•21 years ago
|
||
Comment on attachment 130358 [details] [diff] [review] Incorporated all review comments except the 2nd oops, you're right... i should have looked at nsHttpHandler.cpp before making that comment instead of trusting my memory :( anyhow, i think this patch is good. the only point i want to make is that (i think) it will require the preference to be set in all.js as a default preference. i.e., changes to the pref using about:config won't work, and might not take effect at all if the user is prompted for a profile to use. to solve this problem you might want to add a AddObserver(BROWSER_PREF("disk_cache_ssl")) with the other AddObserver calls. then you will support dynamic pref changes.
Comment 18•21 years ago
|
||
Added addObserver for "browser.cache.disk_cache_ssl" as suggested by Darin but I have not tested this particular change. Should I submit a patch by changing the deafult for this feature to "true" in the all.js file ? Regards -Pawan
Attachment #130358 -
Attachment is obsolete: true
Assignee | ||
Comment 19•21 years ago
|
||
thanks for the revised patch.
>Should I submit a patch by changing
>the deafult for this feature to "true" in the all.js file ?
no, the default behavior should remain as is. we have an existing policy of not
writing HTTPS content to disk without the user's explicit consent. i don't
think we should change that (yet).
Assignee | ||
Comment 20•21 years ago
|
||
Comment on attachment 130458 [details] [diff] [review] Added addObserver for "browser.cache.disk_cache_ssl" r+sr=darin the only other change i might make is to kill the LOG line since HTTP logging is enabled in Mozilla release builds. if you do not have CVS write access, then i check this in and make that change for you. otherwise, please make that change before landing this patch. also, this will have to wait until the trunk opens for 1.6 alpha.
Attachment #130458 -
Flags: superreview+
Attachment #130458 -
Flags: review+
Assignee | ||
Updated•21 years ago
|
Whiteboard: [ready to land]
Target Milestone: Future → mozilla1.6alpha
Comment 21•21 years ago
|
||
I do not have CVS write access, Please make that change for me and check that in. Thanks and Regards -Pawan
Assignee | ||
Comment 22•21 years ago
|
||
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 23•21 years ago
|
||
updated pref docs: http://www.mozilla.org/quality/networking/docs/netprefs.html
You need to log in
before you can comment on or make changes to this bug.
Description
•