205921 - Preference browser.cache.disk_cache_ssl does nothing

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Tudor TEUSAN]

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.

Comment 1

[Darin Fisher]

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

Comment 2

[gordon]

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

Comment 3

[Tudor TEUSAN]

Attachment: browser.cache.disk_cache_ssl implementation

Comment 4

[benc]

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

Comment 5

[benc]

I'm trying to have the prefs cleaned up for 1.4f...

Comment 6

[Tudor TEUSAN]

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

[gordon]

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.

Comment 8

[David A. Madore]

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).

Comment 9

[Darin Fisher]

ok, i can see the utility of this.

Comment 10

[Darin Fisher]

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.

Comment 11

[Tudor TEUSAN]

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 ?

Comment 12

[Darin Fisher]

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 :)

Comment 13

[pkjoshi]

Attachment: Persistent HTTPS caching implementation

Implementation of browser.cache.disk_cache_ssl parameter.


Regards
-Pawan

Comment 14

[David A. Madore]

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).

Comment 15

[Darin Fisher]

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.

Comment 16

[pkjoshi]

Attachment: Incorporated all review comments except the 2nd

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

Comment 17

[Darin Fisher]

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

[pkjoshi]

Attachment: Added addObserver for "browser.cache.disk_cache_ssl"

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

Comment 19

[Darin Fisher]

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).

Comment 20

[Darin Fisher]

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.

Comment 21

[pkjoshi]

I do not have CVS write access, Please make that change for me and check that 
in.

Thanks and Regards
-Pawan 

Comment 22

[Darin Fisher]

fixed-on-trunk

Comment 23

[benc]

updated pref docs:
http://www.mozilla.org/quality/networking/docs/netprefs.html