VolumeManager::OnExternalStorageDisabledChanged() can be busy loop. |
||||||||||
Issue descriptionVersion: ToT OS: ChromeOS https://cs.chromium.org/chromium/src/chrome/browser/chromeos/file_manager/volume_manager.cc?q=OnExternalStorageDisabledChanged&sq=package:chromium&dr=CSs&l=693 // We do not iterate on mount_points directly, because mount_points can // be changed by UnmountPath(). // TODO(hidehiko): Is it necessary to unmount mounted archives, too, here? while (!disk_mount_manager_->mount_points().empty()) { std::string mount_path = disk_mount_manager_->mount_points().begin()->second.mount_path; disk_mount_manager_->UnmountPath( mount_path, chromeos::UNMOUNT_OPTIONS_NONE, chromeos::disks::DiskMountManager::UnmountPathCallback()); } The loop is wrong, because DiskMountManager::UnmountPath() is async call, so the mount_points() will not be updated in the next task cycle, but not in inside of the UnmountPath() method call. It could cause busy unmount loop. Yamaguchi-san, would you mind to take a look?
,
Sep 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/91a38bcb48c7e1eb4fb232d9e562c3c614fc19b8 commit 91a38bcb48c7e1eb4fb232d9e562c3c614fc19b8 Author: yamaguchi <yamaguchi@chromium.org> Date: Tue Sep 27 10:24:40 2016 Fix a bug that unmounting devices on the policy update can be a busy loop. TEST=unit_tests passed BUG= 643085 Review-Url: https://codereview.chromium.org/2348813002 Cr-Commit-Position: refs/heads/master@{#421163} [modify] https://crrev.com/91a38bcb48c7e1eb4fb232d9e562c3c614fc19b8/chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc [modify] https://crrev.com/91a38bcb48c7e1eb4fb232d9e562c3c614fc19b8/chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h [modify] https://crrev.com/91a38bcb48c7e1eb4fb232d9e562c3c614fc19b8/chrome/browser/chromeos/file_manager/volume_manager.cc [modify] https://crrev.com/91a38bcb48c7e1eb4fb232d9e562c3c614fc19b8/chrome/browser/chromeos/file_manager/volume_manager.h [modify] https://crrev.com/91a38bcb48c7e1eb4fb232d9e562c3c614fc19b8/chrome/browser/chromeos/file_manager/volume_manager_unittest.cc
,
Sep 29 2016
,
Oct 5 2016
Now I confirmed that, with revision 91a38bcb48c7e1eb4fb232d9e562c3c614fc19b8^ (the commit right before the fix), the remount loop did not finish even after a minute and freezes the entire UI. It could also be confirmed by cros-disks log to /var/log/messages, as it repeatedly fails as the path is already unmounted. > 2016-10-05T15:14:24.497692+09:00 ERR disks[3645]: Path '/media/removable/UNTITLED' is not mounted > 2016-10-05T15:14:24.503796+09:00 ERR disks[3645]: last message repeated 19 times (actually, the call happens more than thousands of times in a few second.) After the change, the devices were successfully remounted. Files app can receive the "unmount finish" signal and reflects it to the UI. Here is the detailed verification process. REPRO STEPS: 1. Turn on device management and setup manager user, and a test user signed in to a client machine. see: https://support.google.com/chrome/a/answer/2657289?hl=en&ref_topic=2936229 2. Sign in to Google admin console as a managing user. Find the policy option there. - Device management > Chrome > User Settings - Hardware - External Storage Devices 3. [On the client machine] Plug 1 or more external storage devices. 4. Open Files app and confirm those volumes appear in the left pane. 5. Open chrome://policy, filter by "ExternalStorage", and check "Show policies with no value set". Confirm ExternalStorageDisabled is not set. 6. [On Google admin console] Turn on ExternalStorageDisabled. 7. Choose turn "External Storage Devices" to "Disallow external storage devices", then save. 8. [On the client] Wait a few seconds or click "Reload policies" on the client, to wait until the policy is applied. EXPECTED: [On the client] The external storage devices will disappear. ACTUAL: [On the client] The entire screen freezes.
,
Oct 5 2016
Thank you for verifying the fix! Busy loops are evil.
,
Oct 7 2016
,
Nov 19 2016
,
Jan 21 2017
,
Mar 4 2017
,
Apr 17 2017
,
May 30 2017
,
Aug 1 2017
,
Oct 14 2017
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by yamaguchi@chromium.org
, Sep 27 2016