Eliminate CHECK from CryptohomeLibrary::LoadSystemSalt

Eliminate CHECK from CryptohomeLibrary::LoadSystemSalt

This change makes callers responsible for ensuring that the returned
system salt is valid.

It also includes removal of some stale unused code.

BUG=288210
For c/b/e/aoi/music_manager_private:
TBR=asargent@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224248

[stevenjb, 2013-09-16 21:54:12]

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (left):

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:115: // in case the
legacy id doesn't exist.
Since Chrome and ChromeOS are synced, I believe that it should be safe to remove
this now?

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:134: std::string
machine_id = GetMachineID();
Calling this on the IO thread was actually incorrect. There should be no need at
this point to call anything here from the IO thread.

[raymes, 2013-09-16 22:13:38]

https://codereview.chromium.org/23903051/diff/1/chrome/browser/pepper_flash_s...
File chrome/browser/pepper_flash_settings_manager.cc (left):

https://codereview.chromium.org/23903051/diff/1/chrome/browser/pepper_flash_s...
chrome/browser/pepper_flash_settings_manager.cc:455: // devices have been
migrated.
Again, I think we need to keep this.

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (left):

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:115: // in case the
legacy id doesn't exist.
Unfortunately I think we have to keep this. If someone using ChromeOS was using
the device ID code prior to when it was changed, they may have a device ID that
was computed in a different way. We need to add some uma stats to decide when we
can remove it.

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:134: std::string
machine_id = GetMachineID();
On 2013/09/16 21:54:12, stevenjb wrote:
> Calling this on the IO thread was actually incorrect. There should be no need
at
> this point to call anything here from the IO thread.

Thanks for catching this. That makes sense :)

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (right):

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:47: const int64
kRequestStstemSaltDelayMs = 500;
-Ststem -> System
-Seems ok to retry but how did you decide upon the delay? Are there other
options you considered?

[stevenjb, 2013-09-16 22:32:48]

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (left):

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:115: // in case the
legacy id doesn't exist.
On 2013/09/16 22:13:38, raymes wrote:
> Unfortunately I think we have to keep this. If someone using ChromeOS was
using
> the device ID code prior to when it was changed, they may have a device ID
that
> was computed in a different way. We need to add some uma stats to decide when
we
> can remove it.

Ugh, you man this isn't something we can automatically transition from? That is
very unfortunate, it means we have to maintain this code for quite a while. In
the meanwhile I will go ahead and this without eliminating the legacy code then.

It would be helpful to file a tracking issue with details on what the criteria
for removing this will be, and adding that to the comments.

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (right):

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:47: const int64
kRequestStstemSaltDelayMs = 500;
On 2013/09/16 22:13:38, raymes wrote:
> -Ststem -> System
> -Seems ok to retry but how did you decide upon the delay? Are there other
> options you considered?
The delay is designed to be long enough not to spam the log with failed
attempts, but short enough not to trigger any timeouts. Unfortunately we do not
currently have an observer mechanism to inform the class when cryptohome has
started and the system salt is ready.

[stevenjb, 2013-09-16 23:16:21]

OK, maintaining the legacy code turned out not to be too bad with some minor
re-factoring.

PTAL

[Nikita (slow), 2013-09-17 09:54:24]

chromeos/login lgtm

Please file a tracking issue to check return value.

[pneubeck (no reviews), 2013-09-17 15:01:10]

+Ryo, who might have an opinion on the usage of blocking calls.

As a quick fix this CL might be ok. But overall I think this is going in the
wrong direction:
- It continues to rely on blocking dbus calls
- More arbitrary retry-delay constants are added, which potentially influence
startup time

Please see my comment on the bug tracker for an alternative (maybe R32). I'd
also volunteer to implement that...

[stevenjb, 2013-09-17 15:24:20]

On 2013/09/17 15:01:10, pneubeck wrote:
> +Ryo, who might have an opinion on the usage of blocking calls.
> 
> As a quick fix this CL might be ok. But overall I think this is going in the
> wrong direction:
> - It continues to rely on blocking dbus calls
> - More arbitrary retry-delay constants are added, which potentially influence
> startup time
> 
> Please see my comment on the bug tracker for an alternative (maybe R32). I'd
> also volunteer to implement that...

I did investigate fixing this with non blocking calls, but changing the login
code is significantly non-trivial and this is a frequent crash that needs fixing
quickly. 

I will add a tracking bug for a proper fix, but we should really fix all of
cryptohome at once.

[pneubeck (no reviews), 2013-09-17 15:25:34]

chromeos/cryptohome/ lgtm

[stevenjb, 2013-09-17 15:25:35]

On 2013/09/17 15:24:20, stevenjb wrote:
> On 2013/09/17 15:01:10, pneubeck wrote:
> > +Ryo, who might have an opinion on the usage of blocking calls.
> > 
> > As a quick fix this CL might be ok. But overall I think this is going in the
> > wrong direction:
> > - It continues to rely on blocking dbus calls
> > - More arbitrary retry-delay constants are added, which potentially
influence
> > startup time
> > 
> > Please see my comment on the bug tracker for an alternative (maybe R32). I'd
> > also volunteer to implement that...
> 
> I did investigate fixing this with non blocking calls, but changing the login
> code is significantly non-trivial and this is a frequent crash that needs
fixing
> quickly. 
> 
> I will add a tracking bug for a proper fix, but we should really fix all of
> cryptohome at once.

Also, the delay intentionally does not affect Chrome's startup time. It only
affects the start time of extensions using the music manager API.

[hashimoto, 2013-09-18 05:53:19]

I'd be really happy to see the real solution being landed, I tried to figure out
how it could be achieved but failed.
Looks acceptable as a short-term solution.

https://codereview.chromium.org/23903051/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/music_manager_private/device_id_chromeos.cc
(right):

https://codereview.chromium.org/23903051/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/music_manager_private/device_id_chromeos.cc:20:
const int64 kRequestStstemSaltDelayMs = 500;
nit: s/Ststem/System/?

https://codereview.chromium.org/23903051/diff/21001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (right):

https://codereview.chromium.org/23903051/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:47: const int64
kRequestStstemSaltDelayMs = 500;
nit: s/Ststem/System/?

https://codereview.chromium.org/23903051/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:66:
weak_ptr_factory_(this) {
DeviceIDFetcher is ref counted. Is threre any reason to bind WeakPtr, instead of
scoped_refptr, to callbacks?

https://codereview.chromium.org/23903051/diff/21001/chromeos/cryptohome/crypt...
File chromeos/cryptohome/cryptohome_library.cc (right):

https://codereview.chromium.org/23903051/diff/21001/chromeos/cryptohome/crypt...
chromeos/cryptohome/cryptohome_library.cc:168: bool LoadSystemSaltKey() {
nit: This method no longer loads the key, could you rename this to
PrepareSystemSaltKey or SetSystemSaltKey or something?

[stevenjb, 2013-09-18 19:23:35]

PTAL

https://codereview.chromium.org/23903051/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/music_manager_private/device_id_chromeos.cc
(right):

https://codereview.chromium.org/23903051/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/music_manager_private/device_id_chromeos.cc:20:
const int64 kRequestStstemSaltDelayMs = 500;
On 2013/09/18 05:53:19, hashimoto wrote:
> nit: s/Ststem/System/?

Done.

https://codereview.chromium.org/23903051/diff/21001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (right):

https://codereview.chromium.org/23903051/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:47: const int64
kRequestStstemSaltDelayMs = 500;
On 2013/09/18 05:53:19, hashimoto wrote:
> nit: s/Ststem/System/?

Done.

https://codereview.chromium.org/23903051/diff/21001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:66:
weak_ptr_factory_(this) {
On 2013/09/18 05:53:19, hashimoto wrote:
> DeviceIDFetcher is ref counted. Is threre any reason to bind WeakPtr, instead
of
> scoped_refptr, to callbacks?
Facepalm. Thanks. I don't know how I missed that. I've gotten too used to using
WeakPtr. Reverted.

[raymes, 2013-09-18 23:21:20]

pepper lgtm

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (left):

https://codereview.chromium.org/23903051/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:115: // in case the
legacy id doesn't exist.
Not easily. There's a bug filed at
https://code.google.com/p/chromium/issues/detail?id=234411

https://codereview.chromium.org/23903051/diff/28001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (right):

https://codereview.chromium.org/23903051/diff/28001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:39: void
GetMachineIdAsync(const DeviceIDFetcher::IDCallback& callback) {
nit: Id->ID for consistency in the file.

https://codereview.chromium.org/23903051/diff/28001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:203: // If we didn't
find an ID, get the machine Id and call the new code path to
nit: Id -> ID for consistency.

[stevenjb, 2013-09-18 23:50:51]

https://codereview.chromium.org/23903051/diff/28001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (right):

https://codereview.chromium.org/23903051/diff/28001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:39: void
GetMachineIdAsync(const DeviceIDFetcher::IDCallback& callback) {
On 2013/09/18 23:21:21, raymes wrote:
> nit: Id->ID for consistency in the file.

Done.

https://codereview.chromium.org/23903051/diff/28001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:203: // If we didn't
find an ID, get the machine Id and call the new code path to
On 2013/09/18 23:21:21, raymes wrote:
> nit: Id -> ID for consistency.

Done.

[hashimoto, 2013-09-19 02:54:43]

lgtm

[pneubeck (no reviews), 2013-09-19 14:45:45]

chromeos/ lgtm

[commit-bot: I haz the power, 2013-09-19 16:55:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/23903051/46001

[commit-bot: I haz the power, 2013-09-20 00:16:47]

Change committed as 224248

[zel, 2013-09-20 16:28:05]

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/chr...
File chrome/browser/chromeos/login/managed/managed_user_authenticator.cc
(right):

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/chr...
chrome/browser/chromeos/login/managed/managed_user_authenticator.cc:107:
CHECK(!ascii_salt.empty());
same here

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/chr...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/chr...
chrome/browser/chromeos/login/parallel_authenticator.cc:191:
CHECK(!ascii_salt.empty());
how's this helping to fix this problem?

you've just move the assert from one place to another

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/ren...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (right):

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/ren...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:45: if (result.empty())
{
this logic should move to async version of CryptohomeLibrary::GetSystemSalt()

https://chromiumcodereview.appspot.com/23903051/diff/46001/chromeos/cryptohom...
File chromeos/cryptohome/cryptohome_library.h (right):

https://chromiumcodereview.appspot.com/23903051/diff/46001/chromeos/cryptohom...
chromeos/cryptohome/cryptohome_library.h:65: virtual std::string GetSystemSalt()
= 0;
this method should obviously be converted to async

[stevenjb, 2013-09-20 16:42:23]

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/chr...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/chr...
chrome/browser/chromeos/login/parallel_authenticator.cc:191:
CHECK(!ascii_salt.empty());
On 2013/09/20 16:28:05, zel wrote:
> how's this helping to fix this problem?
> 
> you've just move the assert from one place to another

The crash we were seeing occurred because some extensions code was trying to
access system salt early. We still need to ensure that we don't try to login
without system salt. We should absolutely change this to be more robust, but
someone more familiar with the login code should do this. There's a long
standing bug to fix this, crbug.com/141009.

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/ren...
File chrome/browser/renderer_host/pepper/device_id_fetcher.cc (right):

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/ren...
chrome/browser/renderer_host/pepper/device_id_fetcher.cc:45: if (result.empty())
{
On 2013/09/20 16:28:05, zel wrote:
> this logic should move to async version of CryptohomeLibrary::GetSystemSalt()

Agreed. This will require some significant changes to the login code however.

[zel, 2013-09-20 16:48:40]

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/chr...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/chr...
chrome/browser/chromeos/login/parallel_authenticator.cc:191:
CHECK(!ascii_salt.empty());
On 2013/09/20 16:42:23, stevenjb wrote:
> On 2013/09/20 16:28:05, zel wrote:
> > how's this helping to fix this problem?
> > 
> > you've just move the assert from one place to another
> 
> The crash we were seeing occurred because some extensions code was trying to
> access system salt early. We still need to ensure that we don't try to login
> without system salt. We should absolutely change this to be more robust, but
> someone more familiar with the login code should do this. There's a long
> standing bug to fix this, crbug.com/141009.

Yes, I would rather:
a) Find out which extension is responsible for that (there are not that many of
them running at login screen) and figure out if it's really launched too early,
and
b) Revert this CL, and
c) Fix http://crbug.com/141009 properly

[zel, 2013-09-20 16:54:30]

On 2013/09/20 16:48:40, zel wrote:
>
https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/chr...
> File chrome/browser/chromeos/login/parallel_authenticator.cc (right):
> 
>
https://chromiumcodereview.appspot.com/23903051/diff/46001/chrome/browser/chr...
> chrome/browser/chromeos/login/parallel_authenticator.cc:191:
> CHECK(!ascii_salt.empty());
> On 2013/09/20 16:42:23, stevenjb wrote:
> > On 2013/09/20 16:28:05, zel wrote:
> > > how's this helping to fix this problem?
> > > 
> > > you've just move the assert from one place to another
> > 
> > The crash we were seeing occurred because some extensions code was trying to
> > access system salt early. We still need to ensure that we don't try to login
> > without system salt. We should absolutely change this to be more robust, but
> > someone more familiar with the login code should do this. There's a long
> > standing bug to fix this, crbug.com/141009.
> 
> Yes, I would rather:
> a) Find out which extension is responsible for that (there are not that many
of
> them running at login screen) and figure out if it's really launched too
early,
> and
> b) Revert this CL, and
> c) Fix http://crbug.com/141009 properly

To answer (a): PrefMetricsService::PrefMetricsService() seems to be calling
extensions::api::DeviceId::GetDeviceId(). That's what caused the flood of these
crashes in M31.