The default bug view has changed. See this FAQ.

Make nsNSSSocketInfo::GetErrorMessage() lazy

RESOLVED FIXED in mozilla11

Status

()

Core
Security: PSM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

(Blocks: 1 bug)

unspecified
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 575400 [details] [diff] [review]
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.
Attachment #575400 - Flags: review?(honzab.moz)
Created attachment 575692 [details] [diff] [review]
Make nsNSSSocketInfo::GetErrorMessage() lazily format the error message [v2]
Attachment #575400 - Attachment is obsolete: true
Attachment #575400 - Flags: review?(honzab.moz)
Attachment #575692 - Flags: review?(honzab.moz)
Duplicate of this bug: 698988
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.
Attachment #575692 - Flags: review?(honzab.moz) → review+
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?
(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.
Created attachment 577678 [details] [diff] [review]
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.
Attachment #575692 - Attachment is obsolete: true
Attachment #577678 - Flags: review?(honzab.moz)
(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 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..
Attachment #577678 - Flags: review?(honzab.moz) → review+
Created attachment 577830 [details] [diff] [review]
Make nsNSSSocketInfo::GetErrorMessage() lazily format the error message lazily [v4]

Review comments addressed
Attachment #577678 - Attachment is obsolete: true
Attachment #577830 - Flags: review+

Comment 10

5 years ago
Do you really want to use ASCII variant of string append functions?
Why is this safe for international host names?
(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.
(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

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

sr=kaie
Attachment #577830 - Flags: superreview+
https://hg.mozilla.org/mozilla-central/rev/7cd14fbf2789
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.