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

Source/core/loader/ImageLoader.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2009, 2010 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
@@ skipping 104 matching lines @@
     if (m_image)
         m_image->removeClient(this);
 
     ASSERT(m_hasPendingLoadEvent || !loadEventSender().hasPendingEvents(this));
     if (m_hasPendingLoadEvent)
         loadEventSender().cancelEvent(this);
 
     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)
 {
     setImageWithoutConsideringPendingLoadEvent(newImage);
 
     // Only consider updating the protection ref-count of the Element immediately before returning
     // from this function as doing so might result in the destruction of this ImageLoader.
     updatedHasPendingEvent();
 }
@@ skipping 266 matching lines @@
     // If an Element that does image loading is removed from the DOM the load/error event for the image is still observable.
     // As long as the ImageLoader is actively loading, the Element itself needs to be ref'ed to keep it from being
     // destroyed by DOM manipulation or garbage collection.
     // If such an Element wishes for the load to stop when removed from the DOM it needs to stop the ImageLoader explicitly.
     bool wasProtected = m_elementIsProtected;
     m_elementIsProtected = m_hasPendingLoadEvent || m_hasPendingErrorEvent;
     if (wasProtected == m_elementIsProtected)
         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);
     }
 }
 
 void ImageLoader::timerFired(Timer<ImageLoader>*)
 {
+#if ENABLE(OILPAN)
+    m_keepAlive.clear();
+#else
     m_element->deref();
+#endif
 }
 
 void ImageLoader::dispatchPendingEvent(ImageEventSender* eventSender)
 {
     ASSERT(eventSender == &loadEventSender() || eventSender == &errorEventSender());
     const AtomicString& eventType = eventSender->eventType();
     if (eventType == EventTypeNames::load)
         dispatchPendingLoadEvent();
     if (eventType == EventTypeNames::error)
         dispatchPendingErrorEvent();
@@ skipping 100 matching lines @@
 }
 
 #if ENABLE(OILPAN)
 ImageLoader::ImageLoaderClientRemover::~ImageLoaderClientRemover()
 {
     m_loader.willRemoveClient(m_client);
 }
 #endif
 
 }