Close AppWindow to perform clean shutdown

Close AppWindow to perform clean shutdown

It results in a use-after-free if AppWindow is not closed.

Add a new method DesktopController::CreateAppWindow()
-Rename existing CreateAppWindow() method to CreateShellAppWindow().
-Add a new method CreateAppWindow() to remember the AppWindow.
-Close the AppWindow in CloseAppWindows().

Implement ShellNativeAppWindow::Close()
-Call AppWindow::OnNativeClose() to destruct the AppWindow.

BUG=387288
Committed: https://crrev.com/06aaa078382af19a198b9669ffc4e6add92aa53d
Cr-Commit-Position: refs/heads/master@{#294124}

[hashimoto, 2014-09-08 09:14:31]

Patchset #1 (id:1) has been deleted

[hashimoto, 2014-09-08 09:22:07]

hashimoto@chromium.org changed reviewers:
+ jamescook@chromium.org

[James Cook, 2014-09-08 18:14:31]

I don't understand who owns the AppWindow* returned by CreateAppWindow(). Could
you clarify?

Also, is the plan to eliminate the existing ShellAppWindow? That might make the
code clearer -- then it would be more clear that DesktopController only owns one
window.

https://codereview.chromium.org/543123004/diff/20001/athena/main/athena_main.cc
File athena/main/athena_main.cc (right):

https://codereview.chromium.org/543123004/diff/20001/athena/main/athena_main....
athena/main/athena_main.cc:67: // Creates a new app window and adds it to the
desktop. The desktop maintains
The desktop maintains ownership? I thought it had a non-owning pointer?

https://codereview.chromium.org/543123004/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/543123004/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller.h:139: AppWindow* app_window_;
nit: Maybe comment that this is not owned?

[hashimoto, 2014-09-09 00:30:41]

On 2014/09/08 18:14:31, James Cook wrote:
> I don't understand who owns the AppWindow* returned by CreateAppWindow().
Could
> you clarify?
AppWindow is owned by ShellDesktopController, but scoped_ptr is not used because
the dtor of AppWindow is not public.

On Chrome, when the associated views::Widget gets closed, NativeAppWindowViews
calls AppWindow::OnNativeClose() and the AppWindow gets destroyed.
So when one wants to destroy an AppWindow, NativeAppWindowViews::Close() should
be called instead of destructing the AppWindow directly.
This is why AppWindow's dtor is not public.

On app_shell, the aura::Window associated with the AppWindow is owned by
AppWindowContents which itself is owned by AppWindow.
So to close the aura::Window, ShellNativeAppWindow::Close() calls
AppWindow::OnNativeClose() directly to destroy the AppWindow and the
aura::Window owned by it.
> 
> Also, is the plan to eliminate the existing ShellAppWindow? That might make
the
> code clearer -- then it would be more clear that DesktopController only owns
one
> window.
> 
>
https://codereview.chromium.org/543123004/diff/20001/athena/main/athena_main.cc
> File athena/main/athena_main.cc (right):
> 
>
https://codereview.chromium.org/543123004/diff/20001/athena/main/athena_main....
> athena/main/athena_main.cc:67: // Creates a new app window and adds it to the
> desktop. The desktop maintains
> The desktop maintains ownership? I thought it had a non-owning pointer?
> 
>
https://codereview.chromium.org/543123004/diff/20001/extensions/shell/browser...
> File extensions/shell/browser/shell_desktop_controller.h (right):
> 
>
https://codereview.chromium.org/543123004/diff/20001/extensions/shell/browser...
> extensions/shell/browser/shell_desktop_controller.h:139: AppWindow*
app_window_;
> nit: Maybe comment that this is not owned?

[James Cook, 2014-09-09 15:32:23]

LGTM. I think this code will be clearer when we delete ShellAppWIndow.

https://codereview.chromium.org/543123004/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/543123004/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller.h:139: AppWindow* app_window_;
On 2014/09/08 18:14:31, James Cook wrote:
> nit: Maybe comment that this is not owned?

Or comment that the memory is cleaned up by the NativeAppWindow?

It's just very confusing that the DesktopController "owns" a ShellAppWindow or
an AppWindow, but is responsible for deleting the ShellAppWindow and is not
(directly) responsible for deleting the AppWindow.

[hashimoto, 2014-09-09 16:32:44]

https://codereview.chromium.org/543123004/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/543123004/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller.h:139: AppWindow* app_window_;
On 2014/09/09 15:32:23, James Cook wrote:
> On 2014/09/08 18:14:31, James Cook wrote:
> > nit: Maybe comment that this is not owned?
> 
> Or comment that the memory is cleaned up by the NativeAppWindow?
> 
> It's just very confusing that the DesktopController "owns" a ShellAppWindow or
> an AppWindow, but is responsible for deleting the ShellAppWindow and is not
> (directly) responsible for deleting the AppWindow.

Added comments here and in ShellDesktopController::CloseAppWindows().

[hashimoto, 2014-09-09 16:36:23]

hashimoto@chromium.org changed reviewers:
+ oshima@chromium.org

[hashimoto, 2014-09-09 16:36:24]

oshima@, could you review athena_main.cc as an owner?
Once we switch to app.window.create API and remove shell.createWindow, we can
get rid of AthenaDesktopController at all. (GetHost() is not used by anyone.
CreateShellAppWindow() will be unused. Other methods are not implemented.)

[oshima, 2014-09-09 17:28:03]

athena/ lgtm

[hashimoto, 2014-09-10 04:46:30]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-09-10 04:48:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/543123004/40001

[commit-bot: I haz the power, 2014-09-10 05:06:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 05:06:13]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[hashimoto, 2014-09-10 06:08:34]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-09-10 06:09:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/543123004/40001

[commit-bot: I haz the power, 2014-09-10 06:33:57]

Committed patchset #2 (id:40001) as 30b0773ff45b62e3dd5f4fbfafdc4787850ccf13

[commit-bot: I haz the power, 2014-09-10 06:36:25]

Patchset 2 (id:??) landed as
https://crrev.com/06aaa078382af19a198b9669ffc4e6add92aa53d
Cr-Commit-Position: refs/heads/master@{#294124}