Oilpan: dispose an HTMLImportsController on document detach/removal.

Source/core/html/imports/HTMLImportsController.cpp

Index: Source/core/html/imports/HTMLImportsController.cpp
diff --git a/Source/core/html/imports/HTMLImportsController.cpp b/Source/core/html/imports/HTMLImportsController.cpp
index dde4127a28b524dfd83b0e014e4f6701b5e81acf..9a24fb7f3609fb0062c59329599c3d06f8695c8d 100644
--- a/Source/core/html/imports/HTMLImportsController.cpp
+++ b/Source/core/html/imports/HTMLImportsController.cpp
@@ -57,6 +57,9 @@ void HTMLImportsController::provideTo(Document& master)
 
 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);
 }
@@ -70,12 +73,20 @@ HTMLImportsController::HTMLImportsController(Document& master)
 HTMLImportsController::~HTMLImportsController()
 {
 #if !ENABLE(OILPAN)
-    m_root.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]->importDestroyed();
+        m_loaders[i]->dispose();
     m_loaders.clear();
-#endif
 }
 
 static bool makesCycle(HTMLImport* parent, const KURL& url)