CATransactionGPUCoordinator: Re-enable and make lifetime explicit

Disalbing the CATransactionGPUCoordinator caused memory corruption bugs
to go away.

Fix two potential bugs:

1.  Don't post tasks (and potentially change the reference count) inside
    CATransactionGPUCoordinator's constructor. Do this in a separate
    explicit Init function.

1a. Move this initialization to the end of GpuProcessHost:Init (instead
    of being at the beginning of the constructor).

2.  Make CATransactionCoordinator explicitly retain PostCommitObserver
    (which includes CATransactionGPUCoordinator). This fixes a bug
    whereby at shutdown, destroying not-yet-executed posted tasks caused
    the CATransactionGPUCoordinator to be destroyed while the
    CATransactionCoordinator still had a raw pointer to it.

Bug: 871430
Change-Id: Ie144071cce9ce48e0187cdaf1fcf32df7b62ed75
Reviewed-on: https://chromium-review.googlesource.com/1171657
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582378}

content/browser/gpu/ca_transaction_gpu_coordinator.cc

@@ -13,30 +13,52 @@
 
 namespace content {
 
-CATransactionGPUCoordinator::CATransactionGPUCoordinator(GpuProcessHost* host)
-    : host_(host) {
+// static
+scoped_refptr<CATransactionGPUCoordinator> CATransactionGPUCoordinator::Create(
+    GpuProcessHost* host) {
+  scoped_refptr<CATransactionGPUCoordinator> result(
+      new CATransactionGPUCoordinator(host));
+  // Avoid modifying result's refcount in the constructor by performing this
+  // PostTask afterward.
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   ui::WindowResizeHelperMac::Get()->task_runner()->PostTask(
       FROM_HERE,
-      base::BindOnce(&ui::CATransactionCoordinator::AddPostCommitObserver,
-                     base::Unretained(&ui::CATransactionCoordinator::Get()),
-                     base::RetainedRef(this)));
+      base::BindOnce(
+          &CATransactionGPUCoordinator::AddPostCommitObserverOnUIThread,
+          result));
+  return result;
 }
 
+CATransactionGPUCoordinator::CATransactionGPUCoordinator(GpuProcessHost* host)
+    : host_(host) {}
+
 CATransactionGPUCoordinator::~CATransactionGPUCoordinator() {
   DCHECK(!host_);
+  DCHECK(!registered_as_observer_);
 }
 
 void CATransactionGPUCoordinator::HostWillBeDestroyed() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   ui::WindowResizeHelperMac::Get()->task_runner()->PostTask(
       FROM_HERE,
-      base::BindOnce(&ui::CATransactionCoordinator::RemovePostCommitObserver,
-                     base::Unretained(&ui::CATransactionCoordinator::Get()),
-                     base::RetainedRef(this)));
+      base::BindOnce(
+          &CATransactionGPUCoordinator::RemovePostCommitObserverOnUIThread,
+          this));
   host_ = nullptr;
 }
 
+void CATransactionGPUCoordinator::AddPostCommitObserverOnUIThread() {
+  DCHECK(!registered_as_observer_);
+  ui::CATransactionCoordinator::Get().AddPostCommitObserver(this);
+  registered_as_observer_ = true;
+}
+
+void CATransactionGPUCoordinator::RemovePostCommitObserverOnUIThread() {
+  DCHECK(registered_as_observer_);
+  ui::CATransactionCoordinator::Get().RemovePostCommitObserver(this);
+  registered_as_observer_ = false;
+}
+
 void CATransactionGPUCoordinator::OnActivateForTransaction() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   BrowserThread::PostTask(

content/browser/gpu/ca_transaction_gpu_coordinator.h

@@ -17,30 +17,41 @@ class GpuProcessHost;
 
 // Synchronizes CATransaction commits between the browser and GPU processes.
 class CATransactionGPUCoordinator
-    : public base::RefCountedThreadSafe<CATransactionGPUCoordinator>,
-      public ui::CATransactionCoordinator::PostCommitObserver {
+    : public ui::CATransactionCoordinator::PostCommitObserver {
  public:
-  CATransactionGPUCoordinator(GpuProcessHost* host);
-
+  static scoped_refptr<CATransactionGPUCoordinator> Create(
+      GpuProcessHost* host);
   void HostWillBeDestroyed();
 
  private:
   friend class base::RefCountedThreadSafe<CATransactionGPUCoordinator>;
-  virtual ~CATransactionGPUCoordinator();
+  CATransactionGPUCoordinator(GpuProcessHost* host);
+  ~CATransactionGPUCoordinator() override;
 
   // ui::CATransactionObserver implementation
   void OnActivateForTransaction() override;
   void OnEnterPostCommit() override;
   bool ShouldWaitInPostCommit() override;
 
+  void AddPostCommitObserverOnUIThread();
+  void RemovePostCommitObserverOnUIThread();
+
   void OnActivateForTransactionOnIO();
   void OnEnterPostCommitOnIO();
   void OnCommitCompletedOnIO();
   void OnCommitCompletedOnUI();
 
-  GpuProcessHost* host_;  // weak
+  // The GpuProcessHost to use to initiate GPU-side CATransactions. This is only
+  // to be accessed on the IO thread.
+  GpuProcessHost* host_ = nullptr;
+
+  // The number CATransactions that have not yet completed. This is only to be
+  // accessed on the UI thread.
   int pending_commit_count_ = 0;
 
+  // Egregious state tracking to debug https://crbug.com/871430
+  bool registered_as_observer_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(CATransactionGPUCoordinator);
 };
 

content/browser/gpu/gpu_process_host.cc

@@ -702,11 +702,6 @@ GpuProcessHost::GpuProcessHost(int host_id, GpuProcessKind kind)
       kind_(kind),
       process_launched_(false),
       status_(UNKNOWN),
-#if defined(OS_MACOSX)
-      // TODO(ccameron): This is temporarily disabled to see if it is the cause
-      // of https://crbug.com/871430
-      ca_transaction_gpu_coordinator_(nullptr),
-#endif
       gpu_host_binding_(this),
       weak_ptr_factory_(this) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
@@ -938,6 +933,10 @@ bool GpuProcessHost::Init() {
   InitOzone();
 #endif  // defined(USE_OZONE)
 
+#if defined(OS_MACOSX)
+  ca_transaction_gpu_coordinator_ = CATransactionGPUCoordinator::Create(this);
+#endif
+
   return true;
 }
 

ui/accelerated_widget_mac/ca_transaction_observer.h

@@ -51,11 +51,21 @@ class ACCELERATED_WIDGET_MAC_EXPORT CATransactionCoordinator {
     virtual base::TimeDelta PreCommitTimeout() = 0;
   };
 
-  class PostCommitObserver {
+  // PostCommitObserver sub-classes must communicate with the IO thread. The
+  // CATransactionCoordinator will retain registered observers to ensure that
+  // they are not deleted while registered.
+  class PostCommitObserver
+      : public base::RefCountedThreadSafe<PostCommitObserver> {
    public:
     virtual void OnActivateForTransaction() = 0;
     virtual void OnEnterPostCommit() = 0;
     virtual bool ShouldWaitInPostCommit() = 0;
+
+   protected:
+    virtual ~PostCommitObserver() {}
+
+   private:
+    friend class base::RefCountedThreadSafe<PostCommitObserver>;
   };
 
   static CATransactionCoordinator& Get();
@@ -66,8 +76,8 @@ class ACCELERATED_WIDGET_MAC_EXPORT CATransactionCoordinator {
   void AddPreCommitObserver(PreCommitObserver*);
   void RemovePreCommitObserver(PreCommitObserver*);
 
-  void AddPostCommitObserver(PostCommitObserver*);
-  void RemovePostCommitObserver(PostCommitObserver*);
+  void AddPostCommitObserver(scoped_refptr<PostCommitObserver>);
+  void RemovePostCommitObserver(scoped_refptr<PostCommitObserver>);
 
  private:
   friend class base::NoDestructor<CATransactionCoordinator>;
@@ -82,7 +92,7 @@ class ACCELERATED_WIDGET_MAC_EXPORT CATransactionCoordinator {
   bool active_ = false;
   bool disabled_for_testing_ = false;
   base::ObserverList<PreCommitObserver> pre_commit_observers_;
-  base::ObserverList<PostCommitObserver> post_commit_observers_;
+  std::set<scoped_refptr<PostCommitObserver>> post_commit_observers_;
 
   DISALLOW_COPY_AND_ASSIGN(CATransactionCoordinator);
 };

ui/accelerated_widget_mac/ca_transaction_observer.mm

@@ -49,7 +49,7 @@ void CATransactionCoordinator::SynchronizeImpl() {
   active_ = true;
 
   for (auto& observer : post_commit_observers_)
-    observer.OnActivateForTransaction();
+    observer->OnActivateForTransaction();
 
   [CATransaction addCommitHandler:^{
     PreCommitHandler();
@@ -90,7 +90,7 @@ void CATransactionCoordinator::PostCommitHandler() {
   TRACE_EVENT0("ui", "CATransactionCoordinator: post-commit handler");
 
   for (auto& observer : post_commit_observers_)
-    observer.OnEnterPostCommit();
+    observer->OnEnterPostCommit();
 
   auto* clock = base::DefaultTickClock::GetInstance();
   const base::TimeTicks deadline = clock->NowTicks() + kPostCommitTimeout;
@@ -131,13 +131,15 @@ void CATransactionCoordinator::RemovePreCommitObserver(
 }
 
 void CATransactionCoordinator::AddPostCommitObserver(
-    PostCommitObserver* observer) {
-  post_commit_observers_.AddObserver(observer);
+    scoped_refptr<PostCommitObserver> observer) {
+  DCHECK(!post_commit_observers_.count(observer));
+  post_commit_observers_.insert(std::move(observer));
 }
 
 void CATransactionCoordinator::RemovePostCommitObserver(
-    PostCommitObserver* observer) {
-  post_commit_observers_.RemoveObserver(observer);
+    scoped_refptr<PostCommitObserver> observer) {
+  DCHECK(post_commit_observers_.count(observer));
+  post_commit_observers_.erase(std::move(observer));
 }
 
 }  // namespace ui