Prevent crash when removing a team drive to not be in the list of loaders.

This is a speculative fix for crash in drive::FileSystem::OnTeamDrivesChanged,
that appears to be caused by passing an invalid iterator to map.erase().

The change uses the key rather than in iterator, which will not crash if the key
is not present.

We have test coverage for this code path, but the fake_drive_service does not
support us triggering this issue, which I think it a race between loading the
initial list of team drives and processing the users first change list.
Instead I added a unit test to ensure that removing a team drive id that is
not in the map will not cause a crash.

Bug: 881679
Change-Id: I16f71f7984b5d4f7fed4cb9e939824fac1c813e8
Reviewed-on: https://chromium-review.googlesource.com/1212371
Commit-Queue: Stuart Langley <slangley@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589448}

components/drive/chromeos/file_system.cc

@@ -935,13 +935,11 @@ void FileSystem::OnTeamDrivesChanged(const FileChange& changed_team_drives) {
     for (const auto& change : entry.second.list()) {
       DCHECK(!change.team_drive_id().empty());
       if (change.IsDelete()) {
-        const auto it =
-            team_drive_change_list_loaders_.find(change.team_drive_id());
-        DCHECK(it != team_drive_change_list_loaders_.end());
-        team_drive_change_list_loaders_.erase(it);
-        // If we were tracking the update status we can remove that as well.
-        last_update_metadata_.erase(change.team_drive_id());
-        removed_team_drives.insert(change.team_drive_id());
+        if (team_drive_change_list_loaders_.erase(change.team_drive_id()) > 0) {
+          // If we were tracking the update status we can remove that as well.
+          last_update_metadata_.erase(change.team_drive_id());
+          removed_team_drives.insert(change.team_drive_id());
+        }
       } else if (change.IsAddOrUpdate()) {
         // If this is an update (e.g. a renamed team drive), then just erase the
         // existing entry so we can re-add it with the new path.

components/drive/file_system_unittest.cc

@@ -1500,4 +1500,34 @@ TEST_F(FileSystemTest, CheckUpdatesWithIds) {
   EXPECT_LE(now, team_drive_metadata["td_id_2_2"].last_update_check_time);
 }
 
+TEST_F(FileSystemTest, RemoveNonExistingTeamDrive) {
+  ASSERT_NO_FATAL_FAILURE(SetUpTestFileSystem(USE_SERVER_TIMESTAMP));
+  ASSERT_TRUE(SetupTeamDrives());
+
+  // The first load will trigger the loading of team drives.
+  ReadDirectorySync(base::FilePath::FromUTF8Unsafe("."));
+
+  // Create a file change with a delete team drive, ensure file_system_ does not
+  // crash.
+  const base::FilePath path =
+      util::GetDriveTeamDrivesRootPath().Append("team_drive_2");
+  std::unique_ptr<ResourceEntry> entry = GetResourceEntrySync(path);
+  ASSERT_TRUE(entry);
+
+  drive::FileChange change;
+  change.Update(path, *entry, FileChange::CHANGE_TYPE_DELETE);
+
+  // First time should be removed.
+  file_system_->OnTeamDrivesChanged(change);
+  std::set<std::string> expected_changes = {"td_id_2"};
+  EXPECT_EQ(expected_changes,
+            mock_directory_observer_->removed_team_drive_ids());
+
+  // Second time should be no changes, and no crash.
+  file_system_->OnTeamDrivesChanged(change);
+  expected_changes = {};
+  EXPECT_EQ(expected_changes,
+            mock_directory_observer_->removed_team_drive_ids());
+}
+
 }   // namespace drive