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/thread_checker.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.
 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,
                  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),
+        tile_(tile),
         resource_(resource),
         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 {
+    DCHECK(origin_thread_checker_.CalledOnValidThread());
+
     tile_manager_->OnRasterTaskCompleted(std::move(raster_buffer_), tile_,
                                          resource_, state().IsCanceled());
   }
 
  protected:
   ~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_;
   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/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_;
   int source_frame_number_;
   std::unique_ptr<RasterBuffer> raster_buffer_;
 
   DISALLOW_COPY_AND_ASSIGN(RasterTaskImpl);
 };
 
 TaskCategory TaskCategoryForTileTask(TileTask* task,
@@ skipping 799 matching lines @@
       ++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());
   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));
 }
 
 void TileManager::OnRasterTaskCompleted(
     std::unique_ptr<RasterBuffer> raster_buffer,
     Tile* tile,
     Resource* resource,
     bool was_canceled) {
   DCHECK(tile);
@@ skipping 323 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