Fixed crash if tab closes while WebShare picker dialog is open.

chrome/browser/webshare/share_service_impl_unittest.cc

Index: chrome/browser/webshare/share_service_impl_unittest.cc
diff --git a/chrome/browser/webshare/share_service_impl_unittest.cc b/chrome/browser/webshare/share_service_impl_unittest.cc
index 11f4be7dfe7e6b408da294bd3794c6e1f50d1f04..7f41bc4a4878f46ce10895e3fa8ed92ca29c7882 100644
--- a/chrome/browser/webshare/share_service_impl_unittest.cc
+++ b/chrome/browser/webshare/share_service_impl_unittest.cc
@@ -34,6 +34,11 @@ constexpr char kManifestUrlLow[] =
 constexpr char kManifestUrlMin[] =
     "https://www.example-min.com/target/manifest.json";
 
+void DidShare(const base::Optional<std::string>& expected_error,
+              const base::Optional<std::string>& error) {
+  EXPECT_EQ(expected_error, error);
+}
+
 class ShareServiceTestImpl : public ShareServiceImpl {
  public:
   explicit ShareServiceTestImpl(blink::mojom::ShareServiceRequest request)
@@ -45,10 +50,6 @@ class ShareServiceTestImpl : public ShareServiceImpl {
         prefs::kWebShareVisitedTargets);
   }
 
-  void set_picker_result(base::Optional<std::string> result) {
-    picker_result_ = result;
-  }
-
   void AddShareTargetToPrefs(const std::string& manifest_url,
                              const std::string& name,
                              const std::string& url_template) {
@@ -74,19 +75,32 @@ class ShareServiceTestImpl : public ShareServiceImpl {
     engagement_map_[manifest_url] = level;
   }
 
+  void set_runloop(base::RunLoop* runloop) {

[Sam McNally, 2017/02/16 02:12:24]

run_loop

[Matt Giuca, 2017/02/16 02:25:38]

Done.

+    quit_runloop_ = runloop->QuitClosure();
+  }
+
   const std::string& GetLastUsedTargetURL() { return last_used_target_url_; }
 
   const std::vector<std::pair<base::string16, GURL>>& GetTargetsInPicker() {
     return targets_in_picker_;
   }
 
+  const base::Callback<void(base::Optional<std::string>)>& picker_callback() {
+    return picker_callback_;
+  }
+
  private:
   void ShowPickerDialog(
       const std::vector<std::pair<base::string16, GURL>>& targets,
       const base::Callback<void(base::Optional<std::string>)>& callback)
       override {
+    // Store the arguments passed to the picker dialog.
     targets_in_picker_ = targets;
-    callback.Run(picker_result_);
+    picker_callback_ = callback;
+
+    // Quit the test's run loop. It is the test's responsibility to call the
+    // callback, to simulate the user's choice.
+    quit_runloop_.Run();
   }
 
   void OpenTargetURL(const GURL& target_url) override {
@@ -100,12 +114,19 @@ class ShareServiceTestImpl : public ShareServiceImpl {
   }
 
   mojo::Binding<blink::mojom::ShareService> binding_;
-
-  base::Optional<std::string> picker_result_;
-  std::string last_used_target_url_;
   std::unique_ptr<TestingPrefServiceSimple> pref_service_;
+
   std::map<std::string, blink::mojom::EngagementLevel> engagement_map_;
+  // Closure to quit the test's run loop.
+  base::Closure quit_runloop_;

[Sam McNally, 2017/02/16 02:12:24]

quit_run_loop_

[Matt Giuca, 2017/02/16 02:25:39]

Done.

+
+  // The last URL passed to OpenTargetURL.
+  std::string last_used_target_url_;
+  // The targets passed to ShowPickerDialog.
   std::vector<std::pair<base::string16, GURL>> targets_in_picker_;
+  // The callback passed to ShowPickerDialog (which is supposed to be called
+  // with the user's chosen result, or nullopt if cancelled).
+  base::Callback<void(base::Optional<std::string>)> picker_callback_;
 };
 
 class ShareServiceImplUnittest : public ChromeRenderViewHostTestHarness {
@@ -129,27 +150,19 @@ class ShareServiceImplUnittest : public ChromeRenderViewHostTestHarness {
 
   void TearDown() override { ChromeRenderViewHostTestHarness::TearDown(); }
 
-  void DidShare(const std::vector<std::pair<base::string16, GURL>>&
-                    expected_targets_in_picker,
-                const std::string& expected_target_url,
-                const base::Optional<std::string>& expected_error,
-                const base::Optional<std::string>& error) {
-    std::vector<std::pair<base::string16, GURL>> targets_in_picker =
-        share_service_helper_->GetTargetsInPicker();
-    EXPECT_EQ(expected_targets_in_picker, targets_in_picker);
-
-    std::string target_url = share_service_helper_->GetLastUsedTargetURL();
-    EXPECT_EQ(expected_target_url, target_url);
-
-    EXPECT_EQ(expected_error, error);
+  blink::mojom::ShareService* share_service() const {
+    return share_service_.get();
+  }
 
-    if (!on_callback_.is_null())
-      on_callback_.Run();
+  ShareServiceTestImpl* share_service_helper() const {
+    return share_service_helper_.get();
   }
 
+  void DeleteShareService() { share_service_helper_.reset(); }
+
+ private:
   blink::mojom::ShareServicePtr share_service_;
   std::unique_ptr<ShareServiceTestImpl> share_service_helper_;
-  base::Closure on_callback_;
 };
 
 }  // namespace
@@ -157,80 +170,92 @@ class ShareServiceImplUnittest : public ChromeRenderViewHostTestHarness {
 // Basic test to check the Share method calls the callback with the expected
 // parameters.
 TEST_F(ShareServiceImplUnittest, ShareCallbackParams) {
-  share_service_helper_->set_picker_result(
-      base::Optional<std::string>(kManifestUrlLow));
-
-  share_service_helper_->AddShareTargetToPrefs(kManifestUrlLow, kTargetName,
-                                               kUrlTemplate);
-  share_service_helper_->AddShareTargetToPrefs(kManifestUrlHigh, kTargetName,
-                                               kUrlTemplate);
+  share_service_helper()->AddShareTargetToPrefs(kManifestUrlLow, kTargetName,
+                                                kUrlTemplate);
+  share_service_helper()->AddShareTargetToPrefs(kManifestUrlHigh, kTargetName,
+                                                kUrlTemplate);
 
   std::string expected_url =
       "https://www.example-low.com/target/"
       "share?title=My%20title&text=My%20text&url=https%3A%2F%2Fwww."
       "google.com%2F";
-
   std::vector<std::pair<base::string16, GURL>> expected_targets{

[Sam McNally, 2017/02/16 02:12:24]

Create these closer to where they're used. Also consider making them constants.
Fix everywhere.

[Matt Giuca, 2017/02/16 02:25:39]

Done.

       make_pair(base::UTF8ToUTF16(kTargetName), GURL(kManifestUrlHigh)),
       make_pair(base::UTF8ToUTF16(kTargetName), GURL(kManifestUrlLow))};
+
   base::Callback<void(const base::Optional<std::string>&)> callback =
-      base::Bind(&ShareServiceImplUnittest::DidShare, base::Unretained(this),
-                 expected_targets, expected_url, base::Optional<std::string>());
+      base::Bind(&DidShare, base::Optional<std::string>());
 
   base::RunLoop run_loop;
-  on_callback_ = run_loop.QuitClosure();
+  share_service_helper()->set_runloop(&run_loop);
 
   const GURL url(kUrlSpec);
-  share_service_->Share(kTitle, kText, url, callback);
+  share_service()->Share(kTitle, kText, url, callback);
 
   run_loop.Run();
+
+  EXPECT_EQ(expected_targets, share_service_helper()->GetTargetsInPicker());
+
+  // Pick example-low.com.
+  share_service_helper()->picker_callback().Run(
+      base::Optional<std::string>(kManifestUrlLow));
+
+  EXPECT_EQ(expected_url, share_service_helper()->GetLastUsedTargetURL());
 }
 
 // Tests the result of cancelling the share in the picker UI, that doesn't have
 // any targets.
 TEST_F(ShareServiceImplUnittest, ShareCancelNoTargets) {
-  // picker_result_ is set to nullopt by default, so this imitates the user
-  // cancelling a share.
   // Expect an error message in response.
   base::Callback<void(const base::Optional<std::string>&)> callback =
-      base::Bind(&ShareServiceImplUnittest::DidShare, base::Unretained(this),
-                 std::vector<std::pair<base::string16, GURL>>(), std::string(),
-                 base::Optional<std::string>("Share was cancelled"));
+      base::Bind(&DidShare, base::Optional<std::string>("Share was cancelled"));
 
   base::RunLoop run_loop;
-  on_callback_ = run_loop.QuitClosure();
+  share_service_helper()->set_runloop(&run_loop);
 
   const GURL url(kUrlSpec);
-  share_service_->Share(kTitle, kText, url, callback);
+  share_service()->Share(kTitle, kText, url, callback);
 
   run_loop.Run();
+
+  std::vector<std::pair<base::string16, GURL>> expected_targets;  // Empty.
+  EXPECT_EQ(expected_targets, share_service_helper()->GetTargetsInPicker());

[Sam McNally, 2017/02/16 02:12:24]

EXPECT_TRUE(share_service_helper()->GetTargetsInPicker().empty());

[Matt Giuca, 2017/02/16 02:25:39]

Done.

+
+  // Cancel the dialog.
+  share_service_helper()->picker_callback().Run(base::nullopt);
+
+  EXPECT_EQ(GURL(), share_service_helper()->GetLastUsedTargetURL());
 }
 
 // Tests the result of cancelling the share in the picker UI, that has targets.
 TEST_F(ShareServiceImplUnittest, ShareCancelWithTargets) {
-  // picker_result_ is set to nullopt by default, so this imitates the user
-  // cancelling a share.
-  share_service_helper_->AddShareTargetToPrefs(kManifestUrlHigh, kTargetName,
-                                               kUrlTemplate);
-  share_service_helper_->AddShareTargetToPrefs(kManifestUrlLow, kTargetName,
-                                               kUrlTemplate);
+  share_service_helper()->AddShareTargetToPrefs(kManifestUrlHigh, kTargetName,
+                                                kUrlTemplate);
+  share_service_helper()->AddShareTargetToPrefs(kManifestUrlLow, kTargetName,
+                                                kUrlTemplate);
 
   std::vector<std::pair<base::string16, GURL>> expected_targets{
       make_pair(base::UTF8ToUTF16(kTargetName), GURL(kManifestUrlHigh)),
       make_pair(base::UTF8ToUTF16(kTargetName), GURL(kManifestUrlLow))};
+
   // Expect an error message in response.
   base::Callback<void(const base::Optional<std::string>&)> callback =
-      base::Bind(&ShareServiceImplUnittest::DidShare, base::Unretained(this),
-                 expected_targets, std::string(),
-                 base::Optional<std::string>("Share was cancelled"));
+      base::Bind(&DidShare, base::Optional<std::string>("Share was cancelled"));
 
   base::RunLoop run_loop;
-  on_callback_ = run_loop.QuitClosure();
+  share_service_helper()->set_runloop(&run_loop);
 
   const GURL url(kUrlSpec);
-  share_service_->Share(kTitle, kText, url, callback);
+  share_service()->Share(kTitle, kText, url, callback);
 
   run_loop.Run();
+
+  EXPECT_EQ(expected_targets, share_service_helper()->GetTargetsInPicker());
+
+  // Cancel the dialog.
+  share_service_helper()->picker_callback().Run(base::nullopt);
+
+  EXPECT_EQ(GURL(), share_service_helper()->GetLastUsedTargetURL());

[Sam McNally, 2017/02/16 02:12:24]

EXPECT_TRUE(share_service_helper()->GetLastUsedTargetURL().is_empty());

[Matt Giuca, 2017/02/16 02:25:39]

Done.

 }
 
 // Test to check that only targets with enough engagement were in picker.
@@ -239,28 +264,66 @@ TEST_F(ShareServiceImplUnittest, ShareWithSomeInsufficientlyEngagedTargets) {
       "https://www.example-low.com/target/"
       "share?title=My%20title&text=My%20text&url=https%3A%2F%2Fwww."
       "google.com%2F";
+  std::vector<std::pair<base::string16, GURL>> expected_targets{
+      make_pair(base::UTF8ToUTF16(kTargetName), GURL(kManifestUrlLow))};
+
+  share_service_helper()->AddShareTargetToPrefs(kManifestUrlMin, kTargetName,
+                                                kUrlTemplate);
+  share_service_helper()->AddShareTargetToPrefs(kManifestUrlLow, kTargetName,
+                                                kUrlTemplate);
+
+  base::Callback<void(const base::Optional<std::string>&)> callback =
+      base::Bind(&DidShare, base::Optional<std::string>());
+
+  base::RunLoop run_loop;
+  share_service_helper()->set_runloop(&run_loop);
 
-  share_service_helper_->set_picker_result(
+  const GURL url(kUrlSpec);
+  share_service()->Share(kTitle, kText, url, callback);
+
+  run_loop.Run();
+
+  EXPECT_EQ(expected_targets, share_service_helper()->GetTargetsInPicker());
+
+  // Pick example-low.com.
+  share_service_helper()->picker_callback().Run(
       base::Optional<std::string>(kManifestUrlLow));
 
-  share_service_helper_->AddShareTargetToPrefs(kManifestUrlMin, kTargetName,
-                                               kUrlTemplate);
-  share_service_helper_->AddShareTargetToPrefs(kManifestUrlLow, kTargetName,
-                                               kUrlTemplate);
+  EXPECT_EQ(expected_url, share_service_helper()->GetLastUsedTargetURL());
+}
 
+// Test that deleting the share service while the picker is open does not crash
+// (https://crbug.com/690775).
+TEST_F(ShareServiceImplUnittest, ShareServiceDeletion) {
   std::vector<std::pair<base::string16, GURL>> expected_targets{
       make_pair(base::UTF8ToUTF16(kTargetName), GURL(kManifestUrlLow))};
-  base::Callback<void(const base::Optional<std::string>&)> callback =
-      base::Bind(&ShareServiceImplUnittest::DidShare, base::Unretained(this),
-                 expected_targets, expected_url, base::Optional<std::string>());
+
+  share_service_helper()->AddShareTargetToPrefs(kManifestUrlLow, kTargetName,
+                                                kUrlTemplate);
 
   base::RunLoop run_loop;
-  on_callback_ = run_loop.QuitClosure();
+  share_service_helper()->set_runloop(&run_loop);
 
   const GURL url(kUrlSpec);
-  share_service_->Share(kTitle, kText, url, callback);
+  // Expect the callback to never be called (since the share service is
+  // destroyed before the picker is closed).
+  // TODO(mgiuca): This probably should still complete the share, if not
+  // cancelled, even if the underlying tab is closed.
+  base::Callback<void(const base::Optional<std::string>&)> callback =
+      base::Bind([](const base::Optional<std::string>& error) { FAIL(); });
+  share_service()->Share(kTitle, kText, url, callback);
 
   run_loop.Run();
+
+  EXPECT_EQ(expected_targets, share_service_helper()->GetTargetsInPicker());
+
+  const base::Callback<void(base::Optional<std::string>)> picker_callback =
+      share_service_helper()->picker_callback();
+
+  DeleteShareService();
+
+  // Pick example-low.com.
+  picker_callback.Run(base::Optional<std::string>(kManifestUrlLow));
 }
 
 // Replace various numbers of placeholders in various orders. Placeholders are