linux_aura: Fix cursor memeory leak, which was redding the lsan bot.

linux_aura: Fix cursor memeory leak, which was redding the lsan bot.

views::corewm::CursorManager was no longer owned by anything, nor did it
register with the base::AtExitManager. This refcounts the number of DNWAs
and frees the shared CursorManager when the last one is destroyed. (We
can't just use a base::LazyInstance<> since that doesn't have any useful
guarantees on destruction ordering; we need to destroy CursorManager
before we tear down the X11 connection.)

BUG=334333
R=tdanderson
TBR=sadrul

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245090

[Elliot Glaysher, 2014-01-14 01:26:47]

tdanderson: review
sadrul: stamp

[tdanderson, 2014-01-14 03:56:22]

On 2014/01/14 01:26:47, Elliot Glaysher wrote:
> tdanderson: review
> sadrul: stamp

I will take a closer look at this first thing tomorrow.

(In the meanwhile, just an FYI I'm considering reverting the code
which makes cursor state global due to some fundamental problems
with how we can/cannot apply proper flags to incoming Windows mouse
messages. See issues 332430 and 333952 if you're interested in
the details.)

[tdanderson, 2014-01-14 17:42:27]

LGTM. Is there a bug/report corresponding to the lsan failures that you can link
to from the issue description?

https://codereview.chromium.org/134103006/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/134103006/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:213:
DesktopNativeCursorManager* DesktopNativeWidgetAura::native_cursor_manager_ =
|native_cursor_manager_| is OK to leave as a raw pointer here because it is
owned by the |cursor_manager_|. Is that correct?

[Elliot Glaysher, 2014-01-14 18:41:33]

On 2014/01/14 03:56:22, tdanderson wrote:
> On 2014/01/14 01:26:47, Elliot Glaysher wrote:
> > tdanderson: review
> > sadrul: stamp
> 
> I will take a closer look at this first thing tomorrow.
> 
> (In the meanwhile, just an FYI I'm considering reverting the code
> which makes cursor state global due to some fundamental problems
> with how we can/cannot apply proper flags to incoming Windows mouse
> messages. See issues 332430 and 333952 if you're interested in
> the details.)

I actually now suspect that this patch is wrong. Making this a LazyInstance
doesn't enforce shutdown order properly; the new crashes on the linux_aura
tryjobs happen because the DesktopNativeCursorManager is torn down *after* the
X11 connection is closed.

+1 to just reverting the global state and giving these objects a clear owner. Do
you have any other ideas?

[tdanderson, 2014-01-14 20:04:27]

On 2014/01/14 18:41:33, Elliot Glaysher wrote:
> On 2014/01/14 03:56:22, tdanderson wrote:
> > On 2014/01/14 01:26:47, Elliot Glaysher wrote:
> > > tdanderson: review
> > > sadrul: stamp
> > 
> > I will take a closer look at this first thing tomorrow.
> > 
> > (In the meanwhile, just an FYI I'm considering reverting the code
> > which makes cursor state global due to some fundamental problems
> > with how we can/cannot apply proper flags to incoming Windows mouse
> > messages. See issues 332430 and 333952 if you're interested in
> > the details.)
> 
> I actually now suspect that this patch is wrong. Making this a LazyInstance
> doesn't enforce shutdown order properly; the new crashes on the linux_aura
> tryjobs happen because the DesktopNativeCursorManager is torn down *after* the
> X11 connection is closed.
> 
> +1 to just reverting the global state and giving these objects a clear owner.
Do
> you have any other ideas?

Unfortunately I don't. I CC'ed you on issue 333952 and recorded your opinion
that we should revert the global state. If you have anything else to add, please
share it on that bug.

I have been holding off on reverting this since I'm waiting on ananta@ to look
at issue 332430, which is the biggest problem we have to solve on Windows if
the global cursor state code is to stay in. But I'm very doubtful that we will
be able to come up with a proper solution.

I'll ping ananta@ again.

[tdanderson, 2014-01-14 22:31:19]

On 2014/01/14 20:04:27, tdanderson wrote:
> On 2014/01/14 18:41:33, Elliot Glaysher wrote:
> > On 2014/01/14 03:56:22, tdanderson wrote:
> > > On 2014/01/14 01:26:47, Elliot Glaysher wrote:
> > > > tdanderson: review
> > > > sadrul: stamp
> > > 
> > > I will take a closer look at this first thing tomorrow.
> > > 
> > > (In the meanwhile, just an FYI I'm considering reverting the code
> > > which makes cursor state global due to some fundamental problems
> > > with how we can/cannot apply proper flags to incoming Windows mouse
> > > messages. See issues 332430 and 333952 if you're interested in
> > > the details.)
> > 
> > I actually now suspect that this patch is wrong. Making this a LazyInstance
> > doesn't enforce shutdown order properly; the new crashes on the linux_aura
> > tryjobs happen because the DesktopNativeCursorManager is torn down *after*
the
> > X11 connection is closed.
> > 
> > +1 to just reverting the global state and giving these objects a clear
owner.
> Do
> > you have any other ideas?
> 
> Unfortunately I don't. I CC'ed you on issue 333952 and recorded your opinion
> that we should revert the global state. If you have anything else to add,
please
> share it on that bug.
> 
> I have been holding off on reverting this since I'm waiting on ananta@ to look
> at issue 332430, which is the biggest problem we have to solve on Windows if
> the global cursor state code is to stay in. But I'm very doubtful that we will
> be able to come up with a proper solution.
> 
> I'll ping ananta@ again.

I spoke with ananta@, and it looks like I am going to revert r240976 but leave
the global cursor state patch in (r240827). Here is ananta@'s suggestion
for fixing the memory issues:

instead of a lazy instance perhaps you can allocate it once
and then delete it when the last rootwindow goes away
and then allocate it back
if needed
that would also solve the memory issue on linux

[Elliot Glaysher, 2014-01-15 01:37:51]

ptal. changed to manual refcounting.

[tdanderson, 2014-01-15 20:40:26]

Manual ref-counting patch LGTM with one nit.

https://codereview.chromium.org/134103006/diff/430001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right):

https://codereview.chromium.org/134103006/diff/430001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.h:298: static int
cursor_reference_count_;
Can you add a brief comment to explain why it is necessary to perform manual ref
counting in this case?

[commit-bot: I haz the power, 2014-01-15 21:28:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/134103006/500001

[sadrul, 2014-01-15 22:15:01]

LGTM

https://codereview.chromium.org/134103006/diff/500001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/134103006/diff/500001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:312: }
This looks a bit hairy. Perhaps there could be a
[D]CHECK_GE(cursor_reference_count_, 0) here

[commit-bot: I haz the power, 2014-01-16 03:18:37]

Change committed as 245090