DevTools: fix concurrency problems in DevToolsFrameTraceRecorder

content/browser/devtools/devtools_frame_trace_recorder.cc

Index: content/browser/devtools/devtools_frame_trace_recorder.cc
diff --git a/content/browser/devtools/devtools_frame_trace_recorder.cc b/content/browser/devtools/devtools_frame_trace_recorder.cc
index 3821b85b1d26cde1e60948701f7e619e7565601b..2533e2c4afee3ac0129800fcb5e8bb96fd339dc4 100644
--- a/content/browser/devtools/devtools_frame_trace_recorder.cc
+++ b/content/browser/devtools/devtools_frame_trace_recorder.cc
@@ -29,29 +29,63 @@ static base::subtle::Atomic32 frame_data_count = 0;
 static int kMaximumFrameDataCount = 150;
 static size_t kFrameAreaLimit = 256000;
 
-}  // namespace
-
-class DevToolsFrameTraceRecorderData
+class TraceableDevToolsScreenshot
     : public base::trace_event::ConvertableToTraceFormat {
  public:
-  DevToolsFrameTraceRecorderData(const cc::CompositorFrameMetadata& metadata)
-      : metadata_(metadata),
-        weak_factory_(this) {
+  TraceableDevToolsScreenshot(const SkBitmap& bitmap) : frame_(bitmap) {}
+
+  void AppendAsTraceFormat(std::string* out) const override {
+    out->append("\"");
+    if (!frame_.drawsNothing()) {
+      std::vector<unsigned char> data;
+      SkAutoLockPixels lock_image(frame_);
+      bool encoded = gfx::PNGCodec::Encode(
+          reinterpret_cast<unsigned char*>(frame_.getAddr32(0, 0)),
+          gfx::PNGCodec::FORMAT_SkBitmap,
+          gfx::Size(frame_.width(), frame_.height()),
+          frame_.width() * frame_.bytesPerPixel(), false,
+          std::vector<gfx::PNGCodec::Comment>(), &data);
+      if (encoded) {
+        std::string encoded_data;
+        base::Base64Encode(
+            base::StringPiece(reinterpret_cast<char*>(&data[0]), data.size()),
+            &encoded_data);
+        out->append(encoded_data);
+      }
+    }
+    out->append("\"");
   }
 
-  base::WeakPtr<DevToolsFrameTraceRecorderData> GetWeakPtr() {
-    return weak_factory_.GetWeakPtr();
+ private:
+  ~TraceableDevToolsScreenshot() override {
+    base::subtle::NoBarrier_AtomicIncrement(&frame_data_count, -1);
   }
 
+  SkBitmap frame_;
+};
+
+}  // namespace
+
+class DevToolsFrameTraceRecorderData
+    : public base::RefCounted<DevToolsFrameTraceRecorderData> {
+ public:
+  DevToolsFrameTraceRecorderData(const cc::CompositorFrameMetadata& metadata)
+      : metadata_(metadata), timestamp_(base::TraceTicks::Now()) {}
+
   void FrameCaptured(const SkBitmap& bitmap, ReadbackResponse response) {
     if (response != READBACK_SUCCESS)
       return;
     int current_frame_count = base::subtle::NoBarrier_Load(&frame_data_count);
     if (current_frame_count >= kMaximumFrameDataCount)
       return;
-    frame_ = bitmap;
-    if (!frame_.drawsNothing())
-      base::subtle::NoBarrier_AtomicIncrement(&frame_data_count, 1);
+    if (bitmap.drawsNothing())
+      return;
+    base::subtle::NoBarrier_AtomicIncrement(&frame_data_count, 1);
+    TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID_AND_TIMESTAMP(
+        TRACE_DISABLED_BY_DEFAULT("devtools.screenshot"), "Screenshot", 1,

[dgozman, 2015/06/16 15:19:28]

What are these "Screenshot" and 1?

+        timestamp_.ToInternalValue(),
+        scoped_refptr<base::trace_event::ConvertableToTraceFormat>(
+            new TraceableDevToolsScreenshot(bitmap)));
   }
 
   void CaptureFrame(RenderFrameHostImpl* host) {
@@ -68,44 +102,18 @@ class DevToolsFrameTraceRecorderData
       scale = sqrt(kFrameAreaLimit / area);
     gfx::Size snapshot_size(gfx::ToRoundedSize(gfx::ScaleSize(
         metadata_.scrollable_viewport_size, scale)));
-    view->CopyFromCompositingSurface(gfx::Rect(), snapshot_size,
-        base::Bind(
-            &DevToolsFrameTraceRecorderData::FrameCaptured,
-            GetWeakPtr()),
+    view->CopyFromCompositingSurface(
+        gfx::Rect(), snapshot_size,
+        base::Bind(&DevToolsFrameTraceRecorderData::FrameCaptured, this),
         kN32_SkColorType);
   }
 
-  void AppendAsTraceFormat(std::string* out) const override {
-    out->append("\"");
-    if (!frame_.drawsNothing()) {
-      std::vector<unsigned char> data;
-      SkAutoLockPixels lock_image(frame_);
-      bool encoded = gfx::PNGCodec::Encode(
-          reinterpret_cast<unsigned char*>(frame_.getAddr32(0, 0)),
-          gfx::PNGCodec::FORMAT_SkBitmap,
-          gfx::Size(frame_.width(), frame_.height()),
-          frame_.width() * frame_.bytesPerPixel(),
-          false, std::vector<gfx::PNGCodec::Comment>(), &data);
-      if (encoded) {
-        std::string encoded_data;
-        base::Base64Encode(
-            base::StringPiece(reinterpret_cast<char*>(&data[0]), data.size()),
-            &encoded_data);
-        out->append(encoded_data);
-      }
-    }
-    out->append("\"");
-  }
-
  private:
-  ~DevToolsFrameTraceRecorderData() override {
-    if (!frame_.drawsNothing())
-      base::subtle::NoBarrier_AtomicIncrement(&frame_data_count, -1);
-  }
+  friend class base::RefCounted<DevToolsFrameTraceRecorderData>;
+  ~DevToolsFrameTraceRecorderData() {}
 
   cc::CompositorFrameMetadata metadata_;
-  SkBitmap frame_;
-  base::WeakPtrFactory<DevToolsFrameTraceRecorderData> weak_factory_;
+  base::TraceTicks timestamp_;
 
   DISALLOW_COPY_AND_ASSIGN(DevToolsFrameTraceRecorderData);
 };
@@ -126,18 +134,9 @@ void DevToolsFrameTraceRecorder::OnSwapCompositorFrame(
   if (!enabled)
     return;
 
-  if (last_event_data_.get())
-    last_event_data_->CaptureFrame(host);
-
-  scoped_refptr<DevToolsFrameTraceRecorderData> data(
-      new DevToolsFrameTraceRecorderData(frame_metadata));
-  last_event_data_ = data->GetWeakPtr();
-  TRACE_EVENT_INSTANT1(
-      TRACE_DISABLED_BY_DEFAULT("devtools.screenshot"),
-      "CaptureFrame",
-      TRACE_EVENT_SCOPE_THREAD,
-      "data",
-      scoped_refptr<base::trace_event::ConvertableToTraceFormat>(data));
+  if (pending_frame_data_.get())
+    pending_frame_data_->CaptureFrame(host);
+  pending_frame_data_ = new DevToolsFrameTraceRecorderData(frame_metadata);

[dgozman, 2015/06/16 15:19:28]

Let's delete this object once category or tracing is disabled.

 }
 
 }  // namespace content