Changeset 85375 in webkit

Timestamp:

Apr 29, 2011 5:57:39 PM (13 years ago)

Author:

ggaren@apple.com

Message:

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 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.

Location:

trunk

Files:

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

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/bindings/js/JSNodeCustom.cpp

@@ -28 +28 @@
 
 #include "Attr.h"
+#include "CachedImage.h"
+#include "CachedScript.h"
 #include "CDATASection.h"
 #include "Comment.h"
@@ -104 +106 @@
 {
     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
 

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();
 
 protected:

trunk/Source/WebCore/loader/ImageLoader.cpp

@@ -68 +68 @@
     void dispatchPendingEvents();
 
-#if !ASSERT_DISABLED
     bool hasPendingEvents(ImageLoader* loader) { return m_dispatchSoonList.find(loader) != notFound; }
-#endif
 
 private:
@@ -313 +311 @@
 }
 
+bool ImageLoader::hasPendingLoadEvent()
+{
+    return loadEventSender().hasPendingEvents(this);
+}
+
 ImageEventSender::ImageEventSender(const AtomicString& eventType)
     : m_eventType(eventType)

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

@@ -250 +250 @@
         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;