Bug 1493226, part 1 - Statically prevent trivial calls to do_QueryInterface r=froydnj
authorAndrew McCreight <continuation@gmail.com>
Thu, 04 Oct 2018 19:16:28 +0000
changeset 495392 a5ebcd6b0fbe7f84220120d60daac61cc9bc5eb9
parent 495391 90f4fff5f1453a2eb43c5c7d37a380dde2c7b204
child 495393 4bada21c38bf68253216b06b7f21e2beaf047f6e
push id9984
push userffxbld-merge
push dateMon, 15 Oct 2018 21:07:35 +0000
treeherdermozilla-beta@183d27ea8570 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1493226
milestone64.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 1493226, part 1 - Statically prevent trivial calls to do_QueryInterface r=froydnj This patch adds a static assert to enforce that do_QueryInterface is not used to go from a class to a base class, because that can be done more efficiently via a static_cast. This is done by putting the type of the source into the nsQueryInterface type. Once that is done, it is easy to check the source and destination type. This has to be done carefully so that in non-DEBUG builds, where NSCAP_FEATURE_USE_BASE is defined, we don't generate any additional code. The first step is to rename nsQueryInterface to nsQueryInterfaceISupports. In DEBUG builds, I then add a new subtype nsQueryInterface<T>, where T is the type of the object we are going to QI. This class is a thin wrapper around nsQueryInterfaceISupports that only forwards operations to the base class. The main bit of trickery here is PointedToType<T>, which is needed to get the type parameter for nsQueryInterface. This dereferences the type, gets the address of it, then does RemovePointer. This is needed because a wide variety of pointer types are passed in to do_QueryInterface, including RefPtr<>, nsCOMPtr<>, raw pointers, and OwningNonNull<>. PointedToType<RefPtr<T>> is equal to T, PointedToType<T*> is equal to T, and so on. In NSCAP_FEATURE_USE_BASE builds (opt), we only use nsQueryInterfaceISupports, but in debug builds we use nsQueryInterface<> where possible. The latter is also used for the nsCOMPtr<nsISupports> overload, because we can always QI to nsISupports, because that is sometimes used for canonicalization. Another gross bit is that Assert_NoQueryNeeded can no longer use nsCOMPtr<>, because it works by QIing T to T, which is banned by the static analysis. Instead I had to reimplement it by hand. Depends on D7527 Differential Revision: https://phabricator.services.mozilla.com/D7553
xpcom/base/nsCOMPtr.cpp
xpcom/base/nsCOMPtr.h
--- a/xpcom/base/nsCOMPtr.cpp
+++ b/xpcom/base/nsCOMPtr.cpp
@@ -2,17 +2,17 @@
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsCOMPtr.h"
 
 nsresult
-nsQueryInterface::operator()(const nsIID& aIID, void** aAnswer) const
+nsQueryInterfaceISupports::operator()(const nsIID& aIID, void** aAnswer) const
 {
   nsresult status;
   if (mRawPtr) {
     status = mRawPtr->QueryInterface(aIID, aAnswer);
   } else {
     status = NS_ERROR_NULL_POINTER;
   }
 
@@ -40,17 +40,17 @@ nsCOMPtr_base::assign_with_AddRef(nsISup
 {
   if (aRawPtr) {
     NSCAP_ADDREF(this, aRawPtr);
   }
   assign_assuming_AddRef(aRawPtr);
 }
 
 void
-nsCOMPtr_base::assign_from_qi(const nsQueryInterface aQI, const nsIID& aIID)
+nsCOMPtr_base::assign_from_qi(const nsQueryInterfaceISupports aQI, const nsIID& aIID)
 {
   void* newRawPtr;
   if (NS_FAILED(aQI(aIID, &newRawPtr))) {
     newRawPtr = nullptr;
   }
   assign_assuming_AddRef(static_cast<nsISupports*>(newRawPtr));
 }
 
--- a/xpcom/base/nsCOMPtr.h
+++ b/xpcom/base/nsCOMPtr.h
@@ -150,50 +150,85 @@ public:
 };
 
 /*
  * nsQueryInterface could have been implemented as an nsCOMPtr_helper to avoid
  * adding specialized machinery in nsCOMPtr, but do_QueryInterface is called
  * often enough that the codesize savings are big enough to warrant the
  * specialcasing.
  */
-class MOZ_STACK_CLASS nsQueryInterface final
+class MOZ_STACK_CLASS nsQueryInterfaceISupports
 {
 public:
   explicit
-  nsQueryInterface(nsISupports* aRawPtr) : mRawPtr(aRawPtr) {}
+  nsQueryInterfaceISupports(nsISupports* aRawPtr) : mRawPtr(aRawPtr) {}
 
   nsresult NS_FASTCALL operator()(const nsIID& aIID, void**) const;
 
 private:
   nsISupports* MOZ_OWNING_REF mRawPtr;
 };
 
+#ifndef NSCAP_FEATURE_USE_BASE
+template<typename T>
+class MOZ_STACK_CLASS nsQueryInterface final : public nsQueryInterfaceISupports
+{
+public:
+  explicit
+  nsQueryInterface(T* aRawPtr) : nsQueryInterfaceISupports(aRawPtr) {}
+
+  nsresult NS_FASTCALL operator()(const nsIID& aIID, void** aAnswer) const {
+    return nsQueryInterfaceISupports::operator()(aIID, aAnswer);
+  }
+};
+#endif // #ifndef NSCAP_FEATURE_USE_BASE
+
 class nsQueryInterfaceWithError final
 {
 public:
   nsQueryInterfaceWithError(nsISupports* aRawPtr, nsresult* aError)
     : mRawPtr(aRawPtr)
     , mErrorPtr(aError)
   {
   }
 
   nsresult NS_FASTCALL operator()(const nsIID& aIID, void**) const;
 
 private:
   nsISupports* MOZ_OWNING_REF mRawPtr;
   nsresult* mErrorPtr;
 };
 
-inline nsQueryInterface
+#ifdef NSCAP_FEATURE_USE_BASE
+
+inline nsQueryInterfaceISupports
 do_QueryInterface(nsISupports* aRawPtr)
 {
-  return nsQueryInterface(aRawPtr);
+  return nsQueryInterfaceISupports(aRawPtr);
 }
 
+#else
+
+namespace mozilla {
+// PointedToType<> is needed so that do_QueryInterface() will work with a
+// variety of smart pointer types in addition to raw pointers. These types
+// include RefPtr<>, nsCOMPtr<>, and OwningNonNull<>.
+template<class T>
+using PointedToType = typename mozilla::RemovePointer<decltype(&*mozilla::DeclVal<T>())>::Type;
+} // namespace mozilla
+
+template<class T>
+inline nsQueryInterface<mozilla::PointedToType<T>>
+do_QueryInterface(T aPtr)
+{
+  return nsQueryInterface<mozilla::PointedToType<T>>(aPtr);
+}
+
+#endif // ! #ifdef NSCAP_FEATURE_USE_BASE
+
 inline nsQueryInterfaceWithError
 do_QueryInterface(nsISupports* aRawPtr, nsresult* aError)
 {
   return nsQueryInterfaceWithError(aRawPtr, aError);
 }
 
 template<class T>
 inline void
@@ -312,17 +347,17 @@ public:
     if (mRawPtr) {
       NSCAP_RELEASE(this, mRawPtr);
     }
   }
 
   void NS_FASTCALL
   assign_with_AddRef(nsISupports*);
   void NS_FASTCALL
-  assign_from_qi(const nsQueryInterface, const nsIID&);
+  assign_from_qi(const nsQueryInterfaceISupports, const nsIID&);
   void NS_FASTCALL
   assign_from_qi_with_error(const nsQueryInterfaceWithError&, const nsIID&);
   void NS_FASTCALL
   assign_from_gs_cid(const nsGetServiceByCID, const nsIID&);
   void NS_FASTCALL
   assign_from_gs_cid_with_error(const nsGetServiceByCIDWithError&, const nsIID&);
   void NS_FASTCALL
   assign_from_gs_contractid(const nsGetServiceByContractID, const nsIID&);
@@ -374,17 +409,18 @@ class MOZ_IS_REFPTR nsCOMPtr final
 
 #ifdef NSCAP_FEATURE_USE_BASE
   #define NSCAP_CTOR_BASE(x) nsCOMPtr_base(x)
 #else
   #define NSCAP_CTOR_BASE(x) mRawPtr(x)
 
 private:
   void assign_with_AddRef(nsISupports*);
-  void assign_from_qi(const nsQueryInterface, const nsIID&);
+  template<typename U>
+  void assign_from_qi(const nsQueryInterface<U>, const nsIID&);
   void assign_from_qi_with_error(const nsQueryInterfaceWithError&, const nsIID&);
   void assign_from_gs_cid(const nsGetServiceByCID, const nsIID&);
   void assign_from_gs_cid_with_error(const nsGetServiceByCIDWithError&,
                                      const nsIID&);
   void assign_from_gs_contractid(const nsGetServiceByContractID, const nsIID&);
   void assign_from_gs_contractid_with_error(
     const nsGetServiceByContractIDWithError&, const nsIID&);
   void assign_from_query_referent(const nsQueryReferent&, const nsIID&);
@@ -427,18 +463,23 @@ public:
     }
   }
 #endif
 
 #ifdef NSCAP_FEATURE_TEST_DONTQUERY_CASES
   void Assert_NoQueryNeeded()
   {
     if (mRawPtr) {
-      nsCOMPtr<T> query_result(do_QueryInterface(mRawPtr));
-      NS_ASSERTION(query_result.get() == mRawPtr, "QueryInterface needed");
+      // This can't be defined in terms of do_QueryInterface because
+      // that bans casts from a class to itself.
+      void* out = nullptr;
+      mRawPtr->QueryInterface(NS_GET_TEMPLATE_IID(T), &out);
+      T* query_result = static_cast<T*>(out);
+      MOZ_ASSERT(query_result == mRawPtr, "QueryInterface needed");
+      NS_RELEASE(query_result);
     }
   }
 
   #define NSCAP_ASSERT_NO_QUERY_NEEDED() Assert_NoQueryNeeded();
 #else
   #define NSCAP_ASSERT_NO_QUERY_NEEDED()
 #endif
 
@@ -554,17 +595,22 @@ public:
     // But make sure that U actually inherits from T.
     static_assert(mozilla::IsBaseOf<T, U>::value,
                   "U is not a subclass of T");
     NSCAP_LOG_ASSIGNMENT(this, static_cast<T*>(mRawPtr));
     NSCAP_ASSERT_NO_QUERY_NEEDED();
   }
 
   // Construct from |do_QueryInterface(expr)|.
-  MOZ_IMPLICIT nsCOMPtr(const nsQueryInterface aQI)
+#ifdef NSCAP_FEATURE_USE_BASE
+  MOZ_IMPLICIT nsCOMPtr(const nsQueryInterfaceISupports aQI)
+#else
+  template<typename U>
+  MOZ_IMPLICIT nsCOMPtr(const nsQueryInterface<U> aQI)
+#endif // ! #ifdef NSCAP_FEATURE_USE_BASE
     : NSCAP_CTOR_BASE(nullptr)
   {
     assert_validity();
     NSCAP_LOG_ASSIGNMENT(this, nullptr);
     assign_from_qi(aQI, NS_GET_TEMPLATE_IID(T));
   }
 
   // Construct from |do_QueryInterface(expr, &rv)|.
@@ -706,17 +752,22 @@ public:
     static_assert(mozilla::IsBaseOf<T, U>::value,
                   "U is not a subclass of T");
     assign_assuming_AddRef(static_cast<T*>(aRhs.take()));
     NSCAP_ASSERT_NO_QUERY_NEEDED();
     return *this;
   }
 
   // Assign from |do_QueryInterface(expr)|.
-  nsCOMPtr<T>& operator=(const nsQueryInterface aRhs)
+#ifdef NSCAP_FEATURE_USE_BASE
+  nsCOMPtr<T>& operator=(const nsQueryInterfaceISupports aRhs)
+#else
+  template<typename U>
+  nsCOMPtr<T>& operator=(const nsQueryInterface<U> aRhs)
+#endif // ! #ifdef NSCAP_FEATURE_USE_BASE
   {
     assign_from_qi(aRhs, NS_GET_TEMPLATE_IID(T));
     return *this;
   }
 
   // Assign from |do_QueryInterface(expr, &rv)|.
   nsCOMPtr<T>& operator=(const nsQueryInterfaceWithError& aRhs)
   {
@@ -938,17 +989,17 @@ public:
   // Construct from |otherComPtr.forget()|.
   MOZ_IMPLICIT nsCOMPtr(already_AddRefed<nsISupports>&& aSmartPtr)
     : nsCOMPtr_base(aSmartPtr.take())
   {
     NSCAP_LOG_ASSIGNMENT(this, mRawPtr);
   }
 
   // Construct from |do_QueryInterface(expr)|.
-  MOZ_IMPLICIT nsCOMPtr(const nsQueryInterface aQI)
+  MOZ_IMPLICIT nsCOMPtr(const nsQueryInterfaceISupports aQI)
     : nsCOMPtr_base(nullptr)
   {
     NSCAP_LOG_ASSIGNMENT(this, nullptr);
     assign_from_qi(aQI, NS_GET_IID(nsISupports));
   }
 
   // Construct from |do_QueryInterface(expr, &rv)|.
   MOZ_IMPLICIT nsCOMPtr(const nsQueryInterfaceWithError& aQI)
@@ -1038,17 +1089,17 @@ public:
   // Assign from |otherComPtr.forget()|.
   nsCOMPtr<nsISupports>& operator=(already_AddRefed<nsISupports>&& aRhs)
   {
     assign_assuming_AddRef(aRhs.take());
     return *this;
   }
 
   // Assign from |do_QueryInterface(expr)|.
-  nsCOMPtr<nsISupports>& operator=(const nsQueryInterface aRhs)
+  nsCOMPtr<nsISupports>& operator=(const nsQueryInterfaceISupports aRhs)
   {
     assign_from_qi(aRhs, NS_GET_IID(nsISupports));
     return *this;
   }
 
   // Assign from |do_QueryInterface(expr, &rv)|.
   nsCOMPtr<nsISupports>& operator=(const nsQueryInterfaceWithError& aRhs)
   {
@@ -1210,19 +1261,23 @@ nsCOMPtr<T>::assign_with_AddRef(nsISuppo
 {
   if (aRawPtr) {
     NSCAP_ADDREF(this, aRawPtr);
   }
   assign_assuming_AddRef(reinterpret_cast<T*>(aRawPtr));
 }
 
 template<class T>
+template<typename U>
 void
-nsCOMPtr<T>::assign_from_qi(const nsQueryInterface aQI, const nsIID& aIID)
+nsCOMPtr<T>::assign_from_qi(const nsQueryInterface<U> aQI, const nsIID& aIID)
 {
+  static_assert(!(mozilla::IsSame<T, U>::value ||
+                  mozilla::IsBaseOf<T, U>::value),
+                "don't use do_QueryInterface for compile-time-determinable casts");
   void* newRawPtr;
   if (NS_FAILED(aQI(aIID, &newRawPtr))) {
     newRawPtr = nullptr;
   }
   assign_assuming_AddRef(static_cast<T*>(newRawPtr));
 }
 
 template<class T>