NetworkStateNotifier: also remove NetworkStateObserver in finalizer too

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2011, 2012 Apple Inc. All
  * rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  * (http://www.torchmobile.com/)
  * Copyright (C) 2008, 2009, 2011, 2012 Google Inc. All rights reserved.
@@ skipping 388 matching lines @@
 }
 
 static void recordLoadReasonToHistogram(WouldLoadReason reason) {
   DEFINE_STATIC_LOCAL(
       EnumerationHistogram, unseenFrameHistogram,
       ("Navigation.DeferredDocumentLoading.StatesV3", WouldLoadReasonEnd));
   unseenFrameHistogram.count(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)
       : ContextLifecycleObserver(&document) {
     networkStateNotifier().addOnLineObserver(
         this,
         TaskRunnerHelper::get(TaskType::Networking, getExecutionContext()));
   }
 
   void onLineStateChange(bool onLine) override {
     AtomicString eventName =
         onLine ? EventTypeNames::online : EventTypeNames::offline;
     Document* document = toDocument(getExecutionContext());
     if (!document->domWindow())
       return;
     document->domWindow()->dispatchEvent(Event::create(eventName));
     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,
                    DocumentClassFlags documentClasses)
     : ContainerNode(0, CreateDocument),
       TreeScope(*this),
       m_hasNodesWithPlaceholderStyle(false),
       m_evaluateMediaQueriesOnStyleRecalc(false),
       m_pendingSheetLayout(NoLayoutWithPendingSheets),
       m_frame(initializer.frame()),
@@ skipping 6204 matching lines @@
 }
 
 void showLiveDocumentInstances() {
   WeakDocumentSet& set = liveDocumentSet();
   fprintf(stderr, "There are %u documents currently alive:\n", set.size());
   for (blink::Document* document : set)
     fprintf(stderr, "- Document %p URL: %s\n", document,
             document->url().getString().utf8().data());
 }
 #endif