The current "creation_time" is really provision time. Maybe we can add a new "generation_time" for the case where an origin ID has been generated but not provisioned... WDYT?

https://cs.chromium.org/chromium/src/components/cdm/browser/media_drm_storage_impl.cc?type=cs&q=MediaDrmStorageImpl&g=0&l=380

We have a DCHECK here. Are you able to reproduce this locally? Seems like we're creating more than one MediaDrmStorageImpl for the same origin.

I would suggest we change the DCHECK to an early return.

MediaDrmStorageImpl is created per MojoMediaDrmStorage, which is per-AndroidCdmFactory. We have one AndroidCdmFactory per render frame. So all MediaDrmBridge instances within the same RenderFrame should share the same MediaDrmStorageImpl instance.

So you are right that there should be only one MediaDrmStorageImpl, but we'll still hit the bug since we call storage_->Initialize() for each MediaDrmBridge creation:

https://cs.chromium.org/chromium/src/media/base/android/media_drm_bridge_factory.cc?rcl=391406340cb5144eb0a02335d035f7c47e9ae3bb&l=80

Also, we could have two tabs opening (two RenderFrames) which are from the same origin, then it'll hit the case in this bug report.

For repro steps, please follow repro steps in https://bugs.chromium.org/p/chromium/issues/detail?id=803524
where we'll create multiple MediaDrmBridge instances on the same page (same RenderFrame).

I'll prefer we keep the origin ID in memory, before provision is finished. Also, any reason we need to differentiate the two cases?
1. origin ID was just created.
2. origin ID was created and provisioned.

I think MediaDrm should be the true source on whether the origin is provisioned or not.

Plan to have a object for origin ID, which will be shared among different MediaDrmStorageImpl.

Since we don't care about case 1 vs case 2 mentioned in Comment 5. I'll change the class to store origin ID when it's generated (in Initialize).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8853657c98dcadb4cf5b799adb2785dbdf2a5ced

commit 8853657c98dcadb4cf5b799adb2785dbdf2a5ced
Author: yucliu <yucliu@chromium.org>
Date: Tue Jun 05 19:58:03 2018

Update base::Value function calls in MediaDrmStorageImpl

Lots of base::Value APIs are deprecated. Update the callsite in
MediaDrmStorageImpl to use new APIs:

1. Replace DictionaryValue with Value.
2. Use Value directly, instead of wrapping in std::unique_ptr.
3. Update value get function with FindKeyOfType.

BUG= 846168 
TEST=Shaka demo, clear media licenses.

Change-Id: I4137060753280e605dc34e37a6f0df9da148f6b5
Reviewed-on: https://chromium-review.googlesource.com/1080224
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: John Rummell <jrummell@chromium.org>
Commit-Queue: Yuchen Liu <yucliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564621}
[modify] https://crrev.com/8853657c98dcadb4cf5b799adb2785dbdf2a5ced/components/cdm/browser/media_drm_storage_impl.cc
[modify] https://crrev.com/8853657c98dcadb4cf5b799adb2785dbdf2a5ced/components/cdm/browser/media_drm_storage_impl_unittest.cc

Plan to write origin ID, together with provision time, to pref_service when MediaDrmStorageImpl::Initialize is called and origin ID doesn't exist. Previously it's written to pref_service when MediaDrmStorageImpl::OnProvision is called, which happens later than Initialize.

The drawback is that when provision failed, or interrupted, the origin ID is already persisted on the device. When the device do provision again, it will re-use the old origin ID and provision time. The provision time is used when clearing licenses: if provision time is greater, the creation time of the licenses will not be checked. Probably user only cares about license creation time, it should be safe to move the logic into Initialize.

As a result, we can completely remove OnProvision from MediaDrmStorage (from Java in GPU to C++ in browser).

Please let me know if there're any potential issues.

Thanks!

Just find that when resetting device credential, we actually use device provision. With my approach, the storage won't be cleared. As a result, the next time device is provisioned, the old license is still in the storage, although it's not accessible by MediaDrm. So we should keep the "OnProvision" path, and make it reset the storage associated with current origin.

The proposal on OnProvision() sgtm. 

I am not entirely clear how clear media license works with creation time now. Let's chat offline on that.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e62e97671aa76b276794bc7a84e73090086c27df

commit e62e97671aa76b276794bc7a84e73090086c27df
Author: yucliu <yucliu@chromium.org>
Date: Fri Jul 20 00:20:34 2018

[Android EME] Persist origin ID immediately when it's generated.

MediaDrmStorageImpl::Initialize will write an origin ID to storage when
a new origin ID is generated. The origin ID is persisted along with the
current time as the creation time, so that if multiple MediaDrmBridge
instances from the same origin start provisioning concurrently, they
will get the same origin ID.

BUG= 846168 
TEST=Test mentioned in  https://crbug.com/803524  (provision happens only once)
TEST=components_unittests for MediaDrmStorageImpl

Change-Id: I77a9dc1ef902dff699f21c8bfb7014ae4f3af7c1
Reviewed-on: https://chromium-review.googlesource.com/1093436
Reviewed-by: David Dorwin <ddorwin@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: John Rummell <jrummell@chromium.org>
Commit-Queue: Yuchen Liu <yucliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576709}
[modify] https://crrev.com/e62e97671aa76b276794bc7a84e73090086c27df/components/cdm/browser/media_drm_storage_impl.cc
[modify] https://crrev.com/e62e97671aa76b276794bc7a84e73090086c27df/components/cdm/browser/media_drm_storage_impl_unittest.cc
[modify] https://crrev.com/e62e97671aa76b276794bc7a84e73090086c27df/media/mojo/interfaces/media_drm_storage.mojom

This bug requires manual review: We don't branch M69 until 2018-07-19.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Seems like the cut is after my commit

Removing merge labels as this made 69 before the cut.

You're the owner of a Pri-1 M-69 chrome media issue. M-69 is now in beta and will ship to stable in coming weeks. See go/chromeschedule. Please work on resolving your issue ASAP if it needs fixing for the M-69 branch.

Pri-1 means the work is required for the branch. Alternatively, update the milestone to M-70 or remove the milestone and drop the priority to P-3.

yucliu: Can we mark this as fixed now?

It's fixed now.