703508 - Make nsNSSSocketInfo::GetErrorMessage() lazy

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Make nsNSSSocketInfo::GetErrorMessage() lazily format the error message

This is needed in order to simplify the SSL thread removal patch. It is also helpful for bug 697781.

The idea is that we store the error code in nsNSSocketInfo, and we delay formatting the error message returned from GetErrorMessage() until the first time GetErrorMessage() is called.

This + bug 697781 will also be the removal of all the non-main-thread accesses to PSM's string bundles; once both are done, we can avoid initializing the string bundles in nsNSSComponent::Init(). Probably not much impact on startup performance, but this line of thinking is what lead me to doing things this way in the SSL thread removal patch.

I added a mutex to ensure there are no races w.r.t. the lazy calculating of the error message. This mutex should be used in other places too; that is part of bug 427948.

Previously, we *assumed* that the application always set the error message when it calls SetCanceled(). Now, we enforce that.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Make nsNSSSocketInfo::GetErrorMessage() lazily format the error message [v2]

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 575692 [details] [diff] [review]
Make nsNSSSocketInfo::GetErrorMessage() lazily format the error message [v2]

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

r=honzab with some question answered, comments addressed.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +292,2 @@
>  {
> +  MutexAutoLock lock(mMutex);

Please add a blank line after the lock to emphasize its usage.

@@ +305,3 @@
>  }
>  
> +

Remove this blank line.

@@ +508,5 @@
> +static nsresult
> +getInvalidCertErrorMessage(nsISSLStatus & sslStatus,
> +                           PRErrorCode errorCodeToReport, 
> +                           const nsXPIDLCString &host, PRInt32 port,
> +                           nsString &returnedMessage);

Be consistent with spaces around & and *.  I like to have both spaces.

Some comments what the functions do and when are called would be good.

@@ +521,5 @@
> +  if (mErrorCode == 0 || mErrorMessageType == SuppressedErrorMessage) {
> +    return NS_OK;
> +  }
> +
> +  if (mErrorMessageCached.IsEmpty()) {

Rather

if (!mErrorMessageCached.IsEmpty())
  return NS_OK;

?

@@ +527,5 @@
> +    NS_ConvertASCIItoUTF16 hostNameU(mHostName);
> +    nsCOMPtr<nsINSSComponent> nss = do_GetService(kNSSComponentCID, &rv);
> +    if (NS_SUCCEEDED(rv)) {
> +      NS_ASSERTION(mErrorMessageType != CertErrorMessage || 
> +                   (mSSLStatus != nsnull && mSSLStatus->mServerCert),

Check just mSSLStatus (w/o != nsnull).  Is the condition really correct?

@@ +536,5 @@
> +                                        mHostName, mPort, mErrorMessageCached);
> +      } else {
> +        rv = getPlainErrorMessage(mHostName, mPort, mErrorCode, nss,
> +                                  mErrorMessageCached);
> +      }

I would prefer this code as a switch with all values of the type enum, each case having a comment what the message is, where it gets set etc and each also having its own assertions (even duplicated).  This code is very hard to read and understand.

I think nsINSSComponent doesn't need to be passed as an arg but rather get in each method.

@@ +650,5 @@
>    stream->Write32(version | 0xFFFF0000);
>    stream->Write32(mSecurityState);
>    stream->WriteWStringZ(mShortDesc.get());
> +
> +  // XXX: uses nsNSSComponent string bundles off the main thread

String bundles are thread safe.

@@ +1366,5 @@
>    }
>  }
>  
> +static nsresult
> +getInvalidCertErrorMessage(nsISSLStatus & sslStatus,

I would prefer to use pointer here, there is no obvious need for passing by reference.  I and other probably too often search for >Method(..) to see only calls on objects.

@@ +1374,5 @@
>  {
>    const PRUnichar *params[1];
>    nsresult rv;
> +  nsString hostWithPort;
> +  nsString hostWithoutPort;

use nsAutoString?

@@ +1385,5 @@
>    // in error pages in the common case.
>    
> +  if (port == 443) {
> +    hostWithoutPort.AppendASCII(host);
> +    params[0] = hostWithoutPort.get();

Do you need this swap through hostWithoutPort?

@@ +1489,5 @@
> +  nsNSSSocketInfo::ErrorMessageType errorMessageType
> +      = socketInfo->GetExternalErrorReporting()
> +      ? nsNSSSocketInfo::PlainErrorMessage
> +      : nsNSSSocketInfo::SuppressedErrorMessage;
> +  socketInfo->SetCanceled(err, errorMessageType);

In the unpatched code there are two callers of this method:
- CertErrorRunnable::RunOnTargetThread that calls SetCanceled(true) right after it (so no change)
- nsSSLThread::checkHandshake -> SSLErrorRunnable that doesn't call SetCanceled(true) AFAIK (maybe does through the ssl thread)

So, in the letter case I'm not sure we should cancel here, but the whole patch queue works for me, so there is probably no problem.

@@ +3320,2 @@
>  {
> +  // XXX: Should SuppressedErrorMessage be PlainErrorMessage instead?

Probably yes.

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +174,5 @@
>  
> +  enum ErrorMessageType {
> +    SuppressedErrorMessage = 0,
> +    PlainErrorMessage = 1,
> +    CertErrorMessage = 2

Add comments to what each type means, at least.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 575692 [details] [diff] [review]
Make nsNSSSocketInfo::GetErrorMessage() lazily format the error message [v2]

>-cancel_and_failure(nsNSSSocketInfo* infoObject)
>+cancel_and_failure(PRErrorCode errorCode, nsNSSSocketInfo* infoObject)
> {
>-  infoObject->SetCanceled(true);
>+  // XXX: Should SuppressedErrorMessage be PlainErrorMessage instead?
>+  infoObject->SetCanceled(errorCode, nsNSSSocketInfo::SuppressedErrorMessage);

You actually change this even to CertErrorMessage in the ssl thread removal patch.  nsNSSSocketInfo::SetCertVerificationResult is in place of cancel_and_failure in that patch.

Did you think of adding an argument for the message type to GetErrorMessage method?

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> Comment on attachment 575692 [details] [diff] [review] [diff] [details] [review]
> Make nsNSSSocketInfo::GetErrorMessage() lazily format the error message [v2]
> 
> >-cancel_and_failure(nsNSSSocketInfo* infoObject)
> >+cancel_and_failure(PRErrorCode errorCode, nsNSSSocketInfo* infoObject)
> > {
> >-  infoObject->SetCanceled(true);
> >+  // XXX: Should SuppressedErrorMessage be PlainErrorMessage instead?
> >+  infoObject->SetCanceled(errorCode, nsNSSSocketInfo::SuppressedErrorMessage);
> 
> You actually change this even to CertErrorMessage in the ssl thread removal
> patch.  nsNSSSocketInfo::SetCertVerificationResult is in place of
> cancel_and_failure in that patch.
> 
> Did you think of adding an argument for the message type to GetErrorMessage
> method?

I think the thing to do is change SetCertVerificationResult to take the message type, and forward it to SetCanceled.

I believe CertErrorMessage should only be used when there are only overridable errors.

The main thing that I didn't and still don't understand is when/why to suppress the error message.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Make nsNSSSocketInfo::GetErrorMessage() lazily format the error message [v3]

This change addresses Honza's review comments and removes GetExternalErrorReporting and the idea of "Suppressed" errors. I think that our previous work on removing the default error reporting dialog boxes made these ideas obsolete.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> I would prefer this code as a switch with all values of the type enum, each
> case having a comment what the message is, where it gets set etc and each
> also having its own assertions (even duplicated).  This code is very hard to
> read and understand.

I think removing the SuppressedErrorMessage case helps considerably to understand the code. Now there are only two options: overridable cert errors, and all other errors.

> > +  // XXX: uses nsNSSComponent string bundles off the main thread
> 
> String bundles are thread safe.

In nsNSSComponent::Init, we have code to preload the string bundle to avoid getting errors about loading the string bundle off the main thread. I would like to remove that code as part of the effort to streamline nsNSSComponent initialization; to do so, we only need to remove this off-main-thread usage and one more off-main-thread-usage in HandshakeCallback. These comments should help Jiten with bug 697781, which is a prerequisite to that.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 577678 [details] [diff] [review]
Make nsNSSSocketInfo::GetErrorMessage() lazily format the error message [v3]

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

I like this!

r=honzab with:

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +505,5 @@
> +  if (mErrorCode == 0) {
> +    return NS_OK;
> +  }
> +
> +  if (mErrorMessageCached.IsEmpty()) {

I'd still be more fun of early return here, I prefer it in general BTW (makes the code more readable, code might be more optimal in some cases).

@@ +512,5 @@
> +    NS_ASSERTION(mErrorMessageType != CertErrorMessage || 
> +                  (mSSLStatus && mSSLStatus->mServerCert),
> +                  "GetErrorMessage called for cert error without cert");
> +    if (mErrorMessageType == CertErrorMessage && 
> +        mSSLStatus != nsnull && mSSLStatus->mServerCert) {

mSSLStatus && mSSLStatus->mServerCert

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +173,5 @@
>    void GetPreviousCert(nsIX509Cert** _result);
>  
> +  enum ErrorMessageType {
> +    CertErrorMessage  = 1, // for *overridable* certificate errors
> +    PlainErrorMessage = 2, // all other errors

Maybe think of a better name for this const now..

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Make nsNSSSocketInfo::GetErrorMessage() lazily format the error message lazily [v4]

Review comments addressed

Comment 10

[Kai Engert (:KaiE:)]

Do you really want to use ASCII variant of string append functions?
Why is this safe for international host names?

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Kai Engert (:kaie) from comment #10)
> Do you really want to use ASCII variant of string append functions?
> Why is this safe for international host names?

When I made this patch, there was a change to the type of string passed in, and the existing code wouldn't. I looked at the code and saw, like Honza noted in bug 674147, that this string is already ASCII and assumed to be ASCII in other places, so I thought/think AssignASCII is the most appropriate method. If there is an alternate solution that is clearer, then I am happy to make that change given a suggestion as to what to change it to. (Though, at this point I think we should do so in another bug.)

I filed bug 706691 which describes a possible way to make this and other hostname-in-string code clearer in PSM and Necko.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #11)
> When I made this patch, there was a change to the type of string passed in,
> and the existing code wouldn't

...compile.

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 577830 [details] [diff] [review]
Make nsNSSSocketInfo::GetErrorMessage() lazily format the error message lazily [v4]

sr=kaie

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/mozilla-central/rev/7cd14fbf2789