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 be62b2fa2f5a59990c54f1ed5346ce25d041ed78..4a07c5b01108b2f26758716ce1b3a635660218f7 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/thread_checker.h"
 #include "base/trace_event/trace_event_argument.h"
 #include "cc/base/histograms.h"
 #include "cc/debug/devtools_instrumentation.h"
@@ -41,11 +42,10 @@ DEFINE_SCOPED_UMA_HISTOGRAM_AREA_TIMER(
     "Compositing.%s.RasterTask.RasterUs",
     "Compositing.%s.RasterTask.RasterPixelsPerMs");
 
-// TODO(prashant.n): Separate out members needed in RunOnWorkerThread() and
-// OnTaskCompleted() in RasterTaskImpl. crbug.com/613812.
 class RasterTaskImpl : public TileTask {
  public:
   RasterTaskImpl(TileManager* tile_manager,

[vmpstr, 2016/05/26 19:37:45]

Quite a few of these variables are coming from the tile object itself, so I
would prefer that we just pass the tile and pull off relevant data into
variables. Kind of like the following:

RasterTaskImpl(..., Tile* tile, ...)
  : tile_(tile),
    content_rect_(tile->content_rect()),
    contents_scale_(tile->contents_scale())
    ... {}

[prashant.n, 2016/05/26 22:02:18]

I'll check and modify.

+                 Tile* tile,
                  Resource* resource,
                  scoped_refptr<RasterSource> raster_source,
                  const gfx::Rect& content_rect,
@@ -55,7 +55,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,
@@ -65,6 +65,7 @@ class RasterTaskImpl : public TileTask {
                  bool supports_concurrent_execution)
       : TileTask(supports_concurrent_execution, dependencies),
         tile_manager_(tile_manager),
+        tile_(tile),
         resource_(resource),
         raster_source_(std::move(raster_source)),
         content_rect_(content_rect),
@@ -74,7 +75,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 +91,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,6 +104,8 @@ class RasterTaskImpl : public TileTask {
 
   // Overridden from TileTask:
   void OnTaskCompleted() override {
+    DCHECK(origin_thread_checker_.CalledOnValidThread());
+
     tile_manager_->OnRasterTaskCompleted(std::move(raster_buffer_), tile_,
                                          resource_, state().IsCanceled());
   }
@@ -111,8 +114,16 @@ class RasterTaskImpl : public TileTask {
   ~RasterTaskImpl() override { DCHECK(!raster_buffer_); }
 
  private:
+  base::ThreadChecker origin_thread_checker_;

[vmpstr, 2016/05/26 19:37:45]

How does this "bind" to the correct thread? Since OnTaskCompleted is expected to
run exactly once, if the first CalledOnValidThread also binds to the thread,
then it's kind of pointless. Maybe also call it at the construction time to
bind?

[prashant.n, 2016/05/26 22:02:18]

In ctor it will be called once.
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

So it binds to origin thread.

Actually definition of CalledOnValidThread() looks strange, it should not call
EnsureThreadIdAssigned().

For making explicit shall I call CalledOnValidThread() function in ctor and
dtor?

+
+  // The following members are needed for processing completion of this task on
+  // origin thread. These are not thread-safe and should be accessed only in
+  // origin thread. Ensure their access by checking CalledOnValidThread().
   TileManager* tile_manager_;

[vmpstr, 2016/05/26 19:37:45]

I was thinking more explicit

struct OriginThreadData {
  TileManager* tile_manager;
  Tile* tile;
  Resource* resource;
} origin_data;

style thing... I guess comments work as well, but it's not as clear in future
changes.

[prashant.n, 2016/05/26 22:02:18]

How about the way I implemented in patchset 2. Calling this as the
TaskCompletionData and we extend the class as RasterTaskCompletionData and
implement it after RasterTaskImpl class, so that is becomes strictly private?

[prashant.n, 2016/05/27 05:05:08]

I'll keep with comments now. Later if needed we can implement as I implemented
in patchset 2.

+  Tile* tile_;
   Resource* resource_;
+
+  // The following members should be used for running the task.
   scoped_refptr<RasterSource> raster_source_;
   gfx::Rect content_rect_;
   gfx::Rect invalid_content_rect_;
@@ -121,7 +132,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/26 19:21:58]

Can you change this to a Tile::Id?
https://code.google.com/p/chromium/codesearch#chromium/src/cc/tiles/tile.h&l=53

[vmpstr, 2016/05/26 19:37:45]

I think the tracing thing uses the address of the tile as an id so that it can
tie it together in frame viewer.

[reveman, 2016/05/26 19:45:04]

Ok, we should probably change that to use the Tile::Id now that we have such a
thing. Avoids having one tile be confused with another after deleting a tile and
creating a new with the same address.

For now, just calling this variable tile_tracing_id is sufficient to make me
happy.

[prashant.n, 2016/05/26 22:02:18]

Same comment like vmpstr@. For time being I'll rename this as you suggested.

   uint64_t new_content_id_;
   uint64_t previous_content_id_;
   uint64_t resource_content_id_;
@@ -941,10 +952,11 @@ scoped_refptr<TileTask> TileManager::CreateRasterTask(
       tile_task_manager_->GetRasterBufferProvider()->AcquireBufferForRaster(
           resource, resource_content_id, tile->invalidated_id());
   return make_scoped_refptr(new RasterTaskImpl(
-      this, resource, 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(),
+      this, tile, resource, 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_, 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));
 }