RAII way for handling observers of NetworkStateNotifier

This CL changes how to manage observers of NetworkStateNotifier to use
handles in RAII way. Before this CL, we depended on the fact that
WebTaskRunner objects returned from TaskRunnerHelper::Get were always
same for the same execution context. Now we are trying to change the
fact and WebTaskRunnerImpl will always be created for each call.

This is part of the big CL for changing WebFrameSchedulerImpl::
*TaskRunner() to create WebTaskRunnerImpl anytime:
https://chromium-review.googlesource.com/c/chromium/src/+/704459

Bug: 762453
Change-Id: I5308e7cb0493d84c25e4da7f3562cd0fcff0f3af
Reviewed-on: https://chromium-review.googlesource.com/722859
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510012}

third_party/WebKit/Source/core/dom/Document.cpp

@@ -524,7 +524,7 @@ class Document::NetworkStateObserver final
  public:
   explicit NetworkStateObserver(Document& document)
       : ContextLifecycleObserver(&document) {
-    GetNetworkStateNotifier().AddOnLineObserver(
+    online_observer_handle_ = GetNetworkStateNotifier().AddOnLineObserver(
         this,
         TaskRunnerHelper::Get(TaskType::kNetworking, GetExecutionContext()));
   }
@@ -545,11 +545,14 @@ class Document::NetworkStateObserver final
 
   void UnregisterAsObserver(ExecutionContext* context) {
     DCHECK(context);
-    GetNetworkStateNotifier().RemoveOnLineObserver(
-        this, TaskRunnerHelper::Get(TaskType::kNetworking, context));
+    online_observer_handle_ = nullptr;
   }
 
   DEFINE_INLINE_VIRTUAL_TRACE() { ContextLifecycleObserver::Trace(visitor); }
+
+ private:
+  std::unique_ptr<NetworkStateNotifier::NetworkStateObserverHandle>
+      online_observer_handle_;
 };
 
 Document* Document::Create(const Document& document) {

third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp

@@ -66,13 +66,17 @@ NetworkInformation* NetworkInformation::Create(ExecutionContext* context) {
 }
 
 NetworkInformation::~NetworkInformation() {
-  DCHECK(!observing_);
+  DCHECK(!IsObserving());
 }
 
+bool NetworkInformation::IsObserving() const {
+  return !!connection_observer_handle_;
+};
+
 String NetworkInformation::type() const {
   // type_ is only updated when listening for events, so ask
   // networkStateNotifier if not listening (crbug.com/379841).
-  if (!observing_)
+  if (!IsObserving())
     return ConnectionTypeToString(GetNetworkStateNotifier().ConnectionType());
 
   // If observing, return m_type which changes when the event fires, per spec.
@@ -80,7 +84,7 @@ String NetworkInformation::type() const {
 }
 
 double NetworkInformation::downlinkMax() const {
-  if (!observing_)
+  if (!IsObserving())
     return GetNetworkStateNotifier().MaxBandwidth();
 
   return downlink_max_mbps_;
@@ -89,7 +93,7 @@ double NetworkInformation::downlinkMax() const {
 String NetworkInformation::effectiveType() const {
   // effective_type_ is only updated when listening for events, so ask
   // networkStateNotifier if not listening (crbug.com/379841).
-  if (!observing_) {
+  if (!IsObserving()) {
     return EffectiveConnectionTypeToString(
         GetNetworkStateNotifier().EffectiveType());
   }
@@ -99,14 +103,14 @@ String NetworkInformation::effectiveType() const {
 }
 
 unsigned long NetworkInformation::rtt() const {
-  if (!observing_)
+  if (!IsObserving())
     return RoundRtt(GetNetworkStateNotifier().HttpRtt());
 
   return http_rtt_msec_;
 }
 
 double NetworkInformation::downlink() const {
-  if (!observing_)
+  if (!IsObserving())
     return RoundMbps(GetNetworkStateNotifier().DownlinkThroughputMbps());
 
   return downlink_mbps_;
@@ -187,10 +191,10 @@ void NetworkInformation::RemoveAllEventListeners() {
 }
 
 bool NetworkInformation::HasPendingActivity() const {
-  DCHECK(context_stopped_ || observing_ == HasEventListeners());
+  DCHECK(context_stopped_ || IsObserving() == HasEventListeners());
 
   // Prevent collection of this object when there are active listeners.
-  return observing_;
+  return IsObserving();
 }
 
 void NetworkInformation::ContextDestroyed(ExecutionContext*) {
@@ -199,21 +203,20 @@ void NetworkInformation::ContextDestroyed(ExecutionContext*) {
 }
 
 void NetworkInformation::StartObserving() {
-  if (!observing_ && !context_stopped_) {
+  if (!IsObserving() && !context_stopped_) {
     type_ = GetNetworkStateNotifier().ConnectionType();
-    GetNetworkStateNotifier().AddConnectionObserver(
-        this,
-        TaskRunnerHelper::Get(TaskType::kNetworking, GetExecutionContext()));
-    observing_ = true;
+    DCHECK(!connection_observer_handle_);
+    connection_observer_handle_ =
+        GetNetworkStateNotifier().AddConnectionObserver(
+            this, TaskRunnerHelper::Get(TaskType::kNetworking,
+                                        GetExecutionContext()));
   }
 }
 
 void NetworkInformation::StopObserving() {
-  if (observing_) {
-    GetNetworkStateNotifier().RemoveConnectionObserver(
-        this,
-        TaskRunnerHelper::Get(TaskType::kNetworking, GetExecutionContext()));
-    observing_ = false;
+  if (IsObserving()) {
+    DCHECK(connection_observer_handle_);
+    connection_observer_handle_ = nullptr;
   }
 }
 
@@ -225,7 +228,6 @@ NetworkInformation::NetworkInformation(ExecutionContext* context)
       http_rtt_msec_(RoundRtt(GetNetworkStateNotifier().HttpRtt())),
       downlink_mbps_(
           RoundMbps(GetNetworkStateNotifier().DownlinkThroughputMbps())),
-      observing_(false),
       context_stopped_(false) {
   DCHECK_LE(1u, GetNetworkStateNotifier().RandomizationSalt());
   DCHECK_GE(20u, GetNetworkStateNotifier().RandomizationSalt());

third_party/WebKit/Source/modules/netinfo/NetworkInformation.h

@@ -83,6 +83,9 @@ class NetworkInformation final
   // returned value is in Mbps.
   double RoundMbps(const Optional<double>& downlink_mbps) const;
 
+  // Whether this object is listening for events from NetworkStateNotifier.
+  bool IsObserving() const;
+
   // Touched only on context thread.
   WebConnectionType type_;
 
@@ -102,11 +105,11 @@ class NetworkInformation final
   // only on context thread.
   double downlink_mbps_;
 
-  // Whether this object is listening for events from NetworkStateNotifier.
-  bool observing_;
-
   // Whether ContextLifecycleObserver::contextDestroyed has been called.
   bool context_stopped_;
+
+  std::unique_ptr<NetworkStateNotifier::NetworkStateObserverHandle>
+      connection_observer_handle_;
 };
 
 }  // namespace blink

third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp

@@ -65,14 +65,29 @@ NetworkStateNotifier::ScopedNotifier::~ScopedNotifier() {
        after.downlink_throughput_mbps != before_.downlink_throughput_mbps) &&
       before_.connection_initialized) {
     notifier_.NotifyObservers(notifier_.connection_observers_,
-                              ObserverType::CONNECTION_TYPE, after);
+                              ObserverType::kConnectionType, after);
   }
   if (after.on_line != before_.on_line && before_.on_line_initialized) {
     notifier_.NotifyObservers(notifier_.on_line_state_observers_,
-                              ObserverType::ONLINE_STATE, after);
+                              ObserverType::kOnLineState, after);
   }
 }
 
+NetworkStateNotifier::NetworkStateObserverHandle::NetworkStateObserverHandle(
+    NetworkStateNotifier* notifier,
+    NetworkStateNotifier::ObserverType type,
+    NetworkStateNotifier::NetworkStateObserver* observer,
+    RefPtr<WebTaskRunner> task_runner)
+    : notifier_(notifier),
+      type_(type),
+      observer_(observer),
+      task_runner_(std::move(task_runner)) {}
+
+NetworkStateNotifier::NetworkStateObserverHandle::
+    ~NetworkStateObserverHandle() {
+  notifier_->RemoveObserver(type_, observer_, std::move(task_runner_));
+}
+
 void NetworkStateNotifier::SetOnLine(bool on_line) {
   DCHECK(IsMainThread());
   ScopedNotifier notifier(*this);
@@ -122,28 +137,20 @@ void NetworkStateNotifier::SetNetworkQuality(WebEffectiveConnectionType type,
   }
 }
 
-void NetworkStateNotifier::AddConnectionObserver(
-    NetworkStateObserver* observer,
-    RefPtr<WebTaskRunner> task_runner) {
-  AddObserver(connection_observers_, observer, std::move(task_runner));
-}
-
-void NetworkStateNotifier::AddOnLineObserver(
-    NetworkStateObserver* observer,
-    RefPtr<WebTaskRunner> task_runner) {
-  AddObserver(on_line_state_observers_, observer, std::move(task_runner));
-}
-
-void NetworkStateNotifier::RemoveConnectionObserver(
-    NetworkStateObserver* observer,
-    RefPtr<WebTaskRunner> task_runner) {
-  RemoveObserver(connection_observers_, observer, std::move(task_runner));
+std::unique_ptr<NetworkStateNotifier::NetworkStateObserverHandle>
+NetworkStateNotifier::AddConnectionObserver(NetworkStateObserver* observer,
+                                            RefPtr<WebTaskRunner> task_runner) {
+  AddObserverToMap(connection_observers_, observer, task_runner);
+  return std::make_unique<NetworkStateNotifier::NetworkStateObserverHandle>(
+      this, ObserverType::kConnectionType, observer, task_runner);
 }
 
-void NetworkStateNotifier::RemoveOnLineObserver(
-    NetworkStateObserver* observer,
-    RefPtr<WebTaskRunner> task_runner) {
-  RemoveObserver(on_line_state_observers_, observer, std::move(task_runner));
+std::unique_ptr<NetworkStateNotifier::NetworkStateObserverHandle>
+NetworkStateNotifier::AddOnLineObserver(NetworkStateObserver* observer,
+                                        RefPtr<WebTaskRunner> task_runner) {
+  AddObserverToMap(on_line_state_observers_, observer, task_runner);
+  return std::make_unique<NetworkStateNotifier::NetworkStateObserverHandle>(
+      this, ObserverType::kOnLineState, observer, task_runner);
 }
 
 void NetworkStateNotifier::SetNetworkConnectionInfoOverride(
@@ -227,10 +234,10 @@ void NetworkStateNotifier::NotifyObserversOnTaskRunner(
     if (!observer_list->observers[i])
       continue;
     switch (type) {
-      case ObserverType::ONLINE_STATE:
+      case ObserverType::kOnLineState:
         observer_list->observers[i]->OnLineStateChange(state.on_line);
         continue;
-      case ObserverType::CONNECTION_TYPE:
+      case ObserverType::kConnectionType:
         observer_list->observers[i]->ConnectionChange(
             state.type, state.max_bandwidth_mbps, state.effective_type,
             state.http_rtt, state.transport_rtt,
@@ -246,9 +253,9 @@ void NetworkStateNotifier::NotifyObserversOnTaskRunner(
     CollectZeroedObservers(*map, observer_list, std::move(task_runner));
 }
 
-void NetworkStateNotifier::AddObserver(ObserverListMap& map,
-                                       NetworkStateObserver* observer,
-                                       RefPtr<WebTaskRunner> task_runner) {
+void NetworkStateNotifier::AddObserverToMap(ObserverListMap& map,
+                                            NetworkStateObserver* observer,
+                                            RefPtr<WebTaskRunner> task_runner) {
   DCHECK(task_runner->RunsTasksInCurrentSequence());
   DCHECK(observer);
 
@@ -262,9 +269,25 @@ void NetworkStateNotifier::AddObserver(ObserverListMap& map,
   result.stored_value->value->observers.push_back(observer);
 }
 
-void NetworkStateNotifier::RemoveObserver(ObserverListMap& map,
+void NetworkStateNotifier::RemoveObserver(ObserverType type,
                                           NetworkStateObserver* observer,
                                           RefPtr<WebTaskRunner> task_runner) {
+  switch (type) {
+    case ObserverType::kConnectionType:
+      RemoveObserverFromMap(connection_observers_, observer,
+                            std::move(task_runner));
+      break;
+    case ObserverType::kOnLineState:
+      RemoveObserverFromMap(on_line_state_observers_, observer,
+                            std::move(task_runner));
+      break;
+  }
+}
+
+void NetworkStateNotifier::RemoveObserverFromMap(
+    ObserverListMap& map,
+    NetworkStateObserver* observer,
+    RefPtr<WebTaskRunner> task_runner) {
   DCHECK(task_runner->RunsTasksInCurrentSequence());
   DCHECK(observer);
 

third_party/WebKit/Source/platform/network/NetworkStateNotifier.h

@@ -76,8 +76,37 @@ class PLATFORM_EXPORT NetworkStateNotifier {
     virtual void OnLineStateChange(bool on_line) {}
   };
 
+  enum class ObserverType {
+    kOnLineState,
+    kConnectionType,
+  };
+
+  class PLATFORM_EXPORT NetworkStateObserverHandle {
+    USING_FAST_MALLOC(NetworkStateObserverHandle);
+
+   public:
+    NetworkStateObserverHandle(NetworkStateNotifier*,
+                               ObserverType,
+                               NetworkStateObserver*,
+                               RefPtr<WebTaskRunner>);
+    ~NetworkStateObserverHandle();
+
+   private:
+    NetworkStateNotifier* notifier_;
+    ObserverType type_;
+    NetworkStateObserver* observer_;
+    RefPtr<WebTaskRunner> task_runner_;
+
+    DISALLOW_COPY_AND_ASSIGN(NetworkStateObserverHandle);
+  };
+
   NetworkStateNotifier() : has_override_(false) {}
 
+  ~NetworkStateNotifier() {
+    DCHECK(connection_observers_.IsEmpty());
+    DCHECK(on_line_state_observers_.IsEmpty());
+  }
+
   // Can be called on any thread.
   bool OnLine() const {
     MutexLocker locker(mutex_);
@@ -186,10 +215,12 @@ class PLATFORM_EXPORT NetworkStateNotifier {
   // before the observer or its execution context goes away. It's possible for
   // an observer to be called twice for the same event if it is first removed
   // and then added during notification.
-  void AddConnectionObserver(NetworkStateObserver*, RefPtr<WebTaskRunner>);
-  void AddOnLineObserver(NetworkStateObserver*, RefPtr<WebTaskRunner>);
-  void RemoveConnectionObserver(NetworkStateObserver*, RefPtr<WebTaskRunner>);
-  void RemoveOnLineObserver(NetworkStateObserver*, RefPtr<WebTaskRunner>);
+  std::unique_ptr<NetworkStateObserverHandle> AddConnectionObserver(
+      NetworkStateObserver*,
+      RefPtr<WebTaskRunner>);
+  std::unique_ptr<NetworkStateObserverHandle> AddOnLineObserver(
+      NetworkStateObserver*,
+      RefPtr<WebTaskRunner>);
 
   // Returns the randomization salt (weak and insecure) that should be used when
   // adding noise to the network quality metrics. This is known only to the
@@ -198,6 +229,8 @@ class PLATFORM_EXPORT NetworkStateNotifier {
   uint8_t RandomizationSalt() const { return randomization_salt_; }
 
  private:
+  friend class NetworkStateObserverHandle;
+
   struct ObserverList {
     ObserverList() : iterating(false) {}
     bool iterating;
@@ -219,11 +252,6 @@ class PLATFORM_EXPORT NetworkStateNotifier {
     NetworkState before_;
   };
 
-  enum class ObserverType {
-    ONLINE_STATE,
-    CONNECTION_TYPE,
-  };
-
   // The ObserverListMap is cross-thread accessed, adding/removing Observers
   // running on a task runner.
   using ObserverListMap =
@@ -235,12 +263,15 @@ class PLATFORM_EXPORT NetworkStateNotifier {
                                    RefPtr<WebTaskRunner>,
                                    const NetworkState&);
 
-  void AddObserver(ObserverListMap&,
-                   NetworkStateObserver*,
-                   RefPtr<WebTaskRunner>);
-  void RemoveObserver(ObserverListMap&,
+  void AddObserverToMap(ObserverListMap&,
+                        NetworkStateObserver*,
+                        RefPtr<WebTaskRunner>);
+  void RemoveObserver(ObserverType,
                       NetworkStateObserver*,
                       RefPtr<WebTaskRunner>);
+  void RemoveObserverFromMap(ObserverListMap&,
+                             NetworkStateObserver*,
+                             RefPtr<WebTaskRunner>);
 
   ObserverList* LockAndFindObserverList(ObserverListMap&,
                                         RefPtr<WebTaskRunner>);