Bug 1346315 - Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/. r=keeler
authorCykesiopka <cykesiopka.bmo@gmail.com>
Tue, 04 Apr 2017 16:56:26 +0800
changeset 351400 11a29a22159a8132996ded7f288e6b6b0b1ece2b
parent 351399 35e5c3c8a2af6e5658d5307e8f10dc02a3f273ca
child 351401 796b747e3efc9f6b56b29301112221de28d3c56a
push id31610
push usercbook@mozilla.com
push dateThu, 06 Apr 2017 09:36:41 +0000
treeherdermozilla-central@3c68d659c2b7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1346315
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1346315 - Enable gcc/clang -Wextra for security/apps/, security/manager/pki/ and security/manager/ssl/. r=keeler -Wextra implies -Wmissing-field-initializers, but since the latter warning seems to warn about mostly uninteresting instances (XPCOM module definitions etc), we disable it for now. (Note that -Wall is already enabled by default for all directories for gcc and clang.) MozReview-Commit-ID: 8RdF51sLPC8
security/apps/moz.build
security/manager/pki/moz.build
security/manager/ssl/SSLServerCertVerification.cpp
security/manager/ssl/moz.build
--- a/security/apps/moz.build
+++ b/security/apps/moz.build
@@ -19,16 +19,26 @@ LOCAL_INCLUDES += [
     '/security/manager/ssl',
     '/security/pkix/include',
 ]
 
 DEFINES['NSS_ENABLE_ECC'] = 'True'
 for var in ('DLL_PREFIX', 'DLL_SUFFIX'):
     DEFINES[var] = '"%s"' % CONFIG[var]
 
+if CONFIG['GNU_CXX']:
+    CXXFLAGS += [
+        '-Wextra',
+    ]
+
+    # Gecko headers aren't warning-free enough for us to enable these warnings.
+    CXXFLAGS += [
+        '-Wno-unused-parameter',
+    ]
+
 test_ssl_path = '/security/manager/ssl/tests/unit'
 
 headers_arrays_certs = [
     ('xpcshell.inc', 'xpcshellRoot', test_ssl_path + '/test_signed_apps/trusted_ca1.der'),
     ('addons-public.inc', 'addonsPublicRoot', 'addons-public.crt'),
     ('addons-stage.inc', 'addonsStageRoot', 'addons-stage.crt'),
     ('privileged-package-root.inc', 'privilegedPackageRoot', 'privileged-package-root.der'),
 ]
--- a/security/manager/pki/moz.build
+++ b/security/manager/pki/moz.build
@@ -19,8 +19,20 @@ UNIFIED_SOURCES += [
     'nsPKIModule.cpp',
 ]
 
 LOCAL_INCLUDES += [
     '!/dist/public/nss',
 ]
 
 FINAL_LIBRARY = 'xul'
+
+if CONFIG['GNU_CXX']:
+    CXXFLAGS += [
+        '-Wextra',
+        # -Wextra enables this warning, but it's too noisy to be useful.
+        '-Wno-missing-field-initializers',
+    ]
+
+    # Gecko headers aren't warning-free enough for us to enable these warnings.
+    CXXFLAGS += [
+        '-Wno-unused-parameter',
+    ]
--- a/security/manager/ssl/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/SSLServerCertVerification.cpp
@@ -1521,16 +1521,21 @@ SSLServerCertVerificationJob::Dispatch(
 {
   // Runs on the socket transport thread
   if (!certVerifier || !infoObject || !serverCert) {
     NS_ERROR("Invalid parameters for SSL server cert validation");
     PR_SetError(PR_INVALID_ARGUMENT_ERROR, 0);
     return SECFailure;
   }
 
+  if (!gCertVerificationThreadPool) {
+    PR_SetError(PR_INVALID_STATE_ERROR, 0);
+    return SECFailure;
+  }
+
   // Copy the certificate list so the runnable can take ownership of it in the
   // constructor.
   // We can safely skip checking if NSS has already shut down here since we're
   // in the middle of verifying a certificate.
   nsNSSShutDownPreventionLock lock;
   UniqueCERTCertList peerCertChainCopy =
     nsNSSCertList::DupCertList(peerCertChain, lock);
   if (!peerCertChainCopy) {
@@ -1539,38 +1544,32 @@ SSLServerCertVerificationJob::Dispatch(
   }
 
   RefPtr<SSLServerCertVerificationJob> job(
     new SSLServerCertVerificationJob(certVerifier, fdForLogging, infoObject,
                                      serverCert, Move(peerCertChainCopy),
                                      stapledOCSPResponse, sctsFromTLSExtension,
                                      providerFlags, time, prtime));
 
-  nsresult nrv;
-  if (!gCertVerificationThreadPool) {
-    nrv = NS_ERROR_NOT_INITIALIZED;
-  } else {
-    nrv = gCertVerificationThreadPool->Dispatch(job, NS_DISPATCH_NORMAL);
-  }
+  nsresult nrv = gCertVerificationThreadPool->Dispatch(job, NS_DISPATCH_NORMAL);
   if (NS_FAILED(nrv)) {
     // We can't call SetCertVerificationResult here to change
     // mCertVerificationState because SetCertVerificationResult will call
     // libssl functions that acquire SSL locks that are already being held at
-    // this point. infoObject->mCertVerificationState will be stuck at
-    // waiting_for_cert_verification here, but that is OK because we already
-    // have to be able to handle cases where we encounter non-cert errors while
-    // in that state.
+    // this point. However, we can set an error with PR_SetError and return
+    // SECFailure, and the correct thing will happen (the error will be
+    // propagated and this connection will be terminated).
     PRErrorCode error = nrv == NS_ERROR_OUT_OF_MEMORY
-                      ? SEC_ERROR_NO_MEMORY
+                      ? PR_OUT_OF_MEMORY_ERROR
                       : PR_INVALID_STATE_ERROR;
-    PORT_SetError(error);
+    PR_SetError(error, 0);
     return SECFailure;
   }
 
-  PORT_SetError(PR_WOULD_BLOCK_ERROR);
+  PR_SetError(PR_WOULD_BLOCK_ERROR, 0);
   return SECWouldBlock;
 }
 
 NS_IMETHODIMP
 SSLServerCertVerificationJob::Run()
 {
   // Runs on a cert verification thread
 
--- a/security/manager/ssl/moz.build
+++ b/security/manager/ssl/moz.build
@@ -187,9 +187,18 @@ for var in ('DLL_PREFIX', 'DLL_SUFFIX'):
 if not CONFIG['MOZ_SYSTEM_NSS']:
     USE_LIBS += [
         'crmf',
     ]
 
 include('/ipc/chromium/chromium-config.mozbuild')
 
 if CONFIG['GNU_CXX']:
-    CXXFLAGS += ['-Wno-error=shadow']
+    CXXFLAGS += [
+        '-Wextra',
+        # -Wextra enables this warning, but it's too noisy to be useful.
+        '-Wno-missing-field-initializers',
+    ]
+
+    # Gecko headers aren't warning-free enough for us to enable these warnings.
+    CXXFLAGS += [
+        '-Wno-unused-parameter',
+    ]