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.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@87628 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 003106c..4867805 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,56 @@
+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>
Reviewed by Alexey Proskuryakov.
diff --git a/Source/WebCore/dom/ScriptElement.cpp b/Source/WebCore/dom/ScriptElement.cpp
index bdaca77..c8cc351 100644
--- a/Source/WebCore/dom/ScriptElement.cpp
+++ b/Source/WebCore/dom/ScriptElement.cpp
@@ -296,7 +296,7 @@
ASSERT(cachedScript);
if (cachedScript->errorOccurred())
dispatchErrorEvent();
- else {
+ else if (!cachedScript->wasCanceled()) {
executeScript(ScriptSourceCode(cachedScript));
dispatchLoadEvent();
}
diff --git a/Source/WebCore/dom/XMLDocumentParser.cpp b/Source/WebCore/dom/XMLDocumentParser.cpp
index e1ff065..6d8094f 100644
--- a/Source/WebCore/dom/XMLDocumentParser.cpp
+++ b/Source/WebCore/dom/XMLDocumentParser.cpp
@@ -325,6 +325,7 @@
ScriptSourceCode sourceCode(m_pendingScript.get());
bool errorOccurred = m_pendingScript->errorOccurred();
+ bool wasCanceled = m_pendingScript->wasCanceled();
m_pendingScript->removeClient(this);
m_pendingScript = 0;
@@ -340,7 +341,7 @@
if (errorOccurred)
scriptElement->dispatchErrorEvent();
- else {
+ else if (!wasCanceled) {
scriptElement->executeScript(sourceCode);
scriptElement->dispatchLoadEvent();
}
diff --git a/Source/WebCore/html/HTMLImageElement.cpp b/Source/WebCore/html/HTMLImageElement.cpp
index da80090..d66075e 100644
--- a/Source/WebCore/html/HTMLImageElement.cpp
+++ b/Source/WebCore/html/HTMLImageElement.cpp
@@ -385,11 +385,6 @@
return m_imageLoader.imageComplete();
}
-bool HTMLImageElement::hasPendingActivity()
-{
- return (cachedImage() && cachedImage()->isLoading()) || m_imageLoader.hasPendingLoadEvent();
-}
-
void HTMLImageElement::addSubresourceAttributeURLs(ListHashSet<KURL>& urls) const
{
HTMLElement::addSubresourceAttributeURLs(urls);
diff --git a/Source/WebCore/html/HTMLImageElement.h b/Source/WebCore/html/HTMLImageElement.h
index 57c0d77..29f42a1 100644
--- a/Source/WebCore/html/HTMLImageElement.h
+++ b/Source/WebCore/html/HTMLImageElement.h
@@ -73,7 +73,7 @@
bool complete() const;
bool haveFiredLoadEvent() const { return m_imageLoader.haveFiredLoadEvent(); }
- bool hasPendingActivity();
+ bool hasPendingActivity() const { return !m_imageLoader.haveFiredLoadEvent(); }
virtual bool canContainRangeEndPoint() const { return false; }
diff --git a/Source/WebCore/html/HTMLLinkElement.cpp b/Source/WebCore/html/HTMLLinkElement.cpp
index 4d5671c5..9c25d8c 100644
--- a/Source/WebCore/html/HTMLLinkElement.cpp
+++ b/Source/WebCore/html/HTMLLinkElement.cpp
@@ -448,7 +448,7 @@
ASSERT_UNUSED(timer, timer == &m_onloadTimer);
if (m_cachedLinkResource->errorOccurred())
dispatchEvent(Event::create(eventNames().errorEvent, false, false));
- else
+ else if (!m_cachedLinkResource->wasCanceled())
dispatchEvent(Event::create(eventNames().loadEvent, false, false));
m_cachedLinkResource->removeClient(this);
diff --git a/Source/WebCore/loader/ImageLoader.cpp b/Source/WebCore/loader/ImageLoader.cpp
index 2b96952..99507ec 100644
--- a/Source/WebCore/loader/ImageLoader.cpp
+++ b/Source/WebCore/loader/ImageLoader.cpp
@@ -68,7 +68,12 @@
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:
void timerFired(Timer<ImageEventSender>*);
@@ -217,9 +222,10 @@
updateFromElement();
}
-void ImageLoader::notifyFinished(CachedResource*)
+void ImageLoader::notifyFinished(CachedResource* resource)
{
ASSERT(m_failedLoadURL.isEmpty());
+ ASSERT_UNUSED(m_image, resource == m_image.get());
m_imageComplete = true;
if (haveFiredBeforeLoadEvent())
@@ -228,6 +234,9 @@
if (m_firedLoad)
return;
+ if (resource->wasCanceled())
+ return;
+
loadEventSender().dispatchEventSoon(this);
}
@@ -319,11 +328,6 @@
setImage(0);
}
-bool ImageLoader::hasPendingLoadEvent()
-{
- return loadEventSender().hasPendingEvents(this);
-}
-
ImageEventSender::ImageEventSender(const AtomicString& eventType)
: m_eventType(eventType)
, m_timer(this, &ImageEventSender::timerFired)
@@ -371,6 +375,7 @@
size_t size = m_dispatchingList.size();
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();
else
diff --git a/Source/WebCore/loader/ImageLoader.h b/Source/WebCore/loader/ImageLoader.h
index c603e00..9bf7624 100644
--- a/Source/WebCore/loader/ImageLoader.h
+++ b/Source/WebCore/loader/ImageLoader.h
@@ -58,7 +58,6 @@
bool haveFiredBeforeLoadEvent() const { return m_firedBeforeLoad; }
bool haveFiredLoadEvent() const { return m_firedLoad; }
- bool hasPendingLoadEvent();
static void dispatchPendingBeforeLoadEvents();
static void dispatchPendingLoadEvents();
diff --git a/Source/WebCore/loader/cache/CachedResource.cpp b/Source/WebCore/loader/cache/CachedResource.cpp
index 31ccdc3..de9e3a9 100644
--- a/Source/WebCore/loader/cache/CachedResource.cpp
+++ b/Source/WebCore/loader/cache/CachedResource.cpp
@@ -262,11 +262,13 @@
m_request = request;
// 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())
delete this;
diff --git a/Source/WebCore/loader/cache/CachedResource.h b/Source/WebCore/loader/cache/CachedResource.h
index 5019592..d679063 100644
--- a/Source/WebCore/loader/cache/CachedResource.h
+++ b/Source/WebCore/loader/cache/CachedResource.h
@@ -77,6 +77,7 @@
Unknown, // let cache decide what to do with it
Pending, // only partially loaded
Cached, // regular case
+ Canceled,
LoadError,
DecodeError
};
@@ -189,7 +190,8 @@
String accept() const { return m_accept; }
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; }
