Preference browser.cache.disk_cache_ssl does nothing

RESOLVED FIXED in mozilla1.6alpha

Status

()

defect
--
minor
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: ttudor, Assigned: darin.moz)

Tracking

Trunk
mozilla1.6alpha
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ready to land])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

16 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 1

16 years ago
benc is doing pref cleanup.. maybe he already has this on his list.

Comment 2

16 years ago
Yeah, that pref has been in all.js since version 1.1.

Comment 4

16 years ago
gordon: should we remove this pref, or implement the feature?

Comment 5

16 years ago
I'm trying to have the prefs cleaned up for 1.4f...
Blocks: 205077

Updated

16 years ago
No longer blocks: 205077
Reporter

Comment 6

16 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 ?

Comment 7

16 years ago
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

16 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

16 years ago
ok, i can see the utility of this.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.5alpha
Assignee

Updated

16 years ago
Attachment #123700 - Flags: review?(darin)
Assignee

Comment 10

16 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

16 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

16 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
Assignee

Updated

16 years ago
Blocks: jar:https

Comment 13

16 years ago
Implementation of browser.cache.disk_cache_ssl parameter.


Regards
-Pawan

Comment 14

16 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).
Attachment #129015 - Flags: review?(darin)
Assignee

Comment 15

16 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

16 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

16 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

16 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

16 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

16 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

16 years ago
Whiteboard: [ready to land]
Target Milestone: Future → mozilla1.6alpha

Comment 21

16 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

16 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.