Oilpan: dispose an HTMLImportsController on document detach/removal.

Source/core/html/imports/HTMLImportsController.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 37 matching lines @@
     return name;
 }
 
 void HTMLImportsController::provideTo(Document& master)
 {
     OwnPtrWillBeRawPtr<HTMLImportsController> controller = adoptPtrWillBeNoop(new HTMLImportsController(master));
     master.setImportsController(controller.get());
     DocumentSupplement::provideTo(master, supplementName(), controller.release());
 }
 
 void HTMLImportsController::removeFrom(Document& master)

[haraken, 2015/03/26 23:27:26]

It seems that HTMLImportsController::removeFrom is not at all called in oilpan
(HTMLImportsController::removeFrom is called only in !ENABLE(OILPAN)).

Probably do we need to call HTMLImportsController::dispose in
HTMLImportsController's pre-finalizer?

Even if I'm missing something and HTMLImportsController::removeFrom is called
somewhere, is it guaranteed that HTMLImportsController::removeFrom is _always_
called for all the HTMLImportsControllers that are going to be destructed? If
no, that would be problematic. If yes, we won't need to call dispose() in
~HTMLImportsController().

[sof, 2015/03/27 06:30:23]

removeFrom() is called from Document upon detach; see the Document code +
https://codereview.chromium.org/1035803002/ for code that touches on it (and
triggers the need for this CL.)

 {
+    HTMLImportsController* controller = master.importsController();
+    ASSERT(controller);
+    controller->dispose();
     static_cast<DocumentSupplementable&>(master).removeSupplement(supplementName());
     master.setImportsController(nullptr);
 }
 
 HTMLImportsController::HTMLImportsController(Document& master)
     : m_root(HTMLImportTreeRoot::create(&master))
 {
     UseCounter::count(master, UseCounter::HTMLImports);
 }
 
 HTMLImportsController::~HTMLImportsController()
 {
 #if !ENABLE(OILPAN)
-    m_root.clear();
-
-    for (size_t i = 0; i < m_loaders.size(); ++i)
-        m_loaders[i]->importDestroyed();
-    m_loaders.clear();
+    dispose();

[haraken, 2015/03/26 23:27:26]

As described above, I'm wondering why we need to call dispose() here.

[sof, 2015/03/27 08:44:31]

You're right, it's not needed. Replaced with a non-oilpan assert.

 #endif

[haraken, 2015/03/26 23:27:26]

In oilpan, we want to have an ASSERT to verify that dispose() has been called.
The same comment for other dispose()s in this CL.

 }
 
+void HTMLImportsController::dispose()
+{
+    if (m_root) {
+        m_root->dispose();
+        m_root.clear();
+    }
+
+    for (size_t i = 0; i < m_loaders.size(); ++i)
+        m_loaders[i]->dispose();
+    m_loaders.clear();
+}
+
 static bool makesCycle(HTMLImport* parent, const KURL& url)
 {
     for (HTMLImport* ancestor = parent; ancestor; ancestor = ancestor->parent()) {
         if (!ancestor->isRoot() && equalIgnoringFragmentIdentifier(toHTMLImportChild(parent)->url(), url))
             return true;
     }
 
     return false;
 }
 
@@ skipping 75 matching lines @@
 }
 
 DEFINE_TRACE(HTMLImportsController)
 {
     visitor->trace(m_root);
     visitor->trace(m_loaders);
     DocumentSupplement::trace(visitor);
 }
 
 } // namespace blink