Don't GC img elements blocked by CSP until error events fire.

https://bugs.webkit.org/show_bug.cgi?id=94677 Patch by Mike West <mkwst@chromium.org> on 2012-09-17 Reviewed by Jochen Eisinger. Source/WebCore: Currently, the GC checks that no load events are pending for an image element before reclaiming its memory. It's not, however, checking that error events are taken care of. This leads to the potential of firing an event on a DOM element that we've already collected. That's a Bad Thing. This patch adjusts the check to catch error events as well as load events, which should ensure that the element isn't collected until it's really ready. As a drive-by, it also changes the name of the check to 'hasPendingActivity' from 'hasPendingLoadEvent' for clarity. http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html should no longer crash, and the new http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html and fast/events/onerror-img-after-gc.html shouldn't crash either. Tests: fast/events/onerror-img-after-gc.html http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html * bindings/v8/V8GCController.cpp: (WebCore::calculateGroupId): Switch to using ImageLoader::hasPendingActivity(). * html/HTMLImageElement.h: (WebCore::HTMLImageElement::hasPendingActivity): Switch to using ImageLoader::hasPendingActivity(). * loader/ImageLoader.h: (WebCore::ImageLoader::hasPendingActivity): Added a check against pending error events in order to ensure that elements aren't garbage collected prematurely. Aslo renamed from ImageLoader::hasPendingLoadEvent for clarity. * svg/SVGImageElement.cpp: (WebCore::SVGImageElement::haveLoadedRequiredResources): Switch to using ImageLoader::hasPendingActivity(). LayoutTests: * fast/events/onerror-img-after-gc.html: * fast/events/onerror-img-after-gc-expected.txt: * http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html: * http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash-expected.txt: Explicitly triggering GC before the error in the hopes of proving that we don't crash anymore. * platform/gtk/TestExpectations: * platform/qt/Skipped: Unskipping no-longer-crashing test. Canonical link: https://commits.webkit.org/114812@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@128730 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Sep 17, 2012 · 5c812eda560c70e0c4d91f9adc3c0c4bac41a2c1 · 5c812ed
1 parent cd22a9d
commit 5c812eda560c70e0c4d91f9adc3c0c4bac41a2c1
Showing 12 changed files with 144 additions and 12 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,20 @@
+2012-09-17  Mike West  <mkwst@chromium.org>
+
+        Don't GC img elements blocked by CSP until error events fire.
+        https://bugs.webkit.org/show_bug.cgi?id=94677
+
+        Reviewed by Jochen Eisinger.
+
+        * fast/events/onerror-img-after-gc.html:
+        * fast/events/onerror-img-after-gc-expected.txt:
+        * http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html:
+        * http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash-expected.txt:
+            Explicitly triggering GC before the error in the hopes of proving
+            that we don't crash anymore.
+        * platform/gtk/TestExpectations:
+        * platform/qt/Skipped:
+            Unskipping no-longer-crashing test.
+
 2012-09-17  Philip Rogers  <pdr@google.com>
 
         Make SVGPathSegList.append O(1) instead of O(n)

diff --git a/LayoutTests/fast/events/onerror-img-after-gc-expected.txt b/LayoutTests/fast/events/onerror-img-after-gc-expected.txt
@@ -0,0 +1,2 @@
+ALERT: PASS (1/1)
+This test ensures that a normal image error doesn't crash if GC occurs before the error event fires.
diff --git a/LayoutTests/fast/events/onerror-img-after-gc.html b/LayoutTests/fast/events/onerror-img-after-gc.html
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../js/resources/js-test-pre.js"></script>
+<script>
+    if (window.testRunner)
+        testRunner.waitUntilDone();
+
+    function test() {
+        (function () {
+            var img = document.createElement('img');
+            img.onload = function () {
+                alert('FAIL (1/1)');
+                finishTesting();
+            };
+            img.onerror = function () {
+                alert('PASS (1/1)');
+                finishTesting();
+            };
+            img.src = "foo";
+        })();
+        gc();
+    }
+
+    function finishTesting() {
+        if (window.testRunner)
+            setTimeout(function () { testRunner.notifyDone(); }, 0);
+        return true;
+    }
+</script>
+</head>
+<body onload='test();'>
+    <p>
+        This test ensures that a normal image error doesn't crash if GC occurs
+        before the error event fires.
+    </p>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash-expected.txt b/LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash-expected.txt
@@ -0,0 +1,4 @@
+CONSOLE MESSAGE: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png' because it violates the following Content Security Policy directive: "img-src 'none'".
+
+ALERT: PASS (1/1)
+This test ensures that blocking an image via CSP doesn't crash if GC executes before the error event fires.
diff --git a/LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html b/LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="/resources/js-test-pre.js"></script>
+<meta http-equiv="X-WebKit-CSP" content="img-src 'none'; script-src 'unsafe-inline'">
+<script>
+    if (window.testRunner)
+        testRunner.waitUntilDone();
+
+    function test() {
+        (function () {
+            var img = document.createElement('img');
+            img.onload = function () {
+                alert('FAIL (1/1)');
+                finishTesting();
+            };
+            img.onerror = function () {
+                alert('PASS (1/1)');
+                finishTesting();
+            };
+            img.src = "../resources/abe.png";
+        })();
+        gc();
+    }
+
+    function finishTesting() {
+        if (window.testRunner)
+            setTimeout(function () { testRunner.notifyDone(); }, 0);
+        return true;
+    }
+</script>
+</head>
+<body onload='test();'>
+    <p>
+        This test ensures that blocking an image via CSP doesn't crash if GC
+        executes before the error event fires.
+    </p>
+</body>
+</html>
diff --git a/LayoutTests/platform/gtk/TestExpectations b/LayoutTests/platform/gtk/TestExpectations
@@ -630,8 +630,6 @@ BUGWK61118 : fullscreen/full-screen-restrictions.html = TIMEOUT
 
 BUGWK85700 : fullscreen/non-ancestor-iframe.html = TIMEOUT
 
-BUGWK94677 : http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html = TIMEOUT
-
 BUGWK95530 : http/tests/security/inactive-document-with-empty-security-origin.html = TIMEOUT
 
 BUGWK72698 : media/audio-garbage-collect.html = TIMEOUT

diff --git a/LayoutTests/platform/qt/Skipped b/LayoutTests/platform/qt/Skipped
@@ -779,10 +779,6 @@ http/tests/cookies/js-get-and-set-http-only-cookie.html
 http/tests/w3c/webperf/approved/navigation-timing/html/test_performance_attributes_exist_in_object.html
 http/tests/w3c/webperf/approved/navigation-timing/html/test_timing_xserver_redirect.html
 
-# [Qt][GTK] REGRESSION(r126194): http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html fails
-# https://bugs.webkit.org/show_bug.cgi?id=94677
-http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html
-
 # =========================================================================== #
 #       Failing xmlhttprequest tests                                          #
 # =========================================================================== #

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,43 @@
+2012-09-17  Mike West  <mkwst@chromium.org>
+
+        Don't GC img elements blocked by CSP until error events fire.
+        https://bugs.webkit.org/show_bug.cgi?id=94677
+
+        Reviewed by Jochen Eisinger.
+
+        Currently, the GC checks that no load events are pending for an image
+        element before reclaiming its memory. It's not, however, checking that
+        error events are taken care of. This leads to the potential of firing an
+        event on a DOM element that we've already collected. That's a Bad Thing.
+
+        This patch adjusts the check to catch error events as well as load
+        events, which should ensure that the element isn't collected until it's
+        really ready. As a drive-by, it also changes the name of the check to
+        'hasPendingActivity' from 'hasPendingLoadEvent' for clarity.
+
+        http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html
+        should no longer crash, and the new
+        http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html
+        and fast/events/onerror-img-after-gc.html shouldn't crash either.
+
+        Tests: fast/events/onerror-img-after-gc.html
+               http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html
+
+        * bindings/v8/V8GCController.cpp:
+        (WebCore::calculateGroupId):
+            Switch to using ImageLoader::hasPendingActivity().
+        * html/HTMLImageElement.h:
+        (WebCore::HTMLImageElement::hasPendingActivity):
+            Switch to using ImageLoader::hasPendingActivity().
+        * loader/ImageLoader.h:
+        (WebCore::ImageLoader::hasPendingActivity):
+            Added a check against pending error events in order to ensure that
+            elements aren't garbage collected prematurely. Aslo renamed from
+            ImageLoader::hasPendingLoadEvent for clarity.
+        * svg/SVGImageElement.cpp:
+        (WebCore::SVGImageElement::haveLoadedRequiredResources):
+            Switch to using ImageLoader::hasPendingActivity().
+
 2012-09-17  Philip Rogers  <pdr@google.com>
 
         Make SVGPathSegList.appendItem O(1) instead of O(n)

diff --git a/Source/WebCore/bindings/v8/V8GCController.cpp b/Source/WebCore/bindings/v8/V8GCController.cpp
@@ -241,7 +241,7 @@ typedef Vector<GrouperItem> GrouperList;
 // element of the tree to which it belongs.
 static GroupId calculateGroupId(Node* node)
 {
-    if (node->inDocument() || (node->hasTagName(HTMLNames::imgTag) && static_cast<HTMLImageElement*>(node)->hasPendingLoadEvent()))
+    if (node->inDocument() || (node->hasTagName(HTMLNames::imgTag) && static_cast<HTMLImageElement*>(node)->hasPendingActivity()))
         return GroupId(node->document());
 
     Node* root = node;

diff --git a/Source/WebCore/html/HTMLImageElement.h b/Source/WebCore/html/HTMLImageElement.h
@@ -83,9 +83,7 @@ class HTMLImageElement : public HTMLElement, public ImageElement, public ImageLo
 
     bool complete() const;
 
-    // FIXME: Why do we have two names for the same thing?
-    bool hasPendingLoadEvent() const { return m_imageLoader.hasPendingLoadEvent(); }
-    bool hasPendingActivity() const { return m_imageLoader.hasPendingLoadEvent(); }
+    bool hasPendingActivity() const { return m_imageLoader.hasPendingActivity(); }
 
     virtual bool canContainRangeEndPoint() const { return false; }
 

diff --git a/Source/WebCore/loader/ImageLoader.h b/Source/WebCore/loader/ImageLoader.h
@@ -66,7 +66,7 @@ class ImageLoader : public CachedImageClient {
     void setLoadManually(bool loadManually) { m_loadManually = loadManually; }
 
     bool hasPendingBeforeLoadEvent() const { return m_hasPendingBeforeLoadEvent; }
-    bool hasPendingLoadEvent() const { return m_hasPendingLoadEvent; }
+    bool hasPendingActivity() const { return m_hasPendingLoadEvent || m_hasPendingErrorEvent; }
 
     void dispatchPendingEvent(ImageEventSender*);
 

diff --git a/Source/WebCore/svg/SVGImageElement.cpp b/Source/WebCore/svg/SVGImageElement.cpp
@@ -196,7 +196,7 @@ RenderObject* SVGImageElement::createRenderer(RenderArena* arena, RenderStyle*)
 
 bool SVGImageElement::haveLoadedRequiredResources()
 {
-    return !externalResourcesRequiredBaseValue() || !m_imageLoader.hasPendingLoadEvent();
+    return !externalResourcesRequiredBaseValue() || !m_imageLoader.hasPendingActivity();
 }
 
 void SVGImageElement::attach()