Changeset 172317 in webkit

Timestamp:

Aug 7, 2014, 5:35:33 PM (11 years ago)

Author:

Simon Fraser

Message:

HTML <sub> and <sup> elements do not work in some 64-bit builds
​https://bugs.webkit.org/show_bug.cgi?id=135736

Reviewed by Tim Horton.

RootInlineBox::verticalPositionForBox() had some implicit conversions between
LayoutUnit and int that caused overflow, and resulted in different comparison
behavior with an int constant in different architectures, since overflow behavior
is undefined.

Specifically, VerticalPositionCache was written in terms of ints with a special
0x80000000 "not found" value. However, 0x80000000 was being assigned to
a LayoutUnit, which multiplies by 64 causing overflow. The result was then
compared again with 0x80000000 which could pass or fail depending on overflow
behavior.

Fix by converting VerticalPositionCache to use LayoutUnits, and to have a bool
return value with a result out param, instead of a special return value.

Not easily testable, since the difference does not show in DRT output,
and a ref test would be flakey.

• rendering/RootInlineBox.cpp:

(WebCore::RootInlineBox::ascentAndDescentForBox):

• rendering/VerticalPositionCache.h:

(WebCore::VerticalPositionCache::get):
(WebCore::VerticalPositionCache::set):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• rendering/RootInlineBox.cpp (modified) (1 diff)
• rendering/VerticalPositionCache.h (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-08-07  Simon Fraser  <simon.fraser@apple.com>
+
+        HTML <sub> and <sup> elements do not work in some 64-bit builds
+        https://bugs.webkit.org/show_bug.cgi?id=135736
+
+        Reviewed by Tim Horton.
+        
+        RootInlineBox::verticalPositionForBox() had some implicit conversions between
+        LayoutUnit and int that caused overflow, and resulted in different comparison
+        behavior with an int constant in different architectures, since overflow behavior
+        is undefined.
+        
+        Specifically, VerticalPositionCache was written in terms of ints with a special
+        0x80000000 "not found" value. However, 0x80000000 was being assigned to
+        a LayoutUnit, which multiplies by 64 causing overflow. The result was then
+        compared again with 0x80000000 which could pass or fail depending on overflow
+        behavior.
+        
+        Fix by converting VerticalPositionCache to use LayoutUnits, and to have a bool
+        return value with a result out param, instead of a special return value.
+
+        Not easily testable, since the difference does not show in DRT output,
+        and a ref test would be flakey.
+
+        * rendering/RootInlineBox.cpp:
+        (WebCore::RootInlineBox::ascentAndDescentForBox):
+        * rendering/VerticalPositionCache.h:
+        (WebCore::VerticalPositionCache::get):
+        (WebCore::VerticalPositionCache::set):
+
 2014-08-07  Benjamin Poulain  <bpoulain@apple.com>
 

trunk/Source/WebCore/rendering/RootInlineBox.cpp

@@ -935 +935 @@
     bool isRenderInline = renderer->isRenderInline();
     if (isRenderInline && !firstLine) {
-        LayoutUnit verticalPosition = verticalPositionCache.get(renderer, baselineType());
-        if (verticalPosition != PositionUndefined)
-            return verticalPosition;
+        LayoutUnit cachedPosition;
+        if (verticalPositionCache.get(renderer, baselineType(), cachedPosition))
+            return cachedPosition;
     }
 

trunk/Source/WebCore/rendering/VerticalPositionCache.h

@@ -34 +34 @@
 class RenderObject;
 
-// Values for vertical alignment.
-const int PositionUndefined = 0x80000000;
-
 class VerticalPositionCache {
     WTF_MAKE_NONCOPYABLE(VerticalPositionCache);
@@ -43 +40 @@
     { }
     
-    int get(RenderObject* renderer, FontBaseline baselineType) const
+    bool get(RenderObject* renderer, FontBaseline baselineType, LayoutUnit& result) const
     {
-        const HashMap<RenderObject*, int>& mapToCheck = baselineType == AlphabeticBaseline ? m_alphabeticPositions : m_ideographicPositions;
-        const HashMap<RenderObject*, int>::const_iterator it = mapToCheck.find(renderer);
+        const HashMap<RenderObject*, LayoutUnit>& mapToCheck = baselineType == AlphabeticBaseline ? m_alphabeticPositions : m_ideographicPositions;
+        const HashMap<RenderObject*, LayoutUnit>::const_iterator it = mapToCheck.find(renderer);
         if (it == mapToCheck.end())
-            return PositionUndefined;
-        return it->value;
+            return false;
+
+        result = it->value;
+        return true;
     }
     
-    void set(RenderObject* renderer, FontBaseline baselineType, int position)
+    void set(RenderObject* renderer, FontBaseline baselineType, LayoutUnit position)
     {
         if (baselineType == AlphabeticBaseline)
@@ -61 +60 @@
 
 private:
-    HashMap<RenderObject*, int> m_alphabeticPositions;
-    HashMap<RenderObject*, int> m_ideographicPositions;
+    HashMap<RenderObject*, LayoutUnit> m_alphabeticPositions;
+    HashMap<RenderObject*, LayoutUnit> m_ideographicPositions;
 };