Fix a bug that unmounting devices on the policy update can be a busy loop.

Fix a bug that unmounting devices on the policy update can be a busy loop.

TEST=unit_tests passed
BUG=643085
Committed: https://crrev.com/91a38bcb48c7e1eb4fb232d9e562c3c614fc19b8
Cr-Commit-Position: refs/heads/master@{#421163}

[yamaguchi, 2016-09-16 03:18:28]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 03:18:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2016-09-16 03:21:46]

yamaguchi@chromium.org changed reviewers:
+ fukino@chromium.org

[commit-bot: I haz the power, 2016-09-16 04:02:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 04:02:39]

Dry run: This issue passed the CQ dry run.

[fukino, 2016-09-16 07:31:35]

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc (right):

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc:112:
pending_unmount_callbacks_.push(callback);
Why do we need FinishAllUnmountPathRequests() called?
Is it possible to call each pending callbacks asynchronously without
FinishAllUnmountPathRequest()?

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h (right):

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h:79: bool
FinishAllUnmountPathRequests();
This is not an override of DiskMountManager.

We need to move it outside "// DiskMountManager overrides." block or remove it.

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:687: if
(disk_mount_manager_->mount_points().empty()) {
We usually omit {} for single-line if statements.

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:708: if
(disk_mount_manager_->mount_points().empty()) {
ditto.

[hidehiko, 2016-09-16 08:14:32]

hidehiko@chromium.org changed reviewers:
+ hidehiko@chromium.org

[hidehiko, 2016-09-16 08:14:33]

fyi

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:691: std::string
mount_path =
nit: const &. Ditto for below.

[yamaguchi, 2016-09-16 08:49:51]

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc (right):

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc:112:
pending_unmount_callbacks_.push(callback);
On 2016/09/16 07:31:34, fukino wrote:
> Why do we need FinishAllUnmountPathRequests() called?
> Is it possible to call each pending callbacks asynchronously without
> FinishAllUnmountPathRequest()?

The main purpose of this change in the test & the fake class is to detect the
bug with the original code.
If we combine the test with the existing code, it will form an infinite loop
(and will crash by overflow) because just calling the UnmountPath function does
not finish unmount right away.
("synchronous" here means to finish call in sync with the invocation of
UnmountPath,)

[yamaguchi, 2016-09-26 02:25:28]

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h (right):

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h:79: bool
FinishAllUnmountPathRequests();
On 2016/09/16 07:31:34, fukino wrote:
> This is not an override of DiskMountManager.
> 
> We need to move it outside "// DiskMountManager overrides." block or remove
it.

Done.

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:687: if
(disk_mount_manager_->mount_points().empty()) {
On 2016/09/16 07:31:35, fukino wrote:
> We usually omit {} for single-line if statements.

Done.

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:691: std::string
mount_path =
On 2016/09/16 08:14:33, hidehiko wrote:
> nit: const &. Ditto for below.

Done.

https://codereview.chromium.org/2348813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:708: if
(disk_mount_manager_->mount_points().empty()) {
On 2016/09/16 07:31:35, fukino wrote:
> ditto.

Done.

[fukino, 2016-09-27 09:06:32]

lgtm

[yamaguchi, 2016-09-27 09:11:44]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 09:12:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 09:35:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 09:35:09]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2016-09-27 10:19:42]

The CQ bit was checked by yamaguchi@chromium.org

[commit-bot: I haz the power, 2016-09-27 10:19:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 10:25:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-27 10:26:41]

Description was changed from

==========
Fix a bug that unmounting devices on the policy update can be a busy loop.

TEST=unit_tests passed
BUG=643085
==========

to

==========
Fix a bug that unmounting devices on the policy update can be a busy loop.

TEST=unit_tests passed
BUG=643085

Committed: https://crrev.com/91a38bcb48c7e1eb4fb232d9e562c3c614fc19b8
Cr-Commit-Position: refs/heads/master@{#421163}
==========

[commit-bot: I haz the power, 2016-09-27 10:26:43]

Patchset 3 (id:??) landed as
https://crrev.com/91a38bcb48c7e1eb4fb232d9e562c3c614fc19b8
Cr-Commit-Position: refs/heads/master@{#421163}

[Lei Zhang, 2016-09-27 17:21:01]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2016-09-27 17:21:02]

https://codereview.chromium.org/2348813002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h (right):

https://codereview.chromium.org/2348813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h:92:
std::queue<UnmountPathCallback> pending_unmount_callbacks_;
With this, we can no longer depend on base/callback_forward.h, and we have to
include base/callback.h.

https://codereview.chromium.org/2376733002