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

Source/core/loader/ImageLoader.h

Index: Source/core/loader/ImageLoader.h
diff --git a/Source/core/loader/ImageLoader.h b/Source/core/loader/ImageLoader.h
index 1e0cdc426af7bc28729f3a8821b069c885551461..9ab71e6b77e164952294926d71122f5ff50858a4 100644
--- a/Source/core/loader/ImageLoader.h
+++ b/Source/core/loader/ImageLoader.h
@@ -55,10 +55,11 @@ class RenderImageResource;
 template<typename T> class EventSender;
 typedef EventSender<ImageLoader> ImageEventSender;
 
-class ImageLoader : public ImageResourceClient {
+class ImageLoader : public NoBaseWillBeGarbageCollectedFinalized<ImageLoader>, public ImageResourceClient {

[zerny-chromium, 2014/06/10 12:54:28]

Nit: FINAL

 public:
     explicit ImageLoader(Element*);
     virtual ~ImageLoader();
+    void trace(Visitor*);
 
     enum LoadType {
         LoadNormally,
@@ -133,8 +134,12 @@ private:
 
     void willRemoveClient(ImageLoaderClient&);
 
-    Element* m_element;
+    RawPtrWillBeMember<Element> m_element;
     ResourcePtr<ImageResource> m_image;
+    // FIXME: Oilpan: We might be able to remove this Persistent hack when
+    // ImageResourceClient is traceable.
+    GC_PLUGIN_IGNORE("http://crbug.com/353083")

[Mads Ager (chromium), 2014/06/11 06:38:41]

I think this persistent should use a separate bug number and an explanation that
it is here to keep the ImageLoader alive when a pending timer is the only
reference to the imageloader and its element. This is the bug number for
allowing the persistent to ensure that weak processing takes place for a
collection also in the GC round where the containing object is unreachable.

[tkent, 2014/06/11 06:58:28]

ok, will file new bug.

+    RefPtrWillBePersistent<Element> m_keepAlive;

[Mads Ager (chromium), 2014/06/10 12:34:33]

I don't think this persistent is doing anything in the oilpan build? m_keepAlive
is always either 0 or equal to m_element. Since m_element is already traced if
this object is alive it doesn't add anything to have the persistent here. The
persistent is holding something alive that is always alive anyway when this
object is alive?

[haraken, 2014/06/10 12:47:59]

Good point. I agree that we can remove this Persistent.

[zerny-chromium, 2014/06/10 12:54:28]

Yes. But this does point at a behavior change. The behavior semi-preserving
change is that m_element is a RawPtrWillBeWeakMember and the m_keepAlive is a
RefPtrWillBeMember. I can't really see if the m_elementIsProtected implies that
the element must be allowed to die or if it is OK to strongly trace m_element
regardless of m_elementIsProtected.

[haraken, 2014/06/10 12:58:11]

You're right, but as far as I see, m_element is a strong, back pointer to the
Element. (For example, we don't have any code that clears m_element in
non-oilpan builds.) So I think it's safe to use a Member in oilpan and thus
remove m_keepAlive.

[tkent, 2014/06/10 23:56:57]

Thank you for comments.

If we don't have m_keepAlive and an ImageLoader and its owner Element are
unreachable from GC roots, they are swept.  m_keepAlive prevents it because it
makes them reachable.

[Mads Ager (chromium), 2014/06/11 06:35:14]

OK, I see. This is needed because the ImageLoader is reachable from a scheduled
timer but oilpan does not know about that pointer. Maybe we can update the
comment or bug report with that? We might want to consider implementing support
for HeapTimer which keeps its containing object alive when it is scheduled. That
would make sure that the containing object could never disappear while there is
a pending timer without relying on hacks like these.

[haraken, 2014/06/11 06:45:03]

Just a drive-by comment not related to the essence of this CL: I considered
introducing HeapTimer, but I'm afraid that it won't solve our issue.

class ImageLoader : public GarbageCollected<ImageLoader> {
  HeapTimer<ImageLoader> m_timer;  // This won't keep the ImageLoader alive, so
this won't solve our issue.
};

class ImageLoader : public GarbageCollected<ImageLoader> {
  PersistentHeapTimer<ImageLoader> m_timer;  // This will solve our issue, but
we cannot use this programming pattern in general. In most cases, we don't want
to have the timer to keep the object alive. Instead we want to cancel the timer
when the object is destructed.
};

Given the above, the current approach of using m_keepAlive seems reasonable to
me.

[tkent, 2014/06/11 06:58:28]

No, Timer is not related to this issue. 
As the code comment already says, I think this issue will be resolved when a
loading ImageResource has strong references to ImageResourceClient objects.

[Mads Ager (chromium), 2014/06/11 07:09:15]

Thank you Kent. I misunderstood the code. The Timer is only used when we no
longer need to keep the element alive. This persistent is here so that the
ImageLoader can keep an unreachable ImageElement and itself alive until the
image element has been loaded. That seems a little strange, but it is what other
browsers do as well.

Sorry for all the noise on this code review. I'll shut up now. ;-)

 #if ENABLE(OILPAN)
     class ImageLoaderClientRemover {
     public:
@@ -146,6 +151,9 @@ private:
         ImageLoaderClient& m_client;
     };
     friend class ImageLoaderClientRemover;
+    // Oilpan: This ImageLoader object must outlive its clients because they
+    // need to call ImageLoader::willRemoveClient before they die.
+    GC_PLUGIN_IGNORE("http://crbug.com/353083")

[Mads Ager (chromium), 2014/06/10 12:34:33]

Why does not map have to be a PersistentHeapHashMap? Weak processing only
happens if this object is alive anyway, so it should make a difference whether
or not this hash map is a PersistentHeapHashMap or just a HeapHashMap that is
traced. In both situations, the weak processing will only happen if this object
ImageLoader object is alive.

[Mads Ager (chromium), 2014/06/10 12:37:45]

Sorry, that should be: why does /the/ map have to be ...

[Mads Ager (chromium), 2014/06/10 12:44:51]

Oh, is that the point here? This persistent makes sure that the map gets traced
in the GC where this object dies which means that we get an extra round of weak
processing for the map and can therefore do the cleanup during weak processing
and not during destruction. Auch, that is complicated. :(

[haraken, 2014/06/10 12:47:59]

The problem is that the weak processing does not happen if the ImageLoader and
the ImageLoaderClient die together, but the weak processing must happen even in
that case to keep the consistency of the memory cache.

That's why we need to have ugly code like this.

Another suggestion that doesn't use persistent maps is written in comment #17.

[Mads Ager (chromium), 2014/06/10 12:54:39]

The weak processing doesn't happen if it is not persistent and we cannot let the
destructor kill the ImageLoaderClientRemover because it touches the ImageLoader
itself. OK, got it. We did this once before in database code; I think Kent wrote
that earlier in this review as well and I just didn't get it. :)

OK, I understand this persistent. Maybe we can extend the comment to say that
this persistent is here to make sure that all ImageLoaderClientRemovers are
destructed in weak processing even when the ImageLoader is unreachable? It would
be great to get rid of it long term, and there is a bug report to track that. 

     PersistentHeapHashMap<WeakMember<ImageLoaderClient>, OwnPtr<ImageLoaderClientRemover> > m_clients;
 #else
     HashSet<ImageLoaderClient*> m_clients;