Adjust gfx::Rect's width and height to avoid integer overflow

ui/gfx/geometry/rect.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/geometry/rect.h"
 
 #include <algorithm>
 
 #if defined(OS_WIN)
 #include <windows.h>
@@ skipping 124 matching lines @@
 void Rect::Union(const Rect& rect) {
   if (IsEmpty()) {
     *this = rect;
     return;
   }
   if (rect.IsEmpty())
     return;
 
   int rx = std::min(x(), rect.x());
   int ry = std::min(y(), rect.y());
-  int rr = std::max(right(), rect.right());
-  int rb = std::max(bottom(), rect.bottom());
 
-  SetRect(rx, ry, rr - rx, rb - ry);
+  // This is a temporary fix to avoid integer overflow, gfx::Rect is not
+  // supposed to have instances whose right or bottom is greater than maximum
+  // integer. This hack should be removed if root cause is fixed.

[danakj, 2016/08/23 17:47:56]

I'm not sure what this comment means, there are like infinity root causes. Rect
should simply protect against being invalid.

[sunxd, 2016/08/23 19:08:24]

I'll remove this comment.

+  int64_t right = static_cast<int64_t>(x()) + width();

[danakj, 2016/08/23 17:47:56]

why not just use right()? Does the rect already have an invalid right()? Can you
fix the place that is causing that in this class too?

[sunxd, 2016/08/23 19:08:24]

It does not. Is there any difference between right() and bottom() that makes the
invalid value come with one but not with the other?

[danakj, 2016/08/23 19:25:12]

No, I mean it just depends on the inputs. Neither should be allowed to happen in
this class.

+  int64_t rect_right = static_cast<int64_t>(rect.x()) + rect.width();

[danakj, 2016/08/23 17:47:56]

What performs better? int64_ts with (properly) saturating downcast or
CheckedNumeric? Why did you choose int64?

[sunxd, 2016/08/23 19:08:24]

Hmm actually I have no idea which has a better performance. I basically just
copied the solution in PictureLayerImpl::SafeIntersectRects which was landed to
fix a related clusterfuzz bug.

I'll switch to CheckedNumeric if that's faster.

[danakj, 2016/08/23 19:25:12]

You could do a simple test to figure out what's better ala cc_perftests.

+  int64_t bottom = static_cast<int64_t>(y()) + height();
+  int64_t rect_bottom = static_cast<int64_t>(rect.y()) + rect.height();
+
+  int64_t rr = std::max(right, rect_right);
+  int64_t rb = std::max(bottom, rect_bottom);
+
+  SetRect(rx, ry, static_cast<int>(rr - rx), static_cast<int>(rb - ry));

[danakj, 2016/08/23 17:47:56]

Does static cast really do what you want?

http://stackoverflow.com/questions/5990576/static-cast-with-bounded-types

Can you demonstrate that this is doing what you expect with tests, to show that
we're not going to get a right/bottom() that overflow?

[sunxd, 2016/08/23 19:08:24]

Working on tests.

 }
 
 void Rect::Subtract(const Rect& rect) {
   if (!Intersects(rect))
     return;
   if (rect.Contains(*this)) {
     SetRect(0, 0, 0, 0);
     return;
   }
 
@@ skipping 125 matching lines @@
 
 Rect BoundingRect(const Point& p1, const Point& p2) {
   int rx = std::min(p1.x(), p2.x());
   int ry = std::min(p1.y(), p2.y());
   int rr = std::max(p1.x(), p2.x());
   int rb = std::max(p1.y(), p2.y());
   return Rect(rx, ry, rr - rx, rb - ry);
 }
 
 }  // namespace gfx