Fix notifying observers in SpecialStoragePolicy

This CL restricts adding, removing and notifying observers in
SpecialStoragePolicy within one thread using sequence checker. It also
makes all call sites to notify observers from the correct (IO) thread.

The fix should also mitigate MigrationSingleClientTest tests flakiness.

Bug: 1094907
Change-Id: Ife32fe48af22129ef0e04779c62c707b5f20f3db
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2301972Reviewed-by: David Bertoni <dbertoni@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789733}

chrome/browser/extensions/extension_special_storage_policy.cc

@@ -74,7 +74,7 @@ class ExtensionSpecialStoragePolicy::CookieSettingsObserver
     // Post a task to avoid any potential re-entrancy issues with
     // |NotifyPolicyChangedImpl()| since it holds a lock while calling back into
     // ExtensionSpecialStoragePolicy.
-    content::GetUIThreadTaskRunner({})->PostTask(
+    content::GetIOThreadTaskRunner({})->PostTask(
         FROM_HERE,
         base::BindOnce(&CookieSettingsObserver::NotifyPolicyChangedImpl,
                        base::Unretained(this)));

storage/browser/quota/special_storage_policy.cc

@@ -8,20 +8,25 @@ namespace storage {
 
 SpecialStoragePolicy::Observer::~Observer() = default;
 
-SpecialStoragePolicy::SpecialStoragePolicy() = default;
+SpecialStoragePolicy::SpecialStoragePolicy() {
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+}
 
 SpecialStoragePolicy::~SpecialStoragePolicy() = default;
 
 void SpecialStoragePolicy::AddObserver(Observer* observer) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   observers_.AddObserver(observer);
 }
 
 void SpecialStoragePolicy::RemoveObserver(Observer* observer) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   observers_.RemoveObserver(observer);
 }
 
 void SpecialStoragePolicy::NotifyGranted(const url::Origin& origin,
                                          int change_flags) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   scoped_refptr<SpecialStoragePolicy> protect(this);
   for (auto& observer : observers_)
     observer.OnGranted(origin, change_flags);
@@ -30,6 +35,7 @@ void SpecialStoragePolicy::NotifyGranted(const url::Origin& origin,
 
 void SpecialStoragePolicy::NotifyRevoked(const url::Origin& origin,
                                          int change_flags) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   scoped_refptr<SpecialStoragePolicy> protect(this);
   for (auto& observer : observers_)
     observer.OnRevoked(origin, change_flags);
@@ -37,6 +43,7 @@ void SpecialStoragePolicy::NotifyRevoked(const url::Origin& origin,
 }
 
 void SpecialStoragePolicy::NotifyCleared() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   scoped_refptr<SpecialStoragePolicy> protect(this);
   for (auto& observer : observers_)
     observer.OnCleared();
@@ -44,6 +51,7 @@ void SpecialStoragePolicy::NotifyCleared() {
 }
 
 void SpecialStoragePolicy::NotifyPolicyChanged() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   scoped_refptr<SpecialStoragePolicy> protect(this);
   for (auto& observer : observers_)
     observer.OnPolicyChanged();

storage/browser/quota/special_storage_policy.h

@@ -11,6 +11,7 @@
 #include "base/component_export.h"
 #include "base/memory/ref_counted.h"
 #include "base/observer_list.h"
+#include "base/sequence_checker.h"
 #include "services/network/public/cpp/session_cookie_delete_predicate.h"
 
 class GURL;
@@ -109,7 +110,9 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) SpecialStoragePolicy
   // above notifications.
   void NotifyPolicyChanged();
 
-  base::ObserverList<Observer>::Unchecked observers_;
+  base::ObserverList<Observer>::Unchecked observers_
+      GUARDED_BY_CONTEXT(sequence_checker_);
+  SEQUENCE_CHECKER(sequence_checker_);
 };
 
 }  // namespace storage