Allow PaymentAppContextImpl destruction after ShutdownOnIO.

Today, PaymentAppContext::Shutdown() is called from
~StoragePartitionImpl() after the UI thread is done running tasks
and before the IO thread is done running tasks. ShutdownOnIO() is
successfully posted to the IO thread. After it runs, it tries to
send the DidShutdown() reply to the UI thread, which fails. Because
of how PostTaskAndReply is implemented, the PaymentAppContext is
leaked (cf.
https://cs.chromium.org/chromium/src/base/threading/post_task_and_reply_impl.cc?l=89&rcl=0fe5265959e1da13c1521fd7f5f6275a3b83c614).

We now want to change PostTaskAndReply() to avoid leaking the
reply callback and its arguments when the reply thread can't run
tasks. To make that possible, we need to change the DCHECK in
~PaymentAppContextImpl() so that it only fails if ShutdownOnIO()
didn't run, not if DidShutdown() didn't run.

Bug: 807013
Change-Id: Ie893c70d423fed7c617cec31f01dfe1763501913
Reviewed-on: https://chromium-review.googlesource.com/929592Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538853}

content/browser/payments/payment_app_context_impl.cc

@@ -10,18 +10,19 @@
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "content/browser/payments/payment_manager.h"
-#include "content/public/browser/browser_thread.h"
 
 namespace content {
 
-PaymentAppContextImpl::PaymentAppContextImpl() : is_shutdown_(false) {
+PaymentAppContextImpl::PaymentAppContextImpl() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 }
 
 void PaymentAppContextImpl::Init(
     scoped_refptr<ServiceWorkerContextWrapper> service_worker_context) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(!is_shutdown_);
+#if DCHECK_IS_ON()
+  DCHECK(!did_shutdown_on_io_.IsSet());
+#endif
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
@@ -32,10 +33,13 @@ void PaymentAppContextImpl::Init(
 void PaymentAppContextImpl::Shutdown() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  BrowserThread::PostTaskAndReply(
+  // Schedule a ShutdownOnIO() callback that holds a reference to |this| on the
+  // IO thread. When the last reference to |this| is released, |this| is
+  // automatically scheduled for deletion on the UI thread (see
+  // content::BrowserThread::DeleteOnUIThread in the header file).
+  BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&PaymentAppContextImpl::ShutdownOnIO, this),
-      base::BindOnce(&PaymentAppContextImpl::DidShutdown, this));
+      base::BindOnce(&PaymentAppContextImpl::ShutdownOnIO, this));
 }
 
 void PaymentAppContextImpl::CreatePaymentManager(
@@ -63,7 +67,9 @@ PaymentAppDatabase* PaymentAppContextImpl::payment_app_database() const {
 
 PaymentAppContextImpl::~PaymentAppContextImpl() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(is_shutdown_);
+#if DCHECK_IS_ON()
+  DCHECK(did_shutdown_on_io_.IsSet());
+#endif
 }
 
 void PaymentAppContextImpl::CreatePaymentAppDatabaseOnIO(
@@ -86,12 +92,10 @@ void PaymentAppContextImpl::ShutdownOnIO() {
 
   payment_managers_.clear();
   payment_app_database_.reset();
-}
-
-void PaymentAppContextImpl::DidShutdown() {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  is_shutdown_ = true;
+#if DCHECK_IS_ON()
+  did_shutdown_on_io_.Set();
+#endif
 }
 
 }  // namespace content

content/browser/payments/payment_app_context_impl.h

@@ -8,10 +8,13 @@
 #include <map>
 #include <memory>
 
+#include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/synchronization/atomic_flag.h"
 #include "content/browser/payments/payment_app_database.h"
 #include "content/common/content_export.h"
+#include "content/public/browser/browser_thread.h"
 #include "third_party/WebKit/public/platform/modules/payments/payment_app.mojom.h"
 
 namespace content {
@@ -41,7 +44,9 @@ class ServiceWorkerContextWrapper;
 //   4) Shutdown()
 //   5) Destructor
 class CONTENT_EXPORT PaymentAppContextImpl
-    : public base::RefCountedThreadSafe<PaymentAppContextImpl> {
+    : public base::RefCountedThreadSafe<
+          PaymentAppContextImpl,
+          content::BrowserThread::DeleteOnUIThread> {
  public:
   PaymentAppContextImpl();
 
@@ -65,7 +70,9 @@ class CONTENT_EXPORT PaymentAppContextImpl
 
  private:
   friend class PaymentAppContentUnitTestBase;
-  friend class base::RefCountedThreadSafe<PaymentAppContextImpl>;
+  friend struct content::BrowserThread::DeleteOnThread<
+      content::BrowserThread::UI>;
+  friend class base::DeleteHelper<PaymentAppContextImpl>;
   ~PaymentAppContextImpl();
 
   void CreatePaymentAppDatabaseOnIO(
@@ -75,7 +82,6 @@ class CONTENT_EXPORT PaymentAppContextImpl
       mojo::InterfaceRequest<payments::mojom::PaymentManager> request);
 
   void ShutdownOnIO();
-  void DidShutdown();
 
   // Only accessed on the IO thread.
   std::unique_ptr<PaymentAppDatabase> payment_app_database_;
@@ -85,8 +91,11 @@ class CONTENT_EXPORT PaymentAppContextImpl
   // PaymentManagerHadConnectionError. Only accessed on the IO thread.
   std::map<PaymentManager*, std::unique_ptr<PaymentManager>> payment_managers_;
 
-  // Only accessed on the UI thread.
-  bool is_shutdown_;
+#if DCHECK_IS_ON()
+  // Set after ShutdownOnIO() has run on the IO thread. |this| shouldn't be
+  // deleted before this is set.
+  base::AtomicFlag did_shutdown_on_io_;
+#endif
 
   DISALLOW_COPY_AND_ASSIGN(PaymentAppContextImpl);
 };