Make Mandoline shut down cleanly.

Make Mandoline shut down cleanly.

This involves:
-making an app notice when all the objects it vended are gone so that it can terminate
-fixing double deletes because of cycles in destruction callbacks
-ensuring that child X windows are destructed before the parent

BUG=484234

[jam, 2015-05-11 22:05:55]

Patchset #2 (id:20001) has been deleted

[jam, 2015-05-13 17:17:08]

Patchset #6 (id:110001) has been deleted

[jam, 2015-05-13 18:18:55]

Patchset #6 (id:130001) has been deleted

[jam, 2015-05-13 18:28:12]

Patchset #6 (id:150001) has been deleted

[jam, 2015-05-13 19:50:19]

jam@chromium.org changed reviewers:
+ erg@chromium.org

[jam, 2015-05-13 19:55:09]

https://codereview.chromium.org/1139673003/diff/170001/components/native_view...
File components/native_viewport/main.cc (right):

https://codereview.chromium.org/1139673003/diff/170001/components/native_view...
components/native_viewport/main.cc:39: event_source_ =
ui::PlatformEventSource::CreateDefault();
this is moved to the app instead of NativeViewPortImpl because:
1) this is a singleton, and once we have multi-window we will have multiple
NativeViewPortImpl
2) this needs to outlive NativeViewPortImpl

[Ben Goodger (Google), 2015-05-13 20:21:06]

ben@chromium.org changed reviewers:
+ ben@chromium.org

[Ben Goodger (Google), 2015-05-13 20:21:07]

https://codereview.chromium.org/1139673003/diff/170001/components/native_view...
File components/native_viewport/main.cc (right):

https://codereview.chromium.org/1139673003/diff/170001/components/native_view...
components/native_viewport/main.cc:81: mojo::AppLifetimeHelper
app_lifetime_helper;
app_lifetime_helper_

https://codereview.chromium.org/1139673003/diff/170001/mojo/application/app_l...
File mojo/application/app_lifetime_helper.h (right):

https://codereview.chromium.org/1139673003/diff/170001/mojo/application/app_l...
mojo/application/app_lifetime_helper.h:17: ServiceRefcount(AppLifetimeHelper*
app_lifetime_helper);
explicit

https://codereview.chromium.org/1139673003/diff/170001/mojo/services/network/...
File mojo/services/network/network_service_delegate.h (right):

https://codereview.chromium.org/1139673003/diff/170001/mojo/services/network/...
mojo/services/network/network_service_delegate.h:35: mojo::AppLifetimeHelper
app_lifetime_helper;
__________________________________________

https://codereview.chromium.org/1139673003/diff/170001/ui/platform_window/win...
File ui/platform_window/win/win_window.cc (right):

https://codereview.chromium.org/1139673003/diff/170001/ui/platform_window/win...
ui/platform_window/win/win_window.cc:180: if (destroyed_)
Instead we should make ~WinWindow not call Destroy(). This may result in some
other code not closing the window but we should find those and change them to
call Close() prior to destructing this object.

[jam, 2015-05-14 14:42:48]

https://codereview.chromium.org/1139673003/diff/170001/components/native_view...
File components/native_viewport/main.cc (right):

https://codereview.chromium.org/1139673003/diff/170001/components/native_view...
components/native_viewport/main.cc:81: mojo::AppLifetimeHelper
app_lifetime_helper;
On 2015/05/13 20:21:07, Ben Goodger (Google) wrote:
> app_lifetime_helper_

Done.

https://codereview.chromium.org/1139673003/diff/170001/mojo/application/app_l...
File mojo/application/app_lifetime_helper.h (right):

https://codereview.chromium.org/1139673003/diff/170001/mojo/application/app_l...
mojo/application/app_lifetime_helper.h:17: ServiceRefcount(AppLifetimeHelper*
app_lifetime_helper);
On 2015/05/13 20:21:07, Ben Goodger (Google) wrote:
> explicit

Done.

https://codereview.chromium.org/1139673003/diff/170001/mojo/services/network/...
File mojo/services/network/network_service_delegate.h (right):

https://codereview.chromium.org/1139673003/diff/170001/mojo/services/network/...
mojo/services/network/network_service_delegate.h:35: mojo::AppLifetimeHelper
app_lifetime_helper;
On 2015/05/13 20:21:07, Ben Goodger (Google) wrote:
> __________________________________________

Done.

https://codereview.chromium.org/1139673003/diff/170001/ui/platform_window/win...
File ui/platform_window/win/win_window.cc (right):

https://codereview.chromium.org/1139673003/diff/170001/ui/platform_window/win...
ui/platform_window/win/win_window.cc:180: if (destroyed_)
On 2015/05/13 20:21:07, Ben Goodger (Google) wrote:
> Instead we should make ~WinWindow not call Destroy(). This may result in some
> other code not closing the window but we should find those and change them to
> call Close() prior to destructing this object.

Done.

[yzshen1, 2015-05-14 16:16:37]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2015-05-14 16:16:38]

https://codereview.chromium.org/1139673003/diff/260001/mojo/application/app_l...
File mojo/application/app_lifetime_helper.h (right):

https://codereview.chromium.org/1139673003/diff/260001/mojo/application/app_l...
mojo/application/app_lifetime_helper.h:56:
base::WeakPtrFactory<AppLifetimeHelper> weak_factory_;
According to our offline discussion:
A service may create objects living on different threads. However, Weak pointer
is not thread safe.

[Elliot Glaysher, 2015-05-14 22:53:40]

lgtm

https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
File components/native_viewport/onscreen_context_provider.cc (right):

https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
components/native_viewport/onscreen_context_provider.cc:22: for(const auto&
driver : command_buffers_)
for (

[Ben Goodger (Google), 2015-05-14 22:57:18]

https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
File components/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
components/native_viewport/native_viewport_impl.cc:160: delete this;
will this ever result in a double delete?

[jam, 2015-05-15 02:15:31]

https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
File components/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
components/native_viewport/native_viewport_impl.cc:160: delete this;
On 2015/05/14 22:57:18, Ben Goodger (Google) wrote:
> will this ever result in a double delete?

Not that I saw.

https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
File components/native_viewport/onscreen_context_provider.cc (right):

https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
components/native_viewport/onscreen_context_provider.cc:22: for(const auto&
driver : command_buffers_)
On 2015/05/14 22:53:40, Elliot Glaysher wrote:
> for (

Done.

https://codereview.chromium.org/1139673003/diff/260001/mojo/application/app_l...
File mojo/application/app_lifetime_helper.h (right):

https://codereview.chromium.org/1139673003/diff/260001/mojo/application/app_l...
mojo/application/app_lifetime_helper.h:56:
base::WeakPtrFactory<AppLifetimeHelper> weak_factory_;
On 2015/05/14 16:16:38, yzshen1 wrote:
> According to our offline discussion:
> A service may create objects living on different threads. However, Weak
pointer
> is not thread safe.

Thanks, nice catch. I've switched to use thread hopping.

[yzshen1, 2015-05-15 06:56:12]

https://codereview.chromium.org/1139673003/diff/300001/mojo/application/app_l...
File mojo/application/app_lifetime_helper.cc (right):

https://codereview.chromium.org/1139673003/diff/300001/mojo/application/app_l...
mojo/application/app_lifetime_helper.cc:38:
base::Bind(&AppLifetimeHelper::AddRef,
Is it possible to have races because of this AddRef delay? For example,

=================
Thread-1: (the thread on which AppLifetimeHelper lives)
...
condition_var.Wait();
app_ref_count_1.reset();
...
=================
Thread-2:
...
app_ref_count_2 = app_ref_count_1.Clone();
condition_var.Signal();
...
=================

If |app_ref_count_1| is the only ref count before |app_ref_count_2| is created,
then it seems possible that the application is terminated before AddRef for
|app_ref_count_2| executes.

One idea is to make AppRefCount a RefCountedThreadSafe object, services hold on
to scoped_refptr<AppRefCount>. In ~AppRefCount(), a task is posted to the
application thread to do Terminate.

What do you think? Thanks!

[jam, 2015-05-15 07:08:45]

https://codereview.chromium.org/1139673003/diff/300001/mojo/application/app_l...
File mojo/application/app_lifetime_helper.cc (right):

https://codereview.chromium.org/1139673003/diff/300001/mojo/application/app_l...
mojo/application/app_lifetime_helper.cc:38:
base::Bind(&AppLifetimeHelper::AddRef,
On 2015/05/15 06:56:12, yzshen1 wrote:
> Is it possible to have races because of this AddRef delay? For example,
> 
> =================
> Thread-1: (the thread on which AppLifetimeHelper lives)
> ...
> condition_var.Wait();
> app_ref_count_1.reset();
> ...
> =================
> Thread-2:
> ...
> app_ref_count_2 = app_ref_count_1.Clone();
> condition_var.Signal();
> ...
> =================
> 
> If |app_ref_count_1| is the only ref count before |app_ref_count_2| is
created,
> then it seems possible that the application is terminated before AddRef for
> |app_ref_count_2| executes.
> 
> One idea is to make AppRefCount a RefCountedThreadSafe object, services hold
on
> to scoped_refptr<AppRefCount>. In ~AppRefCount(), a task is posted to the
> application thread to do Terminate.
> 
> What do you think? Thanks!

I had thought about this, but I convinced myself that it's not a problem. Feel
free to poke holes through this logic:

I thought that if thread-2 is doing a Clone, then that means that thread 2 is
where a service is living. That means that thread 2 will be the one that
releases the object on which Clone is called on. So when that is released, it
will post task the Release, which will be behind the AddRef.

[yzshen1, 2015-05-15 07:17:09]

On 2015/05/15 07:08:45, jam wrote:
>
https://codereview.chromium.org/1139673003/diff/300001/mojo/application/app_l...
> File mojo/application/app_lifetime_helper.cc (right):
> 
>
https://codereview.chromium.org/1139673003/diff/300001/mojo/application/app_l...
> mojo/application/app_lifetime_helper.cc:38:
> base::Bind(&AppLifetimeHelper::AddRef,
> On 2015/05/15 06:56:12, yzshen1 wrote:
> > Is it possible to have races because of this AddRef delay? For example,
> > 
> > =================
> > Thread-1: (the thread on which AppLifetimeHelper lives)
> > ...
> > condition_var.Wait();
> > app_ref_count_1.reset();
> > ...
> > =================
> > Thread-2:
> > ...
> > app_ref_count_2 = app_ref_count_1.Clone();
> > condition_var.Signal();
> > ...
> > =================
> > 
> > If |app_ref_count_1| is the only ref count before |app_ref_count_2| is
> created,
> > then it seems possible that the application is terminated before AddRef for
> > |app_ref_count_2| executes.
> > 
> > One idea is to make AppRefCount a RefCountedThreadSafe object, services hold
> on
> > to scoped_refptr<AppRefCount>. In ~AppRefCount(), a task is posted to the
> > application thread to do Terminate.
> > 
> > What do you think? Thanks!
> 
> I had thought about this, but I convinced myself that it's not a problem. Feel
> free to poke holes through this logic:
> 
> I thought that if thread-2 is doing a Clone, then that means that thread 2 is
> where a service is living. That means that thread 2 will be the one that
> releases the object on which Clone is called on. So when that is released, it
> will post task the Release, which will be behind the AddRef.

I agree. If we enforce that AppRefCount's Clone() can only be called on the
thread where it is destructed, then the problem will go away.
It would be nice to have some check about this restriction. Otherwise it LGTM.
Thanks. :)

[jam, 2015-05-15 09:11:43]

Patchset #14 (id:100020) has been deleted

[jam, 2015-05-15 15:35:00]

Patchset #14 (id:100020) has been deleted

[jam, 2015-05-15 15:36:21]

jam@chromium.org changed reviewers:
+ sky@chromium.org

[jam, 2015-05-15 15:37:57]

On 2015/05/15 07:17:09, yzshen1 wrote:
> On 2015/05/15 07:08:45, jam wrote:
> >
>
https://codereview.chromium.org/1139673003/diff/300001/mojo/application/app_l...
> > File mojo/application/app_lifetime_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/1139673003/diff/300001/mojo/application/app_l...
> > mojo/application/app_lifetime_helper.cc:38:
> > base::Bind(&AppLifetimeHelper::AddRef,
> > On 2015/05/15 06:56:12, yzshen1 wrote:
> > > Is it possible to have races because of this AddRef delay? For example,
> > > 
> > > =================
> > > Thread-1: (the thread on which AppLifetimeHelper lives)
> > > ...
> > > condition_var.Wait();
> > > app_ref_count_1.reset();
> > > ...
> > > =================
> > > Thread-2:
> > > ...
> > > app_ref_count_2 = app_ref_count_1.Clone();
> > > condition_var.Signal();
> > > ...
> > > =================
> > > 
> > > If |app_ref_count_1| is the only ref count before |app_ref_count_2| is
> > created,
> > > then it seems possible that the application is terminated before AddRef
for
> > > |app_ref_count_2| executes.
> > > 
> > > One idea is to make AppRefCount a RefCountedThreadSafe object, services
hold
> > on
> > > to scoped_refptr<AppRefCount>. In ~AppRefCount(), a task is posted to the
> > > application thread to do Terminate.
> > > 
> > > What do you think? Thanks!
> > 
> > I had thought about this, but I convinced myself that it's not a problem.
Feel
> > free to poke holes through this logic:
> > 
> > I thought that if thread-2 is doing a Clone, then that means that thread 2
is
> > where a service is living. That means that thread 2 will be the one that
> > releases the object on which Clone is called on. So when that is released,
it
> > will post task the Release, which will be behind the AddRef.
> 
> I agree. If we enforce that AppRefCount's Clone() can only be called on the
> thread where it is destructed, then the problem will go away.
> It would be nice to have some check about this restriction. Otherwise it LGTM.
> Thanks. :)

Good idea, done.

Scott: please review the changes to view_manager_init.*. I ported the code from
window_manager_app.* to there after syncing past your change. I had to do this
or else the app didn't shutdown.

[Ben Goodger (Google), 2015-05-15 16:19:07]

https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
File components/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
components/native_viewport/native_viewport_impl.cc:160: delete this;
On 2015/05/15 02:15:31, jam wrote:
> On 2015/05/14 22:57:18, Ben Goodger (Google) wrote:
> > will this ever result in a double delete?
> 
> Not that I saw.

the reason why I ask is that see below in OnConnectionError. If the pipe is ever
broken this object will be deleted also. Perhaps that function should destroy
the platform viewport rather than deleting this object directly? Then this
function will be called by the platform viewport.

[jam, 2015-05-18 15:19:15]

On 2015/05/15 16:19:07, Ben Goodger (Google) wrote:
>
https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
> File components/native_viewport/native_viewport_impl.cc (right):
> 
>
https://codereview.chromium.org/1139673003/diff/260001/components/native_view...
> components/native_viewport/native_viewport_impl.cc:160: delete this;
> On 2015/05/15 02:15:31, jam wrote:
> > On 2015/05/14 22:57:18, Ben Goodger (Google) wrote:
> > > will this ever result in a double delete?
> > 
> > Not that I saw.
> 
> the reason why I ask is that see below in OnConnectionError. If the pipe is
ever
> broken this object will be deleted also.

But both code paths can't get hit for the same object, since if it's deleted in
OnDestroyed, the OnConnectionError method won't get called.


> Perhaps that function should destroy
> the platform viewport rather than deleting this object directly? Then this
> function will be called by the platform viewport.

[sky, 2015-05-18 15:55:08]

view_manager_init LGTM

[jam, 2015-05-22 20:34:35]

Closing this since I landed all this patch in smaller pieces.