- Revision
- 87628
- Author
- [email protected]
- Date
- 2011-05-28 18:37:59 -0700 (Sat, 28 May 2011)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (87627 => 87628)
--- trunk/LayoutTests/ChangeLog 2011-05-28 23:45:17 UTC (rev 87627)
+++ trunk/LayoutTests/ChangeLog 2011-05-29 01:37:59 UTC (rev 87628)
@@ -1,3 +1,14 @@
+2011-05-28 Alexey Proskuryakov <[email protected]>
+
+ 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 <[email protected]>
Skip a failing test and better classify some existing skipped tests.
Added: trunk/LayoutTests/fast/dom/gc-image-element-2-expected.txt (0 => 87628)
--- trunk/LayoutTests/fast/dom/gc-image-element-2-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/dom/gc-image-element-2-expected.txt 2011-05-29 01:37:59 UTC (rev 87628)
@@ -0,0 +1,14 @@
+Tests for image elements firing their load events even when they're not in the document. Should say "onload fired" ten times, and then "PASS".
+
+onload fired...
+onload fired...
+onload fired...
+onload fired...
+onload fired...
+onload fired...
+onload fired...
+onload fired...
+onload fired...
+onload fired...
+PASS
+
Added: trunk/LayoutTests/fast/dom/gc-image-element-2.html (0 => 87628)
--- trunk/LayoutTests/fast/dom/gc-image-element-2.html (rev 0)
+++ trunk/LayoutTests/fast/dom/gc-image-element-2.html 2011-05-29 01:37:59 UTC (rev 87628)
@@ -0,0 +1,45 @@
+<p>Tests for image elements firing their load events even when they're not in the document. Should say "onload fired" ten times, and then "PASS".</p>
+<pre id="console"></pre>
+
+<script src=""
+<script>
+if (window.layoutTestController) {
+ layoutTestController.dumpAsText();
+ layoutTestController.waitUntilDone();
+}
+
+function $(id)
+{
+ return document.getElementById(id);
+}
+
+function log(s)
+{
+ $("console").appendChild(document.createTextNode(s + "\n"));
+}
+
+var imageCount = 0;
+
+function createImage()
+{
+ ++imageCount;
+ var image = new Image;
+ image.src = ""
+ image._onload_ = function () {
+ log("onload fired...");
+ gc();
+
+ --imageCount;
+ if (!imageCount) {
+ log("PASS");
+ if (window.layoutTestController)
+ layoutTestController.notifyDone();
+ }
+ };
+ image = null;
+}
+
+for (var i = 0; i < 10; ++i)
+ createImage();
+
+</script>
Modified: trunk/Source/WebCore/ChangeLog (87627 => 87628)
--- trunk/Source/WebCore/ChangeLog 2011-05-28 23:45:17 UTC (rev 87627)
+++ trunk/Source/WebCore/ChangeLog 2011-05-29 01:37:59 UTC (rev 87628)
@@ -1,3 +1,56 @@
+2011-05-28 Alexey Proskuryakov <[email protected]>
+
+ 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 <[email protected]>
Reviewed by Alexey Proskuryakov.
Modified: trunk/Source/WebCore/dom/ScriptElement.cpp (87627 => 87628)
--- trunk/Source/WebCore/dom/ScriptElement.cpp 2011-05-28 23:45:17 UTC (rev 87627)
+++ trunk/Source/WebCore/dom/ScriptElement.cpp 2011-05-29 01:37:59 UTC (rev 87628)
@@ -296,7 +296,7 @@
ASSERT(cachedScript);
if (cachedScript->errorOccurred())
dispatchErrorEvent();
- else {
+ else if (!cachedScript->wasCanceled()) {
executeScript(ScriptSourceCode(cachedScript));
dispatchLoadEvent();
}
Modified: trunk/Source/WebCore/dom/XMLDocumentParser.cpp (87627 => 87628)
--- trunk/Source/WebCore/dom/XMLDocumentParser.cpp 2011-05-28 23:45:17 UTC (rev 87627)
+++ trunk/Source/WebCore/dom/XMLDocumentParser.cpp 2011-05-29 01:37:59 UTC (rev 87628)
@@ -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();
}
Modified: trunk/Source/WebCore/html/HTMLImageElement.cpp (87627 => 87628)
--- trunk/Source/WebCore/html/HTMLImageElement.cpp 2011-05-28 23:45:17 UTC (rev 87627)
+++ trunk/Source/WebCore/html/HTMLImageElement.cpp 2011-05-29 01:37:59 UTC (rev 87628)
@@ -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);
Modified: trunk/Source/WebCore/html/HTMLImageElement.h (87627 => 87628)
--- trunk/Source/WebCore/html/HTMLImageElement.h 2011-05-28 23:45:17 UTC (rev 87627)
+++ trunk/Source/WebCore/html/HTMLImageElement.h 2011-05-29 01:37:59 UTC (rev 87628)
@@ -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; }
Modified: trunk/Source/WebCore/html/HTMLLinkElement.cpp (87627 => 87628)
--- trunk/Source/WebCore/html/HTMLLinkElement.cpp 2011-05-28 23:45:17 UTC (rev 87627)
+++ trunk/Source/WebCore/html/HTMLLinkElement.cpp 2011-05-29 01:37:59 UTC (rev 87628)
@@ -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);
Modified: trunk/Source/WebCore/loader/ImageLoader.cpp (87627 => 87628)
--- trunk/Source/WebCore/loader/ImageLoader.cpp 2011-05-28 23:45:17 UTC (rev 87627)
+++ trunk/Source/WebCore/loader/ImageLoader.cpp 2011-05-29 01:37:59 UTC (rev 87628)
@@ -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
Modified: trunk/Source/WebCore/loader/ImageLoader.h (87627 => 87628)
--- trunk/Source/WebCore/loader/ImageLoader.h 2011-05-28 23:45:17 UTC (rev 87627)
+++ trunk/Source/WebCore/loader/ImageLoader.h 2011-05-29 01:37:59 UTC (rev 87628)
@@ -58,7 +58,6 @@
bool haveFiredBeforeLoadEvent() const { return m_firedBeforeLoad; }
bool haveFiredLoadEvent() const { return m_firedLoad; }
- bool hasPendingLoadEvent();
static void dispatchPendingBeforeLoadEvents();
static void dispatchPendingLoadEvents();
Modified: trunk/Source/WebCore/loader/cache/CachedResource.cpp (87627 => 87628)
--- trunk/Source/WebCore/loader/cache/CachedResource.cpp 2011-05-28 23:45:17 UTC (rev 87627)
+++ trunk/Source/WebCore/loader/cache/CachedResource.cpp 2011-05-29 01:37:59 UTC (rev 87628)
@@ -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;
Modified: trunk/Source/WebCore/loader/cache/CachedResource.h (87627 => 87628)
--- trunk/Source/WebCore/loader/cache/CachedResource.h 2011-05-28 23:45:17 UTC (rev 87627)
+++ trunk/Source/WebCore/loader/cache/CachedResource.h 2011-05-29 01:37:59 UTC (rev 87628)
@@ -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; }
