Changeset 172317 in webkit


Ignore:
Timestamp:
Aug 7, 2014 5:35:33 PM (10 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:
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r172316 r172317  
     12014-08-07  Simon Fraser  <simon.fraser@apple.com>
     2
     3        HTML <sub> and <sup> elements do not work in some 64-bit builds
     4        https://bugs.webkit.org/show_bug.cgi?id=135736
     5
     6        Reviewed by Tim Horton.
     7       
     8        RootInlineBox::verticalPositionForBox() had some implicit conversions between
     9        LayoutUnit and int that caused overflow, and resulted in different comparison
     10        behavior with an int constant in different architectures, since overflow behavior
     11        is undefined.
     12       
     13        Specifically, VerticalPositionCache was written in terms of ints with a special
     14        0x80000000 "not found" value. However, 0x80000000 was being assigned to
     15        a LayoutUnit, which multiplies by 64 causing overflow. The result was then
     16        compared again with 0x80000000 which could pass or fail depending on overflow
     17        behavior.
     18       
     19        Fix by converting VerticalPositionCache to use LayoutUnits, and to have a bool
     20        return value with a result out param, instead of a special return value.
     21
     22        Not easily testable, since the difference does not show in DRT output,
     23        and a ref test would be flakey.
     24
     25        * rendering/RootInlineBox.cpp:
     26        (WebCore::RootInlineBox::ascentAndDescentForBox):
     27        * rendering/VerticalPositionCache.h:
     28        (WebCore::VerticalPositionCache::get):
     29        (WebCore::VerticalPositionCache::set):
     30
    1312014-08-07  Benjamin Poulain  <bpoulain@apple.com>
    232
  • trunk/Source/WebCore/rendering/RootInlineBox.cpp

    r170774 r172317  
    935935    bool isRenderInline = renderer->isRenderInline();
    936936    if (isRenderInline && !firstLine) {
    937         LayoutUnit verticalPosition = verticalPositionCache.get(renderer, baselineType());
    938         if (verticalPosition != PositionUndefined)
    939             return verticalPosition;
     937        LayoutUnit cachedPosition;
     938        if (verticalPositionCache.get(renderer, baselineType(), cachedPosition))
     939            return cachedPosition;
    940940    }
    941941
  • trunk/Source/WebCore/rendering/VerticalPositionCache.h

    r130612 r172317  
    3434class RenderObject;
    3535
    36 // Values for vertical alignment.
    37 const int PositionUndefined = 0x80000000;
    38 
    3936class VerticalPositionCache {
    4037    WTF_MAKE_NONCOPYABLE(VerticalPositionCache);
     
    4340    { }
    4441   
    45     int get(RenderObject* renderer, FontBaseline baselineType) const
     42    bool get(RenderObject* renderer, FontBaseline baselineType, LayoutUnit& result) const
    4643    {
    47         const HashMap<RenderObject*, int>& mapToCheck = baselineType == AlphabeticBaseline ? m_alphabeticPositions : m_ideographicPositions;
    48         const HashMap<RenderObject*, int>::const_iterator it = mapToCheck.find(renderer);
     44        const HashMap<RenderObject*, LayoutUnit>& mapToCheck = baselineType == AlphabeticBaseline ? m_alphabeticPositions : m_ideographicPositions;
     45        const HashMap<RenderObject*, LayoutUnit>::const_iterator it = mapToCheck.find(renderer);
    4946        if (it == mapToCheck.end())
    50             return PositionUndefined;
    51         return it->value;
     47            return false;
     48
     49        result = it->value;
     50        return true;
    5251    }
    5352   
    54     void set(RenderObject* renderer, FontBaseline baselineType, int position)
     53    void set(RenderObject* renderer, FontBaseline baselineType, LayoutUnit position)
    5554    {
    5655        if (baselineType == AlphabeticBaseline)
     
    6160
    6261private:
    63     HashMap<RenderObject*, int> m_alphabeticPositions;
    64     HashMap<RenderObject*, int> m_ideographicPositions;
     62    HashMap<RenderObject*, LayoutUnit> m_alphabeticPositions;
     63    HashMap<RenderObject*, LayoutUnit> m_ideographicPositions;
    6564};
    6665
Note: See TracChangeset for help on using the changeset viewer.