Bug 1665614 - Make mozilla::Result work with non-copyable/non-param error types. r=emilio,jandem

Among other things, there were some misuses of std::forward, and
GenericErrorResult was (presumably accidentally) instatiated with
references as the template argument type, e.g. const nsresult&,
which circumvented the check for not calling it with NS_OK in
ResultExtensions.h

Differential Revision: https://phabricator.services.mozilla.com/D90561

dom/quota/test/gtest/TestQuotaCommon.cpp

@@ -1261,9 +1261,13 @@ struct Failed {
   int x;
 };
 
+inline auto Err(Failed& aErrorValue) {
+  return mozilla::GenericErrorResult<Failed&>(aErrorValue);
+}
+
 static GenericErrorResult<Failed&> Fail() {
   static Failed failed;
-  return Err<Failed&>(failed);
+  return Err(failed);
 }
 
 static Result<Ok, Failed&> Task1(bool pass) {

editor/libeditor/EditorUtils.cpp

@@ -115,7 +115,7 @@ AutoRangeArray::ExtendAnchorFocusRangeFor(
     return Err(NS_ERROR_NOT_INITIALIZED);
   }
 
-  Result<RefPtr<nsRange>, nsresult> result(NS_ERROR_UNEXPECTED);
+  Result<RefPtr<nsRange>, nsresult> result = Err(NS_ERROR_UNEXPECTED);
   nsIEditor::EDirection directionAndAmountResult = aDirectionAndAmount;
   switch (aDirectionAndAmount) {
     case nsIEditor::eNextWord:

js/public/Result.h

@@ -215,4 +215,14 @@ static_assert(sizeof(Result<int*, Error&>) == sizeof(uintptr_t),
 
 }  // namespace JS
 
+namespace mozilla {
+inline auto Err(JS::Error& aErrorValue) {
+  return mozilla::GenericErrorResult<JS::Error&>(aErrorValue);
+}
+
+inline auto Err(JS::OOM& aErrorValue) {
+  return mozilla::GenericErrorResult<JS::OOM&>(aErrorValue);
+}
+}  // namespace mozilla
+
 #endif  // js_Result_h

mfbt/Result.h

@@ -148,8 +148,11 @@ class ResultImplementationNullIsOkBase {
     }
   }
   explicit ResultImplementationNullIsOkBase(E aErrorValue)
-      : mValue(std::piecewise_construct, std::tuple<>(),
-               std::tuple(UnusedZero<E>::Store(aErrorValue))) {
+      : mValue(
+            std::piecewise_construct, std::tuple<>(),
+            std::tuple(UnusedZero<E>::Store(
+                static_cast<std::conditional_t<std::is_reference_v<E>, E, E&&>>(
+                    aErrorValue)))) {
     MOZ_ASSERT(mValue.second() != kNullValue);
   }
 
@@ -471,7 +474,10 @@ class MOZ_MUST_USE_TYPE Result final {
   MOZ_IMPLICIT Result(const V& aValue) : mImpl(aValue) { MOZ_ASSERT(isOk()); }
 
   /** Create an error result. */
-  explicit Result(E aErrorValue) : mImpl(std::forward<E>(aErrorValue)) {
+  explicit Result(
+      std::conditional_t<std::is_reference_v<E>, E, E&&> aErrorValue)
+      : mImpl(static_cast<std::conditional_t<std::is_reference_v<E>, E, E&&>>(
+            aErrorValue)) {
     MOZ_ASSERT(isErr());
   }
 
@@ -481,7 +487,8 @@ class MOZ_MUST_USE_TYPE Result final {
    */
   template <typename E2>
   MOZ_IMPLICIT Result(GenericErrorResult<E2>&& aErrorResult)
-      : mImpl(std::forward<E2>(aErrorResult.mErrorValue)) {
+      : mImpl(static_cast<std::conditional_t<std::is_reference_v<E>, E2, E2&&>>(
+            aErrorResult.mErrorValue)) {
     static_assert(std::is_convertible_v<E2, E>, "E2 must be convertible to E");
     MOZ_ASSERT(isErr());
   }
@@ -730,13 +737,27 @@ class MOZ_MUST_USE_TYPE GenericErrorResult {
   friend class Result;
 
  public:
+  explicit GenericErrorResult(const E& aErrorValue)
+      : mErrorValue(aErrorValue) {}
+
   explicit GenericErrorResult(E&& aErrorValue)
-      : mErrorValue(std::forward<E>(aErrorValue)) {}
+      : mErrorValue(std::move(aErrorValue)) {}
 };
 
 template <typename E>
-inline GenericErrorResult<E> Err(E&& aErrorValue) {
-  return GenericErrorResult<E>(std::forward<E>(aErrorValue));
+class MOZ_MUST_USE_TYPE GenericErrorResult<E&> {
+  E& mErrorValue;
+
+  template <typename V, typename E2>
+  friend class Result;
+
+ public:
+  explicit GenericErrorResult(E& aErrorValue) : mErrorValue(aErrorValue) {}
+};
+
+template <typename E>
+inline auto Err(E&& aErrorValue) {
+  return GenericErrorResult<std::decay_t<E>>(std::forward<E>(aErrorValue));
 }
 
 }  // namespace mozilla

mfbt/ResultExtensions.h

@@ -33,6 +33,13 @@ class MOZ_MUST_USE_TYPE GenericErrorResult<nsresult> {
   operator nsresult() const { return mErrorValue; }
 };
 
+// Do not instantiate these...
+template <>
+class GenericErrorResult<nsresult&>;
+
+template <>
+class GenericErrorResult<const nsresult&>;
+
 // Allow MOZ_TRY to handle `PRStatus` values.
 inline Result<Ok, nsresult> ToResult(PRStatus aValue);
 
@@ -104,7 +111,7 @@ using to_result_retval_t = decltype(
     std::declval<Func&>()(std::declval<Args&&>()...,
                           std::declval<typename RArg<decltype(
                               ResultRefAsParam(std::declval<R&>()))>::type>()),
-    Result<R, nsresult>(NS_OK));
+    Result<R, nsresult>(Err(NS_ERROR_FAILURE)));
 
 // There are two ToResultInvokeSelector overloads, which cover the cases of a) a
 // pointer-typed output parameter, and b) a reference-typed output parameter,

mfbt/tests/TestResult.cpp

@@ -84,9 +84,17 @@ static_assert(sizeof(Foo32) >= sizeof(uintptr_t) ||
                   sizeof(Result<Foo16, Foo32>) <= sizeof(uintptr_t),
               "Result with small types should be pointer-sized");
 
+inline auto Err(Failed& aErrorValue) {
+  return mozilla::GenericErrorResult<Failed&>(aErrorValue);
+}
+
+inline auto Err(double& aErrorValue) {
+  return mozilla::GenericErrorResult<double&>(aErrorValue);
+}
+
 static GenericErrorResult<Failed&> Fail() {
   static Failed failed;
-  return Err<Failed&>(failed);
+  return Err(failed);
 }
 
 static GenericErrorResult<UnusedZeroEnum> FailUnusedZeroEnum() {
@@ -276,7 +284,7 @@ static void MapTest() {
   // Mapping over error values.
   {
     MyError err(1);
-    Result<char, MyError> res(err);
+    Result<char, MyError> res = Err(err);
     MOZ_RELEASE_ASSERT(res.isErr());
     Result<char, MyError> res2 = res.map([](int x) {
       MOZ_RELEASE_ASSERT(false);
@@ -311,7 +319,7 @@ static void MapErrTest() {
   // Mapping over error values, to the same error type.
   {
     MyError err(1);
-    Result<char, MyError> res(err);
+    Result<char, MyError> res = Err(err);
     MOZ_RELEASE_ASSERT(res.isErr());
     bool invoked = false;
     auto res2 = res.mapErr([&invoked](const auto err) {
@@ -327,7 +335,7 @@ static void MapErrTest() {
   // Mapping over error values, to a different error type.
   {
     MyError err(1);
-    Result<char, MyError> res(err);
+    Result<char, MyError> res = Err(err);
     MOZ_RELEASE_ASSERT(res.isErr());
     bool invoked = false;
     auto res2 = res.mapErr([&invoked](const auto err) {
@@ -381,7 +389,7 @@ static void OrElseTest() {
   // variant.
   {
     MyError err(1);
-    Result<char, MyError> res(err);
+    Result<char, MyError> res = Err(err);
     MOZ_RELEASE_ASSERT(res.isErr());
     bool invoked = false;
     auto res2 = res.orElse([&invoked](const auto err) -> Result<char, MyError> {
@@ -401,7 +409,7 @@ static void OrElseTest() {
   // success variant.
   {
     MyError err(42);
-    Result<char, MyError> res(err);
+    Result<char, MyError> res = Err(err);
     MOZ_RELEASE_ASSERT(res.isErr());
     bool invoked = false;
     auto res2 = res.orElse([&invoked](const auto err) -> Result<char, MyError> {
@@ -421,7 +429,7 @@ static void OrElseTest() {
   // error variant.
   {
     MyError err(1);
-    Result<char, MyError> res(err);
+    Result<char, MyError> res = Err(err);
     MOZ_RELEASE_ASSERT(res.isErr());
     bool invoked = false;
     auto res2 =
@@ -442,7 +450,7 @@ static void OrElseTest() {
   // success variant.
   {
     MyError err(42);
-    Result<char, MyError> res(err);
+    Result<char, MyError> res = Err(err);
     MOZ_RELEASE_ASSERT(res.isErr());
     bool invoked = false;
     auto res2 =

netwerk/protocol/res/ExtensionProtocolHandler.cpp

@@ -891,7 +891,7 @@ void ExtensionProtocolHandler::NewSimpleChannel(nsIURI* aURI,
         nsresult rv = origChannel->AsyncOpen(listener);
         if (NS_FAILED(rv)) {
           simpleChannel->Cancel(NS_BINDING_ABORTED);
-          return RequestOrReason(rv);
+          return Err(rv);
         }
         return RequestOrReason(origChannel);
       });

xpcom/ds/ArrayAlgorithm.h

@@ -10,6 +10,7 @@
 #include "nsTArray.h"
 
 #include "mozilla/Algorithm.h"
+#include "mozilla/ResultExtensions.h"
 
 namespace mozilla {
 

xpcom/string/nsTSubstring.h

@@ -18,6 +18,7 @@
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/IntegerTypeTraits.h"
 #include "mozilla/Result.h"
+#include "mozilla/ResultExtensions.h"
 #include "mozilla/Span.h"
 #include "mozilla/Unused.h"