2011-04-29 Geoffrey Garen <ggaren@apple.com>
authorggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 30 Apr 2011 00:57:39 +0000 (00:57 +0000)
committerggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 30 Apr 2011 00:57:39 +0000 (00:57 +0000)
        Reviewed by Alexey Proskuryakov.

        REGRESSION: r83938 abandons GC memory
        https://bugs.webkit.org/show_bug.cgi?id=59604

        This bug was caused by script and image elements waiting indefinitely
        for their loads to finish.

        * bindings/js/JSNodeCustom.cpp:
        (WebCore::isReachableFromDOM): Don't test for the load event firing,
        since the load event doesn't fire in cases of canceled or errored loads.
        Instead, test hasPendingActivity().

        Don't do this test at all for script elements because script elements
        can't load while outside the document. (fast/dom/script-element-gc.html
        verifies that this is correct.)

        * html/HTMLImageElement.cpp:
        (WebCore::HTMLImageElement::hasPendingActivity):
        * html/HTMLImageElement.h:
        * loader/ImageLoader.cpp:
        (WebCore::ImageEventSender::hasPendingEvents):
        (WebCore::ImageLoader::hasPendingLoadEvent):
        * loader/ImageLoader.h: Added API for finding out if an image element
        has pending activity.

        * loader/cache/CachedResource.cpp:
        (WebCore::CachedResource::setRequest): All loads are supposed to end in
        data(allDataReceived = true) or error(), but in the edge case of a
        canceled load, all we get is a call to setRequest(0). Be sure to
        record that we're no longer loading in that case, otherwise our element
        will leak forever, waiting for its load to complete.
2011-04-29  Geoffrey Garen  <ggaren@apple.com>

        Reviewed by Alexey Proskuryakov.

        REGRESSION: r83938 abandons GC memory
        https://bugs.webkit.org/show_bug.cgi?id=59604

        Test an edge case of an image that has finished loading but has not yet
        fired its load event.

        * fast/dom/gc-image-element-expected.txt: Added.
        * fast/dom/gc-image-element.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@85375 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/dom/gc-image-element-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/gc-image-element.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSNodeCustom.cpp
Source/WebCore/html/HTMLImageElement.cpp
Source/WebCore/html/HTMLImageElement.h
Source/WebCore/loader/ImageLoader.cpp
Source/WebCore/loader/ImageLoader.h
Source/WebCore/loader/cache/CachedResource.cpp

index 7b73135..1a54acc 100644 (file)
@@ -1,3 +1,16 @@
+2011-04-29  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Alexey Proskuryakov.
+
+        REGRESSION: r83938 abandons GC memory
+        https://bugs.webkit.org/show_bug.cgi?id=59604
+        
+        Test an edge case of an image that has finished loading but has not yet
+        fired its load event.
+
+        * fast/dom/gc-image-element-expected.txt: Added.
+        * fast/dom/gc-image-element.html: Added.
+
 2011-04-29  Emil Eklund  <eae@chromium.org>
 
         Reviewed by Tony Chang.
diff --git a/LayoutTests/fast/dom/gc-image-element-expected.txt b/LayoutTests/fast/dom/gc-image-element-expected.txt
new file mode 100644 (file)
index 0000000..3350745
--- /dev/null
@@ -0,0 +1,13 @@
+Tests for image elements firing their load events even when they're not in the document.
+
+PASS: loaded
+PASS: loaded
+PASS: loaded
+PASS: loaded
+PASS: loaded
+PASS: loaded
+PASS: loaded
+PASS: loaded
+PASS: loaded
+PASS: loaded
+
diff --git a/LayoutTests/fast/dom/gc-image-element.html b/LayoutTests/fast/dom/gc-image-element.html
new file mode 100644 (file)
index 0000000..37fb563
--- /dev/null
@@ -0,0 +1,47 @@
+<p>Tests for image elements firing their load events even when they're not in the document.</p>
+<pre id="console"></pre>
+
+<script src="../js/resources/js-test-pre.js"></script>
+<script>
+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 = "resources/apple.gif";
+    image.onload = function () {
+        log("PASS: loaded");
+        --imageCount;
+        if (imageCount)
+            return;
+        if (window.layoutTestController)
+            layoutTestController.notifyDone();
+    };
+}
+
+(function () {
+    if (window.layoutTestController) {
+        layoutTestController.dumpAsText();
+        layoutTestController.waitUntilDone();
+    }
+
+    var image = new Image;
+    image.src = "resources/apple.gif";
+    image.onload = function () { // Wait for the image to load so subsequent loads will be synchronous.
+        for (var i = 0; i < 10; ++i)
+            createImage();
+        gc();
+    }
+})();
+</script>
index e037a73..3c70223 100644 (file)
@@ -1,3 +1,38 @@
+2011-04-29  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Alexey Proskuryakov.
+
+        REGRESSION: r83938 abandons GC memory
+        https://bugs.webkit.org/show_bug.cgi?id=59604
+
+        This bug was caused by script and image elements waiting indefinitely
+        for their loads to finish.
+
+        * bindings/js/JSNodeCustom.cpp:
+        (WebCore::isReachableFromDOM): Don't test for the load event firing,
+        since the load event doesn't fire in cases of canceled or errored loads.
+        Instead, test hasPendingActivity().
+        
+        Don't do this test at all for script elements because script elements
+        can't load while outside the document. (fast/dom/script-element-gc.html
+        verifies that this is correct.)
+
+        * html/HTMLImageElement.cpp:
+        (WebCore::HTMLImageElement::hasPendingActivity):
+        * html/HTMLImageElement.h:
+        * loader/ImageLoader.cpp:
+        (WebCore::ImageEventSender::hasPendingEvents):
+        (WebCore::ImageLoader::hasPendingLoadEvent):
+        * loader/ImageLoader.h: Added API for finding out if an image element
+        has pending activity.
+
+        * loader/cache/CachedResource.cpp:
+        (WebCore::CachedResource::setRequest): All loads are supposed to end in
+        data(allDataReceived = true) or error(), but in the edge case of a
+        canceled load, all we get is a call to setRequest(0). Be sure to
+        record that we're no longer loading in that case, otherwise our element
+        will leak forever, waiting for its load to complete.
+
 2011-04-29  Emil Eklund  <eae@chromium.org>
 
         Reviewed by Tony Chang.
index be0a7a1..43f4f66 100644 (file)
@@ -27,6 +27,8 @@
 #include "JSNode.h"
 
 #include "Attr.h"
+#include "CachedImage.h"
+#include "CachedScript.h"
 #include "CDATASection.h"
 #include "Comment.h"
 #include "Document.h"
@@ -103,18 +105,20 @@ static inline bool isObservable(JSNode* jsNode, Node* node)
 static inline bool isReachableFromDOM(JSNode* jsNode, Node* node, SlotVisitor& visitor)
 {
     if (!node->inDocument()) {
-        // If a wrapper is the last reference to an image or script element
+        // If a wrapper is the last reference to an image element
         // that is loading but not in the document, the wrapper is observable
         // because it is the only thing keeping the image element alive, and if
-        // the image element is destroyed, its load event will not fire.
+        // the element is destroyed, its load event will not fire.
         // FIXME: The DOM should manage this issue without the help of JavaScript wrappers.
-        if (node->hasTagName(imgTag) && !static_cast<HTMLImageElement*>(node)->haveFiredLoadEvent())
-            return true;
-        if (node->hasTagName(scriptTag) && !static_cast<HTMLScriptElement*>(node)->haveFiredLoadEvent())
-            return true;
+        if (node->hasTagName(imgTag)) {
+            if (static_cast<HTMLImageElement*>(node)->hasPendingActivity())
+                return true;
+        }
     #if ENABLE(VIDEO)
-        if (node->hasTagName(audioTag) && !static_cast<HTMLAudioElement*>(node)->paused())
-            return true;
+        else if (node->hasTagName(audioTag)) {
+            if (!static_cast<HTMLAudioElement*>(node)->paused())
+                return true;
+        }
     #endif
 
         // If a node is firing event listeners, its wrapper is observable because
index d66075e..da80090 100644 (file)
@@ -385,6 +385,11 @@ bool HTMLImageElement::complete() const
     return m_imageLoader.imageComplete();
 }
 
+bool HTMLImageElement::hasPendingActivity()
+{
+    return (cachedImage() && cachedImage()->isLoading()) || m_imageLoader.hasPendingLoadEvent();
+}
+
 void HTMLImageElement::addSubresourceAttributeURLs(ListHashSet<KURL>& urls) const
 {
     HTMLElement::addSubresourceAttributeURLs(urls);
index 7f38216..861595a 100644 (file)
@@ -73,6 +73,7 @@ public:
     bool complete() const;
 
     bool haveFiredLoadEvent() const { return m_imageLoader.haveFiredLoadEvent(); }
+    bool hasPendingActivity();
 
 protected:
     HTMLImageElement(const QualifiedName&, Document*, HTMLFormElement* = 0);
index 069b40e..ea956ee 100644 (file)
@@ -67,9 +67,7 @@ public:
 
     void dispatchPendingEvents();
 
-#if !ASSERT_DISABLED
     bool hasPendingEvents(ImageLoader* loader) { return m_dispatchSoonList.find(loader) != notFound; }
-#endif
 
 private:
     void timerFired(Timer<ImageEventSender>*);
@@ -312,6 +310,11 @@ void ImageLoader::elementWillMoveToNewOwnerDocument()
     setImage(0);
 }
 
+bool ImageLoader::hasPendingLoadEvent()
+{
+    return loadEventSender().hasPendingEvents(this);
+}
+
 ImageEventSender::ImageEventSender(const AtomicString& eventType)
     : m_eventType(eventType)
     , m_timer(this, &ImageEventSender::timerFired)
index 9bf7624..c603e00 100644 (file)
@@ -58,6 +58,7 @@ public:
 
     bool haveFiredBeforeLoadEvent() const { return m_firedBeforeLoad; }
     bool haveFiredLoadEvent() const { return m_firedLoad; }
+    bool hasPendingLoadEvent();
 
     static void dispatchPendingBeforeLoadEvents();
     static void dispatchPendingLoadEvents();
index 95f5522..04e2cfb 100644 (file)
@@ -249,6 +249,14 @@ void CachedResource::setRequest(CachedResourceRequest* request)
     if (request && !m_request)
         m_status = Pending;
     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)
+        setLoading(false);
+
     if (canDelete() && !inCache())
         delete this;
 }