<rdar://problem/8672000> REGRESSION (r72040): Error image with alt text can cause...
authormitz@apple.com <mitz@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Nov 2010 22:08:26 +0000 (22:08 +0000)
committermitz@apple.com <mitz@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Nov 2010 22:08:26 +0000 (22:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=49579

Reviewed by Simon Fraser.

WebCore:

r72040 introduced a call to setNeedsStyleRecalc() from RenderImage::imageChanged(). When imageChanged()
got called beneath recalcStyle() on some ancestor element, the result was that the ancestor’s
childNeedsStyleRecalc flag got cleared, but its descendants all the way down to the image did not.
Thereafter, setNeedsStyleRecalc() would fail to propagate from that subtree up to the root. The fix
is to avoid the newly-added setNeedsStyleRecalc() in most cases, including during reclacStyle(), and
just keep it for when it is needed.

Tests: fast/block/float/015.html
       fast/images/style-access-during-imageChanged-style-freeze.html

* dom/Document.cpp:
(WebCore::Document::isPendingStyleRecalc): Added.
* dom/Document.h:
* rendering/RenderImage.cpp:
(WebCore::RenderImage::imageChanged): Only defer intrinsic size compoutation if a style recalc
is coming (indicating that current style() is stale).

LayoutTests:

* fast/block/float/015.html: Copied from LayoutTests/fast/block/float/015.html-disabled.
* fast/block/float/015.html-disabled: Removed.
* fast/images/style-access-during-imageChanged-style-freeze-expected.txt: Added.
* fast/images/style-access-during-imageChanged-style-freeze.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/block/float/015.html [moved from LayoutTests/fast/block/float/015.html-disabled with 100% similarity]
LayoutTests/fast/images/style-access-during-imageChanged-style-freeze-expected.txt [new file with mode: 0644]
LayoutTests/fast/images/style-access-during-imageChanged-style-freeze.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/dom/Document.cpp
WebCore/dom/Document.h
WebCore/rendering/RenderImage.cpp

index 3095324..f2f134f 100644 (file)
@@ -1,3 +1,15 @@
+2010-11-16  Dan Bernstein  <mitz@apple.com>
+
+        Reviewed by Simon Fraser.
+
+        <rdar://problem/8672000> REGRESSION (r72040): Error image with alt text can cause style to be frozen in a subtree
+        https://bugs.webkit.org/show_bug.cgi?id=49579
+
+        * fast/block/float/015.html: Copied from LayoutTests/fast/block/float/015.html-disabled.
+        * fast/block/float/015.html-disabled: Removed.
+        * fast/images/style-access-during-imageChanged-style-freeze-expected.txt: Added.
+        * fast/images/style-access-during-imageChanged-style-freeze.html: Added.
+
 2010-11-16  David Levin  <levin@chromium.org>
 
         Unreviewed Chromium expectations update.
diff --git a/LayoutTests/fast/images/style-access-during-imageChanged-style-freeze-expected.txt b/LayoutTests/fast/images/style-access-during-imageChanged-style-freeze-expected.txt
new file mode 100644 (file)
index 0000000..28f4629
--- /dev/null
@@ -0,0 +1,3 @@
+
+This should be green, with a wide broken image above.
+PASS
diff --git a/LayoutTests/fast/images/style-access-during-imageChanged-style-freeze.html b/LayoutTests/fast/images/style-access-during-imageChanged-style-freeze.html
new file mode 100644 (file)
index 0000000..ee05259
--- /dev/null
@@ -0,0 +1,30 @@
+<style>
+    #target[foo] { color: green; }
+</style>
+<div id="container">
+    <div>
+        <img src="x-invalid:" alt="Some wide text">
+        <div id="target">
+            This should be green, with a wide broken image above.
+        </div>
+    </div>
+</div>
+<div id="result"></div>
+<script>
+    onload = function() {
+        if (window.layoutTestController)
+            layoutTestController.dumpAsText();
+
+        document.body.offsetTop;
+        var container = document.getElementById("container");
+        container.style.display = "none";
+        document.body.offsetTop;
+        container.style.removeProperty("display");
+        document.body.offsetTop;
+        var target = document.getElementById("target");
+        target.setAttribute("foo");
+        document.body.offsetTop;
+        var result = document.getElementById("result");
+        result.innerText = getComputedStyle(target).color === "rgb(0, 128, 0)" ? "PASS" : "FAIL";
+    }
+</script>
index e744b66..69e45ee 100644 (file)
@@ -1,3 +1,27 @@
+2010-11-16  Dan Bernstein  <mitz@apple.com>
+
+        Reviewed by Simon Fraser.
+
+        <rdar://problem/8672000> REGRESSION (r72040): Error image with alt text can cause style to be frozen in a subtree
+        https://bugs.webkit.org/show_bug.cgi?id=49579
+
+        r72040 introduced a call to setNeedsStyleRecalc() from RenderImage::imageChanged(). When imageChanged()
+        got called beneath recalcStyle() on some ancestor element, the result was that the ancestor’s
+        childNeedsStyleRecalc flag got cleared, but its descendants all the way down to the image did not.
+        Thereafter, setNeedsStyleRecalc() would fail to propagate from that subtree up to the root. The fix
+        is to avoid the newly-added setNeedsStyleRecalc() in most cases, including during reclacStyle(), and
+        just keep it for when it is needed.
+
+        Tests: fast/block/float/015.html
+               fast/images/style-access-during-imageChanged-style-freeze.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::isPendingStyleRecalc): Added.
+        * dom/Document.h:
+        * rendering/RenderImage.cpp:
+        (WebCore::RenderImage::imageChanged): Only defer intrinsic size compoutation if a style recalc
+        is coming (indicating that current style() is stale).
+
 2010-11-11  Zhenyao Mo  <zmo@google.com>
 
         Reviewed by Kenneth Russell.
index 4c3425a..9ad956f 100644 (file)
@@ -1506,6 +1506,11 @@ void Document::unscheduleStyleRecalc()
     m_pendingStyleRecalcShouldForce = false;
 }
 
+bool Document::isPendingStyleRecalc() const
+{
+    return m_styleRecalcTimer.isActive() && !m_inStyleRecalc;
+}
+
 void Document::styleRecalcTimerFired(Timer<Document>*)
 {
     updateStyleIfNeeded();
index ba48d6f..727cf7c 100644 (file)
@@ -671,6 +671,7 @@ public:
     void scheduleForcedStyleRecalc();
     void scheduleStyleRecalc();
     void unscheduleStyleRecalc();
+    bool isPendingStyleRecalc() const;
     void styleRecalcTimerFired(Timer<Document>*);
 
     void attachNodeIterator(NodeIterator*);
index 92d8d97..fe4373a 100644 (file)
@@ -150,7 +150,7 @@ void RenderImage::imageChanged(WrappedImagePtr newImage, const IntRect* rect)
 
     // Set image dimensions, taking into account the size of the alt text.
     if (m_imageResource->errorOccurred()) {
-        if (!m_altText.isEmpty()) {
+        if (!m_altText.isEmpty() && document()->isPendingStyleRecalc()) {
             ASSERT(node());
             if (node()) {
                 m_needsToSetSizeForAltText = true;
@@ -158,11 +158,7 @@ void RenderImage::imageChanged(WrappedImagePtr newImage, const IntRect* rect)
             }
             return;
         }
-        IntSize errorImageSize = imageSizeForError(m_imageResource->cachedImage());
-        if (errorImageSize != intrinsicSize()) {
-            setIntrinsicSize(errorImageSize);
-            imageSizeChanged = true;
-        }
+        imageSizeChanged = setImageSizeForAltText(m_imageResource->cachedImage());
     }
 
     imageDimensionsChanged(imageSizeChanged, rect);