Bug 205889 - Remove LayoutUnit::operator(const float&) to avoid bad results when the LHS is accidentally made an int
Summary: Remove LayoutUnit::operator(const float&) to avoid bad results when the LHS i...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: EasyFix, GoodFirstBug
Depends on:
Blocks:
 
Reported: 2020-01-07 14:51 PST by Daniel Bates
Modified: 2020-01-07 14:58 PST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2020-01-07 14:51:49 PST
In the patch for bug #205563 I moved some code:

[[
...
LayoutUnit logicalWidth = snappedSelectionRect.width();
if (snappedSelectionRect.x() > logicalRight())
    logicalWidth = 0;
else if (snappedSelectionRect.maxX() > logicalRight())
    logicalWidth = logicalRight() - snappedSelectionRect.x();
...
]]

And in the process changed:

LayoutUnit logicalWidth = snappedSelectionRect.width();

to

auto logicalWidth = snappedSelectionRect.width();

As a result logicalWidth was now an int. This led to bad things when "snappedSelectionRect.maxX() > logicalRight()" evaluates to true because we would then compute "logicalRight() - snappedSelectionRect.x()" (which would be a float since logicalRight() is a float) and then integer truncate to assign to logicalWidth.

This bug would have been prevented if LayoutUnit::operator(const float&) did not exist because it would have forced the code to have been written (pre-my move):

[[
...
LayoutUnit logicalWidth = snappedSelectionRect.width();
if (snappedSelectionRect.x() > logicalRight())
    logicalWidth = 0;
else if (snappedSelectionRect.maxX() > logicalRight())
    logicalWidth = LayoutUnit { logicalRight() - snappedSelectionRect.x() };
...
]]

(Notice the explicit constructor call to LayoutUnit in the second branch).

And so if I had then changed LayoutUnit => auto it would have compile-time failed and I would have seen my mistake because the code would now be trying to illogically assign a LayoutUnit to an int.