cc: Ensure members needed on task completion get called on valid thread.

cc/tiles/tile_manager.cc

Index: cc/tiles/tile_manager.cc
diff --git a/cc/tiles/tile_manager.cc b/cc/tiles/tile_manager.cc
index 743ef85ca4fef1c29668a56a3d42f98d5c810e5a..af016ffbc5eb33194136a2d53100652c7e49b062 100644
--- a/cc/tiles/tile_manager.cc
+++ b/cc/tiles/tile_manager.cc
@@ -18,6 +18,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/threading/platform_thread.h"
 #include "base/trace_event/trace_event_argument.h"
 #include "cc/base/histograms.h"
 #include "cc/debug/devtools_instrumentation.h"
@@ -45,8 +46,14 @@ DEFINE_SCOPED_UMA_HISTOGRAM_AREA_TIMER(
 // OnTaskCompleted() in RasterTaskImpl. crbug.com/613812.

[prashant.n, 2016/05/24 15:46:09]

I'll remove this comment.

 class RasterTaskImpl : public TileTask {
  public:
-  RasterTaskImpl(TileManager* tile_manager,
-                 Resource* resource,
+  struct CompletionData {

[reveman, 2016/05/25 04:19:44]

not sure moving it to a struct makes much difference. we're still relying on it
being used properly based on comments in the code. just comments and no struct
is sufficient to me.

[prashant.n, 2016/05/25 04:25:06]

Hmm. So we can have another approach mentioned in below comment which will have
proper isolation of members.

+    TileManager* tile_manager;
+    Resource* resource;
+    Tile* tile;
+  };

[prashant.n, 2016/05/25 03:48:04]

Another approach - 

1. Have empty CompletionData struct in tile_task.h -
2. Have derived struct in TileManager class.
3. RasterTaskImpl(CompletionData* cd, ...)

4. OnTaskCompleted() {
  OnRasterTaskCompleted(cd)
}

5. In TileManager have non-class member function or static function
OnRasterTaskCompleted and typecast CompletionData to proper struct

+
+  RasterTaskImpl(const CompletionData& completion_data,
+                 base::PlatformThreadId origin_thread_id,
                  scoped_refptr<RasterSource> raster_source,
                  const gfx::Rect& content_rect,
                  const gfx::Rect& invalid_content_rect,
@@ -55,7 +62,7 @@ class RasterTaskImpl : public TileTask {
                  TileResolution tile_resolution,
                  int layer_id,
                  uint64_t source_prepare_tiles_id,
-                 Tile* tile,
+                 void* tile_ptr,
                  uint64_t new_content_id,
                  uint64_t previous_content_id,
                  uint64_t resource_content_id,
@@ -64,8 +71,8 @@ class RasterTaskImpl : public TileTask {
                  TileTask::Vector* dependencies,
                  bool supports_concurrent_execution)
       : TileTask(supports_concurrent_execution, dependencies),
-        tile_manager_(tile_manager),
-        resource_(resource),
+        completion_data_(completion_data),
+        origin_thread_id_(origin_thread_id),
         raster_source_(std::move(raster_source)),
         content_rect_(content_rect),
         invalid_content_rect_(invalid_content_rect),
@@ -74,7 +81,7 @@ class RasterTaskImpl : public TileTask {
         tile_resolution_(tile_resolution),
         layer_id_(layer_id),
         source_prepare_tiles_id_(source_prepare_tiles_id),
-        tile_(tile),
+        tile_ptr_(tile_ptr),
         new_content_id_(new_content_id),
         previous_content_id_(previous_content_id),
         resource_content_id_(resource_content_id),
@@ -90,7 +97,7 @@ class RasterTaskImpl : public TileTask {
     DCHECK(raster_buffer_);
 
     frame_viewer_instrumentation::ScopedRasterTask raster_task(
-        tile_, tile_resolution_, source_frame_number_, layer_id_);
+        tile_ptr_, tile_resolution_, source_frame_number_, layer_id_);
     ScopedRasterTaskTimer timer;
     timer.SetArea(content_rect_.size().GetArea());
 
@@ -103,16 +110,31 @@ class RasterTaskImpl : public TileTask {
 
   // Overridden from TileTask:
   void OnTaskCompleted() override {
-    tile_manager_->OnRasterTaskCompleted(std::move(raster_buffer_), tile_,
-                                         resource_, state().IsCanceled());
+    CompletionData& data = GetCompletionData();
+    data.tile_manager->OnRasterTaskCompleted(std::move(raster_buffer_),
+                                             data.tile, data.resource,
+                                             state().IsCanceled());
   }
 
  protected:
   ~RasterTaskImpl() override { DCHECK(!raster_buffer_); }
 
  private:
-  TileManager* tile_manager_;
-  Resource* resource_;
+  bool IsRunningOnOriginThread() {

[reveman, 2016/05/25 04:19:44]

hm, can we use base::ThreadChecker instead?

+    return origin_thread_id_ == base::PlatformThread::CurrentId();
+  }
+
+  CompletionData& GetCompletionData() {
+    DCHECK(IsRunningOnOriginThread());
+    return completion_data_;
+  }
+
+  // This member is accessed only in origin thread and do not access it
+  // directly. Use accessor method GetCompletionData() instead.
+  CompletionData completion_data_;
+
+  // Members accessed on worker or origin thread.
+  base::PlatformThreadId origin_thread_id_;
   scoped_refptr<RasterSource> raster_source_;
   gfx::Rect content_rect_;
   gfx::Rect invalid_content_rect_;
@@ -121,7 +143,7 @@ class RasterTaskImpl : public TileTask {
   TileResolution tile_resolution_;
   int layer_id_;
   uint64_t source_prepare_tiles_id_;
-  Tile* tile_;
+  void* tile_ptr_;

[reveman, 2016/05/25 04:19:44]

what is this tile_ptr used for? logging? do we have some tile id we can use
instead or just create one from the pointer value to make it more clear that
this should not be used for anything but an Id.

[prashant.n, 2016/05/25 04:25:06]

Yes for logging it is used (frame_viewer_instrumentation::ScopedRasterTask).
I'll check on this if tile_id is sufficient for ScopedRasterTask.

   uint64_t new_content_id_;
   uint64_t previous_content_id_;
   uint64_t resource_content_id_;
@@ -940,13 +962,20 @@ scoped_refptr<TileTask> TileManager::CreateRasterTask(
   std::unique_ptr<RasterBuffer> raster_buffer =
       tile_task_manager_->GetRasterBufferProvider()->AcquireBufferForRaster(
           resource, resource_content_id, tile->invalidated_id());
+  RasterTaskImpl::CompletionData completion_data;
+  completion_data.tile_manager = this;
+  completion_data.resource = resource;
+  completion_data.tile = tile;
+  // TODO(prashant.n): Cache thread id in TileManager.
   return make_scoped_refptr(new RasterTaskImpl(
-      this, resource, prioritized_tile.raster_source(), tile->content_rect(),
+      completion_data, base::PlatformThread::CurrentId(),
+      prioritized_tile.raster_source(), tile->content_rect(),
       tile->invalidated_content_rect(), tile->contents_scale(),
       playback_settings, prioritized_tile.priority().resolution,
-      tile->layer_id(), prepare_tiles_count_, tile, tile->id(),
-      tile->invalidated_id(), resource_content_id, tile->source_frame_number(),
-      std::move(raster_buffer), &decode_tasks, supports_concurrent_execution));
+      tile->layer_id(), prepare_tiles_count_, static_cast<void*>(tile),
+      tile->id(), tile->invalidated_id(), resource_content_id,
+      tile->source_frame_number(), std::move(raster_buffer), &decode_tasks,
+      supports_concurrent_execution));
 }
 
 void TileManager::OnRasterTaskCompleted(