Bug 1880192 - [3.5/5] ThreadUtils.h: remove little-used storage cases r=xpcom-reviewers,necko-reviewers,jesup,nika

`const T&&` parameters don't have associated storage semantics defined
for them. Previously they would end up as `StoreCopyPassByRRef`, which
might even have been intentional. Forbid them, and change the one use
case invoking it to a non-reference (becoming StoreCopyPassByConstLRef).

Additionally, there are four optional storage classes that are never
automatically selected. Two of these are never used, and a third is only
used mistakenly (...ByLRef where only ...ByConstLRef is needed). Adjust
the third's use-sites, and remove all three.

The last, `StoreCopyPassByPtr`, has more of an argument to be kept: it's
simpler to use (and, arguably, to understand) than its lambda-function
equivalent when wrapping an XPCOM method that takes an [in]-pointer
argument -- but it has only one use site in the entire codebase. Replace
and remove it, as well.

No functional changes. All deleted cases remain possible via lambda
functions fed to NS_NewRunnableFunction.

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

dom/media/AudioSegment.h

@@ -318,7 +318,7 @@ struct AudioChunk {
  * A list of audio samples consisting of a sequence of slices of SharedBuffers.
  * The audio rate is determined by the track, not stored in this class.
  */
-class AudioSegment : public MediaSegmentBase<AudioSegment, AudioChunk> {
+class AudioSegment final : public MediaSegmentBase<AudioSegment, AudioChunk> {
   // The channel count that MaxChannelCount() returned last time it was called.
   uint32_t mMemoizedMaxChannelCount = 0;
 

dom/media/VideoFrameConverter.h

@@ -109,8 +109,8 @@ class VideoFrameConverter {
             // for processing so it can be immediately sent.
             mLastFrameQueuedForProcessing.mTime = time;
 
-            MOZ_ALWAYS_SUCCEEDS(mTaskQueue->Dispatch(
-                NewRunnableMethod<StoreCopyPassByLRef<FrameToProcess>>(
+            MOZ_ALWAYS_SUCCEEDS(
+                mTaskQueue->Dispatch(NewRunnableMethod<FrameToProcess>(
                     "VideoFrameConverter::ProcessVideoFrame", this,
                     &VideoFrameConverter::ProcessVideoFrame,
                     mLastFrameQueuedForProcessing)));
@@ -138,8 +138,8 @@ class VideoFrameConverter {
             mLastFrameQueuedForProcessing.mForceBlack = true;
             mLastFrameQueuedForProcessing.mImage = nullptr;
 
-            MOZ_ALWAYS_SUCCEEDS(mTaskQueue->Dispatch(
-                NewRunnableMethod<StoreCopyPassByLRef<FrameToProcess>>(
+            MOZ_ALWAYS_SUCCEEDS(
+                mTaskQueue->Dispatch(NewRunnableMethod<FrameToProcess>(
                     "VideoFrameConverter::ProcessVideoFrame", this,
                     &VideoFrameConverter::ProcessVideoFrame,
                     mLastFrameQueuedForProcessing)));
@@ -293,11 +293,10 @@ class VideoFrameConverter {
       return;
     }
 
-    MOZ_ALWAYS_SUCCEEDS(mTaskQueue->Dispatch(
-        NewRunnableMethod<StoreCopyPassByLRef<FrameToProcess>>(
-            "VideoFrameConverter::ProcessVideoFrame", this,
-            &VideoFrameConverter::ProcessVideoFrame,
-            mLastFrameQueuedForProcessing)));
+    MOZ_ALWAYS_SUCCEEDS(mTaskQueue->Dispatch(NewRunnableMethod<FrameToProcess>(
+        "VideoFrameConverter::ProcessVideoFrame", this,
+        &VideoFrameConverter::ProcessVideoFrame,
+        mLastFrameQueuedForProcessing)));
   }
 
   void ProcessVideoFrame(const FrameToProcess& aFrame) {

dom/media/webspeech/recognition/SpeechRecognition.cpp

@@ -437,12 +437,13 @@ uint32_t SpeechRecognition::ProcessAudioSegment(AudioSegment* aSegment,
   // we need to call the nsISpeechRecognitionService::ProcessAudioSegment
   // in a separate thread so that any eventual encoding or pre-processing
   // of the audio does not block the main thread
-  nsresult rv = mEncodeTaskQueue->Dispatch(
-      NewRunnableMethod<StoreCopyPassByPtr<AudioSegment>, TrackRate>(
-          "nsISpeechRecognitionService::ProcessAudioSegment",
-          mRecognitionService,
-          &nsISpeechRecognitionService::ProcessAudioSegment,
-          std::move(*aSegment), aTrackRate));
+  nsresult rv = mEncodeTaskQueue->Dispatch(NS_NewRunnableFunction(
+      "nsISpeechRecognitionService::ProcessAudioSegment",
+      [=, service = mRecognitionService,
+       segment = std::move(*aSegment)]() mutable {
+        service->ProcessAudioSegment(&segment, aTrackRate);
+      }));
+
   MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
   Unused << rv;
   return samples;

gfx/layers/apz/src/APZCTreeManager.cpp

@@ -394,8 +394,7 @@ void APZCTreeManager::SetAllowedTouchBehavior(
     uint64_t aInputBlockId, const nsTArray<TouchBehaviorFlags>& aValues) {
   if (!APZThreadUtils::IsControllerThread()) {
     APZThreadUtils::RunOnControllerThread(
-        NewRunnableMethod<uint64_t,
-                          StoreCopyPassByLRef<nsTArray<TouchBehaviorFlags>>>(
+        NewRunnableMethod<uint64_t, nsTArray<TouchBehaviorFlags>>(
             "layers::APZCTreeManager::SetAllowedTouchBehavior", this,
             &APZCTreeManager::SetAllowedTouchBehavior, aInputBlockId,
             aValues.Clone()));

netwerk/wifi/nsWifiMonitor.cpp

@@ -364,13 +364,13 @@ nsresult nsWifiMonitor::DoScan() {
   }
 
   return NS_DispatchToMainThread(
-      NewRunnableMethod<const nsTArray<RefPtr<nsIWifiAccessPoint>>&&, bool>(
+      NewRunnableMethod<nsTArray<RefPtr<nsIWifiAccessPoint>>, bool>(
           "CallWifiListeners", this, &nsWifiMonitor::CallWifiListeners,
           mLastAccessPoints.Clone(), accessPointsChanged));
 }
 
 nsresult nsWifiMonitor::CallWifiListeners(
-    nsTArray<RefPtr<nsIWifiAccessPoint>>&& aAccessPoints,
+    const nsTArray<RefPtr<nsIWifiAccessPoint>>& aAccessPoints,
     bool aAccessPointsChanged) {
   MOZ_ASSERT(NS_IsMainThread());
   LOG(("Sending wifi access points to the listeners"));

netwerk/wifi/nsWifiMonitor.h

@@ -69,7 +69,7 @@ class nsWifiMonitor final : public nsIWifiMonitor, public nsIObserver {
   nsresult DoScan();
 
   nsresult CallWifiListeners(
-      nsTArray<RefPtr<nsIWifiAccessPoint>>&& aAccessPoints,
+      const nsTArray<RefPtr<nsIWifiAccessPoint>>& aAccessPoints,
       bool aAccessPointsChanged);
 
   nsresult PassErrorToWifiListeners(nsresult rv);

xpcom/tests/gtest/TestThreadUtils.cpp

@@ -1277,15 +1277,9 @@ TEST(ThreadUtils, main)
 
   static_assert(!IsParameterStorageClass<int>::value,
                 "'int' should not be recognized as Storage Class");
-  static_assert(
-      IsParameterStorageClass<StoreCopyPassByValue<int>>::value,
-      "StoreCopyPassByValue<int> should be recognized as Storage Class");
   static_assert(
       IsParameterStorageClass<StoreCopyPassByConstLRef<int>>::value,
       "StoreCopyPassByConstLRef<int> should be recognized as Storage Class");
-  static_assert(
-      IsParameterStorageClass<StoreCopyPassByLRef<int>>::value,
-      "StoreCopyPassByLRef<int> should be recognized as Storage Class");
   static_assert(
       IsParameterStorageClass<StoreCopyPassByRRef<int>>::value,
       "StoreCopyPassByRRef<int> should be recognized as Storage Class");
@@ -1304,12 +1298,6 @@ TEST(ThreadUtils, main)
   static_assert(
       IsParameterStorageClass<StoreConstPtrPassByConstPtr<int>>::value,
       "StoreConstPtrPassByConstPtr<int> should be recognized as Storage Class");
-  static_assert(
-      IsParameterStorageClass<StoreCopyPassByConstPtr<int>>::value,
-      "StoreCopyPassByConstPtr<int> should be recognized as Storage Class");
-  static_assert(
-      IsParameterStorageClass<StoreCopyPassByPtr<int>>::value,
-      "StoreCopyPassByPtr<int> should be recognized as Storage Class");
 
   RefPtr<ThreadUtilsObject> rpt(new ThreadUtilsObject);
   int count = 0;
@@ -1348,11 +1336,6 @@ TEST(ThreadUtils, main)
                                StoreCopyPassByConstLRef<int>>,
                 "detail::ParameterStorage<int>::Type should be "
                 "StoreCopyPassByConstLRef<int>");
-  static_assert(std::is_same_v<
-                    ::detail::ParameterStorage<StoreCopyPassByValue<int>>::Type,
-                    StoreCopyPassByValue<int>>,
-                "detail::ParameterStorage<StoreCopyPassByValue<int>>::Type "
-                "should be StoreCopyPassByValue<int>");
 
   r1 = NewRunnableMethod<int>("TestThreadUtils::ThreadUtilsObject::Test1i", rpt,
                               &ThreadUtilsObject::Test1i, 12);
@@ -1469,37 +1452,6 @@ TEST(ThreadUtils, main)
     EXPECT_EQ(i, rpt->mA0);
   }
 
-  // Raw pointer to copy.
-  static_assert(std::is_same_v<StoreCopyPassByPtr<int>::stored_type, int>,
-                "StoreCopyPassByPtr<int>::stored_type should be int");
-  static_assert(std::is_same_v<StoreCopyPassByPtr<int>::passed_type, int*>,
-                "StoreCopyPassByPtr<int>::passed_type should be int*");
-  {
-    int i = 1202;
-    r1 = NewRunnableMethod<StoreCopyPassByPtr<int>>(
-        "TestThreadUtils::ThreadUtilsObject::Test1pi", rpt,
-        &ThreadUtilsObject::Test1pi, i);
-    r1->Run();
-    EXPECT_EQ(count += 2, rpt->mCount);
-    EXPECT_EQ(i, rpt->mA0);
-  }
-
-  // Raw pointer to const copy.
-  static_assert(std::is_same_v<StoreCopyPassByConstPtr<int>::stored_type, int>,
-                "StoreCopyPassByConstPtr<int>::stored_type should be int");
-  static_assert(
-      std::is_same_v<StoreCopyPassByConstPtr<int>::passed_type, const int*>,
-      "StoreCopyPassByConstPtr<int>::passed_type should be const int*");
-  {
-    int i = 1203;
-    r1 = NewRunnableMethod<StoreCopyPassByConstPtr<int>>(
-        "TestThreadUtils::ThreadUtilsObject::Test1pci", rpt,
-        &ThreadUtilsObject::Test1pci, i);
-    r1->Run();
-    EXPECT_EQ(count += 2, rpt->mCount);
-    EXPECT_EQ(i, rpt->mA0);
-  }
-
   // nsRefPtr to pointer.
   static_assert(
       std::is_same_v<::detail::ParameterStorage<
@@ -1724,126 +1676,6 @@ TEST(ThreadUtils, main)
 
   // Verify copy/move assumptions.
 
-  Spy::ClearAll();
-  if (gDebug) {
-    printf("%d - Test: Store copy from lvalue, pass by value\n", __LINE__);
-  }
-  {  // Block around nsCOMPtr lifetime.
-    nsCOMPtr<nsIRunnable> r2;
-    {  // Block around Spy lifetime.
-      if (gDebug) {
-        printf("%d - Spy s(10)\n", __LINE__);
-      }
-      Spy s(10);
-      EXPECT_EQ(1, gConstructions);
-      EXPECT_EQ(1, gAlive);
-      if (gDebug) {
-        printf(
-            "%d - r2 = "
-            "NewRunnableMethod<StoreCopyPassByValue<Spy>>(&TestByValue, s)\n",
-            __LINE__);
-      }
-      r2 = NewRunnableMethod<StoreCopyPassByValue<Spy>>(
-          "TestThreadUtils::ThreadUtilsObject::TestByValue", rpt,
-          &ThreadUtilsObject::TestByValue, s);
-      EXPECT_EQ(2, gAlive);
-      EXPECT_LE(1, gCopyConstructions);  // At least 1 copy-construction.
-      Spy::ClearActions();
-      if (gDebug) {
-        printf("%d - End block with Spy s(10)\n", __LINE__);
-      }
-    }
-    EXPECT_EQ(1, gDestructions);
-    EXPECT_EQ(1, gAlive);
-    Spy::ClearActions();
-    if (gDebug) {
-      printf("%d - Run()\n", __LINE__);
-    }
-    r2->Run();
-    EXPECT_LE(1, gCopyConstructions);  // Another copy-construction in call.
-    EXPECT_EQ(10, rpt->mSpy.mID);
-    EXPECT_LE(1, gDestructions);
-    EXPECT_EQ(1, gAlive);
-    Spy::ClearActions();
-    if (gDebug) {
-      printf("%d - End block with r\n", __LINE__);
-    }
-  }
-  if (gDebug) {
-    printf("%d - After end block with r\n", __LINE__);
-  }
-  EXPECT_EQ(1, gDestructions);
-  EXPECT_EQ(0, gAlive);
-
-  Spy::ClearAll();
-  if (gDebug) {
-    printf("%d - Test: Store copy from prvalue, pass by value\n", __LINE__);
-  }
-  {
-    if (gDebug) {
-      printf(
-          "%d - r3 = "
-          "NewRunnableMethod<StoreCopyPassByValue<Spy>>(&TestByValue, "
-          "Spy(11))\n",
-          __LINE__);
-    }
-    nsCOMPtr<nsIRunnable> r3 = NewRunnableMethod<StoreCopyPassByValue<Spy>>(
-        "TestThreadUtils::ThreadUtilsObject::TestByValue", rpt,
-        &ThreadUtilsObject::TestByValue, Spy(11));
-    EXPECT_EQ(1, gAlive);
-    EXPECT_EQ(1, gConstructions);
-    EXPECT_LE(1, gMoveConstructions);
-    Spy::ClearActions();
-    if (gDebug) {
-      printf("%d - Run()\n", __LINE__);
-    }
-    r3->Run();
-    EXPECT_LE(1, gCopyConstructions);  // Another copy-construction in call.
-    EXPECT_EQ(11, rpt->mSpy.mID);
-    EXPECT_LE(1, gDestructions);
-    EXPECT_EQ(1, gAlive);
-    Spy::ClearActions();
-    if (gDebug) {
-      printf("%d - End block with r\n", __LINE__);
-    }
-  }
-  if (gDebug) {
-    printf("%d - After end block with r\n", __LINE__);
-  }
-  EXPECT_EQ(1, gDestructions);
-  EXPECT_EQ(0, gAlive);
-
-  Spy::ClearAll();
-  {  // Store copy from xvalue, pass by value.
-    nsCOMPtr<nsIRunnable> r4;
-    {
-      Spy s(12);
-      EXPECT_EQ(1, gConstructions);
-      EXPECT_EQ(1, gAlive);
-      Spy::ClearActions();
-      r4 = NewRunnableMethod<StoreCopyPassByValue<Spy>>(
-          "TestThreadUtils::ThreadUtilsObject::TestByValue", rpt,
-          &ThreadUtilsObject::TestByValue, std::move(s));
-      EXPECT_LE(1, gMoveConstructions);
-      EXPECT_EQ(1, gAlive);
-      EXPECT_EQ(1, gZombies);
-      Spy::ClearActions();
-    }
-    EXPECT_EQ(1, gDestructions);
-    EXPECT_EQ(1, gAlive);
-    EXPECT_EQ(0, gZombies);
-    Spy::ClearActions();
-    r4->Run();
-    EXPECT_LE(1, gCopyConstructions);  // Another copy-construction in call.
-    EXPECT_EQ(12, rpt->mSpy.mID);
-    EXPECT_LE(1, gDestructions);
-    EXPECT_EQ(1, gAlive);
-    Spy::ClearActions();
-  }
-  EXPECT_EQ(1, gDestructions);
-  EXPECT_EQ(0, gAlive);
-  // Won't test xvalues anymore, prvalues are enough to verify all rvalues.
-
   Spy::ClearAll();
   if (gDebug) {
     printf("%d - Test: Store copy from lvalue, pass by const lvalue ref\n",

xpcom/threads/nsThreadUtils.h

@@ -863,19 +863,6 @@ struct IsParameterStorageClass : public std::false_type {};
 // StoreXPassByY structs used to inform nsRunnableMethodArguments how to
 // store arguments, and how to pass them to the target method.
 
-template <typename T>
-struct StoreCopyPassByValue {
-  using stored_type = std::decay_t<T>;
-  typedef stored_type passed_type;
-  stored_type m;
-  template <typename A>
-  MOZ_IMPLICIT StoreCopyPassByValue(A&& a) : m(std::forward<A>(a)) {}
-  passed_type PassAsParameter() { return m; }
-};
-template <typename S>
-struct IsParameterStorageClass<StoreCopyPassByValue<S>>
-    : public std::true_type {};
-
 template <typename T>
 struct StoreCopyPassByConstLRef {
   using stored_type = std::decay_t<T>;
@@ -889,19 +876,6 @@ template <typename S>
 struct IsParameterStorageClass<StoreCopyPassByConstLRef<S>>
     : public std::true_type {};
 
-template <typename T>
-struct StoreCopyPassByLRef {
-  using stored_type = std::decay_t<T>;
-  typedef stored_type& passed_type;
-  stored_type m;
-  template <typename A>
-  MOZ_IMPLICIT StoreCopyPassByLRef(A&& a) : m(std::forward<A>(a)) {}
-  passed_type PassAsParameter() { return m; }
-};
-template <typename S>
-struct IsParameterStorageClass<StoreCopyPassByLRef<S>> : public std::true_type {
-};
-
 template <typename T>
 struct StoreCopyPassByRRef {
   using stored_type = std::decay_t<T>;
@@ -979,32 +953,6 @@ template <typename S>
 struct IsParameterStorageClass<StoreConstPtrPassByConstPtr<S>>
     : public std::true_type {};
 
-template <typename T>
-struct StoreCopyPassByConstPtr {
-  typedef T stored_type;
-  typedef const T* passed_type;
-  stored_type m;
-  template <typename A>
-  MOZ_IMPLICIT StoreCopyPassByConstPtr(A&& a) : m(std::forward<A>(a)) {}
-  passed_type PassAsParameter() { return &m; }
-};
-template <typename S>
-struct IsParameterStorageClass<StoreCopyPassByConstPtr<S>>
-    : public std::true_type {};
-
-template <typename T>
-struct StoreCopyPassByPtr {
-  typedef T stored_type;
-  typedef T* passed_type;
-  stored_type m;
-  template <typename A>
-  MOZ_IMPLICIT StoreCopyPassByPtr(A&& a) : m(std::forward<A>(a)) {}
-  passed_type PassAsParameter() { return &m; }
-};
-template <typename S>
-struct IsParameterStorageClass<StoreCopyPassByPtr<S>> : public std::true_type {
-};
-
 namespace detail {
 
 template <typename>
@@ -1033,13 +981,9 @@ constexpr static bool HasRefCountMethods =
 // - RefPtr<T>, nsCOMPtr<T>
 //             -> StoreRefPtrPassByPtr<T>        :Store RefPtr<T>, pass T*
 // - Other T   -> StoreCopyPassByConstLRef<T>    :Store T, pass const T&.
-// Other available explicit options:
-// -              StoreCopyPassByValue<T>        :Store T, pass T.
-// -              StoreCopyPassByLRef<T>         :Store T, pass T& (of copy!)
-// -              StoreCopyPassByConstPtr<T>     :Store T, pass const T*
-// -              StoreCopyPassByPtr<T>          :Store T, pass T* (of copy!)
-// Or create your own class with PassAsParameter() method, optional
-// clean-up in destructor, and with associated IsParameterStorageClass<>.
+//
+// For anything less common, please use a lambda function rather than devising
+// new parameter-storage classes. (In fact, consider doing that anyway.)
 
 template <typename T>
 struct OtherParameterStorage;
@@ -1082,6 +1026,12 @@ struct OtherParameterStorage<T&&> {
   using Type = StoreCopyPassByRRef<T>;
 };
 
+template <typename T>
+struct OtherParameterStorage<const T&&> {
+  // This is good advice regardless of the types you're handling.
+  static_assert(!SFINAE1True<T>::value, "please use a lambda function");
+};
+
 // default impl.
 template <typename T>
 struct OtherParameterStorage {