Oilpan: enable appcache + move DocumentLoader to the heap.

Source/core/loader/appcache/ApplicationCacheHost.cpp

 /*
  * Copyright (C) 2009 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 148 matching lines @@
 }
 
 void ApplicationCacheHost::setApplicationCache(ApplicationCache* domApplicationCache)
 {
     ASSERT(!m_domApplicationCache || !domApplicationCache);
     m_domApplicationCache = domApplicationCache;
 }
 
 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)
 {
     if (id != PROGRESS_EVENT)
         InspectorInstrumentation::updateApplicationCacheStatus(m_documentLoader->frame());
 
     if (m_defersEvents) {
         // Event dispatching is deferred until document.onload has fired.
         m_deferredEvents.append(DeferredEvent(id, progressTotal, progressDone, errorReason, errorURL, errorStatus, errorMessage));
@@ skipping 21 matching lines @@
     m_host->getResourceList(&webResources);
     for (size_t i = 0; i < webResources.size(); ++i) {
         resources->append(ResourceInfo(
             webResources[i].url, webResources[i].isMaster, webResources[i].isManifest, webResources[i].isFallback,
             webResources[i].isForeign, webResources[i].isExplicit, webResources[i].size));
     }
 }
 
 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);
     }
     m_deferredEvents.clear();
     m_defersEvents = false;
 }
 
 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
 {
     return m_host ? static_cast<Status>(m_host->status()) : UNCACHED;
 }
 
 bool ApplicationCacheHost::update()
 {
     return m_host ? m_host->startUpdate() : false;
@@ skipping 35 matching lines @@
 }
 
 void ApplicationCacheHost::notifyErrorEventListener(WebApplicationCacheHost::ErrorReason reason, const WebURL& url, int status, const WebString& message)
 {
     notifyApplicationCache(ERROR_EVENT, 0, 0, reason, url.string(), status, message);
 }
 
 DEFINE_TRACE(ApplicationCacheHost)
 {
     visitor->trace(m_domApplicationCache);
+    visitor->trace(m_documentLoader);
 }
 
 } // namespace blink