Bug 1287006 - Adjust media/ code to not pass Maybe (or any class containing a Maybe member) by value, only by reference or pointer. r=jw_wang, r=rjesup

--HG--
extra : rebase_source : 176a2afde5772bbdf43f0f6dc8082201acadcf4f

dom/media/MediaDecoderStateMachine.cpp

@@ -21,6 +21,7 @@
 #include "mediasink/OutputStreamManager.h"
 #include "mediasink/VideoSink.h"
 #include "mozilla/DebugOnly.h"
+#include "mozilla/IndexSequence.h"
 #include "mozilla/Logging.h"
 #include "mozilla/mozalloc.h"
 #include "mozilla/MathAlgorithms.h"
@@ -29,6 +30,7 @@
 #include "mozilla/SizePrintfMacros.h"
 #include "mozilla/Sprintf.h"
 #include "mozilla/TaskQueue.h"
+#include "mozilla/Tuple.h"
 
 #include "nsComponentManagerUtils.h"
 #include "nsContentUtils.h"
@@ -284,17 +286,35 @@ protected:
   MediaQueue<MediaData>& AudioQueue() const { return mMaster->mAudioQueue; }
   MediaQueue<MediaData>& VideoQueue() const { return mMaster->mVideoQueue; }
 
+  template <class S, typename... Args, size_t... Indexes>
+  auto
+  CallEnterMemberFunction(S* aS,
+                          Tuple<Args...>& aTuple,
+                          IndexSequence<Indexes...>)
+    -> decltype(ReturnTypeHelper(&S::Enter))
+  {
+    return aS->Enter(Move(Get<Indexes>(aTuple))...);
+  }
+
   // Note this function will delete the current state object.
   // Don't access members to avoid UAF after this call.
   template <class S, typename... Ts>
-  auto SetState(Ts... aArgs)
+  auto SetState(Ts&&... aArgs)
     -> decltype(ReturnTypeHelper(&S::Enter))
   {
+    // |aArgs| must be passed by reference to avoid passing MOZ_NON_PARAM class
+    // SeekJob by value.  See bug 1287006 and bug 1338374.  But we still *must*
+    // copy the parameters, because |Exit()| can modify them.  See bug 1312321.
+    // So we 1) pass the parameters by reference, but then 2) immediately copy
+    // them into a Tuple to be safe against modification, and finally 3) move
+    // the elements of the Tuple into the final function call.
+    auto copiedArgs = MakeTuple(Forward<Ts>(aArgs)...);
+
     // keep mMaster in a local object because mMaster will become invalid after
     // the current state object is deleted.
     auto master = mMaster;
 
-    auto s = new S(master);
+    auto* s = new S(master);
 
     MOZ_ASSERT(GetState() != s->GetState()
                || GetState() == DECODER_STATE_SEEKING);
@@ -304,7 +324,8 @@ protected:
     Exit();
 
     master->mStateObj.reset(s);
-    return s->Enter(Move(aArgs)...);
+    return CallEnterMemberFunction(s, copiedArgs,
+                                   typename IndexSequenceFor<Ts...>::Type());
   }
 
   RefPtr<MediaDecoder::SeekPromise>
@@ -904,7 +925,7 @@ class MediaDecoderStateMachine::SeekingState
 public:
   explicit SeekingState(Master* aPtr) : StateObject(aPtr) { }
 
-  RefPtr<MediaDecoder::SeekPromise> Enter(SeekJob aSeekJob,
+  RefPtr<MediaDecoder::SeekPromise> Enter(SeekJob&& aSeekJob,
                                           EventVisibility aVisibility)
   {
     mSeekJob = Move(aSeekJob);
@@ -982,7 +1003,7 @@ public:
   {
   }
 
-  RefPtr<MediaDecoder::SeekPromise> Enter(SeekJob aSeekJob,
+  RefPtr<MediaDecoder::SeekPromise> Enter(SeekJob&& aSeekJob,
                                           EventVisibility aVisibility)
   {
     MOZ_ASSERT(aSeekJob.mTarget->IsAccurate() || aSeekJob.mTarget->IsFast());
@@ -1441,7 +1462,7 @@ public:
   {
   }
 
-  RefPtr<MediaDecoder::SeekPromise> Enter(SeekJob aSeekJob,
+  RefPtr<MediaDecoder::SeekPromise> Enter(SeekJob&& aSeekJob,
                                           EventVisibility aVisibility)
   {
     MOZ_ASSERT(aSeekJob.mTarget->IsNextFrame());

dom/media/gmp/GMPVideoEncoderParent.cpp

@@ -257,10 +257,9 @@ GMPVideoEncoderParent::ActorDestroy(ActorDestroyReason aWhy)
   // Must be shut down before VideoEncoderDestroyed(), since this can recurse
   // the GMPThread event loop.  See bug 1049501
   if (mEncodedThread) {
-    // Can't get it to allow me to use WrapRunnable with a nsCOMPtr<nsIThread>()
     NS_DispatchToMainThread(
-      WrapRunnableNM<decltype(&ShutdownEncodedThread),
-                     nsCOMPtr<nsIThread> >(&ShutdownEncodedThread, mEncodedThread));
+      WrapRunnableNM(&ShutdownEncodedThread, nsCOMPtr<nsIThread>(mEncodedThread))
+    );
     mEncodedThread = nullptr;
   }
   if (mPlugin) {

dom/media/gtest/TestMozPromise.cpp

@@ -39,7 +39,7 @@ class DelayedResolveOrReject : public Runnable
 public:
   DelayedResolveOrReject(TaskQueue* aTaskQueue,
                          TestPromise::Private* aPromise,
-                         TestPromise::ResolveOrRejectValue aValue,
+                         const TestPromise::ResolveOrRejectValue& aValue,
                          int aIterations)
   : mTaskQueue(aTaskQueue)
   , mPromise(aPromise)

dom/media/mediasource/SourceBuffer.cpp

@@ -425,7 +425,7 @@ SourceBuffer::AppendData(const uint8_t* aData, uint32_t aLength, ErrorResult& aR
 }
 
 void
-SourceBuffer::AppendDataCompletedWithSuccess(SourceBufferTask::AppendBufferResult aResult)
+SourceBuffer::AppendDataCompletedWithSuccess(const SourceBufferTask::AppendBufferResult& aResult)
 {
   MOZ_ASSERT(mUpdating);
   mPendingAppend.Complete();

dom/media/mediasource/SourceBuffer.h

@@ -164,7 +164,7 @@ private:
                                                   uint32_t aLength,
                                                   ErrorResult& aRv);
 
-  void AppendDataCompletedWithSuccess(SourceBufferTask::AppendBufferResult aResult);
+  void AppendDataCompletedWithSuccess(const SourceBufferTask::AppendBufferResult& aResult);
   void AppendDataErrored(const MediaResult& aError);
 
   RefPtr<MediaSource> mMediaSource;

dom/media/webspeech/synth/SpeechSynthesisUtterance.cpp

@@ -158,7 +158,7 @@ SpeechSynthesisUtterance::GetChosenVoiceURI(nsString& aResult) const
 void
 SpeechSynthesisUtterance::DispatchSpeechSynthesisEvent(const nsAString& aEventType,
                                                        uint32_t aCharIndex,
-                                                       Nullable<uint32_t> aCharLength,
+                                                       const Nullable<uint32_t>& aCharLength,
                                                        float aElapsedTime,
                                                        const nsAString& aName)
 {

dom/media/webspeech/synth/SpeechSynthesisUtterance.h

@@ -97,7 +97,7 @@ private:
 
   void DispatchSpeechSynthesisEvent(const nsAString& aEventType,
                                     uint32_t aCharIndex,
-                                    Nullable<uint32_t> aCharLength,
+                                    const Nullable<uint32_t>& aCharLength,
                                     float aElapsedTime, const nsAString& aName);
 
   nsString mText;

media/mtransport/runnable_utils.h

@@ -110,8 +110,9 @@ class runnable_args_func : public detail::runnable_args_base<detail::NoResult>
 {
 public:
   // |explicit| to pacify static analysis when there are no |args|.
-  explicit runnable_args_func(FunType f, Args&&... args)
-    : mFunc(f), mArgs(Forward<Args>(args)...)
+  template<typename... Arguments>
+  explicit runnable_args_func(FunType f, Arguments&&... args)
+    : mFunc(f), mArgs(Forward<Arguments>(args)...)
   {}
 
   NS_IMETHOD Run() {
@@ -125,10 +126,10 @@ private:
 };
 
 template<typename FunType, typename... Args>
-runnable_args_func<FunType, Args...>*
-WrapRunnableNM(FunType f, Args... args)
+runnable_args_func<FunType, typename mozilla::Decay<Args>::Type...>*
+WrapRunnableNM(FunType f, Args&&... args)
 {
-  return new runnable_args_func<FunType, Args...>(f, Move(args)...);
+  return new runnable_args_func<FunType, typename mozilla::Decay<Args>::Type...>(f, Forward<Args>(args)...);
 }
 
 template<typename Ret, typename FunType, typename... Args>

media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp

@@ -446,7 +446,7 @@ JsepSessionImpl::SetupOfferMSections(const JsepOfferOptions& options, Sdp* sdp)
 
 nsresult
 JsepSessionImpl::SetupOfferMSectionsByType(SdpMediaSection::MediaType mediatype,
-                                           Maybe<size_t> offerToReceiveMaybe,
+                                           const Maybe<size_t>& offerToReceiveMaybe,
                                            Sdp* sdp)
 {
   // Convert the Maybe into a size_t*, since that is more readable, especially

media/webrtc/signaling/src/jsep/JsepSessionImpl.h

@@ -244,7 +244,7 @@ private:
   nsresult SetupOfferMSections(const JsepOfferOptions& options, Sdp* sdp);
   // Non-const so it can assign m-line index to tracks
   nsresult SetupOfferMSectionsByType(SdpMediaSection::MediaType type,
-                                     Maybe<size_t> offerToReceive,
+                                     const Maybe<size_t>& offerToReceive,
                                      Sdp* sdp);
   nsresult BindLocalTracks(SdpMediaSection::MediaType mediatype,
                            Sdp* sdp);

media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp

@@ -93,8 +93,8 @@ public:
     if (mThread != nullptr) {
       MonitorAutoUnlock unlock(mMonitor);
       NS_DispatchToMainThread(
-        WrapRunnableNM<decltype(&ShutdownThread),
-                       nsCOMPtr<nsIThread> >(&ShutdownThread, mThread));
+        WrapRunnableNM(&ShutdownThread, nsCOMPtr<nsIThread>(mThread))
+      );
       mThread = nullptr;
     }
   }

mfbt/IndexSequence.h

@@ -40,7 +40,7 @@
  *   containing indices corresponding to the tuple indices.
  *
  *     template <typename... Args, size_t... Indices>
- *     void Helper(const Tuple<Args...>& t, IndexSequence<Indices>)
+ *     void Helper(const Tuple<Args...>& t, IndexSequence<Indices...>)
  *     {
  *       Foo(Get<Indices>(t)...);
  *     }