Changeset 87628 in webkit

Timestamp:

May 28, 2011 6:37:59 PM (13 years ago)

Author:

ap@apple.com

Message:

Reviewed by Geoff Garen.

REGRESSION (r85375): Load event is sometimes lost when multiple image elements use the same URL
​https://bugs.webkit.org/show_bug.cgi?id=61692
<rdar://problem/9488628>

Test: fast/dom/gc-image-element-2.html

Manually verified that tests from bug 59604 and from bug 40926 still pass.

The problem here was that HTMLImageElement::hasPendingActivity() could return false when
a load (or error) event was still expected to fire.

• loader/cache/CachedResource.cpp: (WebCore::CachedResource::setRequest):
• loader/cache/CachedResource.h: (WebCore::CachedResource::wasCanceled): (WebCore::CachedResource::errorOccurred): Track whether the load was canceled. We want to always notify clients of load outcome, as that's the only way they could make intelligent decisions.

• dom/ScriptElement.cpp: (WebCore::ScriptElement::execute): Cached resource clients now get a notifyFinished call on cancellation. Handle this case, where we don't need the execute the script, but also don't need to fire an error event.

• html/HTMLImageElement.cpp: Moved hasPendingActivity() to header, since it's just a single function call now.

• html/HTMLImageElement.h: (WebCore::HTMLImageElement::hasPendingActivity): There is a large window between when CachedResource::isLoading() becomes false and events are queued. ImageLoader::haveFiredLoadEvent() is a much better indication of whether we are expecting an event to fire.

• html/HTMLLinkElement.cpp: (WebCore::HTMLLinkElement::onloadTimerFired): Again, don't do anything on cancellation.

• loader/ImageLoader.cpp: (WebCore::ImageEventSender::hasPendingEvents): Made it debug-only again, and fixed to give an accurate result while looping over the list of events to dispatch. (WebCore::ImageLoader::notifyFinished): Don't do anything when cancelled. We don't want to switch to a broken image icon, or to dispatch events. (WebCore::ImageEventSender::dispatchPendingEvents): Clear the current loader from dispatching list, as the event is no longer pending when it's being dispatched.

• loader/ImageLoader.h: Removed unnecessary hasPendingLoadEvent(). We don't care whether one is already pending, we only care if one is expected at some time in the future, and !haveFiredLoadEvent() is our best idea of that.

• dom/XMLDocumentParser.cpp: (WebCore::XMLDocumentParser::notifyFinished): Another place to handle cancellation.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/dom/gc-image-element-2-expected.txt (added)
• LayoutTests/fast/dom/gc-image-element-2.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/ScriptElement.cpp (modified) (1 diff)
• Source/WebCore/dom/XMLDocumentParser.cpp (modified) (2 diffs)
• Source/WebCore/html/HTMLImageElement.cpp (modified) (1 diff)
• Source/WebCore/html/HTMLImageElement.h (modified) (1 diff)
• Source/WebCore/html/HTMLLinkElement.cpp (modified) (1 diff)
• Source/WebCore/loader/ImageLoader.cpp (modified) (5 diffs)
• Source/WebCore/loader/ImageLoader.h (modified) (1 diff)
• Source/WebCore/loader/cache/CachedResource.cpp (modified) (1 diff)
• Source/WebCore/loader/cache/CachedResource.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2011-05-28  Alexey Proskuryakov  <ap@apple.com>
+
+        Reviewed by Geoff Garen.
+
+        REGRESSION (r85375): Load event is sometimes lost when multiple image elements use the same URL
+        https://bugs.webkit.org/show_bug.cgi?id=61692
+        <rdar://problem/9488628>
+
+        * fast/dom/gc-image-element-2-expected.txt: Added.
+        * fast/dom/gc-image-element-2.html: Added.
+
 2011-05-28  Martin Robinson  <mrobinson@igalia.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-05-28  Alexey Proskuryakov  <ap@apple.com>
+
+        Reviewed by Geoff Garen.
+
+        REGRESSION (r85375): Load event is sometimes lost when multiple image elements use the same URL
+        https://bugs.webkit.org/show_bug.cgi?id=61692
+        <rdar://problem/9488628>
+
+        Test: fast/dom/gc-image-element-2.html
+
+        Manually verified that tests from bug 59604 and from bug 40926 still pass.
+
+        The problem here was that HTMLImageElement::hasPendingActivity() could return false when
+        a load (or error) event was still expected to fire.
+
+        * loader/cache/CachedResource.cpp:
+        (WebCore::CachedResource::setRequest):
+        * loader/cache/CachedResource.h:
+        (WebCore::CachedResource::wasCanceled):
+        (WebCore::CachedResource::errorOccurred):
+        Track whether the load was canceled. We want to always notify clients of load outcome,
+        as that's the only way they could make intelligent decisions.
+
+        * dom/ScriptElement.cpp: (WebCore::ScriptElement::execute): Cached resource clients now
+        get a notifyFinished call on cancellation. Handle this case, where we don't need the
+        execute the script, but also don't need to fire an error event.
+
+        * html/HTMLImageElement.cpp: Moved hasPendingActivity() to header, since it's just a single
+        function call now.
+
+        * html/HTMLImageElement.h: (WebCore::HTMLImageElement::hasPendingActivity): There is a large
+        window between when CachedResource::isLoading() becomes false and events are queued.
+        ImageLoader::haveFiredLoadEvent() is a much better indication of whether we are expecting
+        an event to fire.
+
+        * html/HTMLLinkElement.cpp: (WebCore::HTMLLinkElement::onloadTimerFired): Again, don't do
+        anything on cancellation.
+
+        * loader/ImageLoader.cpp:
+        (WebCore::ImageEventSender::hasPendingEvents): Made it debug-only again, and fixed to
+        give an accurate result while looping over the list of events to dispatch.
+        (WebCore::ImageLoader::notifyFinished): Don't do anything when cancelled. We don't want to
+        switch to a broken image icon, or to dispatch events.
+        (WebCore::ImageEventSender::dispatchPendingEvents): Clear the current loader from dispatching
+        list, as the event is no longer pending when it's being dispatched.
+
+        * loader/ImageLoader.h: Removed unnecessary hasPendingLoadEvent(). We don't care whether one
+        is already pending, we only care if one is expected at some time in the future, and
+        !haveFiredLoadEvent() is our best idea of that.
+
+        * dom/XMLDocumentParser.cpp: (WebCore::XMLDocumentParser::notifyFinished): Another place to
+        handle cancellation.
+
 2011-05-28  Adam Barth  <abarth@webkit.org>
 

trunk/Source/WebCore/dom/ScriptElement.cpp

@@ -297 +297 @@
     if (cachedScript->errorOccurred())
         dispatchErrorEvent();
-    else {
+    else if (!cachedScript->wasCanceled()) {
         executeScript(ScriptSourceCode(cachedScript));
         dispatchLoadEvent();

trunk/Source/WebCore/dom/XMLDocumentParser.cpp

@@ -326 +326 @@
     ScriptSourceCode sourceCode(m_pendingScript.get());
     bool errorOccurred = m_pendingScript->errorOccurred();
+    bool wasCanceled = m_pendingScript->wasCanceled();
 
     m_pendingScript->removeClient(this);
@@ -341 +342 @@
     if (errorOccurred)
         scriptElement->dispatchErrorEvent();
-    else {
+    else if (!wasCanceled) {
         scriptElement->executeScript(sourceCode);
         scriptElement->dispatchLoadEvent();

trunk/Source/WebCore/html/HTMLImageElement.cpp

@@ -386 +386 @@
 }
 
-bool HTMLImageElement::hasPendingActivity()
-{
-    return (cachedImage() && cachedImage()->isLoading()) || m_imageLoader.hasPendingLoadEvent();
-}
-
 void HTMLImageElement::addSubresourceAttributeURLs(ListHashSet<KURL>& urls) const
 {

trunk/Source/WebCore/html/HTMLImageElement.h

@@ -74 +74 @@
 
     bool haveFiredLoadEvent() const { return m_imageLoader.haveFiredLoadEvent(); }
-    bool hasPendingActivity();
+    bool hasPendingActivity() const { return !m_imageLoader.haveFiredLoadEvent(); }
 
     virtual bool canContainRangeEndPoint() const { return false; }

trunk/Source/WebCore/html/HTMLLinkElement.cpp

@@ -449 +449 @@
     if (m_cachedLinkResource->errorOccurred())
         dispatchEvent(Event::create(eventNames().errorEvent, false, false));
-    else
+    else if (!m_cachedLinkResource->wasCanceled())
         dispatchEvent(Event::create(eventNames().loadEvent, false, false));
 

trunk/Source/WebCore/loader/ImageLoader.cpp

@@ -69 +69 @@
     void dispatchPendingEvents();
 
-    bool hasPendingEvents(ImageLoader* loader) { return m_dispatchSoonList.find(loader) != notFound; }
+#ifndef NDEBUG
+    bool hasPendingEvents(ImageLoader* loader) const
+    {
+        return m_dispatchSoonList.find(loader) != notFound || m_dispatchingList.find(loader) != notFound;
+    }
+#endif
 
 private:
@@ -218 +223 @@
 }
 
-void ImageLoader::notifyFinished(CachedResource*)
+void ImageLoader::notifyFinished(CachedResource* resource)
 {
     ASSERT(m_failedLoadURL.isEmpty());
+    ASSERT_UNUSED(m_image, resource == m_image.get());
 
     m_imageComplete = true;
@@ -227 +233 @@
 
     if (m_firedLoad)
+        return;
+
+    if (resource->wasCanceled())
         return;
 
@@ -320 +329 @@
 }
 
-bool ImageLoader::hasPendingLoadEvent()
-{
-    return loadEventSender().hasPendingEvents(this);
-}
-
 ImageEventSender::ImageEventSender(const AtomicString& eventType)
     : m_eventType(eventType)
@@ -372 +376 @@
     for (size_t i = 0; i < size; ++i) {
         if (ImageLoader* loader = m_dispatchingList[i]) {
+            m_dispatchingList[i] = 0;
             if (m_eventType == eventNames().beforeloadEvent)
                 loader->dispatchPendingBeforeLoadEvent();

trunk/Source/WebCore/loader/ImageLoader.h

@@ -59 +59 @@
     bool haveFiredBeforeLoadEvent() const { return m_firedBeforeLoad; }
     bool haveFiredLoadEvent() const { return m_firedLoad; }
-    bool hasPendingLoadEvent();
 
     static void dispatchPendingBeforeLoadEvents();

trunk/Source/WebCore/loader/cache/CachedResource.cpp

@@ -263 +263 @@
 
     // All loads finish with data(allDataReceived = true) or error(), except for
-    // canceled loads, which silently set our request to 0. Be sure to set our
-    // loading flag to false in that case, so we don't seem to continue loading
-    // forever.
-    if (!m_request)
+    // canceled loads, which silently set our request to 0. Be sure to notify our
+    // client in that case, so we don't seem to continue loading forever.
+    if (!m_request && isLoading()) {
         setLoading(false);
+        setStatus(Canceled);
+        checkNotify();
+    }
 
     if (canDelete() && !inCache())

trunk/Source/WebCore/loader/cache/CachedResource.h

@@ -78 +78 @@
         Pending,      // only partially loaded
         Cached,       // regular case
+        Canceled,
         LoadError,
         DecodeError
@@ -190 +191 @@
     void setAccept(const String& accept) { m_accept = accept; }
 
-    bool errorOccurred() const { return (status() == LoadError || status() == DecodeError); }
+    bool wasCanceled() const { return m_status == Canceled; }
+    bool errorOccurred() const { return (m_status == LoadError || m_status == DecodeError); }
 
     bool sendResourceLoadCallbacks() const { return m_sendResourceLoadCallbacks; }