- Revision
- 128730
- Author
- [email protected]
- Date
- 2012-09-17 02:05:13 -0700 (Mon, 17 Sep 2012)
Log Message
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 <[email protected]> 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (128729 => 128730)
--- trunk/LayoutTests/ChangeLog 2012-09-17 08:56:04 UTC (rev 128729)
+++ trunk/LayoutTests/ChangeLog 2012-09-17 09:05:13 UTC (rev 128730)
@@ -1,3 +1,20 @@
+2012-09-17 Mike West <[email protected]>
+
+ 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 <[email protected]>
Make SVGPathSegList.append O(1) instead of O(n)
Added: trunk/LayoutTests/fast/events/onerror-img-after-gc-expected.txt (0 => 128730)
--- trunk/LayoutTests/fast/events/onerror-img-after-gc-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/events/onerror-img-after-gc-expected.txt 2012-09-17 09:05:13 UTC (rev 128730)
@@ -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.
Added: trunk/LayoutTests/fast/events/onerror-img-after-gc.html (0 => 128730)
--- trunk/LayoutTests/fast/events/onerror-img-after-gc.html (rev 0)
+++ trunk/LayoutTests/fast/events/onerror-img-after-gc.html 2012-09-17 09:05:13 UTC (rev 128730)
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+<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 = ""
+ })();
+ 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>
Added: trunk/LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash-expected.txt (0 => 128730)
--- trunk/LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash-expected.txt 2012-09-17 09:05:13 UTC (rev 128730)
@@ -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.
Added: trunk/LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html (0 => 128730)
--- trunk/LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html (rev 0)
+++ trunk/LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html 2012-09-17 09:05:13 UTC (rev 128730)
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+<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 = ""
+ })();
+ 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>
Modified: trunk/LayoutTests/platform/gtk/TestExpectations (128729 => 128730)
--- trunk/LayoutTests/platform/gtk/TestExpectations 2012-09-17 08:56:04 UTC (rev 128729)
+++ trunk/LayoutTests/platform/gtk/TestExpectations 2012-09-17 09:05:13 UTC (rev 128730)
@@ -630,8 +630,6 @@
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
Modified: trunk/LayoutTests/platform/qt/Skipped (128729 => 128730)
--- trunk/LayoutTests/platform/qt/Skipped 2012-09-17 08:56:04 UTC (rev 128729)
+++ trunk/LayoutTests/platform/qt/Skipped 2012-09-17 09:05:13 UTC (rev 128730)
@@ -779,10 +779,6 @@
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 #
# =========================================================================== #
Modified: trunk/Source/WebCore/ChangeLog (128729 => 128730)
--- trunk/Source/WebCore/ChangeLog 2012-09-17 08:56:04 UTC (rev 128729)
+++ trunk/Source/WebCore/ChangeLog 2012-09-17 09:05:13 UTC (rev 128730)
@@ -1,3 +1,43 @@
+2012-09-17 Mike West <[email protected]>
+
+ 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 <[email protected]>
Make SVGPathSegList.appendItem O(1) instead of O(n)
Modified: trunk/Source/WebCore/bindings/v8/V8GCController.cpp (128729 => 128730)
--- trunk/Source/WebCore/bindings/v8/V8GCController.cpp 2012-09-17 08:56:04 UTC (rev 128729)
+++ trunk/Source/WebCore/bindings/v8/V8GCController.cpp 2012-09-17 09:05:13 UTC (rev 128730)
@@ -241,7 +241,7 @@
// 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;
Modified: trunk/Source/WebCore/html/HTMLImageElement.h (128729 => 128730)
--- trunk/Source/WebCore/html/HTMLImageElement.h 2012-09-17 08:56:04 UTC (rev 128729)
+++ trunk/Source/WebCore/html/HTMLImageElement.h 2012-09-17 09:05:13 UTC (rev 128730)
@@ -83,9 +83,7 @@
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; }
Modified: trunk/Source/WebCore/loader/ImageLoader.h (128729 => 128730)
--- trunk/Source/WebCore/loader/ImageLoader.h 2012-09-17 08:56:04 UTC (rev 128729)
+++ trunk/Source/WebCore/loader/ImageLoader.h 2012-09-17 09:05:13 UTC (rev 128730)
@@ -66,7 +66,7 @@
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*);
Modified: trunk/Source/WebCore/svg/SVGImageElement.cpp (128729 => 128730)
--- trunk/Source/WebCore/svg/SVGImageElement.cpp 2012-09-17 08:56:04 UTC (rev 128729)
+++ trunk/Source/WebCore/svg/SVGImageElement.cpp 2012-09-17 09:05:13 UTC (rev 128730)
@@ -196,7 +196,7 @@
bool SVGImageElement::haveLoadedRequiredResources()
{
- return !externalResourcesRequiredBaseValue() || !m_imageLoader.hasPendingLoadEvent();
+ return !externalResourcesRequiredBaseValue() || !m_imageLoader.hasPendingActivity();
}
void SVGImageElement::attach()
