aw: Not use WeakPtr in TaskForwardingSequence

TaskForwardingSequence created and destroyed on DisplayCompositor
thread. Destruction will cause weak pointers to invalidate. The weak
pointers are checked when RunTask happens and it's on Android Render
Thread. Weak pointers are not thread safe, so this is incorrect.

This CL fixes the issue by not using WeakPtr and binding all necessary
data to task.

Change-Id: I89ef10ff332bd806d660fa4d7b6d9ec6417fcc1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2042384
Commit-Queue: Vasiliy Telezhnikov <vasilyt@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739448}

android_webview/browser/gfx/task_forwarding_sequence.cc

@@ -18,8 +18,7 @@ TaskForwardingSequence::TaskForwardingSequence(
     : gpu::SingleTaskSequence(),
       task_queue_(task_queue),
       sync_point_manager_(sync_point_manager),
-      sync_point_order_data_(sync_point_manager_->CreateSyncPointOrderData()),
-      weak_ptr_factory_(this) {}
+      sync_point_order_data_(sync_point_manager_->CreateSyncPointOrderData()) {}
 
 TaskForwardingSequence::~TaskForwardingSequence() {
   sync_point_order_data_->Destroy();
@@ -38,12 +37,13 @@ void TaskForwardingSequence::ScheduleTask(
     base::OnceClosure task,
     std::vector<gpu::SyncToken> sync_token_fences) {
   uint32_t order_num = sync_point_order_data_->GenerateUnprocessedOrderNumber();
-  // Use a weak ptr because the task executor holds the tasks, and the
-  // sequence will be destroyed before the task executor.
+
+  // |sync_point_manager_| is global so it's safe to pass raw pointer.
+  // sync_point_order_data_ is ThreadSafe.
   task_queue_->ScheduleTask(
-      base::BindOnce(&TaskForwardingSequence::RunTask,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(task),
-                     std::move(sync_token_fences), order_num),
+      base::BindOnce(&TaskForwardingSequence::RunTask, std::move(task),
+                     std::move(sync_token_fences), order_num,
+                     sync_point_manager_, sync_point_order_data_),
       false /* out_of_order */);
 }
 
@@ -51,11 +51,13 @@ void TaskForwardingSequence::ScheduleOrRetainTask(
     base::OnceClosure task,
     std::vector<gpu::SyncToken> sync_token_fences) {
   uint32_t order_num = sync_point_order_data_->GenerateUnprocessedOrderNumber();
-  // Use a weak ptr because the task executor holds the tasks, and the
-  // sequence will be destroyed before the task executor.
-  task_queue_->ScheduleOrRetainTask(base::BindOnce(
-      &TaskForwardingSequence::RunTask, weak_ptr_factory_.GetWeakPtr(),
-      std::move(task), std::move(sync_token_fences), order_num));
+
+  // |sync_point_manager_| is global so it's safe to pass raw pointer.
+  // sync_point_order_data_ is ThreadSafe.
+  task_queue_->ScheduleOrRetainTask(
+      base::BindOnce(&TaskForwardingSequence::RunTask, std::move(task),
+                     std::move(sync_token_fences), order_num,
+                     sync_point_manager_, sync_point_order_data_));
 }
 
 // Should not be called because tasks aren't reposted to wait for sync tokens,
@@ -69,13 +71,15 @@ void TaskForwardingSequence::ContinueTask(base::OnceClosure task) {
 void TaskForwardingSequence::RunTask(
     base::OnceClosure task,
     std::vector<gpu::SyncToken> sync_token_fences,
-    uint32_t order_num) {
+    uint32_t order_num,
+    gpu::SyncPointManager* sync_point_manager,
+    scoped_refptr<gpu::SyncPointOrderData> sync_point_order_data) {
   // Block thread when waiting for sync token. This avoids blocking when we
   // encounter the wait command later.
   for (const auto& sync_token : sync_token_fences) {
     base::WaitableEvent completion;
-    if (sync_point_manager_->Wait(
-            sync_token, sync_point_order_data_->sequence_id(), order_num,
+    if (sync_point_manager->Wait(
+            sync_token, sync_point_order_data->sequence_id(), order_num,
             base::BindOnce(&base::WaitableEvent::Signal,
                            base::Unretained(&completion)))) {
       TRACE_EVENT0("android_webview",
@@ -83,9 +87,9 @@ void TaskForwardingSequence::RunTask(
       completion.Wait();
     }
   }
-  sync_point_order_data_->BeginProcessingOrderNumber(order_num);
+  sync_point_order_data->BeginProcessingOrderNumber(order_num);
   std::move(task).Run();
-  sync_point_order_data_->FinishProcessingOrderNumber(order_num);
+  sync_point_order_data->FinishProcessingOrderNumber(order_num);
 }
 
 }  // namespace android_webview

android_webview/browser/gfx/task_forwarding_sequence.h

@@ -10,7 +10,6 @@
 #include <memory>
 #include <utility>
 
-#include "base/memory/weak_ptr.h"
 #include "gpu/ipc/single_task_sequence.h"
 
 namespace gpu {
@@ -49,15 +48,17 @@ class TaskForwardingSequence : public gpu::SingleTaskSequence {
  private:
   // Method to wrap scheduled task with the order number processing required for
   // sync tokens.
-  void RunTask(base::OnceClosure task,
+  static void RunTask(
+      base::OnceClosure task,
       std::vector<gpu::SyncToken> sync_token_fences,
-               uint32_t order_num);
+      uint32_t order_num,
+      gpu::SyncPointManager* sync_point_manager,
+      scoped_refptr<gpu::SyncPointOrderData> sync_point_order_data);
 
   // Raw pointer refer to the global instance.
   TaskQueueWebView* const task_queue_;
   gpu::SyncPointManager* const sync_point_manager_;
   scoped_refptr<gpu::SyncPointOrderData> sync_point_order_data_;
-  base::WeakPtrFactory<TaskForwardingSequence> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(TaskForwardingSequence);
 };