Bug 1668548 - Only pass the error value to a cleanup function with QM_TRY*. r=dom-workers-and-storage-reviewers,janv
authorSimon Giesecke <sgiesecke@mozilla.com>
Fri, 02 Oct 2020 10:04:37 +0000
changeset 551249 65463463ec80ca707fe5011dc42672198cba9dfb
parent 551248 874a5f59d8bec0d0e34ad2049d3e315498211062
child 551250 08ae3ebf13e90bc9aaeeaeff766b47fbbd9bb181
push id37830
push usernbeleuzu@mozilla.com
push dateSat, 03 Oct 2020 10:23:35 +0000
treeherdermozilla-central@7d7faf0b6d7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdom-workers-and-storage-reviewers, janv
bugs1668548
milestone83.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 1668548 - Only pass the error value to a cleanup function with QM_TRY*. r=dom-workers-and-storage-reviewers,janv Differential Revision: https://phabricator.services.mozilla.com/D92067
dom/indexedDB/ActorsParent.cpp
dom/indexedDB/IDBObjectStore.cpp
dom/quota/QuotaCommon.h
dom/quota/test/gtest/TestQuotaCommon.cpp
--- a/dom/indexedDB/ActorsParent.cpp
+++ b/dom/indexedDB/ActorsParent.cpp
@@ -12497,18 +12497,18 @@ void ValueCursorBase::ProcessFiles(Curso
       MOZ_ASSERT(serializedInfo);
       MOZ_ASSERT(serializedInfo->files().IsEmpty());
       MOZ_ASSERT(this->mDatabase);
 
       IDB_TRY_VAR(serializedInfo->files(),
                   SerializeStructuredCloneFiles((*this->mBackgroundParent),
                                                 this->mDatabase, files,
                                                 /* aForPreprocess */ false),
-                  QM_VOID, [&aResponse](auto& result) {
-                    aResponse = ClampResultCode(result.unwrapErr());
+                  QM_VOID, [&aResponse](const nsresult result) {
+                    aResponse = ClampResultCode(result);
                   });
     }
   }
 }
 
 template <IDBCursorType CursorType>
 void Cursor<CursorType>::SendResponseInternal(
     CursorResponse& aResponse, const FilesArrayT<CursorType>& aFiles) {
@@ -21128,18 +21128,17 @@ void ObjectStoreGetRequestOp::GetRespons
               std::make_move_iterator(mResponse.begin()),
               std::make_move_iterator(mResponse.end()),
               [this, &aResponseSize](StructuredCloneReadInfoParent&& info) {
                 *aResponseSize += info.Size();
                 return ConvertResponse<SerializedStructuredCloneReadInfo>(
                     std::move(info));
               },
               fallible),
-          QM_VOID,
-          [&aResponse](auto& result) { aResponse = result.unwrapErr(); });
+          QM_VOID, [&aResponse](const nsresult result) { aResponse = result; });
     }
 
     return;
   }
 
   aResponse = ObjectStoreGetResponse();
   *aResponseSize = 0;
 
@@ -21147,17 +21146,17 @@ void ObjectStoreGetRequestOp::GetRespons
     SerializedStructuredCloneReadInfo& serializedInfo =
         aResponse.get_ObjectStoreGetResponse().cloneInfo();
 
     *aResponseSize += mResponse[0].Size();
     IDB_TRY_VAR(serializedInfo,
                 ConvertResponse<SerializedStructuredCloneReadInfo>(
                     std::move(mResponse[0])),
                 QM_VOID,
-                [&aResponse](auto& result) { aResponse = result.unwrapErr(); });
+                [&aResponse](const nsresult result) { aResponse = result; });
   }
 }
 
 ObjectStoreGetKeyRequestOp::ObjectStoreGetKeyRequestOp(
     SafeRefPtr<TransactionBase> aTransaction, const RequestParams& aParams,
     bool aGetAll)
     : NormalTransactionOp(std::move(aTransaction)),
       mObjectStoreId(
@@ -21626,35 +21625,34 @@ void IndexGetRequestOp::GetResponse(Requ
                       std::make_move_iterator(mResponse.begin()),
                       std::make_move_iterator(mResponse.end()),
                       [convertResponse,
                        &aResponseSize](StructuredCloneReadInfoParent&& info) {
                         *aResponseSize += info.Size();
                         return convertResponse(std::move(info));
                       },
                       fallible),
-                  QM_VOID, [&aResponse](auto& result) {
-                    aResponse = result.unwrapErr();
-                  });
+                  QM_VOID,
+                  [&aResponse](const nsresult result) { aResponse = result; });
     }
 
     return;
   }
 
   aResponse = IndexGetResponse();
   *aResponseSize = 0;
 
   if (!mResponse.IsEmpty()) {
     SerializedStructuredCloneReadInfo& serializedInfo =
         aResponse.get_IndexGetResponse().cloneInfo();
 
     *aResponseSize += mResponse[0].Size();
     IDB_TRY_VAR(serializedInfo, convertResponse(std::move(mResponse[0])),
                 QM_VOID,
-                [&aResponse](auto& result) { aResponse = result.unwrapErr(); });
+                [&aResponse](const nsresult result) { aResponse = result; });
   }
 }
 
 IndexGetKeyRequestOp::IndexGetKeyRequestOp(
     SafeRefPtr<TransactionBase> aTransaction, const RequestParams& aParams,
     bool aGetAll)
     : IndexRequestOpBase(std::move(aTransaction), aParams),
       mOptionalKeyRange(
--- a/dom/indexedDB/IDBObjectStore.cpp
+++ b/dom/indexedDB/IDBObjectStore.cpp
@@ -877,17 +877,17 @@ RefPtr<IDBRequest> IDBObjectStore::AddOr
                 return FileAddInfo{fileActor, file.Type()};
               }
 
               default:
                 MOZ_CRASH("Should never get here!");
             }
           },
           fallible),
-      nullptr, [&aRv](auto& result) { aRv = result.unwrapErr(); });
+      nullptr, [&aRv](const nsresult result) { aRv = result; });
 
   const auto& params =
       aOverwrite ? RequestParams{ObjectStorePutParams(std::move(commonParams))}
                  : RequestParams{ObjectStoreAddParams(std::move(commonParams))};
 
   auto request = GenerateRequest(aCx, this).unwrap();
 
   if (!aFromCursor) {
--- a/dom/quota/QuotaCommon.h
+++ b/dom/quota/QuotaCommon.h
@@ -386,17 +386,17 @@
  * QM_TRY_VOID/QM_TRY_VAR_VOID/QM_FAIL_VOID is not defined on purpose since
  * it's possible to use QM_TRY/QM_TRY_VAR/QM_FAIL even in void functions.
  * However, QM_TRY(Task(), ) would look odd so it's recommended to use a dummy
  * define QM_VOID that evaluates to nothing instead: QM_TRY(Task(), QM_VOID)
  */
 
 #define QM_VOID
 
-#define QM_PROPAGATE tryTempResult.propagateErr()
+#define QM_PROPAGATE Err(tryTempError)
 
 // QM_MISSING_ARGS and QM_HANDLE_ERROR macros are implementation details of
 // QM_TRY/QM_TRY_VAR/QM_FAIL and shouldn't be used directly.
 
 #define QM_MISSING_ARGS(...)                           \
   do {                                                 \
     static_assert(false, "Did you forget arguments?"); \
   } while (0)
@@ -421,31 +421,31 @@
   }
 
 // Handles the four arguments case when a custom return value needs to be
 // returned
 #define QM_TRY_CUSTOM_RET_VAL(ns, tryResult, expr, customRetVal)         \
   auto tryResult = ::mozilla::ToResult(expr);                            \
   static_assert(std::is_empty_v<typename decltype(tryResult)::ok_type>); \
   if (MOZ_UNLIKELY(tryResult.isErr())) {                                 \
-    auto tryTempResult MOZ_MAYBE_UNUSED = std::move(tryResult);          \
+    auto tryTempError MOZ_MAYBE_UNUSED = tryResult.unwrapErr();          \
     ns::QM_HANDLE_ERROR(expr);                                           \
     return customRetVal;                                                 \
   }
 
 // Handles the five arguments case when a cleanup function needs to be called
 // before a custom return value is returned
 #define QM_TRY_CUSTOM_RET_VAL_WITH_CLEANUP(ns, tryResult, expr, customRetVal, \
                                            cleanup)                           \
   auto tryResult = ::mozilla::ToResult(expr);                                 \
   static_assert(std::is_empty_v<typename decltype(tryResult)::ok_type>);      \
   if (MOZ_UNLIKELY(tryResult.isErr())) {                                      \
-    auto tryTempResult MOZ_MAYBE_UNUSED = std::move(tryResult);               \
+    auto tryTempError = tryResult.unwrapErr();                                \
     ns::QM_HANDLE_ERROR(expr);                                                \
-    cleanup(tryTempResult);                                                   \
+    cleanup(tryTempError);                                                    \
     return customRetVal;                                                      \
   }
 
 // Chooses the final implementation macro for given argument count.
 // It can be used by other modules to define module specific error handling.
 // This could use MOZ_PASTE_PREFIX_AND_ARG_COUNT, but explicit named suffxes
 // read slightly better than plain numbers.
 // See also
@@ -498,31 +498,31 @@
   MOZ_REMOVE_PAREN(target) = tryResult.accessFunction();
 
 // Handles the six arguments case when a custom return value needs to be
 // returned
 #define QM_TRY_VAR_CUSTOM_RET_VAL(ns, tryResult, accessFunction, target, expr, \
                                   customRetVal)                                \
   auto tryResult = (expr);                                                     \
   if (MOZ_UNLIKELY(tryResult.isErr())) {                                       \
-    auto tryTempResult MOZ_MAYBE_UNUSED = std::move(tryResult);                \
+    auto tryTempError MOZ_MAYBE_UNUSED = tryResult.unwrapErr();                \
     ns::QM_HANDLE_ERROR(expr);                                                 \
     return customRetVal;                                                       \
   }                                                                            \
   MOZ_REMOVE_PAREN(target) = tryResult.accessFunction();
 
 // Handles the seven arguments case when a cleanup function needs to be called
 // before a custom return value is returned
 #define QM_TRY_VAR_CUSTOM_RET_VAL_WITH_CLEANUP(                         \
     ns, tryResult, accessFunction, target, expr, customRetVal, cleanup) \
   auto tryResult = (expr);                                              \
   if (MOZ_UNLIKELY(tryResult.isErr())) {                                \
-    auto tryTempResult MOZ_MAYBE_UNUSED = std::move(tryResult);         \
+    auto tryTempError = tryResult.unwrapErr();                          \
     ns::QM_HANDLE_ERROR(expr);                                          \
-    cleanup(tryTempResult);                                             \
+    cleanup(tryTempError);                                              \
     return customRetVal;                                                \
   }                                                                     \
   MOZ_REMOVE_PAREN(target) = tryResult.accessFunction();
 
 // Chooses the final implementation macro for given argument count.
 // It can be used by other modules to define module specific error handling.
 // See also the comment for QM_TRY_META.
 #define QM_TRY_VAR_META(...)                                                \
@@ -590,31 +590,31 @@
   }                                                      \
   return tryResult;
 
 // Handles the four arguments case when a custom return value needs to be
 // returned
 #define QM_TRY_RETURN_CUSTOM_RET_VAL(ns, tryResult, expr, customRetVal) \
   auto tryResult = (expr);                                              \
   if (MOZ_UNLIKELY(tryResult.isErr())) {                                \
-    auto tryTempResult MOZ_MAYBE_UNUSED = std::move(tryResult);         \
+    auto tryTempError MOZ_MAYBE_UNUSED = tryResult.unwrapErr();         \
     ns::QM_HANDLE_ERROR(expr);                                          \
     return customRetVal;                                                \
   }                                                                     \
   return tryResult.unwrap();
 
 // Handles the five arguments case when a cleanup function needs to be called
 // before a custom return value is returned
 #define QM_TRY_RETURN_CUSTOM_RET_VAL_WITH_CLEANUP(ns, tryResult, expr,   \
                                                   customRetVal, cleanup) \
   auto tryResult = (expr);                                               \
   if (MOZ_UNLIKELY(tryResult.isErr())) {                                 \
-    auto tryTempResult MOZ_MAYBE_UNUSED = std::move(tryResult);          \
+    auto tryTempError = tryResult.unwrapErr();                           \
     ns::QM_HANDLE_ERROR(expr);                                           \
-    cleanup(tryTempResult);                                              \
+    cleanup(tryTempError);                                               \
     return customRetVal;                                                 \
   }                                                                      \
   return tryResult.unwrap();
 
 // Chooses the final implementation macro for given argument count.
 // It can be used by other modules to define module specific error handling.
 // See also the comment for QM_TRY_META.
 #define QM_TRY_RETURN_META(...)                                           \
--- a/dom/quota/test/gtest/TestQuotaCommon.cpp
+++ b/dom/quota/test/gtest/TestQuotaCommon.cpp
@@ -96,18 +96,17 @@ TEST(QuotaCommon_Try, Failure_NoErr)
 TEST(QuotaCommon_Try, Failure_WithCleanup)
 {
   bool tryCleanupRan = false;
   bool tryDidNotReturn = false;
 
   nsresult rv = [&tryCleanupRan, &tryDidNotReturn]() -> nsresult {
     QM_TRY(NS_ERROR_FAILURE, QM_PROPAGATE,
            [&tryCleanupRan](const auto& result) {
-             EXPECT_TRUE(result.isErr());
-             EXPECT_EQ(result.inspectErr(), NS_ERROR_FAILURE);
+             EXPECT_EQ(result, NS_ERROR_FAILURE);
 
              tryCleanupRan = true;
            });
 
     tryDidNotReturn = true;
 
     return NS_OK;
   }();
@@ -121,20 +120,19 @@ TEST(QuotaCommon_Try, Failure_WithCleanu
 {
   bool tryCleanupRan = false;
   bool tryDidNotReturn = false;
 
   nsresult rv;
 
   [&tryCleanupRan, &tryDidNotReturn](nsresult& aRv) -> void {
     QM_TRY(NS_ERROR_FAILURE, QM_VOID, ([&tryCleanupRan, &aRv](auto& result) {
-             EXPECT_TRUE(result.isErr());
-             EXPECT_EQ(result.inspectErr(), NS_ERROR_FAILURE);
+             EXPECT_EQ(result, NS_ERROR_FAILURE);
 
-             aRv = result.unwrapErr();
+             aRv = result;
 
              tryCleanupRan = true;
            }));
 
     tryDidNotReturn = true;
 
     aRv = NS_OK;
   }(rv);
@@ -455,18 +453,17 @@ TEST(QuotaCommon_TryInspect, Failure_Wit
 {
   bool tryInspectCleanupRan = false;
   bool tryInspectDidNotReturn = false;
 
   nsresult rv = [&tryInspectCleanupRan, &tryInspectDidNotReturn]() -> nsresult {
     QM_TRY_INSPECT(const auto& x,
                    (Result<int32_t, nsresult>{Err(NS_ERROR_FAILURE)}),
                    QM_PROPAGATE, [&tryInspectCleanupRan](const auto& result) {
-                     EXPECT_TRUE(result.isErr());
-                     EXPECT_EQ(result.inspectErr(), NS_ERROR_FAILURE);
+                     EXPECT_EQ(result, NS_ERROR_FAILURE);
 
                      tryInspectCleanupRan = true;
                    });
     Unused << x;
 
     tryInspectDidNotReturn = true;
 
     return NS_OK;
@@ -483,20 +480,19 @@ TEST(QuotaCommon_TryInspect, Failure_Wit
   bool tryInspectDidNotReturn = false;
 
   nsresult rv;
 
   [&tryInspectCleanupRan, &tryInspectDidNotReturn](nsresult& aRv) -> void {
     QM_TRY_INSPECT(const auto& x,
                    (Result<int32_t, nsresult>{Err(NS_ERROR_FAILURE)}), QM_VOID,
                    ([&tryInspectCleanupRan, &aRv](auto& result) {
-                     EXPECT_TRUE(result.isErr());
-                     EXPECT_EQ(result.inspectErr(), NS_ERROR_FAILURE);
+                     EXPECT_EQ(result, NS_ERROR_FAILURE);
 
-                     aRv = result.unwrapErr();
+                     aRv = result;
 
                      tryInspectCleanupRan = true;
                    }));
     Unused << x;
 
     tryInspectDidNotReturn = true;
 
     aRv = NS_OK;
@@ -865,18 +861,17 @@ TEST(QuotaCommon_TryReturn, Failure_With
 {
   bool tryReturnCleanupRan = false;
   bool tryReturnDidNotReturn = false;
 
   auto res = [&tryReturnCleanupRan,
               &tryReturnDidNotReturn]() -> Result<int32_t, nsresult> {
     QM_TRY_RETURN((Result<int32_t, nsresult>{Err(NS_ERROR_FAILURE)}),
                   QM_PROPAGATE, [&tryReturnCleanupRan](const auto& result) {
-                    EXPECT_TRUE(result.isErr());
-                    EXPECT_EQ(result.inspectErr(), NS_ERROR_FAILURE);
+                    EXPECT_EQ(result, NS_ERROR_FAILURE);
 
                     tryReturnCleanupRan = true;
                   });
 
     tryReturnDidNotReturn = true;
   }();
 
   EXPECT_TRUE(tryReturnCleanupRan);