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

chrome/browser/webshare/share_service_impl_unittest.cc

 // Copyright 2016 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 <memory>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 27 matching lines @@
  public:
   explicit ShareServiceTestImpl(blink::mojom::ShareServiceRequest request)
       : binding_(this) {
     binding_.Bind(std::move(request));
 
     pref_service_.reset(new TestingPrefServiceSimple());
     pref_service_->registry()->RegisterDictionaryPref(
         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) {
     constexpr char kUrlTemplateKey[] = "url_template";
     constexpr char kNameKey[] = "name";
 
     DictionaryPrefUpdate update(GetPrefService(),
                                 prefs::kWebShareVisitedTargets);
     base::DictionaryValue* share_target_dict = update.Get();
 
     std::unique_ptr<base::DictionaryValue> origin_dict(
         new base::DictionaryValue);
 
     origin_dict->SetStringWithoutPathExpansion(kUrlTemplateKey, url_template);
     origin_dict->SetStringWithoutPathExpansion(kNameKey, name);
 
     share_target_dict->SetWithoutPathExpansion(manifest_url,
                                                std::move(origin_dict));
   }
 
   void SetEngagementForTarget(const std::string& manifest_url,
                               blink::mojom::EngagementLevel level) {
     engagement_map_[manifest_url] = level;
   }
 
+  void set_runloop(base::RunLoop* runloop) {
+    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 {
     last_used_target_url_ = target_url.spec();
   }
 
   PrefService* GetPrefService() override { return pref_service_.get(); }
 
   blink::mojom::EngagementLevel GetEngagementLevel(const GURL& url) override {
     return engagement_map_[url.spec()];
   }
 
   mojo::Binding<blink::mojom::ShareService> binding_;
+  std::unique_ptr<TestingPrefServiceSimple> pref_service_;
 
-  base::Optional<std::string> picker_result_;
+  std::map<std::string, blink::mojom::EngagementLevel> engagement_map_;
+  // Closure to quit the test's run loop.
+  base::Closure quit_runloop_;
+
+  // The last URL passed to OpenTargetURL.
   std::string last_used_target_url_;
-  std::unique_ptr<TestingPrefServiceSimple> pref_service_;
-  std::map<std::string, blink::mojom::EngagementLevel> engagement_map_;
+  // 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 {
  public:
   ShareServiceImplUnittest() = default;
   ~ShareServiceImplUnittest() override = default;
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
 
     share_service_helper_ = base::MakeUnique<ShareServiceTestImpl>(
         mojo::MakeRequest(&share_service_));
 
     share_service_helper_->SetEngagementForTarget(
         kManifestUrlHigh, blink::mojom::EngagementLevel::HIGH);
     share_service_helper_->SetEngagementForTarget(
         kManifestUrlMin, blink::mojom::EngagementLevel::MINIMAL);
     share_service_helper_->SetEngagementForTarget(
         kManifestUrlLow, blink::mojom::EngagementLevel::LOW);
   }
 
   void TearDown() override { ChromeRenderViewHostTestHarness::TearDown(); }
 
+  blink::mojom::ShareService* share_service() const {
+    return share_service_.get();
+  }
+
+  ShareServiceTestImpl* share_service_helper() const {
+    return share_service_helper_.get();
+  }
+
+  void DeleteShareService() { share_service_helper_.reset(); }
+
   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 =

[Sam McNally, 2017/02/15 06:27:55]

Move these out.

[Matt Giuca, 2017/02/15 08:13:26]

Done. Oh hey, this can be a regular function now (moved up).

Could even be a lambda but I think that makes the tests a bit unreadable.

         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);
-
-    if (!on_callback_.is_null())
-      on_callback_.Run();
   }
 
+ private:
   blink::mojom::ShareServicePtr share_service_;
   std::unique_ptr<ShareServiceTestImpl> share_service_helper_;
-  base::Closure on_callback_;
 };
 
 }  // namespace
 
 // 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{
       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::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();
+
+  // Pick example-low.com.
+  share_service_helper()->picker_callback().Run(
+      base::Optional<std::string>(kManifestUrlLow));
 }
 
 // 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::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();
+
+  // Cancel the dialog.
+  share_service_helper()->picker_callback().Run(base::nullopt);
 }
 
 // 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::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();
+
+  // Cancel the dialog.
+  share_service_helper()->picker_callback().Run(base::nullopt);
 }
 
 // Test to check that only targets with enough engagement were in picker.
 TEST_F(ShareServiceImplUnittest, ShareWithSomeInsufficientlyEngagedTargets) {
   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";
 
-  share_service_helper_->set_picker_result(
-      base::Optional<std::string>(kManifestUrlLow));
-
-  share_service_helper_->AddShareTargetToPrefs(kManifestUrlMin, kTargetName,
-                                               kUrlTemplate);
-  share_service_helper_->AddShareTargetToPrefs(kManifestUrlLow, kTargetName,
-                                               kUrlTemplate);
+  share_service_helper()->AddShareTargetToPrefs(kManifestUrlMin, kTargetName,
+                                                kUrlTemplate);
+  share_service_helper()->AddShareTargetToPrefs(kManifestUrlLow, kTargetName,
+                                                kUrlTemplate);
 
   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>());
 
   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();
+
+  // Pick example-low.com.
+  share_service_helper()->picker_callback().Run(
+      base::Optional<std::string>(kManifestUrlLow));
+}
+
+// Test that deleting the share service while the picker is open does not crash
+// (https://crbug.com/690775).
+TEST_F(ShareServiceImplUnittest, ShareServiceDeletion) {
+  share_service_helper()->AddShareTargetToPrefs(kManifestUrlLow, kTargetName,
+                                                kUrlTemplate);
+
+  base::RunLoop run_loop;
+  share_service_helper()->set_runloop(&run_loop);
+
+  const GURL url(kUrlSpec);
+  // 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) { NOTREACHED(); });

[Sam McNally, 2017/02/15 06:27:55]

FAIL()

[Matt Giuca, 2017/02/15 08:13:26]

Done.

+  share_service()->Share(kTitle, kText, url, callback);
+
+  run_loop.Run();
+
+  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
 // adjacent to eachother; there are no padding characters.
 TEST_F(ShareServiceImplUnittest, ReplacePlaceholders) {
   const GURL url(kUrlSpec);
   std::string url_template_filled;
   bool succeeded;
 
   // No placeholders
@@ skipping 185 matching lines @@
   EXPECT_TRUE(succeeded);
   EXPECT_EQ("%C3%A9", url_template_filled);
 
   // U+1F4A9
   url_template = "{title}";
   succeeded = ShareServiceImpl::ReplacePlaceholders(
       url_template, "\xf0\x9f\x92\xa9", kText, url, &url_template_filled);
   EXPECT_TRUE(succeeded);
   EXPECT_EQ("%F0%9F%92%A9", url_template_filled);
 }