Ensure share discovery callback is still run on failure.

If SmbShareFinder::OnSharesFound early-outs on a share discovery
failure and the pending hosts count becomes 0, the final share discovery
callback won't get run and no discovered shares will be shown to the
user.

BUG=935865

Change-Id: I15e2a102b6208eb0222b706d8984175eb8d265a8
Reviewed-on: https://chromium-review.googlesource.com/c/1490356Reviewed-by: Bailey Berro <baileyberro@chromium.org>
Commit-Queue: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636188}

chrome/browser/chromeos/smb_client/smb_share_finder.cc

@@ -138,7 +138,8 @@ void SmbShareFinder::OnSharesFound(
 
   if (error != smbprovider::ErrorType::ERROR_OK) {
     LOG(ERROR) << "Error finding shares: " << error;
-    return;
+    // Don't early out here because this could be the last host being queried,
+    // and the final share discovery callback may need to run.
   }
 
   for (const smbprovider::DirectoryEntryProto& entry : entries.entries()) {

chrome/browser/chromeos/smb_client/smb_share_finder_unittest.cc

@@ -62,6 +62,12 @@ class SmbShareFinderTest : public testing::Test {
     expected_shares_.insert(server_url + share);
   }
 
+  // Adds a share lookup failure for |resolved_url|.
+  void AddShareFailure(const std::string& resolved_url,
+                       smbprovider::ErrorType error) {
+    fake_client_->AddGetSharesFailure(resolved_url, error);
+  }
+
   // Helper function when expecting shares to be found in the network.
   void StartDiscoveryWhileExpectingSharesFound() {
     share_finder_->GatherSharesInNetwork(
@@ -99,7 +105,10 @@ class SmbShareFinderTest : public testing::Test {
   }
 
   // Helper function that expects expected_shares_ to be empty.
-  void ExpectAllSharesHaveBeenFound() { EXPECT_TRUE(expected_shares_.empty()); }
+  void ExpectAllSharesHaveBeenFound() {
+    EXPECT_TRUE(expected_shares_.empty());
+    EXPECT_TRUE(share_discovery_callback_called_);
+  }
 
   // Helper function that expects |url| to resolve to |expected|.
   void ExpectResolvedHost(const SmbUrl& url, const std::string& expected) {
@@ -141,17 +150,21 @@ class SmbShareFinderTest : public testing::Test {
     for (const SmbUrl& url : shares_found) {
       EXPECT_EQ(1u, expected_shares_.erase(url.ToString()));
     }
+    share_discovery_callback_called_ = true;
   }
 
   void SharesFoundSizeCallback(const std::vector<SmbUrl>& shares_found) {
     EXPECT_GE(shares_found.size(), 0u);
+    share_discovery_callback_called_ = true;
   }
 
   void EmptySharesCallback(const std::vector<SmbUrl>& shares_found) {
     EXPECT_EQ(0u, shares_found.size());
+    share_discovery_callback_called_ = true;
   }
 
   bool discovery_callback_called_ = false;
+  bool share_discovery_callback_called_ = false;
 
   // Keeps track of expected shares across multiple hosts.
   std::set<std::string> expected_shares_;
@@ -189,6 +202,14 @@ TEST_F(SmbShareFinderTest, SharesFoundWithSingleHost) {
   ExpectAllSharesHaveBeenFound();
 }
 
+TEST_F(SmbShareFinderTest, ErroWrithSingleHost) {
+  AddDefaultHost();
+  AddShareFailure(kDefaultResolvedUrl, smbprovider::ErrorType::ERROR_FAILED);
+
+  StartDiscoveryWhileExpectingSharesFound();
+  ExpectAllSharesHaveBeenFound();
+}
+
 TEST_F(SmbShareFinderTest, SharesFoundWithMultipleHosts) {
   AddDefaultHost();
   AddShareToDefaultHost("share1");
@@ -205,6 +226,24 @@ TEST_F(SmbShareFinderTest, SharesFoundWithMultipleHosts) {
   ExpectAllSharesHaveBeenFound();
 }
 
+TEST_F(SmbShareFinderTest, SharesFoundWithMultipleHostsAndLastFailed) {
+  AddDefaultHost();
+  AddShareToDefaultHost("share1");
+
+  const std::string host2 = "host2";
+  const std::string address2 = "4.5.6.7";
+  const std::string resolved_server_url2 = kSmbSchemePrefix + address2;
+  AddHost(host2, address2);
+  // Note: This assumes that hosts will be queried for shares in lexicographical
+  // order. This is currently true because SmbShareFinder receives hosts in an
+  // ordered std::map<>.
+  AddShareFailure(resolved_server_url2,
+                  smbprovider::ErrorType::ERROR_SMB1_UNSUPPORTED);
+
+  StartDiscoveryWhileExpectingSharesFound();
+  ExpectAllSharesHaveBeenFound();
+}
+
 TEST_F(SmbShareFinderTest, SharesFoundOnOneHostWithMultipleHosts) {
   AddDefaultHost();
   AddShareToDefaultHost("share1");

chromeos/dbus/fake_smb_provider_client.cc

@@ -28,6 +28,10 @@ void AddDirectoryEntryToList(smbprovider::DirectoryEntryListProto* entry_list,
 }
 }  // namespace
 
+FakeSmbProviderClient::ShareResult::ShareResult() = default;
+
+FakeSmbProviderClient::ShareResult::~ShareResult() = default;
+
 FakeSmbProviderClient::FakeSmbProviderClient() {}
 
 FakeSmbProviderClient::FakeSmbProviderClient(bool should_run_synchronously)
@@ -183,15 +187,21 @@ void FakeSmbProviderClient::GetDeleteList(int32_t mount_id,
 void FakeSmbProviderClient::GetShares(const base::FilePath& server_url,
                                       ReadDirectoryCallback callback) {
   smbprovider::DirectoryEntryListProto entry_list;
-  for (const std::string& share : shares_[server_url.value()]) {
-    AddDirectoryEntryToList(&entry_list, share);
+
+  smbprovider::ErrorType error = smbprovider::ErrorType::ERROR_OK;
+  auto it = shares_.find(server_url.value());
+  if (it != shares_.end()) {
+    error = it->second.error;
+    for (const std::string& share : it->second.shares) {
+      AddDirectoryEntryToList(&entry_list, share);
+    }
   }
 
   if (should_run_synchronously_) {
-    std::move(callback).Run(smbprovider::ERROR_OK, entry_list);
+    std::move(callback).Run(error, entry_list);
   } else {
     stored_readdir_callback_ =
-        base::BindOnce(std::move(callback), smbprovider::ERROR_OK, entry_list);
+        base::BindOnce(std::move(callback), error, entry_list);
   }
 }
 
@@ -203,7 +213,12 @@ void FakeSmbProviderClient::SetupKerberos(const std::string& account_id,
 
 void FakeSmbProviderClient::AddToShares(const std::string& server_url,
                                         const std::string& share) {
-  shares_[server_url].push_back(share);
+  shares_[server_url].shares.push_back(share);
+}
+
+void FakeSmbProviderClient::AddGetSharesFailure(const std::string& server_url,
+                                                smbprovider::ErrorType error) {
+  shares_[server_url].error = error;
 }
 
 void FakeSmbProviderClient::ParseNetBiosPacket(

chromeos/dbus/fake_smb_provider_client.h

@@ -148,6 +148,10 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeSmbProviderClient
   // Adds |share| to the list of shares for |server_url| in |shares_|.
   void AddToShares(const std::string& server_url, const std::string& share);
 
+  // Adds a failure to get shares for |server_url|.
+  void AddGetSharesFailure(const std::string& server_url,
+                           smbprovider::ErrorType error);
+
   // Clears |shares_|.
   void ClearShares();
 
@@ -155,6 +159,15 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeSmbProviderClient
   void RunStoredReadDirCallback();
 
  private:
+  // Result of a GetShares() call.
+  struct ShareResult {
+    ShareResult();
+    ~ShareResult();
+
+    smbprovider::ErrorType error = smbprovider::ErrorType::ERROR_OK;
+    std::vector<std::string> shares;
+  };
+
   // Controls whether |stored_readdir_callback_| should run synchronously.
   bool should_run_synchronously_ = true;
 
@@ -163,7 +176,7 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeSmbProviderClient
   std::map<uint8_t, std::vector<std::string>> netbios_parse_results_;
 
   // Mapping of a server url to its shares.
-  std::map<std::string, std::vector<std::string>> shares_;
+  std::map<std::string, ShareResult> shares_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeSmbProviderClient);
 };