Oilpan: avoid access to to-be-swept ImageLoader object.

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 62 matching lines @@
 class ImageLoader::Task : public blink::WebThread::Task {
 public:
     static PassOwnPtr<Task> create(ImageLoader* loader, UpdateFromElementBehavior updateBehavior)
     {
         return adoptPtr(new Task(loader, updateBehavior));
     }
 
     Task(ImageLoader* loader, UpdateFromElementBehavior updateBehavior)
         : m_loader(loader)
         , m_shouldBypassMainWorldCSP(shouldBypassMainWorldCSP(loader))
+        , m_updateBehavior(updateBehavior)

[kouhei (in TOK), 2015/02/13 00:25:58]

unintended change?

[sof, 2015/02/13 06:40:48]

It helps to be have the weak factory last, and keep that as a pattern through
the codebase. Which is what I tidied here.

[kouhei (in TOK), 2015/02/13 09:00:34]

sgtm. It might be helpful to mention about this change in CL description.

         , m_weakFactory(this)
-        , m_updateBehavior(updateBehavior)
     {
     }
 
     virtual void run() override
     {
         if (m_loader) {
+#if ENABLE(OILPAN)
+            if (Heap::willObjectBeLazilySwept(m_loader))
+                return;

[kouhei (in TOK), 2015/02/13 00:25:58]

Is this needed for all places where non-Oilpan object access Oilpan object via
raw ptr?
In particular, would this be needed for the below too?:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[haraken, 2015/02/13 01:41:59]

In most cases, no.

This hack is needed only when you cannot clear the raw pointer (for some reason)
before you start sweeping. Unless you have a strong reason, you should try hard
to clear the raw pointer before starting a sweep phase that collects the on-heap
object.

[sof, 2015/02/13 06:40:48]

I will go through the Microtasks and check if they're similarly vulnerable to
this lazy-sweep condition.

+#endif
             m_loader->doUpdateFromElement(m_shouldBypassMainWorldCSP, m_updateBehavior);
         }
     }
 
     void clearLoader()

[haraken, 2015/02/13 01:38:53]

Another possible fix would be to call clearLoader() in a pre-finalizer of
ImageLoader and thus make sure that m_loader is 0 before we start sweeping. What
do you think?

[sof, 2015/02/13 06:40:48]

Wouldn't that lead to more unavoidable GC work, work that almost always could
just as well be done in the finalizer? These task objects aren't entirely
uncommon.

This implements a weak reference from the task, in a way. An alternative is to
have that be strong (Persistent), but I'm not sure I can convince myself that
will hold on to objects for too long/longer than needed. Thoughts?

[haraken, 2015/02/13 09:20:20]

Whether we should use Heap::willObjectBeLazilySwept or prefinalizer depends on
how many objects are created. If it's too many, we should avoid using
prefinalizers (and use Heap::willObjectBeLazilySwept). Timers are in this
category. Otherwise, I'd prefer prefinalizers and explicit pointer clear.

If ImageLoader is created too many, I'm fine with Heap::willObjectBeLazilySwept.

[sof, 2015/02/13 09:38:46]

When time, I need to look into what I thought were surprisingly many
ImageLoaders that were kept alive for dom-modify.html (which is the repro'ing
test for this one, along with external allocations triggering GCs.) We do have
additional lifetime handling in place for ImageLoader with Oilpan.

     {
         m_loader = 0;
     }
 
     WeakPtr<Task> createWeakPtr()
     {
         return m_weakFactory.createWeakPtr();
     }
 
 private:
     ImageLoader* m_loader;
     BypassMainWorldBehavior m_shouldBypassMainWorldCSP;
+    UpdateFromElementBehavior m_updateBehavior;

[kouhei (in TOK), 2015/02/13 00:25:58]

Ditto.

     WeakPtrFactory<Task> m_weakFactory;
-    UpdateFromElementBehavior m_updateBehavior;
 };
 
 ImageLoader::ImageLoader(Element* element)
     : m_element(element)
     , m_image(0)
     , m_derefElementTimer(this, &ImageLoader::timerFired)
     , m_hasPendingLoadEvent(false)
     , m_hasPendingErrorEvent(false)
     , m_imageComplete(true)
     , m_loadingImageDocument(false)
@@ skipping 469 matching lines @@
 #endif
 }
 
 #if ENABLE(OILPAN)
 ImageLoader::ImageLoaderClientRemover::~ImageLoaderClientRemover()
 {
     m_loader.willRemoveClient(m_client);
 }
 #endif
 }