Fix memory leak for extension uninstall dialog.

chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc

Index: chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc
diff --git a/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc b/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc
index eb8799ff8e990522b002d7c2200193f39e1a8eea..80f40fd8d42d0c99febd68d690ef841a86b5d948 100644
--- a/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc
+++ b/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc
@@ -9,6 +9,7 @@
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
 #include "extensions/common/value_builder.h"
@@ -53,7 +54,8 @@ class TestExtensionUninstallDialogDelegate
 typedef InProcessBrowserTest ExtensionUninstallDialogViewBrowserTest;
 
 // Test that ExtensionUninstallDialog cancels the uninstall if the aura::Window
-// which is passed to ExtensionUninstallDialog::Create() is destroyed.
+// which is passed to ExtensionUninstallDialog::Create() is destroyed before
+// ExtensionUninstallDialogDelegateView is created.
 IN_PROC_BROWSER_TEST_F(ExtensionUninstallDialogViewBrowserTest,
                        TrackParentWindowDestruction) {
   // Create a second browser to prevent the app from exiting when the browser is
@@ -68,7 +70,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionUninstallDialogViewBrowserTest,
       extensions::ExtensionUninstallDialog::Create(
           browser()->profile(), browser()->window()->GetNativeWindow(),
           &delegate));

[lgcheng, 2016/11/03 22:30:50]

This test is still working with what I have changed. The uninstall_dialog is
killed because the extension is not added to ExtensionRegistry, not because the
parent is killed. Which means the test does not cover what it claims.

If we fix this, I would imagine that this test will fail on Mac for the same
reason that the test I added below fails.

[Devlin, 2016/11/05 06:26:18]

Yeah, we should make sure the extension is added.

[lgcheng, 2016/11/10 21:39:51]

Done.

-  browser()->window()->Close();
+//  browser()->window()->Close();
   content::RunAllPendingInMessageLoop();
 
   dialog->ConfirmUninstall(extension.get(),
@@ -77,3 +79,41 @@ IN_PROC_BROWSER_TEST_F(ExtensionUninstallDialogViewBrowserTest,
   run_loop.Run();
   EXPECT_TRUE(delegate.canceled());
 }
+
+// Test that ExtensionUninstallDialog cancels the uninstall if the aura::Window
+// which is passed to ExtensionUninstallDialog::Create() is destroyed after
+// ExtensionUninstallDialogDelegateView is created.
+IN_PROC_BROWSER_TEST_F(ExtensionUninstallDialogViewBrowserTest,
+                       TrackParentWindowDestructionAfterViewCreation) {
+  // Create a second browser to prevent the app from exiting when the browser is
+  // closed.
+  CreateBrowser(browser()->profile());
+
+  scoped_refptr<extensions::Extension> extension(BuildTestExtension());
+  extensions::ExtensionRegistry* registry =

[lgcheng, 2016/11/03 22:30:50]

Add extension to ExtensionRegistry so that uninstall_dialog flow works correctly
https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_unin...

The logs show that attempt works as:
[28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(122)]
0x28eaf45255d0 Dialog Created
[28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(186)]
0x28eaf434aba0 Dialog View Created
[28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(198)]
0x28eaf434aba0 Dialog View Died
[28578:28578:1103/140341:ERROR:extension_uninstall_dialog_view.cc(131)]
0x28eaf45255d0 Dialog Died



But I am not sure this is the neat way to do. Any suggestions?

[Devlin, 2016/11/05 06:26:18]

Doing
ExtensionSystem::Get(browser()->profile())->extension_service()->AddExtension(extension.get());
is probably more right - it adds the extension in the normal way and updates any
other listeners.  (Adding the extension directly to the registry would probably
be perfect for a unit test, but might mess us up in a browser test.)

[lgcheng, 2016/11/10 21:39:51]

Done.

+      extensions::ExtensionRegistry::Get(browser()->profile());
+  DCHECK(registry);
+
+  std::string extension_id = extension->id();
+  DCHECK(registry->AddEnabled(extension));

[Devlin, 2016/11/05 06:26:18]

moot with the above comment, but prefer ASSERT_TRUE in cases like this.

[lgcheng, 2016/11/10 21:39:51]

Done.

+
+  base::RunLoop run_loop;
+  TestExtensionUninstallDialogDelegate delegate(run_loop.QuitClosure());
+  std::unique_ptr<extensions::ExtensionUninstallDialog> dialog(
+      extensions::ExtensionUninstallDialog::Create(
+          browser()->profile(), browser()->window()->GetNativeWindow(),
+          &delegate));
+  content::RunAllPendingInMessageLoop();
+
+  dialog->ConfirmUninstall(
+      registry->GetExtensionById(extension_id,
+                                 extensions::ExtensionRegistry::EVERYTHING),
+      extensions::UNINSTALL_REASON_FOR_TESTING,
+      extensions::UNINSTALL_SOURCE_FOR_TESTING);
+

[lgcheng, 2016/11/03 22:30:50]

The test fails on Mac because on Mac it has different implementation.

https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/exten...

It does not care about parent window's life at all and leave that to the
platform. So the browser()->window()->Close() can not kill the dialog thus test
fails due to timeout. 

Shall we only enable this test when the platform is not Mac? 

(I am not sure if the memory leak happens on Mac too since I am not familiar
with that platform. Although it claims "dialog blocks the page from navigating
away and destroying the dialog, so there's no way for the dialog to outlive its
delegate." But I would imagine that user can still force stop Chrome browser
somewhere leaving the extension_uninstall_dialog hang forever.) 

+  // Kill parent window.
+  browser()->window()->Close();
+
+  run_loop.Run();
+  EXPECT_TRUE(delegate.canceled());
+}