Accept invalid Schemas for the managed storage API.

Accept invalid Schemas for the managed storage API.

Extensions won't get installed if they declare a managed storage schema
and the schema is invalid. But if the schema is modified or deleted
after the extension is installed then this CHECK will be hit whenever
Chrome restarts.

If the schema is modified or becomes invalid then the managed storage
API won't pickup the right policies for the extension, but that's
better than crashing the browser.

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

[Joao da Silva, 2014-08-12 13:03:26]

This fixes the crash reported on the bug but I'm not sure if it's the right fix.

If StorageSchemaManifestHandler::Validate() is called on every browser restart
when the extensions get loaded then this CHECK shouldn't be hit, because the
extension shouldn't even have been loaded when the schema.json file was deleted
from the hard drive.

But if StorageSchemaManifestHandler::Validate() is only called at install time
then this fix is correct.

Can you confirm when does Validate() get called?

[Joao da Silva, 2014-08-12 13:14:34]

So I just confirmed this: Validate() is only called when an extension is first
installed; it doesn't get called if it's disabled and enabled again, nor on
subsequent restarts.

In that case this fix is fine :-) PTAL, thanks!

[not at google - send to devlin, 2014-08-12 20:13:17]

https://codereview.chromium.org/464893002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (left):

https://codereview.chromium.org/464893002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:210:
(*components)[(*it)->id()] = schema;
Now you'll be adding a potentially invalid schema to |components|. Is that what
you want?

[Joao da Silva, 2014-08-13 10:05:03]

PTAL

https://codereview.chromium.org/464893002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (left):

https://codereview.chromium.org/464893002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:210:
(*components)[(*it)->id()] = schema;
On 2014/08/12 20:13:17, kalman wrote:
> Now you'll be adding a potentially invalid schema to |components|. Is that
what
> you want?

Not quite, thanks for pointing this out :-) What we want in this case is an
empty schema: the extension is still in the policy registry and will show up in
chrome://policy (and the chrome.storage.managed API exists) but it won't be able
to load any policies.

[not at google - send to devlin, 2014-08-13 17:27:32]

lgtm

https://codereview.chromium.org/464893002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/464893002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:210: // will
be listed in chrome://policy but won't be able to load any policies.
It'd be nice to try to recover the policy in this case, e.g. by re-installing
the extension. I've seen that requested before for enterprise extensions. I
can't find the bug but maybe asargent/erikkay can remember. Anyhow, not
something to try in this CL... but it'd be nice to have a bug to reference.

[Joao da Silva, 2014-08-14 11:56:57]

https://codereview.chromium.org/464893002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/464893002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:210: // will
be listed in chrome://policy but won't be able to load any policies.
On 2014/08/13 17:27:31, kalman wrote:
> It'd be nice to try to recover the policy in this case, e.g. by re-installing
> the extension. I've seen that requested before for enterprise extensions. I
> can't find the bug but maybe asargent/erikkay can remember. Anyhow, not
> something to try in this CL... but it'd be nice to have a bug to reference.

sgtm. Shouldn't we try to do the same recovery for non-enterprise extensions
too?

[Joao da Silva, 2014-08-14 11:57:01]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-08-14 12:00:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/464893002/20001

[not at google - send to devlin, 2014-08-14 14:08:38]

> 
> sgtm. Shouldn't we try to do the same recovery for non-enterprise extensions
> too?

That would be nice. It's especially bad for enterprise extensions because you
can't reinstall them, though.

lgtm for CQ.

[commit-bot: I haz the power, 2014-08-14 17:13:27]

Committed patchset #2 (20001) as 289623