NetworkStateNotifier: also remove NetworkStateObserver in finalizer too

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

Index: third_party/WebKit/Source/core/dom/Document.cpp
diff --git a/third_party/WebKit/Source/core/dom/Document.cpp b/third_party/WebKit/Source/core/dom/Document.cpp
index b8178d8ac6d4d502ab047f41ed3a26978f8b2146..5d1a57ded3208dc801d2172fbdf3d3db7186fa6e 100644
--- a/third_party/WebKit/Source/core/dom/Document.cpp
+++ b/third_party/WebKit/Source/core/dom/Document.cpp
@@ -406,10 +406,11 @@ static void recordLoadReasonToHistogram(WouldLoadReason reason) {
 }
 
 class Document::NetworkStateObserver final
-    : public GarbageCollected<Document::NetworkStateObserver>,
+    : public GarbageCollectedFinalized<Document::NetworkStateObserver>,
       public NetworkStateNotifier::NetworkStateObserver,
       public ContextLifecycleObserver {
   USING_GARBAGE_COLLECTED_MIXIN(Document::NetworkStateObserver);
+  USING_PRE_FINALIZER(Document::NetworkStateObserver, dispose);

[jkarlin, 2017/02/24 13:13:51]

Why PRE_FINALIZER instead of normal destructor?

[kinuko, 2017/02/24 13:56:25]

Removing observer touches on-heap object, so I think it needs to be done in
pre-finalizer or in eagerly finalized way.  Switched to use EAGERLY_FINALIZE to
do it in destructor (I slightly prefer pre-finalizer but either looks ok here)

[jkarlin, 2017/02/24 14:20:06]

What object does it touch that's on the oilpan heap?

[kinuko, 2017/02/24 14:43:16]

m_executionContext in contextLifecycleObserver, and TaskRunnerHelper::get()
derefs it (hm, we could also keep the task runner around).  Also dtor could be
called much later without EAGERLY_FINALIZE, which seems dangerous if we don't
unregister our callbacks earlier.

[jkarlin, 2017/02/24 15:31:27]

Ah yes, thanks!

 
  public:
   explicit NetworkStateObserver(Document& document)
@@ -429,12 +430,21 @@ class Document::NetworkStateObserver final
     InspectorInstrumentation::networkStateChanged(document->frame(), onLine);
   }
 
-  void contextDestroyed(ExecutionContext* context) override {
+  void contextDestroyed(ExecutionContext* context) override { dispose(); }
+
+  void dispose() {
+    if (detached)
+      return;
+    detached = true;
     networkStateNotifier().removeOnLineObserver(
-        this, TaskRunnerHelper::get(TaskType::Networking, context));
+        this,
+        TaskRunnerHelper::get(TaskType::Networking, getExecutionContext()));
   }
 
   DEFINE_INLINE_VIRTUAL_TRACE() { ContextLifecycleObserver::trace(visitor); }
+
+ private:
+  bool detached = false;

[jkarlin, 2017/02/24 13:13:51]

Forgot the m_.

nit: Perhaps m_isObserving?

[kinuko, 2017/02/24 13:56:25]

Removed this flag in favor of checking getExecutionContext().

 };
 
 Document::Document(const DocumentInit& initializer,