[Not committed] Make image load completion async and remove EventSender from ImageLoader

third_party/WebKit/Source/core/loader/ImageLoader.cpp

Index: third_party/WebKit/Source/core/loader/ImageLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/ImageLoader.cpp b/third_party/WebKit/Source/core/loader/ImageLoader.cpp
index 4c1b72a04de8ff47eb4068e57d879862ff7f487e..0f4df8fbfa6a9c495f37403e93a04b05906124c3 100644
--- a/third_party/WebKit/Source/core/loader/ImageLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/ImageLoader.cpp
@@ -30,7 +30,6 @@
 #include "core/dom/Element.h"
 #include "core/dom/IncrementLoadEventDelayCount.h"
 #include "core/events/Event.h"
-#include "core/events/EventSender.h"
 #include "core/fetch/FetchRequest.h"
 #include "core/fetch/MemoryCache.h"
 #include "core/fetch/ResourceFetcher.h"
@@ -48,22 +47,13 @@
 #include "platform/Logging.h"
 #include "platform/weborigin/SecurityOrigin.h"
 #include "platform/weborigin/SecurityPolicy.h"
+#include "public/platform/Platform.h"
+#include "public/platform/WebTaskRunner.h"
+#include "public/platform/WebTraceLocation.h"
 #include "public/platform/WebURLRequest.h"
 
 namespace blink {
 
-static ImageEventSender& loadEventSender()
-{
-    DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<ImageEventSender>, sender, (ImageEventSender::create(EventTypeNames::load)));
-    return *sender;
-}
-
-static ImageEventSender& errorEventSender()
-{
-    DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<ImageEventSender>, sender, (ImageEventSender::create(EventTypeNames::error)));
-    return *sender;
-}
-
 static inline bool pageIsBeingDismissed(Document* document)
 {
     return document->pageDismissalEventBeingDispatched() != Document::NoDismissal;
@@ -149,8 +139,9 @@ private:
 ImageLoader::ImageLoader(Element* element)
     : m_element(element)
     , m_derefElementTimer(this, &ImageLoader::timerFired)
+    , m_finishTask(CancellableTaskFactory::create(this, &ImageLoader::finish))
+    , m_dispatchErrorEventTask(CancellableTaskFactory::create(this, &ImageLoader::dispatchErrorEvent))
     , m_hasPendingLoadEvent(false)
-    , m_hasPendingErrorEvent(false)
     , m_imageComplete(true)
     , m_loadingImageDocument(false)
     , m_elementIsProtected(false)
@@ -169,14 +160,29 @@ ImageLoader::~ImageLoader()
 #endif
 }
 
+void ImageLoader::cancelPendingFinish()
+{
+    m_finishTask->cancel();
+    m_loadDelayCounterForFinish.clear();
+    m_hasPendingLoadEvent = false;
+}
+
+void ImageLoader::cancelPendingErrorEvent()
+{
+    if (m_loadDelayCounterForError) {

[Nate Chapin, 2016/03/29 19:04:13]

Why does this have an if() wrapper, but cancelPendingFinish() doesn't?

[hiroshige, 2017/05/18 22:13:12]

|m_loadDelayCounterForError| is non-null only between scheduleErrorEvent() and
its actual dispatching (i.e. while |m_dispatchErrorEventTask| is on flight), so
we can put Line 173-174 into the if statement. I think we can also remove the if
condition at Line 172 safely.
This is inherited e.g. from L224-227 on the left.

As for cancelPendingFinish(), if we place corresponding if statements, then it
would be:
if (m_loadDelayCounterForFinish) {
    // We have to always cancel in-flight async finish() calls, because
    // we always post |m_finishTask| in notifyFinished(), regardless
    // of |m_hasPendingLoadEvent|.
    // |m_loadDelayCounterForFinish| is true only if |m_finishTask| is
    // in-flight.
    m_finishTask->cancel();
    m_loadDelayCounterForFinish.clear();
}
if (m_hasPendingLoadEvent) {
    // This if statement is inherited from e.g. Line 220 on the left.
    // There was |loadEventSender().cancelEvent(this);| on the left, but
    // we no longer need to cancel in-flight load event, because load event
    // is now dispatched synchronously in finish().
    // I removed this if-statement because it doesn't look meaningful now.
    m_hasPendingLoadEvent = false;
}

+        m_dispatchErrorEventTask->cancel();
+        m_loadDelayCounterForError.clear();
+    }
+}
+
 void ImageLoader::dispose()
 {
-    WTF_LOG(Timers, "~ImageLoader %p; m_hasPendingLoadEvent=%d, m_hasPendingErrorEvent=%d",
-        this, m_hasPendingLoadEvent, m_hasPendingErrorEvent);
+    WTF_LOG(Timers, "~ImageLoader %p; m_hasPendingLoadEvent=%d, hasPendingErrorEvent=%d",
+        this, m_hasPendingLoadEvent, hasPendingError());
 
 #if !ENABLE(OILPAN)
-    if (m_pendingTask)
-        m_pendingTask->clearLoader();
+    if (m_pendingMicrotask)
+        m_pendingMicrotask->clearLoader();
 #endif
 
     if (m_image) {
@@ -184,16 +190,8 @@ void ImageLoader::dispose()
         m_image->removeObserver(this);
         m_image = nullptr;
     }
-
-#if !ENABLE(OILPAN)
-    ASSERT(m_hasPendingLoadEvent || !loadEventSender().hasPendingEvents(this));
-    if (m_hasPendingLoadEvent)
-        loadEventSender().cancelEvent(this);
-
-    ASSERT(m_hasPendingErrorEvent || !errorEventSender().hasPendingEvents(this));
-    if (m_hasPendingErrorEvent)
-        errorEventSender().cancelEvent(this);
-#endif
+    cancelPendingFinish();
+    cancelPendingErrorEvent();
 }
 
 DEFINE_TRACE(ImageLoader)
@@ -217,14 +215,8 @@ void ImageLoader::setImageWithoutConsideringPendingLoadEvent(ImageResource* newI
     ImageResource* oldImage = m_image.get();
     if (newImage != oldImage) {
         m_image = newImage;
-        if (m_hasPendingLoadEvent) {
-            loadEventSender().cancelEvent(this);
-            m_hasPendingLoadEvent = false;
-        }
-        if (m_hasPendingErrorEvent) {
-            errorEventSender().cancelEvent(this);
-            m_hasPendingErrorEvent = false;
-        }
+        cancelPendingFinish();
+        cancelPendingErrorEvent();
         m_imageComplete = true;
         if (newImage) {
             newImage->addClient(this);
@@ -253,10 +245,10 @@ static void configureRequest(FetchRequest& request, ImageLoader::BypassMainWorld
         request.setResourceWidth(toHTMLImageElement(element).getResourceWidth());
 }
 
-inline void ImageLoader::dispatchErrorEvent()
+inline void ImageLoader::scheduleErrorEvent()
 {
-    m_hasPendingErrorEvent = true;
-    errorEventSender().dispatchEventSoon(this);
+    m_loadDelayCounterForError = IncrementLoadEventDelayCount::create(m_element->document());
+    Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, m_dispatchErrorEventTask->cancelAndCreate());
 }
 
 inline void ImageLoader::crossSiteOrCSPViolationOccurred(AtomicString imageSourceURL)
@@ -272,9 +264,9 @@ inline void ImageLoader::clearFailedLoadURL()
 inline void ImageLoader::enqueueImageLoadingMicroTask(UpdateFromElementBehavior updateBehavior, ReferrerPolicy referrerPolicy)
 {
     OwnPtr<Task> task = Task::create(this, updateBehavior, referrerPolicy);
-    m_pendingTask = task->createWeakPtr();
+    m_pendingMicrotask = task->createWeakPtr();
     Microtask::enqueueMicrotask(task.release());
-    m_loadDelayCounter = IncrementLoadEventDelayCount::create(m_element->document());
+    m_loadDelayCounterForMicrotask = IncrementLoadEventDelayCount::create(m_element->document());
 }
 
 void ImageLoader::doUpdateFromElement(BypassMainWorldBehavior bypassBehavior, UpdateFromElementBehavior updateBehavior, ReferrerPolicy referrerPolicy)
@@ -286,11 +278,11 @@ void ImageLoader::doUpdateFromElement(BypassMainWorldBehavior bypassBehavior, Up
     //
     // We don't need to call clearLoader here: Either we were called from the
     // task, or our caller updateFromElement cleared the task's loader (and set
-    // m_pendingTask to null).
-    m_pendingTask.clear();
+    // m_pendingMicrotask to null).
+    m_pendingMicrotask.clear();
     // Make sure to only decrement the count when we exit this function
     OwnPtr<IncrementLoadEventDelayCount> loadDelayCounter;
-    loadDelayCounter.swap(m_loadDelayCounter);
+    loadDelayCounter.swap(m_loadDelayCounterForMicrotask);
 
     Document& document = m_element->document();
     if (!document.isActive())
@@ -335,14 +327,14 @@ void ImageLoader::doUpdateFromElement(BypassMainWorldBehavior bypassBehavior, Up
 
         if (!newImage && !pageIsBeingDismissed(&document)) {
             crossSiteOrCSPViolationOccurred(imageSourceURL);
-            dispatchErrorEvent();
+            scheduleErrorEvent();
         } else {
             clearFailedLoadURL();
         }
     } else {
         if (!imageSourceURL.isNull()) {
             // Fire an error event if the url string is not empty, but the KURL is.
-            dispatchErrorEvent();
+            scheduleErrorEvent();
         }
         noImageResourceToLoad();
     }
@@ -351,19 +343,14 @@ void ImageLoader::doUpdateFromElement(BypassMainWorldBehavior bypassBehavior, Up
     if (updateBehavior == UpdateSizeChanged && m_element->layoutObject() && m_element->layoutObject()->isImage() && newImage == oldImage) {
         toLayoutImage(m_element->layoutObject())->intrinsicSizeChanged();
     } else {
-        if (m_hasPendingLoadEvent) {
-            loadEventSender().cancelEvent(this);
-            m_hasPendingLoadEvent = false;
-        }
+        cancelPendingFinish();
 
         // Cancel error events that belong to the previous load, which is now cancelled by changing the src attribute.
-        // If newImage is null and m_hasPendingErrorEvent is true, we know the error event has been just posted by
+        // If newImage is null and hasPendingError() is true, we know the error event has been just posted by
         // this load and we should not cancel the event.
         // FIXME: If both previous load and this one got blocked with an error, we can receive one error event instead of two.
-        if (m_hasPendingErrorEvent && newImage) {
-            errorEventSender().cancelEvent(this);
-            m_hasPendingErrorEvent = false;
-        }
+        if (newImage)
+            cancelPendingErrorEvent();
 
         m_image = newImage;
         m_hasPendingLoadEvent = newImage;
@@ -403,9 +390,9 @@ void ImageLoader::updateFromElement(UpdateFromElementBehavior updateBehavior, Re
 
     // If we have a pending task, we have to clear it -- either we're
     // now loading immediately, or we need to reset the task's state.
-    if (m_pendingTask) {
-        m_pendingTask->clearLoader();
-        m_pendingTask.clear();
+    if (m_pendingMicrotask) {
+        m_pendingMicrotask->clearLoader();
+        m_pendingMicrotask.clear();
     }
 
     KURL url = imageSourceToKURL(imageSourceURL);
@@ -422,6 +409,8 @@ void ImageLoader::updateFromElement(UpdateFromElementBehavior updateBehavior, Re
             image->removeObserver(this);
         }
         m_image = nullptr;
+        cancelPendingFinish();
+        cancelPendingErrorEvent();
     }
 
     // Don't load images for inactive documents. We don't want to slow down the
@@ -471,45 +460,61 @@ void ImageLoader::notifyFinished(Resource* resource)
     ASSERT(m_failedLoadURL.isEmpty());
     ASSERT(resource == m_image.get());
 
+    scheduleFinish();
+}
+
+void ImageLoader::scheduleFinish()

[Nate Chapin, 2016/03/29 19:04:13]

scheduleFinish() is only called once. Inline it in notifyFinished()?

[hiroshige, 2017/05/18 22:13:14]

Done.

+{
+    m_loadDelayCounterForFinish = IncrementLoadEventDelayCount::create(m_element->document());
+    Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, m_finishTask->cancelAndCreate());
+}
+
+void ImageLoader::finish()
+{
     m_imageComplete = true;
+    ImageResource* cachedImage = m_image.get();

[Nate Chapin, 2016/03/29 19:04:13]

cachedImage->imageResource? CachedImage is what ImageResource was called until
about 2013, and it's confusing to have a new variable with that name.

[hiroshige, 2017/05/18 22:13:14]

Done.

+    ASSERT(cachedImage);

[Nate Chapin, 2016/03/29 19:04:13]

The theory is that the calls to cancelPendingFinish() handle all cases where
m_image might be cleared or modified while waiting for the task to happen?

[hiroshige, 2017/05/18 22:13:13]

Yes. I inserted cancelPendingFinish() at Line 412 to make this sure.

I thought ImageLoader can/should also assume |m_image| non-null here, because
- HTMLImageLoader::notifyFinished() in the current code base assumes |m_image|
to be non-null (no null checks)
- ImageLoader::notifyFinished() contains |ASSERT(resource == m_image.get());|,
which implies |m_image| is non-null.

(ImageLoader::notifyFinished() contains null checks for |m_image| though. Can we
remove those null checks?)

 
-    // Update ImageAnimationPolicy for m_image.
-    if (m_image)
-        m_image->updateImageAnimationPolicy();
+    // Update ImageAnimationPolicy for |cachedImage|.
+    if (cachedImage)
+        cachedImage->updateImageAnimationPolicy();
 
     updateLayoutObject();
 
-    if (m_image && m_image->getImage() && m_image->getImage()->isSVGImage())
-        toSVGImage(m_image->getImage())->updateUseCounters(element()->document());
+    if (cachedImage && cachedImage->getImage() && cachedImage->getImage()->isSVGImage())
+        toSVGImage(cachedImage->getImage())->updateUseCounters(element()->document());
 
-    if (!m_hasPendingLoadEvent)
-        return;
+    finishInternal(cachedImage);
 
-    if (resource->errorOccurred()) {
-        loadEventSender().cancelEvent(this);
+    // Dispatches image's load/error event.
+    if (m_hasPendingLoadEvent) {
         m_hasPendingLoadEvent = false;
 
-        if (resource->resourceError().isAccessCheck())
-            crossSiteOrCSPViolationOccurred(AtomicString(resource->resourceError().failingURL()));
+        if (cachedImage->errorOccurred()) {
+            if (cachedImage->resourceError().isAccessCheck())
+                crossSiteOrCSPViolationOccurred(AtomicString(cachedImage->resourceError().failingURL()));
+
+            // The error event should not fire if the image data update is a result of environment change.
+            // https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element:the-img-element-55
+            if (!m_suppressErrorEvents)
+                dispatchErrorEventWithoutUpdatedHasPendingEvent();
+        } else if (!cachedImage->wasCanceled()) {
+            if (element()->document().frame())
+                dispatchLoadEvent();
+        }
+    }
 
-        // The error event should not fire if the image data update is a result of environment change.
-        // https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element:the-img-element-55
-        if (!m_suppressErrorEvents)
-            dispatchErrorEvent();
+    // Dispatches document's load event if applicable.
+    // Calling checkCompleted() and document load event synchronously here has
+    // no problem regarding reentrancy because ImageLoader::finish() is
+    // always executed as a CancellableTask and not called by others.
+    m_loadDelayCounterForFinish.clear();
+    if (element()->document().frame())
+        element()->document().frame()->loader().checkCompleted();
 
-        // 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();
-        return;
-    }
-    if (resource->wasCanceled()) {
-        m_hasPendingLoadEvent = false;
-        // 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();
-        return;
-    }
-    loadEventSender().dispatchEventSoon(this);
+    // 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();
 }
 
 LayoutImageResource* ImageLoader::layoutImageResource()
@@ -555,7 +560,7 @@ void ImageLoader::updatedHasPendingEvent()
     // 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;
+    m_elementIsProtected = m_hasPendingLoadEvent || hasPendingError();
     if (wasProtected == m_elementIsProtected)
         return;
 
@@ -575,40 +580,16 @@ void ImageLoader::timerFired(Timer<ImageLoader>*)
     m_keepAlive.clear();
 }
 
-void ImageLoader::dispatchPendingEvent(ImageEventSender* eventSender)
-{
-    WTF_LOG(Timers, "ImageLoader::dispatchPendingEvent %p", this);
-    ASSERT(eventSender == &loadEventSender() || eventSender == &errorEventSender());
-    const AtomicString& eventType = eventSender->eventType();
-    if (eventType == EventTypeNames::load)
-        dispatchPendingLoadEvent();
-    if (eventType == EventTypeNames::error)
-        dispatchPendingErrorEvent();
-}
-
-void ImageLoader::dispatchPendingLoadEvent()
+void ImageLoader::dispatchErrorEventWithoutUpdatedHasPendingEvent()
 {
-    if (!m_hasPendingLoadEvent)
-        return;
-    if (!m_image)
-        return;
-    m_hasPendingLoadEvent = false;
     if (element()->document().frame())
-        dispatchLoadEvent();
-
-    // 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();
+        element()->dispatchEvent(Event::create(EventTypeNames::error));
+    m_loadDelayCounterForError.clear();
 }
 
-void ImageLoader::dispatchPendingErrorEvent()
+void ImageLoader::dispatchErrorEvent()
 {
-    if (!m_hasPendingErrorEvent)
-        return;
-    m_hasPendingErrorEvent = false;
-
-    if (element()->document().frame())
-        element()->dispatchEvent(Event::create(EventTypeNames::error));
+    dispatchErrorEventWithoutUpdatedHasPendingEvent();
 
     // 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.
@@ -624,20 +605,14 @@ bool ImageLoader::getImageAnimationPolicy(ImageAnimationPolicy& policy)
     return true;
 }
 
-void ImageLoader::dispatchPendingLoadEvents()
-{
-    loadEventSender().dispatchPendingEvents();
-}
-
-void ImageLoader::dispatchPendingErrorEvents()
-{
-    errorEventSender().dispatchPendingEvents();
-}
-
 void ImageLoader::elementDidMoveToNewDocument()
 {
-    if (m_loadDelayCounter)
-        m_loadDelayCounter->documentChanged(m_element->document());
+    if (m_loadDelayCounterForMicrotask)
+        m_loadDelayCounterForMicrotask->documentChanged(m_element->document());
+    if (m_loadDelayCounterForFinish)
+        m_loadDelayCounterForFinish->documentChanged(m_element->document());
+    if (m_loadDelayCounterForError)
+        m_loadDelayCounterForError->documentChanged(m_element->document());
     clearFailedLoadURL();
     setImage(0);
 }