Bug 14498 - RenderContainer::positionForCoordinates contains an order of operations error
Summary: RenderContainer::positionForCoordinates contains an order of operations error
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-07-02 11:31 PDT by Adam Roben (:aroben)
Modified: 2007-07-02 17:28 PDT (History)
2 users (show)

See Also:


Attachments
Test case for RenderContainer::positionForCoordinates (354 bytes, text/html)
2007-07-02 15:05 PDT, mitz
no flags Details
patch with changelog (6.76 KB, patch)
2007-07-02 16:46 PDT, Adam Roben (:aroben)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2007-07-02 11:31:39 PDT
Here's the code from RenderContainer::positionForCoordinates (also visible at http://trac.webkit.org/projects/webkit/browser/trunk/WebCore/rendering/RenderContainer.cpp#L58 ):

        int top = borderTop() + paddingTop() + isTableRow() ? 0 : renderer->xPos();
        int bottom = top + renderer->contentHeight();
        int left = borderLeft() + paddingLeft() + isTableRow() ? 0 : renderer->yPos();
        int right = left + renderer->contentWidth();

The problem (spotted by prefast) is that the + operator has higher precedence than the ternary operator, so the code evaluates like this (note the parentheses):

        int top = (borderTop() + paddingTop() + isTableRow()) ? 0 : renderer->xPos();
        int bottom = top + renderer->contentHeight();
        int left = (borderLeft() + paddingLeft() + isTableRow()) ? 0 : renderer->yPos();
        int right = left + renderer->contentWidth();

It's easy to fix (just put parentheses around the ternary expression), but I'm not sure how to make a test case.
Comment 1 Adam Roben (:aroben) 2007-07-02 11:33:06 PDT
Sorry, the link to the code should have been http://trac.webkit.org/projects/webkit/browser/trunk/WebCore/rendering/RenderContainer.cpp#L586
Comment 2 mitz 2007-07-02 11:41:14 PDT
(In reply to comment #0)

> It's easy to fix (just put parentheses around the ternary expression)

That "top" goes with "xPos" and "left" goes with "yPos" looks very suspicious as well!
Comment 3 mitz 2007-07-02 15:05:50 PDT
Created attachment 15356 [details]
Test case for RenderContainer::positionForCoordinates
Comment 4 Adam Roben (:aroben) 2007-07-02 16:46:43 PDT
Created attachment 15357 [details]
patch with changelog
Comment 5 Darin Adler 2007-07-02 16:49:02 PDT
Comment on attachment 15357 [details]
patch with changelog

r=me
Comment 6 Adam Roben (:aroben) 2007-07-02 17:28:52 PDT
Landed as r23938