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

cc/tiles/tile_manager.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/tiles/tile_manager.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <limits>
 #include <string>
 
 #include "base/bind.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #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"
 #include "cc/debug/frame_viewer_instrumentation.h"
 #include "cc/debug/traced_value.h"
 #include "cc/layers/picture_layer_impl.h"
 #include "cc/raster/raster_buffer.h"
 #include "cc/raster/task_category.h"
 #include "cc/tiles/tile.h"
 #include "ui/gfx/geometry/rect_conversions.h"
 
 namespace cc {
 namespace {
 
 // Flag to indicate whether we should try and detect that
 // a tile is of solid color.
 const bool kUseColorEstimator = true;
 
 DEFINE_SCOPED_UMA_HISTOGRAM_AREA_TIMER(
     ScopedRasterTaskTimer,
     "Compositing.%s.RasterTask.RasterUs",
     "Compositing.%s.RasterTask.RasterPixelsPerMs");
 
 // TODO(prashant.n): Separate out members needed in RunOnWorkerThread() and
 // 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,
                  float contents_scale,
                  const RasterSource::PlaybackSettings& playback_settings,
                  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,
                  int source_frame_number,
                  std::unique_ptr<RasterBuffer> raster_buffer,
                  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),
         contents_scale_(contents_scale),
         playback_settings_(playback_settings),
         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),
         source_frame_number_(source_frame_number),
         raster_buffer_(std::move(raster_buffer)) {}
 
   // Overridden from Task:
   void RunOnWorkerThread() override {
     TRACE_EVENT1("cc", "RasterizerTaskImpl::RunOnWorkerThread",
                  "source_prepare_tiles_id", source_prepare_tiles_id_);
 
     DCHECK(raster_source_.get());
     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());
 
     DCHECK(raster_source_);
 
     raster_buffer_->Playback(raster_source_.get(), content_rect_,
                              invalid_content_rect_, new_content_id_,
                              contents_scale_, playback_settings_);
   }
 
   // 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_;
   float contents_scale_;
   RasterSource::PlaybackSettings playback_settings_;
   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_;
   int source_frame_number_;
   std::unique_ptr<RasterBuffer> raster_buffer_;
 
   DISALLOW_COPY_AND_ASSIGN(RasterTaskImpl);
 };
 
 TaskCategory TaskCategoryForTileTask(TileTask* task,
@@ skipping 798 matching lines @@
     if (need_to_unref_when_finished)
       ++it;
     else
       it = images.erase(it);
   }
 
   bool supports_concurrent_execution = !use_gpu_rasterization_;
   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(
     std::unique_ptr<RasterBuffer> raster_buffer,
     Tile* tile,
     Resource* resource,
     bool was_canceled) {
   DCHECK(tile);
   DCHECK(tiles_.find(tile->id()) != tiles_.end());
   tile_task_manager_->GetRasterBufferProvider()->ReleaseBufferForRaster(
@@ skipping 321 matching lines @@
 void TileManager::Signals::reset() {
   ready_to_activate = false;
   did_notify_ready_to_activate = false;
   ready_to_draw = false;
   did_notify_ready_to_draw = false;
   all_tile_tasks_completed = false;
   did_notify_all_tile_tasks_completed = false;
 }
 
 }  // namespace cc