Fix the multi-threaded access to WeakPtr<BrowsingDataRemoverImpl>

chrome/browser/browsing_data/browsing_data_remover_impl.h

Index: chrome/browser/browsing_data/browsing_data_remover_impl.h
diff --git a/chrome/browser/browsing_data/browsing_data_remover_impl.h b/chrome/browser/browsing_data/browsing_data_remover_impl.h
index 9889aea080b5a32f73939cc5e63899e996fa35eb..9ff4bc4c5d2c5c440f6d84a8eff33844b19f79ff 100644
--- a/chrome/browser/browsing_data/browsing_data_remover_impl.h
+++ b/chrome/browser/browsing_data/browsing_data_remover_impl.h
@@ -12,6 +12,7 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/synchronization/waitable_event_watcher.h"
@@ -172,6 +173,41 @@ class BrowsingDataRemoverImpl : public BrowsingDataRemover {
     Observer* observer;
   };
 
+  // OriginMaskAndURLMatcher handles the implementation of DoesOriginMatchMask.
+  // As an intrinsic part of BrowsingDataRemoverImpl, it should be destroyed
+  // together with it on the UI thread. However, it is possible that at this
+  // time a StoragePartition is still using it on the IO thread. Therefore,
+  // we hold it ref-counted.
+  class OriginMaskAndURLMatcher
+      : public base::RefCountedThreadSafe<OriginMaskAndURLMatcher> {
+   public:
+    using MaskMatcherFunction =
+        BrowsingDataRemoverDelegate::MaskMatcherFunction;
+
+    OriginMaskAndURLMatcher();
+
+    // Returns whether |origin| matches |origin_type_mask| given the special
+    // storage |policy|; and if |predicate| is not null, then also whether
+    // it matches |predicate|.
+    bool DoesOriginMatchMaskAndURLs(
+        int origin_type_mask,
+        const base::Callback<bool(const GURL&)>& predicate,
+        const GURL& origin,
+        storage::SpecialStoragePolicy* policy) const;
+
+    // Called from BrowsingDataRemoverImpl::SetEmbedderDelegate to inject
+    // the matching function for embedder datatypes.
+    void set_embedder_matcher(const MaskMatcherFunction& embedder_matcher) {
+      embedder_matcher_ = embedder_matcher;
+    }
+
+   private:
+    friend class base::RefCountedThreadSafe<OriginMaskAndURLMatcher>;
+    ~OriginMaskAndURLMatcher();
+
+    MaskMatcherFunction embedder_matcher_;

[Bernhard Bauer, 2017/03/30 10:49:26]

If this is the only state in the class, and it's a callback, so the object
itself is thread-safe, why do we need to wrap it in a class? Couldn't we just
make DoesOriginMatchMaskAndURLs() static and bind this as an additional
parameter?

[msramek, 2017/04/06 12:06:38]

Because I panicked and overengineered it. Done.

+  };
+
   void Shutdown() override;
 
   // Setter for |is_removing_|; DCHECKs that we can only start removing if we're
@@ -252,6 +288,10 @@ class BrowsingDataRemoverImpl : public BrowsingDataRemover {
   // We do not own this.
   content::StoragePartition* storage_partition_for_testing_ = nullptr;
 
+  // Handles the implementation of DoesOriginMatchMask(). Used on the UI and IO
+  // threads.
+  scoped_refptr<OriginMaskAndURLMatcher> origin_mask_and_url_matcher_;
+
   base::WeakPtrFactory<BrowsingDataRemoverImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowsingDataRemoverImpl);