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

Source/core/loader/ImageLoader.h

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2004, 2009 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 37 matching lines @@
     ImageLoaderClient() { }
 };
 
 class Element;
 class ImageLoader;
 class RenderImageResource;
 
 template<typename T> class EventSender;
 typedef EventSender<ImageLoader> ImageEventSender;
 
-class ImageLoader : public ImageResourceClient {
+class ImageLoader : public NoBaseWillBeGarbageCollectedFinalized<ImageLoader>, public ImageResourceClient {
 public:
     explicit ImageLoader(Element*);
     virtual ~ImageLoader();
+    void trace(Visitor*);
 
     enum LoadType {
         LoadNormally,
         ForceLoadImmediately
     };
 
     // This function should be called when the element is attached to a document; starts
     // loading if a load hasn't already been started.
     void updateFromElement(LoadType = LoadNormally);
 
@@ skipping 54 matching lines @@
     void timerFired(Timer<ImageLoader>*);
 
     KURL imageURL() const;
 
     // Used to determine whether to immediately initiate the load
     // or to schedule a microtask.
     bool shouldLoadImmediately(const KURL&) const;
 
     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")
+    RefPtrWillBePersistent<Element> m_keepAlive;
 #if ENABLE(OILPAN)
     class ImageLoaderClientRemover {
     public:
         ImageLoaderClientRemover(ImageLoader& loader, ImageLoaderClient& client) : m_loader(loader), m_client(client) { }
         ~ImageLoaderClientRemover();
 
     private:
         ImageLoader& m_loader;

[haraken, 2014/06/10 04:15:13]

You might want to change this to a Member. It will clarify the relationship that
ImageLoader must outlive its clients.

         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")
     PersistentHeapHashMap<WeakMember<ImageLoaderClient>, OwnPtr<ImageLoaderClientRemover> > m_clients;

[haraken, 2014/06/10 04:15:13]

I'm curious why you need to make this map a persistent. Even if you don't make
this map a persistent, it will be guaranteed that the ImageLoader is alive when
~ImageLoaderClientRemover touches the ImageLoader.

(1) An ImageLoaderClient and an ImageLoader become unreachable.
(2) A GC is triggered.
(3) The ImageLoaderClient is removed from the map by oilpan's weak processing.
~ImageLoaderClientRemover is called. It can touche the ImageLoader, because the
ImageLoader is not yet swept.
(4) The ImageLoader is swept.

[tkent, 2014/06/10 05:32:46]

Though I don't remember the precise mechanism, we need to make this
Persistent-in-GarbageCollectedFinalized<> in this case.  Making this Member<>
causes crash in ImageBitmapTest.ImageBitmapLiveResourcePriority.
Please see modules/webdatabse/DatabaseSync.h. 
modules/webdatabase/DatabseContext.h has non-persistent weak HashMap.  It's ok
because DatabaseCloser doesn't touch the DatabaseContext.

I guess removing weak HashMap entries is done in the weak processing, but
finalizing weak HashMap entries is done in the sweep stage?

[haraken, 2014/06/10 05:54:10]

Given that ImageLoaderClientRemover is off heap, ~ImageLoaderClientRemover
should be called promptly when the OwnPtr is removed from the hash map in the
weak processing phase (which happens before the sweeping phase).

Help me understand: Does the ImageLoaderClient have a Member back to the
ImageLoader? (It was not clear from a quick grep.) If the ImageLoaderClient has
a Member back to the ImageLoader, this change makes sense because the Member
guarantees that the ImageLoader is destructed in the next GC of the GC that
destructed the ImageLoaderClient.

Would it be possible to make this hash map a static hash map? Unlike 
DatabaseContext, I don't see any threading issue here.

 #else
     HashSet<ImageLoaderClient*> m_clients;
 #endif
     Timer<ImageLoader> m_derefElementTimer;
     AtomicString m_failedLoadURL;
     WeakPtr<Task> m_pendingTask; // owned by Microtask
     OwnPtr<IncrementLoadEventDelayCount> m_delayLoad;
     bool m_hasPendingLoadEvent : 1;
     bool m_hasPendingErrorEvent : 1;
     bool m_imageComplete : 1;
     bool m_loadManually : 1;
     bool m_elementIsProtected : 1;
     unsigned m_highPriorityClientCount;
 };
 
 }
 
 #endif