Unmounts volumes before writing to a drive.

Unmounts volumes before writing to a drive.

When writing to a drive with mounted volumes, Chrome OS will recreate the partition table if it is observe to be missing and the volumes are accessed after writing.  This only occurs when erasing just the partition table because enough of the volume information is still present.

We also expand the amount of data erased when destroying partitions to 8k to account for 4k block sizes.  This has the side effect of overwriting the LBA for 512-byte-block drives, which only increases the effect of destroying partitions.

BUG=284834
BUG=335390
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271551

[Drew Haven, 2014-05-13 19:42:28]

Toni,

I made some fixes to our Chrome OS writing to unmount volumes before writing. 
Would you mind taking a look?

-- Drew

[tbarzic, 2014-05-13 20:09:33]

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc
(right):

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:51:
LOG(ERROR) << "Unmounting volumes.";
remove this

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:53:
DiskMountManager::GetInstance()->UnmountDeviceRecursively(
this should be done on UI thread

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:93:
burner->SetEventHandlers(
can you add a todo to handle a case where another image burn is in progress at
this point

[Drew Haven, 2014-05-15 00:30:04]

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc
(right):

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:51:
LOG(ERROR) << "Unmounting volumes.";
On 2014/05/13 20:09:33, tbarzic wrote:
> remove this

Oops.  I had trouble deploying a debug build to my chromebook.  Resorted to
logging obviously instead...

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:53:
DiskMountManager::GetInstance()->UnmountDeviceRecursively(
On 2014/05/13 20:09:33, tbarzic wrote:
> this should be done on UI thread

Really?  It feels like a File/IO thing since it either talks to the system or to
another Chrome OS process.  It executed fine and I didn't see it documented
anywhere.  Will fix though.

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:93:
burner->SetEventHandlers(
On 2014/05/13 20:09:33, tbarzic wrote:
> can you add a todo to handle a case where another image burn is in progress at
> this point

Won't that be handled by the error handler? We make sure that there is only one
burn operation in progress per user-extension already.  I'm guessing that the
ImageBurnerClient cannot handle more than one burn at a time?

[tbarzic, 2014-05-15 00:58:58]

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc
(right):

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:51:
LOG(ERROR) << "Unmounting volumes.";
On 2014/05/15 00:30:04, Drew Haven wrote:
> On 2014/05/13 20:09:33, tbarzic wrote:
> > remove this
> 
> Oops.  I had trouble deploying a debug build to my chromebook.  Resorted to
> logging obviously instead...

hehe, I usually do something similar.

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:53:
DiskMountManager::GetInstance()->UnmountDeviceRecursively(
On 2014/05/15 00:30:04, Drew Haven wrote:
> On 2014/05/13 20:09:33, tbarzic wrote:
> > this should be done on UI thread
> 
> Really?  It feels like a File/IO thing since it either talks to the system or
to
> another Chrome OS process.  It executed fine and I didn't see it documented
> anywhere.  Will fix though.

it's just sending dbus message to disks process which does the real work
(I think dbus communication  is done on a separate thread, but that's handled by
layers under DiskMountManager)

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:93:
burner->SetEventHandlers(
On 2014/05/15 00:30:04, Drew Haven wrote:
> On 2014/05/13 20:09:33, tbarzic wrote:
> > can you add a todo to handle a case where another image burn is in progress
at
> > this point
> 
> Won't that be handled by the error handler? We make sure that there is only
one
> burn operation in progress per user-extension already.  I'm guessing that the
> ImageBurnerClient cannot handle more than one burn at a time?

ImageBurnerClient can handle max one burn operation at a time (though, I'm not
sure anymore if the second request will fail or get queried, probably the
former). The problem here is if there is an operation in progress,
SetEventHandler clears the previous event handlers, so the initial burn
operation will hang (as it won't receive burn finished signal).
I guess this is only problem in multi-profile where you could have more than one
operation-manager (which is profile keyed service).

https://codereview.chromium.org/282853003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc
(right):

https://codereview.chromium.org/282853003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:91:
AddCleanUpFunction(base::Bind(&ClearImageBurner));
AddCleanupFunction has DCHECK that it's on file thread

[Drew Haven, 2014-05-15 02:00:28]

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc
(right):

https://codereview.chromium.org/282853003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:93:
burner->SetEventHandlers(
On 2014/05/15 00:58:58, tbarzic wrote:
> On 2014/05/15 00:30:04, Drew Haven wrote:
> > On 2014/05/13 20:09:33, tbarzic wrote:
> > > can you add a todo to handle a case where another image burn is in
progress
> at
> > > this point
> > 
> > Won't that be handled by the error handler? We make sure that there is only
> one
> > burn operation in progress per user-extension already.  I'm guessing that
the
> > ImageBurnerClient cannot handle more than one burn at a time?
> 
> ImageBurnerClient can handle max one burn operation at a time (though, I'm not
> sure anymore if the second request will fail or get queried, probably the
> former). The problem here is if there is an operation in progress,
> SetEventHandler clears the previous event handlers, so the initial burn
> operation will hang (as it won't receive burn finished signal).
> I guess this is only problem in multi-profile where you could have more than
one
> operation-manager (which is profile keyed service).

I added a guard to only allow one burn operation at a time per profile on Chrome
OS.  I'm not sure what else I could do if they're in multi-profile other than
change the manager to be browser-keyed.

https://codereview.chromium.org/282853003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc
(right):

https://codereview.chromium.org/282853003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:91:
AddCleanUpFunction(base::Bind(&ClearImageBurner));
On 2014/05/15 00:58:58, tbarzic wrote:
> AddCleanupFunction has DCHECK that it's on file thread

Oh, I built release mode when I tested, so I didn't catch that.  You're right. 
It needs to be on the FILE thread so we don't get concurrent access to the
cleanup function list or try to run them while I'm trying to add to them.

I'm going to add the cleanup way before the write.  I believe it should be fine
to call it that early because even if the operation is interrupted the cleanup
call should not cause harm unless another burn operation was already in progress
somehow.  I've done my best to stop that as no one can use the API twice at the
same time, but I don't think I can stop someone from using the Image Burner at
the same time.

[tbarzic, 2014-05-15 16:41:40]

lgtm

https://codereview.chromium.org/282853003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc
(right):

https://codereview.chromium.org/282853003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:21: //
TODO(haven): The Image Burner cannot handle multiple burns. crbug.com/373575
nit: I'd put this in StartWriteOnUIThread

[Drew Haven, 2014-05-15 17:37:20]

https://codereview.chromium.org/282853003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc
(right):

https://codereview.chromium.org/282853003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation_chromeos.cc:21: //
TODO(haven): The Image Burner cannot handle multiple burns. crbug.com/373575
On 2014/05/15 16:41:40, tbarzic wrote:
> nit: I'd put this in StartWriteOnUIThread

Done.  But now it is longer than 80 chars and I have to edit it! :P

[Drew Haven, 2014-05-15 20:17:51]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-05-15 20:18:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haven@chromium.org/282853003/160001

[commit-bot: I haz the power, 2014-05-15 22:15:35]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-15 23:17:50]

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

[commit-bot: I haz the power, 2014-05-15 23:17:51]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Drew Haven, 2014-05-16 17:53:28]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-05-16 17:54:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haven@chromium.org/282853003/180001

[commit-bot: I haz the power, 2014-05-16 21:03:04]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-05-17 08:19:47]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-05-17 11:05:50]

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

[commit-bot: I haz the power, 2014-05-17 11:05:50]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[Drew Haven, 2014-05-19 20:41:43]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-05-19 20:42:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haven@chromium.org/282853003/200001

[commit-bot: I haz the power, 2014-05-20 01:04:09]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-20 03:03:45]

Change committed as 271551