From d6b4819e4a161ebf1e723c13e972d1564ed71348 Mon Sep 17 00:00:00 2001
From: Karl Wiberg <kwiberg@webrtc.org>
Date: Mon, 16 Oct 2017 23:01:06 +0200
Subject: [PATCH] PeerConnection::StartRtcEventLog: Improve callback memory
 safety

By having a unique_ptr own the callback data instead of a raw pointer,
the compiler helps us ensure that it's destroyed exactly once,
and never used after being destroyed.

(This made the callback object move-only, so I had to add support
for move-only callbacks to rtc::Thread::Invoke().)

BUG=webrtc:8111

Change-Id: Ia0804e4662e63e91e5cee18ecc3f38d2cfe8a26b
Reviewed-on: https://webrtc-review.googlesource.com/10812
Commit-Queue: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20317}
---
 pc/peerconnection.cc      | 15 +++++++--------
 rtc_base/messagehandler.h |  4 ++--
 rtc_base/thread.h         |  6 ++++--
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index daf631f94b..a78c29045f 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -1317,15 +1317,14 @@ bool PeerConnection::StartRtcEventLog(rtc::PlatformFile file,
 
 bool PeerConnection::StartRtcEventLog(
     std::unique_ptr<RtcEventLogOutput> output) {
-  // This code assumes that the functor would be executed. Otherwise, this
-  // could leak.
-  // TODO(eladalon): When we switch to C++14 (or later), we can get rid of this
-  // conversion to a raw pointer, passing the unique_ptr directly to the lambda.
-  RtcEventLogOutput* output_raw = output.release();
-  auto functor = [this, output_raw]() {
-    return StartRtcEventLog_w(rtc::WrapUnique<RtcEventLogOutput>(output_raw));
+  // TODO(eladalon): In C++14, this can be done with a lambda.
+  struct Functor {
+    bool operator()() { return pc->StartRtcEventLog_w(std::move(output)); }
+    PeerConnection* const pc;
+    std::unique_ptr<RtcEventLogOutput> output;
   };
-  return worker_thread()->Invoke<bool>(RTC_FROM_HERE, functor);
+  return worker_thread()->Invoke<bool>(RTC_FROM_HERE,
+                                       Functor{this, std::move(output)});
 }
 
 void PeerConnection::StopRtcEventLog() {
diff --git a/rtc_base/messagehandler.h b/rtc_base/messagehandler.h
index 9194c1dc6e..ff953f7898 100644
--- a/rtc_base/messagehandler.h
+++ b/rtc_base/messagehandler.h
@@ -38,8 +38,8 @@ class MessageHandler {
 template <class ReturnT, class FunctorT>
 class FunctorMessageHandler : public MessageHandler {
  public:
-  explicit FunctorMessageHandler(const FunctorT& functor)
-      : functor_(functor) {}
+  explicit FunctorMessageHandler(FunctorT&& functor)
+      : functor_(std::forward<FunctorT>(functor)) {}
   virtual void OnMessage(Message* msg) {
     result_ = functor_();
   }
diff --git a/rtc_base/thread.h b/rtc_base/thread.h
index b4ce7d7542..bd502ebaf2 100644
--- a/rtc_base/thread.h
+++ b/rtc_base/thread.h
@@ -15,6 +15,7 @@
 #include <list>
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #if defined(WEBRTC_POSIX)
@@ -175,8 +176,9 @@ class RTC_LOCKABLE Thread : public MessageQueue {
   // NOTE: This function can only be called when synchronous calls are allowed.
   // See ScopedDisallowBlockingCalls for details.
   template <class ReturnT, class FunctorT>
-  ReturnT Invoke(const Location& posted_from, const FunctorT& functor) {
-    FunctorMessageHandler<ReturnT, FunctorT> handler(functor);
+  ReturnT Invoke(const Location& posted_from, FunctorT&& functor) {
+    FunctorMessageHandler<ReturnT, FunctorT> handler(
+        std::forward<FunctorT>(functor));
     InvokeInternal(posted_from, &handler);
     return handler.MoveResult();
   }