Oilpan: enable appcache + move DocumentLoader to the heap.

Source/core/loader/appcache/ApplicationCacheHost.cpp

Index: Source/core/loader/appcache/ApplicationCacheHost.cpp
diff --git a/Source/core/loader/appcache/ApplicationCacheHost.cpp b/Source/core/loader/appcache/ApplicationCacheHost.cpp
index aca55a24e73aff8ed4a31e2d34c6ef3f67c87007..1f09d158e7852887f25f11c0882464fc9b3d2de0 100644
--- a/Source/core/loader/appcache/ApplicationCacheHost.cpp
+++ b/Source/core/loader/appcache/ApplicationCacheHost.cpp
@@ -166,15 +166,16 @@ void ApplicationCacheHost::setApplicationCache(ApplicationCache* domApplicationC
 
 void ApplicationCacheHost::dispose()
 {
-    // FIXME: Oilpan: remove the dispose step when the owning DocumentLoader
-    // becomes a garbage collected object. Until that time, have the
-    // DocumentLoader dispose and disable this ApplicationCacheHost when
-    // it is finalized. Releasing the WebApplicationCacheHost is needed
-    // to prevent further embedder notifications, which risk accessing an
+#if !ENABLE(OILPAN)

[haraken, 2015/06/22 01:57:58]

Can we move the #if !ENABLE(OILPAN) to the entire dispose() method?

[sof, 2015/06/22 06:49:18]

Renamed as detachFromDocumentLoader() instead (cf. detachFromFrame() naming on
DocumentLoader), and have it be called always (from detachFromFrame().)

+    // The owning DocumentLoader must explicitly dispose of and
+    // disable this ApplicationCacheHost when it is finalized.
+    // Releasing the WebApplicationCacheHost is needed to prevent
+    // further embedder notifications, which risk accessing an
     // invalid DocumentLoader.
     setApplicationCache(0);
     m_host.clear();
     m_documentLoader = nullptr;

[haraken, 2015/06/22 01:57:59]

Now in non-oilpan, both DocumentLoader and ApplicationCacheHost are on the heap.
So it seems unsafe (and not needed) to clear
ApplicationCacheHost::m_documentLoader in dispose() which is called in
DocumentLoader's destructor.

Maybe can we entirely remove the dispose() method from both oilpan and
non-oilpan?

[sof, 2015/06/22 05:03:46]

That will not work. The Persistent<> on DocumentLoader will keep the Host alive
for another GC iteration. In time for the host to be called and a freed
m_documentLoader to be accessed.

[haraken, 2015/06/22 05:28:55]

I'm not sure but maybe can we clear the Persistent<> just after calling
ApplicationCacheHost::dispose()?

+#endif
 }
 
 void ApplicationCacheHost::notifyApplicationCache(EventID id, int progressTotal, int progressDone, WebApplicationCacheHost::ErrorReason errorReason, const String& errorURL, int errorStatus, const String& errorMessage)
@@ -216,7 +217,7 @@ void ApplicationCacheHost::fillResourceList(ResourceInfoList* resources)
 
 void ApplicationCacheHost::stopDeferringEvents()
 {
-    RefPtr<DocumentLoader> protect(documentLoader());
+    RefPtrWillBeRawPtr<DocumentLoader> protect(documentLoader());
     for (unsigned i = 0; i < m_deferredEvents.size(); ++i) {
         const DeferredEvent& deferred = m_deferredEvents[i];
         dispatchDOMEvent(deferred.eventID, deferred.progressTotal, deferred.progressDone, deferred.errorReason, deferred.errorURL, deferred.errorStatus, deferred.errorMessage);
@@ -227,17 +228,18 @@ void ApplicationCacheHost::stopDeferringEvents()
 
 void ApplicationCacheHost::dispatchDOMEvent(EventID id, int progressTotal, int progressDone, WebApplicationCacheHost::ErrorReason errorReason, const String& errorURL, int errorStatus, const String& errorMessage)
 {
-    if (m_domApplicationCache) {
-        const AtomicString& eventType = ApplicationCache::toEventType(id);
-        RefPtrWillBeRawPtr<Event> event = nullptr;
-        if (id == PROGRESS_EVENT)
-            event = ProgressEvent::create(eventType, true, progressDone, progressTotal);
-        else if (id == ERROR_EVENT)
-            event = ApplicationCacheErrorEvent::create(errorReason, errorURL, errorStatus, errorMessage);
-        else
-            event = Event::create(eventType);
-        m_domApplicationCache->dispatchEvent(event, ASSERT_NO_EXCEPTION);
-    }
+    if (!m_domApplicationCache)
+        return;
+
+    const AtomicString& eventType = ApplicationCache::toEventType(id);
+    RefPtrWillBeRawPtr<Event> event = nullptr;
+    if (id == PROGRESS_EVENT)
+        event = ProgressEvent::create(eventType, true, progressDone, progressTotal);
+    else if (id == ERROR_EVENT)
+        event = ApplicationCacheErrorEvent::create(errorReason, errorURL, errorStatus, errorMessage);
+    else
+        event = Event::create(eventType);
+    m_domApplicationCache->dispatchEvent(event, ASSERT_NO_EXCEPTION);
 }
 
 ApplicationCacheHost::Status ApplicationCacheHost::status() const
@@ -293,6 +295,7 @@ void ApplicationCacheHost::notifyErrorEventListener(WebApplicationCacheHost::Err
 DEFINE_TRACE(ApplicationCacheHost)
 {
     visitor->trace(m_domApplicationCache);
+    visitor->trace(m_documentLoader);
 }
 
 } // namespace blink