[Ozone-DRI] Do proper bounds checks when moving the cursor

[Ozone-DRI] Do proper bounds checks when moving the cursor

Related changes:
 - Created a DriWindowManager to keep track of the platform windows.
   These are then used to perform the bounds clipping for the cursor.
 - Moved delegate manager to DriWindowDelegateManager. In a follow-up CL
   the delegate manager in the browser process will be removed in favor
   of holing on to the DriWindowDelegates on the DriWindow.

BUG=none
NOTRY=true

Committed: https://crrev.com/774231cb2bb8fad68edcba218b5ed07ae339770e
Cr-Commit-Position: refs/heads/master@{#294926}

[dnicoara, 2014-09-12 18:46:36]

dnicoara@chromium.org changed reviewers:
+ spang@chromium.org

[dnicoara, 2014-09-12 18:46:37]

PTAL

[spang, 2014-09-12 19:26:53]

On 2014/09/12 18:46:37, dnicoara wrote:
> PTAL

Thanks, it bugged me that the window manager was no longer managing the thing we
called "window".

As an aside, Here's what I would like to see as far as moving parts & their
names (hopefully soon):

DriWindowManagerHost(browser)
DriWindowHost(browser)

// --- IPC boundary ---

DriWindowManager(gpu)
DriWindow(gpu)

I realize that it shuffles the names from what we have now but those names are
surely less surprising to other chromium people. The window is the server side
object; the browser side thing is just its "host".

[spang, 2014-09-12 19:28:39]

lgtm

lgtm with nits

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_cursor.cc (right):

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_cursor.cc:61: if (cursor_widget_ !=
gfx::kNullAcceleratedWidget)
Can you move this into HideCursor and lose the DCHECK?

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_cursor.cc:64: cursor_widget_ = widget;
There's no reason for these assignments to be done conditionally. Assigning
everything is simpler.

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_cursor.cc:65: cursor_window_ =
window_manager_->GetWindow(widget);
How do you ensure this ptr doesn't become dangling?

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_cursor.cc:70: const gfx::Size& size =
cursor_window_->GetBounds().size();
gfx::Size size =

[dnicoara, 2014-09-12 20:24:18]

With regards to your first post:

I'm wondering if we'll even need some of these renames? DriWindow and
DriWindowManager are supposed to be browser side only. DriWindowDelegate is the
class the bridges the 2 processes.

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_cursor.cc (right):

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_cursor.cc:61: if (cursor_widget_ !=
gfx::kNullAcceleratedWidget)
On 2014/09/12 19:28:39, spang wrote:
> Can you move this into HideCursor and lose the DCHECK?

I'd prefer to keep this in here since it is only needed in the initialization
process.

It should never be the NullAcceleratedWidget.

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_cursor.cc:64: cursor_widget_ = widget;
On 2014/09/12 19:28:39, spang wrote:
> There's no reason for these assignments to be done conditionally. Assigning
> everything is simpler.

I felt like cursor_widget_ and cursor_window_ should only be modified at the
same time to make it clearer that they are related. And I didn't want to ask the
window_manager_ for the window every time.

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_cursor.cc:65: cursor_window_ =
window_manager_->GetWindow(widget);
On 2014/09/12 19:28:39, spang wrote:
> How do you ensure this ptr doesn't become dangling?

Setting/moving the cursor on an invalid widget is erroneous. Currently the
cursor is warped to another window when the active WTH is deleted. But I do
understand the paranoia. Would you prefer to get the window every time this
function is called? Or just (D)CHECK that?

[spang, 2014-09-12 21:05:33]

On 2014/09/12 20:24:18, dnicoara wrote:
> With regards to your first post:
> 
> I'm wondering if we'll even need some of these renames? DriWindow and
> DriWindowManager are supposed to be browser side only. DriWindowDelegate is
the
> class the bridges the 2 processes.

I think we should bite the bullet and rename things to match convention. Can you
name any other examples of this pattern (Foo -> controlling object in primary
process, FooDelegate -> controlled object in secondary process)? I can't, every
example I can find is FooHost & Foo.

Foo & FooDelegate makes me think I can plug in my own FooDelegate, but that's
actually not something we want to allow (we should merge DriWindowDelegateProxy
into DriWindow.. they won't need to be separate anymore once we've cleaned
things up).

> 
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> File ui/ozone/platform/dri/dri_cursor.cc (right):
> 
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> ui/ozone/platform/dri/dri_cursor.cc:61: if (cursor_widget_ !=
> gfx::kNullAcceleratedWidget)
> On 2014/09/12 19:28:39, spang wrote:
> > Can you move this into HideCursor and lose the DCHECK?
> 
> I'd prefer to keep this in here since it is only needed in the initialization
> process.
> 
> It should never be the NullAcceleratedWidget.
> 
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> ui/ozone/platform/dri/dri_cursor.cc:64: cursor_widget_ = widget;
> On 2014/09/12 19:28:39, spang wrote:
> > There's no reason for these assignments to be done conditionally. Assigning
> > everything is simpler.
> 
> I felt like cursor_widget_ and cursor_window_ should only be modified at the
> same time to make it clearer that they are related. And I didn't want to ask
the
> window_manager_ for the window every time.
> 
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> ui/ozone/platform/dri/dri_cursor.cc:65: cursor_window_ =
> window_manager_->GetWindow(widget);
> On 2014/09/12 19:28:39, spang wrote:
> > How do you ensure this ptr doesn't become dangling?
> 
> Setting/moving the cursor on an invalid widget is erroneous. Currently the
> cursor is warped to another window when the active WTH is deleted. But I do
> understand the paranoia. Would you prefer to get the window every time this
> function is called? Or just (D)CHECK that?

[spang, 2014-09-12 21:05:47]

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_cursor.cc (right):

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_cursor.cc:65: cursor_window_ =
window_manager_->GetWindow(widget);
On 2014/09/12 20:24:17, dnicoara wrote:
> On 2014/09/12 19:28:39, spang wrote:
> > How do you ensure this ptr doesn't become dangling?
> 
> Setting/moving the cursor on an invalid widget is erroneous. Currently the
> cursor is warped to another window when the active WTH is deleted. But I do
> understand the paranoia. Would you prefer to get the window every time this
> function is called? Or just (D)CHECK that?

What if you delete the window under the cursor, while moving the mouse? You
can't say that evdev is doing erroneous things when it says move cursor 2px
right. It has no concept of windows.

We should also gracefully handle the case where there simply aren't any open
windows.

[dnicoara, 2014-09-12 21:24:52]

PTAL at patchset #3. I've had to revert the way we initialize the cursor_widget
since everything just assumes it is displayed on the first display.

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_cursor.cc (right):

https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_cursor.cc:65: cursor_window_ =
window_manager_->GetWindow(widget);
On 2014/09/12 21:05:47, spang wrote:
> On 2014/09/12 20:24:17, dnicoara wrote:
> > On 2014/09/12 19:28:39, spang wrote:
> > > How do you ensure this ptr doesn't become dangling?
> > 
> > Setting/moving the cursor on an invalid widget is erroneous. Currently the
> > cursor is warped to another window when the active WTH is deleted. But I do
> > understand the paranoia. Would you prefer to get the window every time this
> > function is called? Or just (D)CHECK that?
> 
> What if you delete the window under the cursor, while moving the mouse? You
> can't say that evdev is doing erroneous things when it says move cursor 2px
> right. It has no concept of windows.
> 

Can't happen at the same time since all the changes are done on the UI thread in
the browser.

> We should also gracefully handle the case where there simply aren't any open
> windows.

I think this case is unreasonable. In fact I've had to adjust the way DriCursor
is initialized since everything assumes the cursor is displayed on the first
display (by default).

[dnicoara, 2014-09-12 21:31:07]

On 2014/09/12 21:05:33, spang wrote:
> On 2014/09/12 20:24:18, dnicoara wrote:
> > With regards to your first post:
> > 
> > I'm wondering if we'll even need some of these renames? DriWindow and
> > DriWindowManager are supposed to be browser side only. DriWindowDelegate is
> the
> > class the bridges the 2 processes.
> 
> I think we should bite the bullet and rename things to match convention. Can
you
> name any other examples of this pattern (Foo -> controlling object in primary
> process, FooDelegate -> controlled object in secondary process)? I can't,
every
> example I can find is FooHost & Foo.
> 

I'm fine with re-naming. In fact we should go further and compartmentalize
better (as you've suggested).

> Foo & FooDelegate makes me think I can plug in my own FooDelegate, but that's
> actually not something we want to allow (we should merge
DriWindowDelegateProxy
> into DriWindow.. they won't need to be separate anymore once we've cleaned
> things up).
> 

The idea was that the delegate would only perform actions that needed to be done
in the GPU process while still allowing in-process exection (by swaping the
delegate). I thought it would be wasteful to have an in-process DriWindow
duplicate much of the same functionality.

But now that you're looking at emulating the channel for software rendering, it
may not be an issue anymore.

> > 
> >
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> > File ui/ozone/platform/dri/dri_cursor.cc (right):
> > 
> >
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> > ui/ozone/platform/dri/dri_cursor.cc:61: if (cursor_widget_ !=
> > gfx::kNullAcceleratedWidget)
> > On 2014/09/12 19:28:39, spang wrote:
> > > Can you move this into HideCursor and lose the DCHECK?
> > 
> > I'd prefer to keep this in here since it is only needed in the
initialization
> > process.
> > 
> > It should never be the NullAcceleratedWidget.
> > 
> >
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> > ui/ozone/platform/dri/dri_cursor.cc:64: cursor_widget_ = widget;
> > On 2014/09/12 19:28:39, spang wrote:
> > > There's no reason for these assignments to be done conditionally.
Assigning
> > > everything is simpler.
> > 
> > I felt like cursor_widget_ and cursor_window_ should only be modified at the
> > same time to make it clearer that they are related. And I didn't want to ask
> the
> > window_manager_ for the window every time.
> > 
> >
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> > ui/ozone/platform/dri/dri_cursor.cc:65: cursor_window_ =
> > window_manager_->GetWindow(widget);
> > On 2014/09/12 19:28:39, spang wrote:
> > > How do you ensure this ptr doesn't become dangling?
> > 
> > Setting/moving the cursor on an invalid widget is erroneous. Currently the
> > cursor is warped to another window when the active WTH is deleted. But I do
> > understand the paranoia. Would you prefer to get the window every time this
> > function is called? Or just (D)CHECK that?

[spang, 2014-09-12 21:42:08]

On 2014/09/12 21:24:52, dnicoara wrote:
> PTAL at patchset #3. I've had to revert the way we initialize the
cursor_widget
> since everything just assumes it is displayed on the first display.
> 
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> File ui/ozone/platform/dri/dri_cursor.cc (right):
> 
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> ui/ozone/platform/dri/dri_cursor.cc:65: cursor_window_ =
> window_manager_->GetWindow(widget);
> On 2014/09/12 21:05:47, spang wrote:
> > On 2014/09/12 20:24:17, dnicoara wrote:
> > > On 2014/09/12 19:28:39, spang wrote:
> > > > How do you ensure this ptr doesn't become dangling?
> > > 
> > > Setting/moving the cursor on an invalid widget is erroneous. Currently the
> > > cursor is warped to another window when the active WTH is deleted. But I
do
> > > understand the paranoia. Would you prefer to get the window every time
this
> > > function is called? Or just (D)CHECK that?
> > 
> > What if you delete the window under the cursor, while moving the mouse? You
> > can't say that evdev is doing erroneous things when it says move cursor 2px
> > right. It has no concept of windows.
> > 
> 
> Can't happen at the same time since all the changes are done on the UI thread
in
> the browser.

Concurrency isn't the issue. The issue is whether this is object is in a safe
state when calls to our API return.

Right now tearing down a window can leave dangling pointers inside DriCursor. I
think that's unacceptable.

> > We should also gracefully handle the case where there simply aren't any open
> > windows.
> 
> I think this case is unreasonable. In fact I've had to adjust the way
DriCursor
> is initialized since everything assumes the cursor is displayed on the first
> display (by default).

It seems very natural to place the cursor when a first window opens, if it's not
already placed. That doesn't mean the last window should be prevented from
closing. I don't really understand your thesis here because we definitely have 0
windows at startup, and also at shutdown.

[dnicoara, 2014-09-15 17:41:25]

On 2014/09/12 21:42:08, spang wrote:
> On 2014/09/12 21:24:52, dnicoara wrote:
> > PTAL at patchset #3. I've had to revert the way we initialize the
> cursor_widget
> > since everything just assumes it is displayed on the first display.
> > 
> >
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> > File ui/ozone/platform/dri/dri_cursor.cc (right):
> > 
> >
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> > ui/ozone/platform/dri/dri_cursor.cc:65: cursor_window_ =
> > window_manager_->GetWindow(widget);
> > On 2014/09/12 21:05:47, spang wrote:
> > > On 2014/09/12 20:24:17, dnicoara wrote:
> > > > On 2014/09/12 19:28:39, spang wrote:
> > > > > How do you ensure this ptr doesn't become dangling?
> > > > 
> > > > Setting/moving the cursor on an invalid widget is erroneous. Currently
the
> > > > cursor is warped to another window when the active WTH is deleted. But I
> do
> > > > understand the paranoia. Would you prefer to get the window every time
> this
> > > > function is called? Or just (D)CHECK that?
> > > 
> > > What if you delete the window under the cursor, while moving the mouse?
You
> > > can't say that evdev is doing erroneous things when it says move cursor
2px
> > > right. It has no concept of windows.
> > > 
> > 
> > Can't happen at the same time since all the changes are done on the UI
thread
> in
> > the browser.
> 
> Concurrency isn't the issue. The issue is whether this is object is in a safe
> state when calls to our API return.
> 
> Right now tearing down a window can leave dangling pointers inside DriCursor.
I
> think that's unacceptable.
> 
> > > We should also gracefully handle the case where there simply aren't any
open
> > > windows.
> > 
> > I think this case is unreasonable. In fact I've had to adjust the way
> DriCursor
> > is initialized since everything assumes the cursor is displayed on the first
> > display (by default).
> 
> It seems very natural to place the cursor when a first window opens, if it's
not
> already placed. That doesn't mean the last window should be prevented from
> closing. I don't really understand your thesis here because we definitely have
0
> windows at startup, and also at shutdown.

Right, but as is we don't have that functionality. As is we are still relying on
the fact that no cursor operations are performed on invalid widgets. I'm planing
on a follow-up CL to add more correctness to cursors (involve DriWindowManager
in the setting/updating the cursor widget).

Anyways, I've added more validations to make sure the cursor window is valid.
Let me know if you have any more concerns.

[dnicoara, 2014-09-15 18:15:38]

Updated and moved one early return before getting the DriWindow if widget is
kNullAcceleratedWidget.

[dnicoara, 2014-09-15 18:15:39]

Updated and moved one early return before getting the DriWindow if widget is
kNullAcceleratedWidget.

[spang, 2014-09-15 18:20:05]

On 2014/09/15 17:41:25, dnicoara wrote:
> On 2014/09/12 21:42:08, spang wrote:
> > On 2014/09/12 21:24:52, dnicoara wrote:
> > > PTAL at patchset #3. I've had to revert the way we initialize the
> > cursor_widget
> > > since everything just assumes it is displayed on the first display.
> > > 
> > >
> >
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> > > File ui/ozone/platform/dri/dri_cursor.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/556073003/diff/40001/ui/ozone/platform/dri/dr...
> > > ui/ozone/platform/dri/dri_cursor.cc:65: cursor_window_ =
> > > window_manager_->GetWindow(widget);
> > > On 2014/09/12 21:05:47, spang wrote:
> > > > On 2014/09/12 20:24:17, dnicoara wrote:
> > > > > On 2014/09/12 19:28:39, spang wrote:
> > > > > > How do you ensure this ptr doesn't become dangling?
> > > > > 
> > > > > Setting/moving the cursor on an invalid widget is erroneous. Currently
> the
> > > > > cursor is warped to another window when the active WTH is deleted. But
I
> > do
> > > > > understand the paranoia. Would you prefer to get the window every time
> > this
> > > > > function is called? Or just (D)CHECK that?
> > > > 
> > > > What if you delete the window under the cursor, while moving the mouse?
> You
> > > > can't say that evdev is doing erroneous things when it says move cursor
> 2px
> > > > right. It has no concept of windows.
> > > > 
> > > 
> > > Can't happen at the same time since all the changes are done on the UI
> thread
> > in
> > > the browser.
> > 
> > Concurrency isn't the issue. The issue is whether this is object is in a
safe
> > state when calls to our API return.
> > 
> > Right now tearing down a window can leave dangling pointers inside
DriCursor.
> I
> > think that's unacceptable.
> > 
> > > > We should also gracefully handle the case where there simply aren't any
> open
> > > > windows.
> > > 
> > > I think this case is unreasonable. In fact I've had to adjust the way
> > DriCursor
> > > is initialized since everything assumes the cursor is displayed on the
first
> > > display (by default).
> > 
> > It seems very natural to place the cursor when a first window opens, if it's
> not
> > already placed. That doesn't mean the last window should be prevented from
> > closing. I don't really understand your thesis here because we definitely
have
> 0
> > windows at startup, and also at shutdown.
> 
> Right, but as is we don't have that functionality. As is we are still relying
on
> the fact that no cursor operations are performed on invalid widgets. I'm
planing
> on a follow-up CL to add more correctness to cursors (involve DriWindowManager
> in the setting/updating the cursor widget).
> 
> Anyways, I've added more validations to make sure the cursor window is valid.
> Let me know if you have any more concerns.

This is better.. NULL dereference is a lot easier to diagnose than
use-after-free.

Still need to either reposition or safely disable the cursor when the window
underneath it is destroyed. Does your followup do that?

[spang, 2014-09-15 18:23:34]

https://codereview.chromium.org/556073003/diff/100001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_cursor.cc (right):

https://codereview.chromium.org/556073003/diff/100001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_cursor.cc:66: if (cursor_window_ ==
gfx::kNullAcceleratedWidget || !window)
Shouldn't this just be

if (!window)

https://codereview.chromium.org/556073003/diff/140001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_window_manager.cc (right):

https://codereview.chromium.org/556073003/diff/140001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_window_manager.cc:43: NOTREACHED() << "Attempting to
get non-existing window " << widget;
Wait.. you're handling NULL in the caller, but it's NOTREACHED() here.

Can't have it both ways. Either it's allowed to fail or its not.

[spang, 2014-09-15 18:37:39]

lgtm

[dnicoara, 2014-09-15 18:38:39]

In the follow-up CL I plan on having DriWindowManager update the current_window_
for the cursor when the initial window is created and when the current_window_
is destroyed.

https://codereview.chromium.org/556073003/diff/140001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_window_manager.cc (right):

https://codereview.chromium.org/556073003/diff/140001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_window_manager.cc:43: NOTREACHED() << "Attempting to
get non-existing window " << widget;
On 2014/09/15 18:23:34, spang wrote:
> Wait.. you're handling NULL in the caller, but it's NOTREACHED() here.
> 
> Can't have it both ways. Either it's allowed to fail or its not.

Done.

[dnicoara, 2014-09-15 22:02:22]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-09-15 22:04:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/556073003/160001

[commit-bot: I haz the power, 2014-09-15 23:43:37]

Committed patchset #9 (id:160001) as 1d593111d5e1e6c97eb64f1d399e4a79d1a14ac6

[commit-bot: I haz the power, 2014-09-15 23:48:32]

Patchset 9 (id:??) landed as
https://crrev.com/774231cb2bb8fad68edcba218b5ed07ae339770e
Cr-Commit-Position: refs/heads/master@{#294926}