Monitor VideoResourceUpdater reusing destructed resource in Debug mode.

cc/resources/video_resource_updater.cc

Index: cc/resources/video_resource_updater.cc
diff --git a/cc/resources/video_resource_updater.cc b/cc/resources/video_resource_updater.cc
index 6f8bb4bc6e5ba1e400f01f2b8a89c02f90e67d5c..21e7de139a1dd18f3d4b98da3bbef5edccf16d5b 100644
--- a/cc/resources/video_resource_updater.cc
+++ b/cc/resources/video_resource_updater.cc
@@ -11,6 +11,8 @@
 
 #include "base/bind.h"
 #include "base/bit_cast.h"
+#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/base/math_util.h"
 #include "cc/output/gl_renderer.h"
@@ -112,6 +114,12 @@ class SyncTokenClientImpl : public media::VideoFrame::SyncTokenClient {
   gpu::SyncToken sync_token_;
 };
 
+void OnVideoFrameDestruct(
+    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+    const base::Closure& task) {
+  task_runner->PostTask(FROM_HERE, task);

[miu, 2016/02/19 02:47:46]

This will introduce an unknown time delay before the VideoFrame is invalidated. 
It'd be rare, but not impossible for a new VideoFrame to be created with the
same pointer in the meantime...and even harder bug to catch!  ;-)

Instead of this approach, how about this idea:

1. Add a new VideoFrameMetadata field called RESOURCE_UPDATER_FLAG.

2. When a VideoFrame is passed in a call to SetPlaneResourceUniqueId(), set the
VideoFrame's RESOURCE_UPDATER_FLAG metadata to PlaneResource::resource_id.

3. When a VideoFrame is passed in a call to PlaneResourceMatchesUniqueID(), it
should also check that the RESOURCE_UPDATER_FLAG field is set to the same value
as PlaneResource::resource_id.  (and add a comment about why the
pointer+timestamp sometimes won't work as a unique identifier).

That way, if a VideoFrame is destroyed and re-created with the same pointer, AND
the client always sets the timestamp to zero; it's still possible for
VideoResourceUpdater to determine that this is a new VideoFrame (because it's
FLAG value will be missing).

[xjz, 2016/02/20 00:40:47]

Done. Since each VideoFrame can have three resource_ids, instead of setting
VideoFrame's RESOURCE_UPDATER_FLAG metadata to PlaneResource::resource_id,
setting it to true in SetPlaneResourceUniqueId().

+}
+
 }  // namespace
 
 VideoResourceUpdater::PlaneResource::PlaneResource(
@@ -353,6 +361,12 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
       resource_it =
           AllocateResource(output_plane_resource_size, output_resource_format,
                            !software_compositor, is_immutable);
+      if (resource_it != all_resources_.end()) {
+        video_frame->AddDestructionObserver(base::Bind(
+            &OnVideoFrameDestruct, base::ThreadTaskRunnerHandle::Get(),
+            base::Bind(&VideoResourceUpdater::InvalidateResource, AsWeakPtr(),
+                       resource_it->resource_id)));
+      }
     }
     if (resource_it == all_resources_.end())
       break;
@@ -685,4 +699,24 @@ void VideoResourceUpdater::RecycleResource(
   DCHECK_GE(resource_it->ref_count, 0);
 }
 
+// static
+void VideoResourceUpdater::InvalidateResource(
+    base::WeakPtr<VideoResourceUpdater> updater,
+    ResourceId resource_id) {
+  if (!updater.get()) {
+    return;
+  }
+
+  const ResourceList::iterator resource_it = std::find_if(
+      updater->all_resources_.begin(), updater->all_resources_.end(),
+      [resource_id](const PlaneResource& plane_resource) {
+        return plane_resource.resource_id == resource_id;
+      });
+  if (resource_it == updater->all_resources_.end())
+    return;
+
+  resource_it->ref_count = 0;
+  updater->DeleteResource(resource_it);
+}
+
 }  // namespace cc