Fixing M32 crasher where closing of a windowed V1 app on a visiting desktop will cause crashes late…

chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
@@ skipping 59 matching lines @@
 #include "ui/views/test/test_views_delegate.h"
 #endif
 
 using extensions::Extension;
 using extensions::Manifest;
 using extensions::UnloadedExtensionInfo;
 
 namespace {
 const char* offline_gmail_url = "https://mail.google.com/mail/mu/u";
 const char* gmail_url = "https://mail.google.com/mail/u";
+const char* kGmailLaunchURL = "https://mail.google.com/mail/ca";
 
 // As defined in /chromeos/dbus/cryptohome_client.cc.
 const char kUserIdHashSuffix[] = "-hash";
 
 // An extension prefix.
 const char kCrxAppPrefix[] = "_crx_";
-}
-
-namespace {
 
 // ShelfModelObserver implementation that tracks what messages are invoked.
 class TestShelfModelObserver : public ash::ShelfModelObserver {
  public:
   TestShelfModelObserver()
     : added_(0),
       removed_(0),
       changed_(0) {
   }
 
@@ skipping 208 matching lines @@
                                     "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
                                     &error);
     // Fake gmail extension.
     DictionaryValue manifest_gmail;
     manifest_gmail.SetString(extensions::manifest_keys::kName,
                              "Gmail launcher controller test extension");
     manifest_gmail.SetString(extensions::manifest_keys::kVersion, "1");
     manifest_gmail.SetString(extensions::manifest_keys::kDescription,
                              "for testing pinned Gmail");
     manifest_gmail.SetString(extensions::manifest_keys::kLaunchWebURL,
-                             "https://mail.google.com/mail/ca");
+                             kGmailLaunchURL);
     ListValue* list = new ListValue();
     list->Append(Value::CreateStringValue("*://mail.google.com/mail/ca"));
     manifest_gmail.Set(extensions::manifest_keys::kWebURLs, list);
 
     extension3_ = Extension::Create(base::FilePath(), Manifest::UNPACKED,
                                     manifest_gmail,
                                     Extension::NO_FLAGS,
                                     extension_misc::kGmailAppId,
                                     &error);
 
@@ skipping 386 matching lines @@
       content::WebContents* web_contents) OVERRIDE {
     message_loop_runner_->Quit();
   }
 
   scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(WebContentsDestroyedWatcher);
 };
 
 // A V1 windowed application.
-class V1App {
+class V1App : public TestBrowserWindow {
  public:
   V1App(Profile* profile, const std::string& app_name) {
+    // Create a window.
+    native_window_.reset(new aura::Window(NULL));
+    native_window_->set_id(0);
+    native_window_->SetType(aura::client::WINDOW_TYPE_POPUP);
+    native_window_->Init(ui::LAYER_TEXTURED);
+    native_window_->Show();
+
     Browser::CreateParams params = Browser::CreateParams::CreateForApp(
         Browser::TYPE_POPUP,
         kCrxAppPrefix + app_name,
         gfx::Rect(),
         profile,
         chrome::HOST_DESKTOP_TYPE_ASH);
-    browser_.reset(chrome::CreateBrowserWithTestWindowForParams(&params));
+    params.window = this;
+    browser_.reset(new Browser(params));
     chrome::AddTabAt(browser_.get(), GURL(), 0, true);
   }
 
   virtual ~V1App() {
     // close all tabs. Note that we do not need to destroy the browser itself.
     browser_->tab_strip_model()->CloseAllTabs();
   }
 
   Browser* browser() { return browser_.get(); }
 
+  // TestBrowserWindow override:
+  virtual gfx::NativeWindow GetNativeWindow() OVERRIDE {
+    return native_window_.get();
+  }
+
  private:
   // The associated browser with this app.
   scoped_ptr<Browser> browser_;
 
+  // The native window we use.
+  scoped_ptr<aura::Window> native_window_;
+
   DISALLOW_COPY_AND_ASSIGN(V1App);
 };
 
 // A V2 application which gets created with an |extension| and for a |profile|.
 // Upon destruction it will properly close the application.
 class V2App {
  public:
   V2App(Profile* profile, const extensions::Extension* extension) {
     window_ = new apps::ShellWindow(profile,
                                     new ChromeShellWindowDelegate(),
@@ skipping 122 matching lines @@
     Browser* browser = chrome::CreateBrowserWithTestWindowForParams(&params);
     chrome::NewTab(browser);
 
     BrowserList::SetLastActive(browser);
     NavigateAndCommitActiveTabWithTitle(
         browser, GURL(url), ASCIIToUTF16(title));
     return browser;
   }
 
   // Creates a running V1 application.
+  // Note that with the use of the app_tab_helper as done below, this is only
+  // usable with a single v1 application.
   V1App* CreateRunningV1App(Profile* profile,
                             const std::string& app_name,
                             const std::string& url) {
     V1App* v1_app = new V1App(profile, app_name);
+    // Create a new app tab helper and assign it to the launcher so that this
+    // app gets properly detected.
+    // TODO(skuhne): Create a more intelligent app tab helper which is able to
+    // detect all running apps properly.
+    TestAppTabHelperImpl* app_tab_helper = new TestAppTabHelperImpl;
+    app_tab_helper->SetAppID(
+        v1_app->browser()->tab_strip_model()->GetWebContentsAt(0),
+        app_name);
+    SetAppTabHelper(app_tab_helper);
+
     NavigateAndCommitActiveTabWithTitle(
         v1_app->browser(), GURL(url), ASCIIToUTF16(""));
     return v1_app;
   }
 
   ash::test::TestSessionStateDelegate*
       session_delegate() { return session_delegate_; }
   ash::test::TestShellDelegate* shell_delegate() { return shell_delegate_; }
 
   // Override BrowserWithTestWindowTest:
@@ skipping 597 matching lines @@
     EXPECT_EQ(2, model_->item_count());
     // Note: the closure ends and the browser will go away.
   }
   EXPECT_EQ(2, model_->item_count());
   SwitchActiveUser(profile2->GetProfileName());
   EXPECT_EQ(2, model_->item_count());
   SwitchActiveUser(profile()->GetProfileName());
   EXPECT_EQ(2, model_->item_count());
 }
 
+// Check edge case where a visiting V1 app gets closed (crbug.com/321374).
+TEST_F(MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerTest,
+       V1CloseOnVisitingDesktop) {
+  // Create a browser item in the LauncherController.
+  InitLauncherController();
+
+  chrome::MultiUserWindowManager* manager =
+      chrome::MultiUserWindowManager::GetInstance();
+
+  // First test: Create an app when the user is not active.

[James Cook, 2013/11/22 19:06:09]

nit: Where does the second test begin?  Or is this whole thing one test?

[Mr4D (OOO till 08-26), 2013/11/22 19:17:00]

Changed the comment(s). Thanks!

+  std::string user2 = "user2";
+  TestingProfile* profile2 = CreateMultiUserProfile(user2);
+  {
+    // Create a "windowed gmail app".
+    scoped_ptr<V1App> v1_app(CreateRunningV1App(
+        profile(),
+        extension_misc::kGmailAppId,
+        kGmailLaunchURL));
+    // Trigger an update on the launcher item.

[James Cook, 2013/11/22 19:06:09]

Is this a leftover comment?

[Mr4D (OOO till 08-26), 2013/11/22 19:17:00]

In deed. Done

+    EXPECT_EQ(3, model_->item_count());
+
+    // Transfer the app to the other screen and switch users.
+    manager->ShowWindowForUser(v1_app->browser()->window()->GetNativeWindow(),
+                               user2);
+    EXPECT_EQ(3, model_->item_count());
+    SwitchActiveUser(profile2->GetProfileName());
+    EXPECT_EQ(2, model_->item_count());
+  }
+  // Switching back caused a crash in the past.
+  SwitchActiveUser(profile()->GetProfileName());
+  EXPECT_EQ(2, model_->item_count());
+  {
+    // Create a "windowed gmail app".
+    scoped_ptr<V1App> v1_app(CreateRunningV1App(
+        profile(),
+        extension_misc::kGmailAppId,
+        kGmailLaunchURL));
+    EXPECT_EQ(3, model_->item_count());
+  }
+  SwitchActiveUser(profile2->GetProfileName());
+  EXPECT_EQ(2, model_->item_count());
+}
+
+// Check edge cases with multi profile V1 apps in the shelf.
+TEST_F(MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerTest,
+       V1AppUpdateOnUserSwitchEdgecases2) {
+  // Create a browser item in the LauncherController.
+  InitLauncherController();
+  TestAppTabHelperImpl* app_tab_helper = new TestAppTabHelperImpl;
+  SetAppTabHelper(app_tab_helper);
+
+  // First test: Create an app when the user is not active.
+  std::string user2 = "user2";
+  TestingProfile* profile2 = CreateMultiUserProfile(user2);
+  SwitchActiveUser(profile2->GetProfileName());
+  {
+    // Create a "windowed gmail app".
+    scoped_ptr<V1App> v1_app(CreateRunningV1App(
+        profile(), extension_misc::kGmailAppId, gmail_url));
+    EXPECT_EQ(2, model_->item_count());
+
+    // However - switching to the user should show it.
+    SwitchActiveUser(profile()->GetProfileName());
+    EXPECT_EQ(3, model_->item_count());
+
+    // Second test: Remove the app when the user is not active and see that it
+    // works.
+    SwitchActiveUser(profile2->GetProfileName());
+    EXPECT_EQ(2, model_->item_count());
+    // Note: the closure ends and the browser will go away.

[James Cook, 2013/11/22 19:06:09]

nit: This might be a little clearer if you eliminated the {} block and
explicitly used v1_app.reset().

[Mr4D (OOO till 08-26), 2013/11/22 19:17:00]

Done.

+  }
+  EXPECT_EQ(2, model_->item_count());
+  SwitchActiveUser(profile()->GetProfileName());
+  EXPECT_EQ(2, model_->item_count());
+  SwitchActiveUser(profile2->GetProfileName());
+  EXPECT_EQ(2, model_->item_count());
+}
+
 // Check that activating an item which is on another user's desktop, will bring
 // it back.
 TEST_F(MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerTest,
        TestLauncherActivationPullsBackWindow) {
   // Create a browser item in the LauncherController.
   InitLauncherController();
   chrome::MultiUserWindowManager* manager =
       chrome::MultiUserWindowManager::GetInstance();
 
   // Add two users to the window manager.
@@ skipping 1092 matching lines @@
 
   EXPECT_EQ(1, app_icon_loader->fetch_count());
   ASSERT_EQ(initial_size + 1, model_->items().size());
   EXPECT_TRUE(launcher_controller_->IsAppPinned("1"));
   EXPECT_FALSE(launcher_controller_->IsAppPinned("0"));
   EXPECT_EQ(ash::TYPE_APP_SHORTCUT, model_->items()[app_index].type);
 
   launcher_controller_->UnpinAppWithID("1");
   ASSERT_EQ(initial_size, model_->items().size());
 }