Open Bug 697781 Opened 13 years ago Updated 2 years ago

Cache entries for HTTPS pages store their TLS metadata inefficiently

Categories

(Core :: Networking: Cache, defect, P5)

defect

Tracking

()

mozilla10
Tracking Status
fennec + ---

People

(Reporter: briansmith, Unassigned)

References

Details

(Keywords: platform, Whiteboard: [necko-would-take])

Attachments

(1 file, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #686248 +++
+++ This bug was initially created as a clone of Bug #575950 +++

1. Two copies of the certificate are stored.
3. UI strings for the result of certificate validation are stored, when they should be recalculated by revalidating the certificate (see bug 686248).
3. Error messages are stored; instead, we should just avoid storing pages with SSL errors in the cache.
3. mSubRequests*Security information can be stored more efficiently. (I believe these fields be made booleans instead of counts.)
4. The bitfields in nsSSLStatus can be packed into a single byte.
5. Instead of writing the cipher suite name, we can write the two-byte ID of the cipher suite.
6. We do not need to write mHaveKeyLengthAndCipher because if we don't have that info, we shouldn't be writing the entry to the cache.
7. We don't need to write mHaveCertErrorBits, because if we have cert errors, we should just avoid writing the entry to the cache.
Assignee: nobody → jitenmt
Target Milestone: --- → mozilla10
Michal, Bjarne, Nick: I would like to do this work in such a way that we DO NOT have to change the cache version number. That is, I don't want to have to throw away the cache when the user upgrades just to make this optimization. But, I also want to ensure that if earlier versions of Firefox use the cache, that they gracefully avoid reading the cache entries where the security-info is written in the new format. That is, I want the older versions to read the cache entry and say "hey, this security-info isn't in the form I am expecting, so I will just ignore it." Do you have any suggestions for doing this? 

The biggest win will come from storing only one copy of the certificate instead of two. That is what I would focus on first, and that doesn't require any prerequisite work or special handling for wyciwyg:// or anything like that.

These things can also be done without any prerequisite work, beyond the compatibility stuff I mentioned above:

* mSubRequests*Security information can be stored more efficiently. (I believe these fields be made booleans instead of counts.)
* The bitfields in nsSSLStatus can be packed into a single byte.
* Instead of writing the cipher suite name, we can write the two-byte ID of the cipher suite.
* We do not need to write mHaveKeyLengthAndCipher because if we don't have that info, we shouldn't be writing the entry to the cache.

Also, I think I am wrong about these:
* Error messages are stored; instead, we should just avoid storing pages with SSL errors in the cache.
* We don't need to write mHaveCertErrorBits, because if we have cert errors, we should just avoid writing the entry to the cache.

We may need to support this for WYCIWYG for session restore (only). I suggest that we file a separate bug for "fixing" the caching of resources that were retrieved on a connection with cert errors.

This one can be handled in bug 660749; I would ignore it for now:
* UI strings for the result of certificate validation are stored, when they should be recalculated by revalidating the certificate (see bug 686248).
No longer depends on: CVE-2011-0082
Can you just store the new-style security info in a different (new) metadata field? Say, "security-info-v2"? That way, older versions can just gleefully pretend that field doesn't exist, since they'll have no knowledge of it. Of course, if they'll still try to use cached https pages without a security-info field, that could be problematic, but I imagine that would be a problematic situation regardless of the reason security-info doesn't exist.
(In reply to Nick Hurley from comment #2)
> That way, older versions can just gleefully pretend that field doesn't exist,
> since they'll have no knowledge of it. Of course, if they'll still try to use 
> cached https pages without a security-info field, that could be problematic,
> but I imagine that would be a problematic situation regardless of the reason 
> security-info doesn't exist.

This won't work. We need to prevent older Firefoxes from using these cache entries at all; otherwise (at best) they won't show the right SSL indicators and at worst they could be making incorrect security decisions. I am thinking that, for the new entries, to put something in "security-info" that will cause nsNSSSocketInfo::Read() for older versions to return an error. But, we need to make sure that (a) the cache doesn't continue on using the cache entry when nsNSSSocketInfo::Read() fails, and (b) that a failure in nsNSSSocketInfo::Read() will cause the resource to be fetched from the server, instead of just causing a busted page load.
Attached patch patch_1 (obsolete) — Splinter Review
-Packed bitfields in nsSSLStatus in one structure.
-Instead of writing of cipherSuite name, wrote cipherSuiteID
-Made GetShortSecurityDescription lazy
-Made GetOrganization lazy
Attachment #582517 - Flags: review?(bsmith)
Comment on attachment 582517 [details] [diff] [review]
patch_1

Review of attachment 582517 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start.

It looks like you forgot to remove the redundant writing of the certificate from nsNSSSocketInfo::Write(). The certificate is already written as part of the nsSSLStatus.

I do not think it is necessary to create the nested mDetails field of nsSSLStatus. I guess you did this because you wanted to make the variables bitfields. That isn't necessary. The idea was to pack them into a single byte when writing/reading them. But, in your patch, each bit is still written to disk as a separate byte.

The biggest savings will be from avoiding writing the short security description string and the redundant certificate to disk. I think we don't have to worry now about trying to save space for the boolean flags. We can do that in a follow-up patch.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +844,1 @@
>      return;

We cannot return early like this, because we need the following code to always execute.

Instead, we should use NS_ERROR("SSL_GetChannelInfo failed") and set a flag status->mDetails.mGetChannelInfoFailed = true; Then, check this flag in the functions in nsSSLStatus that need the information from SSL_GetChannelInfo.

@@ +844,3 @@
>      return;
>    }
> +  if (SECSuccess != SSL_GetCipherSuiteInfo(channelInfo.cipherSuite, &cipherSuiteInfo, sizeof cipherSuiteInfo)) {

Do not call SSL_GetCipherSuiteInfo here. Instead, when you need some information about the cipher suite in one of the nsSSLStatus methods, just call SSL_GetCipherSuiteInfo in that method.

@@ +853,5 @@
> +    sslStatus = SSL_SECURITY_STATUS_OFF;}
> +  else if (encryptBits < 90) {
> +    sslStatus = SSL_SECURITY_STATUS_ON_LOW;}
> +  else {
> +    sslStatus = SSL_SECURITY_STATUS_ON_HIGH;}

Move the calculation of encryptBits to 

Note that you will have to save the result of SSL_HandshakeNegotiatedExtension(fd, ssl_renegotiation_info_xtn, &siteSupportsSafeRenego) in a new member variables of nsSSLStatus, since you need to know that to calculate sslStatus.

When all of this is done, all the dependencies on nsIWebProgressListener would be gone from this file, and we can then remove the #include "nsIWebProgressListener.h"

@@ +913,5 @@
>      rv = nssComponent->PIPBundleFormatStringFromName("SignedBy",
>                                                     formatStrings, 1,
>                                                     shortDesc);
>  
>      nsMemory::Free(const_cast<PRUnichar*>(formatStrings[0]));

It looks like you copied all of this code from line 896 (declaration of peerCert) through line 917 (freeing of formatStrings[0]), that deals with the short security description, to nsSSLStatus::GetShortSecurityDescription. That was the correct thing to do. But, also, you need to delete it from this function.

@@ +968,2 @@
>      status->mKeyLength = keyLength;
>      status->mSecretKeyLength = encryptBits;

nsSSLStatus::GetSecretKeyLength() should call SSL_GetCipherSuiteInfo, so that we don't have to have an mSecretKeyLength variable.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +456,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
>  nsNSSSocketInfo::GetShortSecurityDescription(PRUnichar** aText) {

NS_ENSURE_ARG_POINTER(aText);
*aText = nsnull;
nsRefPtr<nsSSLStatus> status = mSSLStatus;
if (!status)
    return NS_ERROR_NOT_AVAILABLE;
nsCOMPtr<nsIX509Cert> serverCert = status->mServerCert;
if (!serverCert)
    return NS_ERROR_NOT_AVAILABLE;

Then, use serverCert below, instead of mSSLtatus->mServerCert.

@@ +476,5 @@
> +  nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
> +  if (NS_SUCCEEDED(rv)) {
> +    rv = nssComponent->PIPBundleFormatStringFromName("SignedBy",
> +                                                   formatStrings, 1,
> +                                                   shortDesc);

The indention is wrong on the two lines above.

@@ +596,5 @@
>    // to distinguish version number from mSecurityState
>    // field stored in times before versioning has been introduced.
>    // This mask value has been chosen as mSecurityState could
>    // never be assigned such value.
>    PRUint32 version = 3;

We need to change the version here (to 4, I guess), since we are changing the format of the security info.

And, in Read(), we have to verify that the new version is being used.

@@ +598,5 @@
>    // This mask value has been chosen as mSecurityState could
>    // never be assigned such value.
>    PRUint32 version = 3;
>    stream->Write32(version | 0xFFFF0000);
>    stream->Write32(mSecurityState);

Instead of writing mSecurityState, we should write the variables that are used to calculate the security state.

@@ +599,5 @@
>    // never be assigned such value.
>    PRUint32 version = 3;
>    stream->Write32(version | 0xFFFF0000);
>    stream->Write32(mSecurityState);
> +  //stream->WriteWStringZ(mShortDesc.get());

Remove this line, instead of commenting it out.

@@ +604,1 @@
>    stream->WriteWStringZ(mErrorMessage.get());

Instead of writing the error string, we should write the error  code. And, in particular, we should avoid calling formatErrorString() in this method.

@@ +687,2 @@
>    if ((version & 0xFFFF0000) == 0xFFFF0000) {
>      version &= ~0xFFFF0000;

We need to return an error if the security info isn't the version that we expect (version 4).

@@ +691,5 @@
>    else {
>      mSecurityState = version;
>      version = 1;
>    }
> +  //stream->ReadString(mShortDesc);

Do not comment out code to remove it; just remove it.
Attachment #582517 - Flags: review?(bsmith) → review-
Note that also, we need to check what happens when the cache tries to read a cache entry that was written using an earlier version (version 3) of the nsNSSSocketInfo/nsSSLstatus serialization. We want to make sure that the cache doesn't use the entry and that instead we fetch the resource from disk.

We should have a test case for that.
>@@ +853,5 @@
>> +    sslStatus = SSL_SECURITY_STATUS_OFF;}
>> +  else if (encryptBits < 90) {
>> +    sslStatus = SSL_SECURITY_STATUS_ON_LOW;}
>> +  else {
>> +    sslStatus = SSL_SECURITY_STATUS_ON_HIGH;}
>
>Move the calculation of encryptBits to 

Brian, I think there's a dangling sentence here.
(In reply to Josh Matthews [:jdm] from comment #7)
> >@@ +853,5 @@
> >> +    sslStatus = SSL_SECURITY_STATUS_OFF;}
> >> +  else if (encryptBits < 90) {
> >> +    sslStatus = SSL_SECURITY_STATUS_ON_LOW;}
> >> +  else {
> >> +    sslStatus = SSL_SECURITY_STATUS_ON_HIGH;}
> >
> >Move the calculation of encryptBits to 

... nsSSLStatus::GetSecretKeyLength.

By the way, here's why: Instead of storing calculated values in the disk cache, we should store only the inputs to the calculations. That way, if the calculations change in the future, we can still get the correct with old cache entries.
Assignee: jitenmt → nobody
Assignee: nobody → jitenmt
Attached patch patch_2 (obsolete) — Splinter Review
-Created version 4 of cache
-I have added mGetChannelInfoFailed field but it is not used anywhere
Attachment #582517 - Attachment is obsolete: true
Attachment #584526 - Flags: feedback?(bsmith)
Comment on attachment 584526 [details] [diff] [review]
patch_2

Review of attachment 584526 [details] [diff] [review]:
-----------------------------------------------------------------

I added Honza as the superreviewer.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ -856,4 @@
>    PRBool siteSupportsSafeRenego;
>    if (SSL_HandshakeNegotiatedExtension(fd, ssl_renegotiation_info_xtn, &siteSupportsSafeRenego) != SECSuccess
>        || !siteSupportsSafeRenego) {
> -

Undo this whitespace change.

@@ -873,5 @@
>          console->LogStringMessage(msg.get());
>        }
>      }
> -    if (nsSSLIOLayerHelpers::treatUnsafeNegotiationAsBroken()) {
> -      secStatus = nsIWebProgressListener::STATE_IS_BROKEN;

You need to save the fact that renegotiation wasn't supported into nsNSSSocketInfo, so that you can calculate the state correctly in nsNSSSocketInfo::GetSecurityState

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +338,5 @@
>  
>  NS_IMETHODIMP
>  nsNSSSocketInfo::GetSecurityState(PRUint32* state)
>  {
> +  if (nsSSLIOLayerHelpers::treatUnsafeNegotiationAsBroken()) {

nsSSLIOLayerHelpers::treatUnsafeNegotiationAsBroken refers to the setting of a preference.

What you need to know here is whether the secure renegotiation extension was negotiated. You can do this by creating a new member variable mSecureRenegotiation in nsNSSSocketInfo and setting it in HandshakeCallback.

Don't forget to serialize the new flag in Write() and deserialize it in Read().

@@ +343,5 @@
> +    *state = nsIWebProgressListener::STATE_IS_BROKEN;
> +    return NS_OK;
> +  }
> +  PRUint32 encryptBits;
> +  mSSLStatus->GetSecretKeyLength(&encryptBits);

nsresult rv = mSSLStatus->GetSecretKeyLength(&encryptBits);
NS_ENSURE_SUCCESS(rv, rv);

(Because mSSLStatus->GetSecretKeyLength can fail.)

@@ +426,5 @@
> +  if (!serverCert)
> +    return NS_ERROR_NOT_AVAILABLE;
> +  const char* caName = nsnull; // caName is a pointer only, no ownership
> +  nsAutoString certOrgName;
> +  mSSLStatus->mServerCert->GetOrganization(certOrgName);

This should be GetIssuerOrganization. We want the name of the CA, not the name of the EE.

Also, check the return value with NS_ENSURE_SUCCESS.

@@ +428,5 @@
> +  const char* caName = nsnull; // caName is a pointer only, no ownership
> +  nsAutoString certOrgName;
> +  mSSLStatus->mServerCert->GetOrganization(certOrgName);
> +  nsresult rv;
> +  caName =  ToNewUTF8String(certOrgName);

ToNewUTF8String will allocate a new string that needs to be freed.

instead, you can do something like:
const PRUnichar* caName
    = certIssuerOrgName == "RSA Data Security, Inc."
    ? NS_LITERAL_STRING("Verisign, Inc.")
    : certIssuerOrgName.get();

Maybe you have to use .Equals() instead, and/or wrap "RSA Data Security, Inc." with NS_LITERAL_STRING.

Make sure you test this in the browser by looking at the larry popup for sites with Verisign and non-Verisign issued certificates.

@@ +438,5 @@
> +    caName = verisignName;
> +  }
> +
> +  nsAutoString shortDesc;
> +  const PRUnichar* formatStrings[1] = {NS_ConvertUTF8toUTF16(caName).get()};

If you make caName PRUnichar*, you will not need the UTF8-to-UTF16 conversion.

@@ +440,5 @@
> +
> +  nsAutoString shortDesc;
> +  const PRUnichar* formatStrings[1] = {NS_ConvertUTF8toUTF16(caName).get()};
> +  nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
> +  if (NS_SUCCEEDED(rv)) {

NS_ENSURE_SUCCESS(rv, rv). Otherwise, you end up returning NS_OK without setting *aText in this (extremely unlikely situation).

@@ +445,5 @@
> +    rv = nssComponent->PIPBundleFormatStringFromName("SignedBy",
> +                                                   formatStrings, 1,
> +                                                   shortDesc);
> +
> +    nsMemory::Free(const_cast<PRUnichar*>(formatStrings[0]));

I think we don't need this call to nsMemory::Free, if we make the changes I suggested above.

@@ -413,5 @@
>    return NS_OK;
>  }
>  
> -nsresult
> -nsNSSSocketInfo::SetShortSecurityDescription(const PRUnichar* aText) {

Remove the declaration from the class definition too.

@@ +689,5 @@
>    MutexAutoLock lock(mMutex);
>  
> +  // Store the flag that the certificate is not present so that previous versions
> +  // don't look for redundant certificate object
> +  stream->WriteBoolean(0);

We don't need to write this flag any more.

Since we are going to need to create a whole new cache version, we don't have to worry about backward compatibility with this function. We can remove all of the junk dealing with the various formats in both Write() and Read() and just straightforwardly read and write the new format you are creating here.

@@ +702,1 @@
>    stream->Write32(version | 0xFFFF0000);

We won't need to mask with 0xFFFF0000 any more.

@@ +703,5 @@
>  
>    // XXX: uses nsNSSComponent string bundles off the main thread
>    nsresult rv = formatErrorMessage(lock); 
>    NS_ENSURE_SUCCESS(rv, rv);
>    stream->WriteWStringZ(mErrorMessageCached.get());

We should be writing the mErrorCode instead of mErrorMessageCached. (And, we consequently, we don't need to call formatErrorMessage anymore.)

@@ +705,5 @@
>    nsresult rv = formatErrorMessage(lock); 
>    NS_ENSURE_SUCCESS(rv, rv);
>    stream->WriteWStringZ(mErrorMessageCached.get());
>  
>    stream->WriteCompoundObject(NS_ISUPPORTS_CAST(nsISSLStatus*, status),

Since this is done holding the lock, there's no use in copy mStatus to status any more.

Add back the warning about serializing a missing nsNSSocketInfo:
if (!mSSLStatus) {
    NS_WARNING("Serializing nsNSSSocketInfo without mSSLStatus");
}

@@ +772,5 @@
>          do_CreateInstance(kNSSCertificateCID, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      // This is the redundant copy of the certificate; just ignore it
>      serializable->Read(stream);

Remove this, because we don't write the redundant copy of the cert any more.

@@ +781,5 @@
>    // If the version field we have just read is not masked with 0xFFFF0000
>    // then it is stored mSecurityState field and this is version 1 of
>    // the binary data stream format.
>    if ((version & 0xFFFF0000) == 0xFFFF0000) {
>      version &= ~0xFFFF0000;

We can remove all of the above masking logic since we don't have to support reading version 1 entries any more.

@@ +783,5 @@
>    // the binary data stream format.
>    if ((version & 0xFFFF0000) == 0xFFFF0000) {
>      version &= ~0xFFFF0000;
> +    if(version!=4){
> +      NS_ERROR("Cache version is not 4");

Instead, use NS_ENSURE_TRUE(version == 4, NS_ERROR_INVALID_DATA);

@@ +794,2 @@
>    stream->ReadString(mErrorMessageCached);
>    mErrorCode = 0;

This should be a read of mErrorCode.

::: security/manager/ssl/src/nsSSLStatus.cpp
@@ +75,5 @@
>      return NS_ERROR_NOT_AVAILABLE;
>  
> +  SSLCipherSuiteInfo cipherSuiteInfo;
> +  if (SECSuccess != SSL_GetCipherSuiteInfo(mCipherSuiteID, &cipherSuiteInfo, sizeof cipherSuiteInfo)) {
> +    NS_ERROR("SSL_GetCipherSuiteInfo");

instead of NS_ERROR, call NS_WARNING and then return NS_ERROR_UNEXPECTED.

@@ +93,5 @@
> +  SSLCipherSuiteInfo info;
> +  SECStatus status;
> +  status = SSL_GetCipherSuiteInfo(mCipherSuiteID, &info, sizeof info);
> +  if (status != SECSuccess) {
> +    printf("SSL_GetCipherSuiteInfo rejected suite 0x%04x\n",

remove this printf.

@@ +95,5 @@
> +  status = SSL_GetCipherSuiteInfo(mCipherSuiteID, &info, sizeof info);
> +  if (status != SECSuccess) {
> +    printf("SSL_GetCipherSuiteInfo rejected suite 0x%04x\n",
> +            mCipherSuiteID);
> +    NS_WARNING("SSL_GetCipherSuiteInfo");

return NS_ERROR_UNEXPECTED;

@@ +102,1 @@
>  

NS_ENSURE_TRUE(*_result, NS_ERROR_OUT_OF_MEMORY);

@@ -134,5 @@
>  
>    rv = stream->Read32(&mKeyLength);
>    NS_ENSURE_SUCCESS(rv, rv);
> -  rv = stream->Read32(&mSecretKeyLength);
> -  NS_ENSURE_SUCCESS(rv, rv);

Do not remove the NS_ENSURE_SUCCESS(...) calls.

In fact, instead we should be adding the missing NS_ENSURE_SUCCESS calls to nsNSSSocketInfo::Read() and Write(). (We can do that in another bug though.)

@@ +150,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  PRUint32 Details;

lowercase details.

::: security/manager/ssl/src/nsSSLStatus.h
@@ +52,5 @@
> +#define ISNOTVALIDATTHISTIME 1
> +#define ISUNTRUSTED 2
> +#define HAVEKEYLENGTHANDCIPHER 3
> +#define HAVECERTERRORBITS 4
> +#define GETCHANNELINFOFAILED 5

Since these do not need to be used outsode of nsSSLStatus.cpp, move them to nsSSLStatus.cpp. Since they don't need to be used by C code, define them like this:

namespace {
const PRUint32 IS_DOMAIN_MISMATCH = 1;
const PRUint32 IS_NOT_VALUE_AT_THIS_TIME = 2;
const PRUint32 IS_UNTRUSTED = 4;
const PRUint32 HAVE_KEY_LENGTH_AND_CIPHER = 8;
...
}

This way, Read() can do things like this:

mIsDomainMismatch = details & IS_DOMAIN_MISMATCH;
mIsUntrusted = details & IS_UNTRUSTED;
...

and Write can be:

details |= mIsDomainMismatch ? IS_DOMAIN_MISMATCH : 0;
details |= mIsUntrusted ? IS_UNTRUSTED : 0;
...

I think this is clearer than the bit shifting.

@@ +83,5 @@
>  
>    /* mHaveCertErrrorBits is relied on to determine whether or not a SPDY
>       connection is eligible for joining in nsNSSSocketInfo::JoinConnection() */
>    bool mHaveCertErrorBits;
> +  bool mGetChannelInfoFailed;

Hmm...It seems like mGetChannelInfoFailed is always equal to mHaveKeyLengthAndCipher so, I guess we don't need it. (This is why you couldn't find a place to test it.)
Attachment #584526 - Flags: superreview?(honzab.moz)
Attachment #584526 - Flags: review-
Attachment #584526 - Flags: feedback?(bsmith)
(In reply to Brian Smith (:bsmith) from comment #10)
> @@ +95,5 @@
> > +  status = SSL_GetCipherSuiteInfo(mCipherSuiteID, &info, sizeof info);
> > +  if (status != SECSuccess) {
> > +    printf("SSL_GetCipherSuiteInfo rejected suite 0x%04x\n",
> > +            mCipherSuiteID);
> > +    NS_WARNING("SSL_GetCipherSuiteInfo");

Remove this printf too.

> @@ -134,5 @@
> >  
> >    rv = stream->Read32(&mKeyLength);
> >    NS_ENSURE_SUCCESS(rv, rv);
> > -  rv = stream->Read32(&mSecretKeyLength);
> > -  NS_ENSURE_SUCCESS(rv, rv);
> 
> Do not remove the NS_ENSURE_SUCCESS(...) calls.

Ignore this comment. I misread the patch.
@@ +705,5 @@
>    nsresult rv = formatErrorMessage(lock); 
>    NS_ENSURE_SUCCESS(rv, rv);
>    stream->WriteWStringZ(mErrorMessageCached.get());
>  
>    stream->WriteCompoundObject(NS_ISUPPORTS_CAST(nsISSLStatus*, status),

Since this is done holding the lock, there's no use in copy mStatus to status any more.

Add back the warning about serializing a missing nsNSSocketInfo:
if (!mSSLStatus) {
    NS_WARNING("Serializing nsNSSSocketInfo without mSSLStatus");
}

Brian, can you elaborate this one?
(In reply to Jiten [:deLta30] from comment #12)
> @@ +705,5 @@
> >    nsresult rv = formatErrorMessage(lock); 
> >    NS_ENSURE_SUCCESS(rv, rv);
> >    stream->WriteWStringZ(mErrorMessageCached.get());
> >  
> >    stream->WriteCompoundObject(NS_ISUPPORTS_CAST(nsISSLStatus*, status),
> 
> Since this is done holding the lock, there's no use in copy mStatus to
> status any more.

I just mean we don't need the "status" local variable any more; we can use "mStatus" everywhere instead.

> Add back the warning about serializing a missing nsNSSocketInfo:
> if (!mSSLStatus) {
>     NS_WARNING("Serializing nsNSSSocketInfo without mSSLStatus");
> }
> 
> Brian, can you elaborate this one?

I believe we should never be serializing an nsNSSSocketInfo that is missing the mSSLStatus member. But, I haven't proved it yet. By adding the warning here, we will see it when debugging, if this case ever comes up. It also helps document that we expect that things might not work if this case comes up.
Attached patch patch_3 (obsolete) — Splinter Review
Mochitests to be done.
Attachment #584526 - Attachment is obsolete: true
Attachment #584526 - Flags: superreview?(honzab.moz)
Comment on attachment 589464 [details] [diff] [review]
patch_3

>+  const PRUnichar* caName;
>+  if(certIssuerOrgName.EqualsLiteral("RSA Data Security, Inc."))
>+    NS_NAMED_LITERAL_STRING(caName, "Verisign, Inc.");
>+  else
>+    caName = certIssuerOrgName.get();

NS_NAMED_LITERAL_STRING does not do what you want here. It just declares a local, shadowed string named caName that goes out of scope immediately and leaves the original caName unchanged.
Can we change shortSecurityDescription to be a DOMString?
Attached patch patch_3Splinter Review
Slight refining. Needs to be tested by mochitest.
Attachment #589464 - Attachment is obsolete: true
Looking good; I don't see any failures that look like they were caused by your changes.
Brian, can you have a look at the patch to finalize?
Comment on attachment 589571 [details] [diff] [review]
patch_3

Looks like this got left on the ground.
Attachment #589571 - Flags: review?(bsmith)
Jiten, I did not forget about this patch. It turns out that there are some complicating factors; the laziness is great for desktop but it might not work well for B2G, and I think we need to have two different serializations (one for interprocess communication, one for storage on disk).

The other issue is that older releases do not implement good error recovery for the case where the security info is not in the old format that they understand. This makes our lives difficult here because we're specifically trying to change on-disk format. We have a mechanism for telling older versions that the *whole cache* is in a new format, but this mechanism currently requires us to throw away the whole cache and start over, which is something we don't want to do. When we started working on this, we had thought that we were going to change the on-disk format of the cache anyway, but that got changed. :(
Comment on attachment 589571 [details] [diff] [review]
patch_3

Clearing review request. I won't have time to look at this any time soon. At this point everything cache-related probably needs to wait for the new cache to be ready.
Attachment #589571 - Flags: review?(brian)
Whiteboard: [necko-would-take]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P5
Assignee: jitenmt → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.