The default bug view has changed. See this FAQ.

Preference browser.cache.disk_cache_ssl does nothing

RESOLVED FIXED in mozilla1.6alpha

Status

()

Core
Networking: HTTP
--
minor
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Tudor TEUSAN, Assigned: Darin Fisher)

Tracking

Trunk
mozilla1.6alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ready to land])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

14 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

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

Comment 2

14 years ago
Yeah, that pref has been in all.js since version 1.1.
(Reporter)

Comment 3

14 years ago
Created attachment 123700 [details] [diff] [review]
browser.cache.disk_cache_ssl implementation

Comment 4

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

Comment 5

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

Updated

14 years ago
No longer blocks: 205077
(Reporter)

Comment 6

14 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

14 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

14 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

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

Updated

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

Comment 10

14 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

14 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

14 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

14 years ago
Blocks: 209560

Comment 13

14 years ago
Created attachment 129015 [details] [diff] [review]
Persistent HTTPS caching implementation

Implementation of browser.cache.disk_cache_ssl parameter.


Regards
-Pawan

Comment 14

14 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

14 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

14 years ago
Created attachment 130358 [details] [diff] [review]
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
Attachment #129015 - Attachment is obsolete: true
(Assignee)

Comment 17

14 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

14 years ago
Created attachment 130458 [details] [diff] [review]
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
Attachment #130358 - Attachment is obsolete: true
(Assignee)

Comment 19

14 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

14 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

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

Comment 21

14 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

14 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 23

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