Oilpan: Prepare to move ImageLoader and its subclasses to Oilpan heap.

Source/core/loader/ImageLoader.cpp

Index: Source/core/loader/ImageLoader.cpp
diff --git a/Source/core/loader/ImageLoader.cpp b/Source/core/loader/ImageLoader.cpp
index f3f4e3ff57b6f34241bfc4592b3c115084178811..d47d0f822d7d599797e1c91bdfcfa911b2c847d1 100644
--- a/Source/core/loader/ImageLoader.cpp
+++ b/Source/core/loader/ImageLoader.cpp
@@ -122,11 +122,18 @@ ImageLoader::~ImageLoader()
     ASSERT(m_hasPendingErrorEvent || !errorEventSender().hasPendingEvents(this));
     if (m_hasPendingErrorEvent)
         errorEventSender().cancelEvent(this);
-
+#if !ENABLE(OILPAN)
     // If the ImageLoader is being destroyed but it is still protecting its image-loading Element,
     // remove that protection here.
     if (m_elementIsProtected)
         m_element->deref();

[haraken, 2014/06/09 11:12:33]

It looks strange that we don't have corresponding code to this. (See my comment
below.)

[tkent, 2014/06/10 01:43:09]

The corresponding code is the destructor of m_keepAlive.

+#endif
+}
+
+void ImageLoader::trace(Visitor* visitor)
+{
+    visitor->trace(m_element);
+    visitor->trace(m_clients);

[tkent, 2014/06/10 01:43:09]

Will remove this.

 }
 
 void ImageLoader::setImage(ImageResource* newImage)
@@ -413,10 +420,15 @@ void ImageLoader::updatedHasPendingEvent()
         return;
 
     if (m_elementIsProtected) {
-        if (m_derefElementTimer.isActive())
+        if (m_derefElementTimer.isActive()) {
             m_derefElementTimer.stop();
-        else
+        } else {
+#if ENABLE(OILPAN)
+            m_keepAlive = this;

[haraken, 2014/06/09 11:12:34]

I'm afraid this will cause memory leak. If timerFired() is not triggered, this
ImageLoader will kept alive forever.

I think a right fix would be:

- Call 'm_keepAlive = m_element' here.
- Call 'm_keepAlive.clear()' in ImageLoader's destructor.

[tkent, 2014/06/10 01:43:09]

I think timerFired() is always triggered. m_keepAlive points this ImageLoader
until timerFired is called.

[haraken, 2014/06/10 01:59:05]

It's not clear to me if timerFired() is always triggered. If timerFired() were
always triggered, we wouldn't need the following code:

  if (m_elementIsProtected)
    m_element->deref();

in ~ImageLoader() in non-oilpan builds. The comment at line 126 also implies
that it's possible that timerFired() is not fired and thus there is a case where
we need to manually deref the element.

If we're not sure, I think it's safer to protect the element than the
ImageLoader. Also, in terms of semantics, I think what we want to protect would
be not the ImageLoader but the element.

[tkent, 2014/06/10 02:37:59]

If enable_oilpan=0 and the owner element releases the ownership of the
ImageLoader before the element dies, it's possible to call ~ImageLoader without
timerFired. However, we have no such code at this moment and the owner element
and the ImageLoader always die together.

We might have such code in the future. If so, Persistent<ImageLoader> actually
has inconsistent semantics with enable_oilpan=0.  I have change it to
Persistent<Element> in Patch Set 2.

+#else
             m_element->ref();
+#endif
+        }
     } else {
         ASSERT(!m_derefElementTimer.isActive());
         m_derefElementTimer.startOneShot(0, FROM_HERE);
@@ -425,7 +437,11 @@ void ImageLoader::updatedHasPendingEvent()
 
 void ImageLoader::timerFired(Timer<ImageLoader>*)
 {
+#if ENABLE(OILPAN)
+    m_keepAlive.clear();
+#else
     m_element->deref();
+#endif
 }
 
 void ImageLoader::dispatchPendingEvent(ImageEventSender* eventSender)