[webkit-changes] [126591] trunk

Fri, 24 Aug 2012 09:39:08 -0700

Title: [126591] trunk
Revision
126591
Author
[email protected]
Date
2012-08-24 09:39:43 -0700 (Fri, 24 Aug 2012)

Log Message

Crash in RenderTableCell::borderTop() due to custom scrollbars after r124168
https://bugs.webkit.org/show_bug.cgi?id=93903

Reviewed by Tony Chang.

Source/WebCore:

r124168 changed when we create scrollbars so that it happens (rightly) at style change time.
However the RenderScrollbar code assumes that any overflow: scroll RenderScrollbar would be
created at layout time as it directly tries to layout to scrollbar parts. The big issues with
the move is that the first style change operates on a detached renderer which means that we
could crash in some situation.

Test: scrollbars/custom-scrollbar-table-cell.html

* rendering/RenderScrollbarPart.cpp:
(WebCore::RenderScrollbarPart::computeScrollbarWidth):
(WebCore::RenderScrollbarPart::computeScrollbarHeight):
Fixed the crash by using style information instead of calling the renderer. This is guaranteed
to be safe but it also means that custom scrollbars sizing is not right on table cells with
collapsing borders. The existing logic was querying layout information at style change so I
wouldn't bet on it working properly anyway.

LayoutTests:

* scrollbars/custom-scrollbar-table-cell-expected.png: Added.
* scrollbars/custom-scrollbar-table-cell-expected.txt: Added.
* scrollbars/custom-scrollbar-table-cell.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (126590 => 126591)


--- trunk/LayoutTests/ChangeLog	2012-08-24 16:37:54 UTC (rev 126590)
+++ trunk/LayoutTests/ChangeLog	2012-08-24 16:39:43 UTC (rev 126591)
@@ -1,3 +1,14 @@
+2012-08-24  Julien Chaffraix  <[email protected]>
+
+        Crash in RenderTableCell::borderTop() due to custom scrollbars after r124168
+        https://bugs.webkit.org/show_bug.cgi?id=93903
+
+        Reviewed by Tony Chang.
+
+        * scrollbars/custom-scrollbar-table-cell-expected.png: Added.
+        * scrollbars/custom-scrollbar-table-cell-expected.txt: Added.
+        * scrollbars/custom-scrollbar-table-cell.html: Added.
+
 2012-08-24  Alexander Pavlov  <[email protected]>
 
         Web Inspector: Unreviewed, fix test flakiness due to the recently introduced lazy panel loading.

Added: trunk/LayoutTests/scrollbars/custom-scrollbar-table-cell-expected.png (0 => 126591)


--- trunk/LayoutTests/scrollbars/custom-scrollbar-table-cell-expected.png	                        (rev 0)
+++ trunk/LayoutTests/scrollbars/custom-scrollbar-table-cell-expected.png	2012-08-24 16:39:43 UTC (rev 126591)
@@ -0,0 +1,6 @@
+\x89PNG
+
+
+IHDR X')tEXtchecksum318b23876b2fc88a43b95536cd392c13.\xDF*
+\xF8IDATx\x9C\xEDر\x8D\xC30\xC1\xE3\xC1e\xB0\xFFƤ>\xE8
+\xD8\xC0ʒ\x8C\x99\x94^\xB8\xF8c\xAD\xF5@\xE7\xFF\xEC\xBFF`\xC4@L`\xC4@L`\xC4@\xECq\xF6\xEEm\xDF\xF7\x8F\xFE\xCF9Z\xD7\xE1\x82X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81{\xBCzc|sױm\xDB\xFB\x9F\xE7\x9C\xC7-\x80\x9Br\xC1\x88	,\x80\x98\xC0\x88	,\x80\x98\xC0\x88	,\x80\x98\xC0\x88	,\x80\x98\xC0\x88	,\x80\x98\xC0\x88	,\x80\x98\xC0\x88	,\x80\x98\xC0\x88\x8D\xB5\xD6\xD9\xB8\xB1}\xDF?\xFA?\xE7<h	\\x87@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@L`\xC4@\xEC\xF1\xEAa\x8C\xF1\xCD\Ƕm\xEF\x9Es\xB7n\xCA &\xB0b &\xB0b &\xB0b &\xB0b &\xB0b &\xB0b &\xB0b 6\xD6Zgo\xF8).X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X1\x81X\xB1'̆\xAB=pK\x83IEND\xAEB`\x82
\ No newline at end of file

Added: trunk/LayoutTests/scrollbars/custom-scrollbar-table-cell-expected.txt (0 => 126591)


--- trunk/LayoutTests/scrollbars/custom-scrollbar-table-cell-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/scrollbars/custom-scrollbar-table-cell-expected.txt	2012-08-24 16:39:43 UTC (rev 126591)
@@ -0,0 +1 @@
+

Added: trunk/LayoutTests/scrollbars/custom-scrollbar-table-cell.html (0 => 126591)


--- trunk/LayoutTests/scrollbars/custom-scrollbar-table-cell.html	                        (rev 0)
+++ trunk/LayoutTests/scrollbars/custom-scrollbar-table-cell.html	2012-08-24 16:39:43 UTC (rev 126591)
@@ -0,0 +1,50 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+::-webkit-scrollbar {
+    width: 16px;
+    height: 16px;
+}
+
+::-webkit-scrollbar-track
+{
+    background-color: #E3E3E3;
+}
+
+::-webkit-scrollbar-thumb
+{
+    background: black;
+}
+
+.scroll-row {
+    display: table-row;
+}
+
+.scroll-cell {
+    display: table-cell;
+    overflow: scroll;
+    width: 50px;
+    height: 50px;
+}
+
+.overflowing {
+    width: 200px;
+    height: 200px;
+}
+</style>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText(true);
+</script>
+</head>
+<body>
+<!--
+    Bug 93903: Crash in RenderTableCell::borderTop() due to custom scrollbars after r124168
+    This test has PASSED if there are 2 grey custom scrollbars with a black thumb on each div below.
+    Note that currently the right scrollbar is missing due to https://bugs.webkit.org/show_bug.cgi?id=94054
+-->
+<div class="scroll-cell"><div class="overflowing"></div></div>
+<div class="scroll-row"><div class="scroll-cell"><div class="overflowing"></div></div></div>
+</body>
+</html>

Modified: trunk/Source/WebCore/ChangeLog (126590 => 126591)


--- trunk/Source/WebCore/ChangeLog	2012-08-24 16:37:54 UTC (rev 126590)
+++ trunk/Source/WebCore/ChangeLog	2012-08-24 16:39:43 UTC (rev 126591)
@@ -1,5 +1,28 @@
 2012-08-24  Julien Chaffraix  <[email protected]>
 
+        Crash in RenderTableCell::borderTop() due to custom scrollbars after r124168
+        https://bugs.webkit.org/show_bug.cgi?id=93903
+
+        Reviewed by Tony Chang.
+
+        r124168 changed when we create scrollbars so that it happens (rightly) at style change time.
+        However the RenderScrollbar code assumes that any overflow: scroll RenderScrollbar would be
+        created at layout time as it directly tries to layout to scrollbar parts. The big issues with
+        the move is that the first style change operates on a detached renderer which means that we
+        could crash in some situation.
+
+        Test: scrollbars/custom-scrollbar-table-cell.html
+
+        * rendering/RenderScrollbarPart.cpp:
+        (WebCore::RenderScrollbarPart::computeScrollbarWidth):
+        (WebCore::RenderScrollbarPart::computeScrollbarHeight):
+        Fixed the crash by using style information instead of calling the renderer. This is guaranteed
+        to be safe but it also means that custom scrollbars sizing is not right on table cells with
+        collapsing borders. The existing logic was querying layout information at style change so I
+        wouldn't bet on it working properly anyway.
+
+2012-08-24  Julien Chaffraix  <[email protected]>
+
         Remove RenderTableSection::removeChild
         https://bugs.webkit.org/show_bug.cgi?id=94883

Modified: trunk/Source/WebCore/rendering/RenderScrollbarPart.cpp (126590 => 126591)


--- trunk/Source/WebCore/rendering/RenderScrollbarPart.cpp	2012-08-24 16:37:54 UTC (rev 126590)
+++ trunk/Source/WebCore/rendering/RenderScrollbarPart.cpp	2012-08-24 16:39:43 UTC (rev 126591)
@@ -91,7 +91,9 @@
     if (!m_scrollbar->owningRenderer())
         return;
     RenderView* renderView = view();
-    int visibleSize = m_scrollbar->owningRenderer()->width() - m_scrollbar->owningRenderer()->borderLeft() - m_scrollbar->owningRenderer()->borderRight();
+    // FIXME: We are querying layout information but nothing guarantees that it's up-to-date, especially since we are called at style change.
+    // FIXME: Querying the style's border information doesn't work on table cells with collapsing borders.
+    int visibleSize = m_scrollbar->owningRenderer()->width() - m_scrollbar->owningRenderer()->style()->borderLeftWidth() - m_scrollbar->owningRenderer()->style()->borderRightWidth();
     int w = calcScrollbarThicknessUsing(MainOrPreferredSize, style()->width(), visibleSize, renderView);
     int minWidth = calcScrollbarThicknessUsing(MinSize, style()->minWidth(), visibleSize, renderView);
     int maxWidth = style()->maxWidth().isUndefined() ? w : calcScrollbarThicknessUsing(MaxSize, style()->maxWidth(), visibleSize, renderView);
@@ -107,7 +109,9 @@
     if (!m_scrollbar->owningRenderer())
         return;
     RenderView* renderView = view();
-    int visibleSize = m_scrollbar->owningRenderer()->height() -  m_scrollbar->owningRenderer()->borderTop() - m_scrollbar->owningRenderer()->borderBottom();
+    // FIXME: We are querying layout information but nothing guarantees that it's up-to-date, especially since we are called at style change.
+    // FIXME: Querying the style's border information doesn't work on table cells with collapsing borders.
+    int visibleSize = m_scrollbar->owningRenderer()->height() -  m_scrollbar->owningRenderer()->style()->borderTopWidth() - m_scrollbar->owningRenderer()->style()->borderBottomWidth();
     int h = calcScrollbarThicknessUsing(MainOrPreferredSize, style()->height(), visibleSize, renderView);
     int minHeight = calcScrollbarThicknessUsing(MinSize, style()->minHeight(), visibleSize, renderView);
     int maxHeight = style()->maxHeight().isUndefined() ? h : calcScrollbarThicknessUsing(MaxSize, style()->maxHeight(), visibleSize, renderView);

_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to