Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2765343003:
media: Add MediaDrmStorage (Closed)
|
Created:
3 years, 9 months ago by xhwang Modified:
3 years, 8 months ago Reviewers:
Ken Rockot(use gerrit already), gab, dcheng, liberato (no reviews please), yzshen1, yucliu1 CC:
chromium-reviews, qsr+mojo_chromium.org, feature-media-reviews_chromium.org, chfremer+watch_chromium.org, viettrungluu+watch_chromium.org, avayvod+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, mcasas+watch+vc_chromium.org, alokp+watch_chromium.org, darin (slow to review), mlamouri+watch-media_chromium.org, alokp, jrummell Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Add MediaDrmStorage
MediaDrmStorage is an interface that provides persistent storage for
MediaDrmBridge. This is needed for per-origin provisioning, and for
persistent license support.
The real storage is implemented using PrefService, and all the read and
write operations happen in the browser process. This makes it easier to
manage concurrent access to the storage by multiple MediaDrmBridge
instances, and by code implementing related features, e.g. clearing
media licenses in "Clear Browsing Data".
A mojom MediaDrmStorage interface is added to connect MediaDrmBridge
(typically running in the GPU process) and the PrefService running in
the browser process. It strictly maps the C++ MediaDrmStorage interface.
Here's a summary of the stack:
MediaDrmBridge (Java) ->
MediaDrmStorageBridge (JNI) ->
MediaDrmStorage (C++ interface) ->
MojoMediaDrmStorage (C++ interface implementation) ->
media::mojom::MojoMediaDrm (mojo interface) ->
------ GPU process ------
mojo IPC
------ Browser process ------
MediaDrmStorageImpl (mojo interface implementation) ->
PrefService
Note that there's a mojo Preferences service (/services/preferences/).
I prototyped it and talked with the owners. It provides some cool
features, but is hard to code against when there are multiple clients
of the preference. In our case, we could have multiple MediaDrmBridge
instances running in the GPU process and UI code clearing media
licenses in the Browser process accessing the preference at the same
time. Plumbing everything to the browser process makes it much easier
to avoid tricky race/synchronization problems, and is recommended by
Preferences owners.
BUG=493521
Review-Url: https://codereview.chromium.org/2765343003
Cr-Commit-Position: refs/heads/master@{#461479}
Committed: https://chromium.googlesource.com/chromium/src/+/116acb96458241e0db329600538a40ae9c3d1abd
Patch Set 1 #
Total comments: 13
Patch Set 2 : Move MediaDrmStorageImpl to components/ #
Total comments: 1
Patch Set 3 : Removed MediaDrmStorageImpl impl (will finish in the next CL) #Patch Set 4 : fix compile error #
Total comments: 46
Patch Set 5 : comments #Patch Set 6 : struct #Patch Set 7 : comments #Patch Set 8 : Watch RenderFrameDeleted #Patch Set 9 : media: Add MediaDrmStorage #Patch Set 10 : rebase only #Patch Set 11 : a few tweaks #Patch Set 12 : media: Add MediaDrmStorage #
Total comments: 10
Patch Set 13 : comments addressed #
Total comments: 2
Patch Set 14 : fix DidFinishNavigation #Patch Set 15 : rebase only #Messages
Total messages: 90 (49 generated)
Description was changed from ========== media: Add MediaDrmStorage MediaDrmStorage is an interface that provides persistent storage for MediaDrmBridge. This is needed for per-origin provisioning, and for persistent license support. The real storage is implemented using PrefService, and all the read and write operations happen in the browser process. This makes it easier to manage concurrent access to the storage by multiple MediaDrmBridge instances, and by code implementing related features, e.g. clearing media licenses in "Clear Browsing Data". A mojom MediaDrmStorage interface is added to connect MediaDrmBridge (typically running in the GPU process) and the PrefService running in the browser process. It strictly maps the C++ MediaDrmStorage interface. BUG=493521 ========== to ========== media: Add MediaDrmStorage MediaDrmStorage is an interface that provides persistent storage for MediaDrmBridge. This is needed for per-origin provisioning, and for persistent license support. The real storage is implemented using PrefService, and all the read and write operations happen in the browser process. This makes it easier to manage concurrent access to the storage by multiple MediaDrmBridge instances, and by code implementing related features, e.g. clearing media licenses in "Clear Browsing Data". A mojom MediaDrmStorage interface is added to connect MediaDrmBridge (typically running in the GPU process) and the PrefService running in the browser process. It strictly maps the C++ MediaDrmStorage interface. Here's a summary of the stack: MediaDrmBridge (Java) -> MediaDrmStorageBridge (JNI) -> MediaDrmStorage (C++ interface) -> MojoMediaDrmStorage (C++ interface implementation) -> media::mojom::MojoMediaDrm (mojo interface) -> ------ GPU process ------ mojo IPC ------ Browser process ------ MediaDrmStorageImpl (mojo interface implementation) -> PrefService BUG=493521 ==========
xhwang@chromium.org changed reviewers: + jrummell@chromium.org, yucliu@chromium.org
yucliu: This is the prototype CL I have. The most important part is the new C++ MediaDrmStorage interface. Please take a look to see whether it'll work for you. The |origin_id| idea is not in this CL yet, which will be added later. If it looks good. I'll probably split the CL to upload all the interfaces and plumbing first, then upload the implementation in MediaDrmStorageImpl with unittests. https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... File media/base/android/media_drm_storage.h (right): https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... media/base/android/media_drm_storage.h:23: class MEDIA_EXPORT MediaDrmStorage { yucliu: This will be the main interface passed to MediaDrmBridge. The bridge code you have can be named MediaDrmStorageBridge which connects Java with MediaDrmStorage.
The patch is good. I think we can use these APIs for storage. https://codereview.chromium.org/2765343003/diff/1/chrome/browser/media/androi... File chrome/browser/media/android/cdm/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/1/chrome/browser/media/androi... chrome/browser/media/android/cdm/media_drm_storage_impl.cc:25: // "origin_id": $origin_id So origin_id is a random string, right? Who's responsible to generate origin_id, in MediaDrmBridge or here? https://codereview.chromium.org/2765343003/diff/1/chrome/browser/media/androi... chrome/browser/media/android/cdm/media_drm_storage_impl.cc:162: callback.Run(false); A bigger questions here, what should we do if error happens in storage, should we simply reject everything and clear everything? https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... media/base/android/media_drm_bridge.cc:775: create_storage_cb_(create_storage_cb), Just curious, why do want to keep create_storage_cb_, since it's only used once in ctor. https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... File media/base/android/media_drm_storage.h (right): https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... media/base/android/media_drm_storage.h:31: const std::vector<uint8_t>& key_set_id, We may want std::string key_set_id. https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... media/base/android/media_drm_storage.h:43: virtual void OnProvisioned(ResultCB result_cb) = 0; Should we have a function to clear the provisioned origin info? Or will OnProvisioned reset the map?
https://codereview.chromium.org/2765343003/diff/1/chrome/browser/media/androi... File chrome/browser/media/android/cdm/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/1/chrome/browser/media/androi... chrome/browser/media/android/cdm/media_drm_storage_impl.cc:25: // "origin_id": $origin_id On 2017/03/23 01:07:16, yucliu1 wrote: > So origin_id is a random string, right? Who's responsible to generate origin_id, > in MediaDrmBridge or here? Good question. It's just a random string. In theory it can be generated anywhere. But the tricky part is we could create two MediaDrmBridge instances for the same origin, both triggering the provisioning flow. In that case, we should have some locking mechanism to make sure we don't have collision. I need to think more about this case. For now, let's forget about |origin_id| and just use the origin. We need to fix this before removing the flag. https://codereview.chromium.org/2765343003/diff/1/chrome/browser/media/androi... chrome/browser/media/android/cdm/media_drm_storage_impl.cc:162: callback.Run(false); On 2017/03/23 01:07:16, yucliu1 wrote: > A bigger questions here, what should we do if error happens in storage, should > we simply reject everything and clear everything? In most cases, we should never fail. The only legit case this (and similarly others) could fail is when we have "clear media licenses" implemented. For example, when user choose to "clear media licenses", we'll unprovision the origin, and then remove the "origin dict", in the browser process. At the same time, SavepersistentSession() is called. Since the origin has been unprovisioned, the license cannot be used anymore, so we should just do nothing. The end result will be the same as we store the session first, then remove the whole origin dict. But I agree these cases could be tricky and we need to think them through. https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... media/base/android/media_drm_bridge.cc:775: create_storage_cb_(create_storage_cb), On 2017/03/23 01:07:16, yucliu1 wrote: > Just curious, why do want to keep create_storage_cb_, since it's only used once > in ctor. This is for the "lazy creation" of the storage. For example, on older (pre M) devices, we'll never need to use the storage. Creation of the storage will trigger a mojo interface request and cause the creation of MediaDrmStorageImpl on the browser side, which we should avoid if we are not going to use it. https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... File media/base/android/media_drm_storage.h (right): https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... media/base/android/media_drm_storage.h:31: const std::vector<uint8_t>& key_set_id, On 2017/03/23 01:07:16, yucliu1 wrote: > We may want std::string key_set_id. Will the Java side pass a string for the key_set_id? I saw it's a byte[] in MediaDrm API. https://codereview.chromium.org/2765343003/diff/1/media/base/android/media_dr... media/base/android/media_drm_storage.h:43: virtual void OnProvisioned(ResultCB result_cb) = 0; On 2017/03/23 01:07:16, yucliu1 wrote: > Should we have a function to clear the provisioned origin info? Or will > OnProvisioned reset the map? This will happen on the browser side, and doesn't have to go through this interface.
https://codereview.chromium.org/2765343003/diff/1/chrome/browser/media/androi... File chrome/browser/media/android/cdm/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/1/chrome/browser/media/androi... chrome/browser/media/android/cdm/media_drm_storage_impl.cc:10: #include "chrome/browser/profiles/profile.h" yucliu: This is the only place where this file depend on chrome/. If we can remove this, I can put this file under components/cdm/browser/ so that it can be reused by ChromeCast. Do you know how ChromeCast get the PrefService from the BrowserContext? (see line 92)
https://codereview.chromium.org/2765343003/diff/1/chrome/browser/media/androi... File chrome/browser/media/android/cdm/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/1/chrome/browser/media/androi... chrome/browser/media/android/cdm/media_drm_storage_impl.cc:10: #include "chrome/browser/profiles/profile.h" On 2017/03/23 05:25:26, xhwang_slow wrote: > yucliu: This is the only place where this file depend on chrome/. If we can > remove this, I can put this file under components/cdm/browser/ so that it can be > reused by ChromeCast. Do you know how ChromeCast get the PrefService from the > BrowserContext? (see line 92) This would be helpful! https://cs.chromium.org/chromium/src/chromecast/browser/cast_browser_main_par... We're able to get PrefService in content client. So probably we can pass PrefService to this class.
alokp@chromium.org changed reviewers: + alokp@chromium.org
drive-by comment https://codereview.chromium.org/2765343003/diff/20001/media/mojo/interfaces/m... File media/mojo/interfaces/media_drm_storage.mojom (right): https://codereview.chromium.org/2765343003/diff/20001/media/mojo/interfaces/m... media/mojo/interfaces/media_drm_storage.mojom:15: interface MediaDrmStorage { If you could access the per-profile Pref registry from the media service, would you still need this interface? Apparently there is a preferences service under construction: https://cs.chromium.org/chromium/src/services/preferences/
On 2017/03/24 04:51:26, alokp wrote: > drive-by comment > > https://codereview.chromium.org/2765343003/diff/20001/media/mojo/interfaces/m... > File media/mojo/interfaces/media_drm_storage.mojom (right): > > https://codereview.chromium.org/2765343003/diff/20001/media/mojo/interfaces/m... > media/mojo/interfaces/media_drm_storage.mojom:15: interface MediaDrmStorage { > If you could access the per-profile Pref registry from the media service, would > you still need this interface? Apparently there is a preferences service under > construction: > > https://cs.chromium.org/chromium/src/services/preferences/ Thanks for pointing this out! Yes, I had a lengthy conversation with the owners of the preferences mojom service and there are multiple reasons it doesn't meet our requirements. Therefore it's recommended that we use our own plumbing and do all the real work in the browser process.
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
media: Add MediaDrmStorage
MediaDrmStorage is an interface that provides persistent storage for
MediaDrmBridge. This is needed for per-origin provisioning, and for
persistent license support.
The real storage is implemented using PrefService, and all the read and
write operations happen in the browser process. This makes it easier to
manage concurrent access to the storage by multiple MediaDrmBridge
instances, and by code implementing related features, e.g. clearing
media licenses in "Clear Browsing Data".
A mojom MediaDrmStorage interface is added to connect MediaDrmBridge
(typically running in the GPU process) and the PrefService running in
the browser process. It strictly maps the C++ MediaDrmStorage interface.
Here's a summary of the stack:
MediaDrmBridge (Java) ->
MediaDrmStorageBridge (JNI) ->
MediaDrmStorage (C++ interface) ->
MojoMediaDrmStorage (C++ interface implementation) ->
media::mojom::MojoMediaDrm (mojo interface) ->
------ GPU process ------
mojo IPC
------ Browser process ------
MediaDrmStorageImpl (mojo interface implementation) ->
PrefService
BUG=493521
==========
to
==========
media: Add MediaDrmStorage
MediaDrmStorage is an interface that provides persistent storage for
MediaDrmBridge. This is needed for per-origin provisioning, and for
persistent license support.
The real storage is implemented using PrefService, and all the read and
write operations happen in the browser process. This makes it easier to
manage concurrent access to the storage by multiple MediaDrmBridge
instances, and by code implementing related features, e.g. clearing
media licenses in "Clear Browsing Data".
A mojom MediaDrmStorage interface is added to connect MediaDrmBridge
(typically running in the GPU process) and the PrefService running in
the browser process. It strictly maps the C++ MediaDrmStorage interface.
Here's a summary of the stack:
MediaDrmBridge (Java) ->
MediaDrmStorageBridge (JNI) ->
MediaDrmStorage (C++ interface) ->
MojoMediaDrmStorage (C++ interface implementation) ->
media::mojom::MojoMediaDrm (mojo interface) ->
------ GPU process ------
mojo IPC
------ Browser process ------
MediaDrmStorageImpl (mojo interface implementation) ->
PrefService
Note that there's a mojo Preferences service (/services/preferences/).
I prototyped it and talked with the owners. It provides some cool
features, but is hard to code against when there are multiple clients
of the preference. In our case, we could have multiple MediaDrmBridge
instances running in the GPU process and UI code clearing media
licenses in the Browser process accessing the preference at the same
time. Plumbing everything to the browser process makes it much easier
to avoid tricky race/synchronization problems, and is recommended by
Preferences owners.
BUG=493521
==========
xhwang@chromium.org changed reviewers: - alokp@chromium.org
xhwang@chromium.org changed reviewers: + liberato@chromium.org - jrummell@chromium.org
alokp/jrummell: FYI https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:71: NOTIMPLEMENTED(); Partial implementation of these functions and some test code in MediaDrmBridge can be found in PS2. I removed them so we can focus on plumbing in this CL. In the next CL, I'll implement this class and add unit tests.
xhwang@chromium.org changed reviewers: + dcheng@chromium.org
yucliu1: Please review everything! liberato: Please review from meida/clank/mojo's perspective. It's harder for me to find good reviewers for EME on Clank, and I hope you can help! Thanks! dcheng: Please review from mojom and security's perspective. Feel free to wait for other reviewers to start. But if you see anything obviously wrong, it'd nice to let me know as early as possible :)
BTW, the CL is mostly interfaces and plumbing so it should be pretty straightforward to review :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:3134: base::Bind(&chrome::CreateMediaDrmStorage, render_frame_host)); Is it possible for media code to know the storage isn't available? If some embedder (cast for Android) forget to add the interface, we should either guide the developer to add the interface or disable the feature. https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... File chrome/browser/media/android/cdm/media_drm_storage_factory.cc (right): https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:41: return; Are we able to launch the page if these early returns happen? If so, should we just disable all the persistent license request? https://codereview.chromium.org/2765343003/diff/80001/media/base/android/medi... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/2765343003/diff/80001/media/base/android/medi... media/base/android/media_drm_bridge.cc:778: create_storage_cb_(create_storage_cb), I don't think we need this member in MediaDrmBridge, because we'll call the callback in ctor. But I can change it in the later patch.
Comments only. https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:3134: base::Bind(&chrome::CreateMediaDrmStorage, render_frame_host)); On 2017/03/27 19:06:41, yucliu1 wrote: > Is it possible for media code to know the storage isn't available? If some > embedder (cast for Android) forget to add the interface, we should either guide > the developer to add the interface or disable the feature. This should be configured correctly during build time. Runtime detection would be too late since we need to answer whether we support persistent license in requestMediaKeySystemAccess() call, when we don't even have a CDM instance created. https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... File chrome/browser/media/android/cdm/media_drm_storage_factory.cc (right): https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:41: return; On 2017/03/27 19:06:41, yucliu1 wrote: > Are we able to launch the page if these early returns happen? If so, should we > just disable all the persistent license request? All of these should never happen in practice :) If they do happen for whatever reason, the MojoMediaDrmStorage will get a ConnectionError. We can make the Initialize() call asyc so that we'll know whether it's initialized successfully. If it failed, we can reject all calls related to persistent license. I'll add a TODO to handle connection error in MojoMediaDrmStorage. https://codereview.chromium.org/2765343003/diff/80001/media/base/android/medi... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/2765343003/diff/80001/media/base/android/medi... media/base/android/media_drm_bridge.cc:778: create_storage_cb_(create_storage_cb), On 2017/03/27 19:06:41, yucliu1 wrote: > I don't think we need this member in MediaDrmBridge, because we'll call the > callback in ctor. But I can change it in the later patch. sgtm
https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:3134: base::Bind(&chrome::CreateMediaDrmStorage, render_frame_host)); On 2017/03/27 20:28:46, xhwang_slow wrote: > On 2017/03/27 19:06:41, yucliu1 wrote: > > Is it possible for media code to know the storage isn't available? If some > > embedder (cast for Android) forget to add the interface, we should either > guide > > the developer to add the interface or disable the feature. > > This should be configured correctly during build time. Runtime detection would > be too late since we need to answer whether we support persistent license in > requestMediaKeySystemAccess() call, when we don't even have a CDM instance > created. Thought about this more, since not all content embedders will support this, we can add a build time gn flag to control this. Basically only when the gn flag and the run time flag are enabled, we will say persistent license is supported in requestMediaKeySystemAccess(). I'll add this later. For now we are good since everything is behind the run-time flag.
FYI, the implementation and test are uploaded in https://chromiumcodereview.appspot.com/2780683002/
lgtm % two questions. thanks -fl https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... File chrome/browser/media/android/cdm/media_drm_storage_factory.cc (right): https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:39: PrefService* pref_service = profile->GetPrefs(); how does lifetime for |pref_service| work? for incognito sessions, at least, the prefs service might be owned by the profile. https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... File chrome/browser/media/android/cdm/media_drm_storage_factory.h (right): https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.h:16: void CreateMediaDrmStorage(content::RenderFrameHost* render_frame_host, i think that the answer to my question is 'leave it as is'. however, here's the discussion that went on in my head. :) should this be exposed on RFH directly? all of the other interfaces are visible in content/ already. of course, RFH would need to ask the embedder to provide the concrete impl. do we believe that the interfaces in media/ are applicable to any embedder? seems like the answer should be 'yes'.
lgtm https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/moj... File media/mojo/services/mojo_media_drm_storage.cc (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_media_drm_storage.cc:21: void MojoMediaDrmStorage::Initialize(const url::Origin& origin) { I think all of the MediaDrm codes are using GURL as origin. Should we change the type there or here? https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_media_drm_storage.cc:66: std::move(result_cb).Run(success); Just curious, what's the benefit for std::move here?
https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/moj... File media/mojo/services/mojo_media_drm_storage.cc (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_media_drm_storage.cc:21: void MojoMediaDrmStorage::Initialize(const url::Origin& origin) { On 2017/03/28 18:10:20, yucliu1 wrote: > I think all of the MediaDrm codes are using GURL as origin. Should we change the > type there or here? We have a bug to replace GURL with url::Origin: https://bugs.chromium.org/p/chromium/issues/detail?id=639438 https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_media_drm_storage.cc:66: std::move(result_cb).Run(success); On 2017/03/28 18:10:20, yucliu1 wrote: > Just curious, what's the benefit for std::move here? It's a OnceCallback, you have to "move" it before you can call it. https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#
dcheng: PTAL!
https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... File chrome/browser/media/android/cdm/media_drm_storage_factory.cc (right): https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:28: if (!web_contents) I'm curious how many of these early returns are actually needed. For example, is it possible for this to return nothing if RenderFrameHost is not null? https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:31: content::BrowserContext* browser_context = web_contents->GetBrowserContext(); Similarly, it doesn't seem like this should /ever/ be null. https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:45: std::move(request)); Nit: #include <utility> https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:63: origin_ = origin.Serialize(); Rather than passing the origin over Mojo, can we just get it from the RenderFrameHost? (This also means we need to close the pipe on navigation) https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:69: DCHECK(!origin_.empty()); Note that we shouldn't be DCHECKing these sorts of things: it's very possible that a malicious renderer could simply not call Initialize(), hence my comment about initializing origin from the RFH. https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.h (right): https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.h:28: ~MediaDrmStorageImpl() final; Alternatively, the entire class can be marked final: class MediaDrmStorageImpl final : public ... { https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... File media/mojo/interfaces/media_drm_storage.mojom (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/media_drm_storage.mojom:21: string session_id, array<uint8> key_set_id, string mime_type) One alternative is to package key_set_id and mime_type in a mojo struct. https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/media_drm_storage.mojom:28: => (bool success, array<uint8> key_set_id, string mime_type); Then here we can just return SessionData? here, and avoid the need for bool success + the two members https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/and... File media/mojo/services/android_mojo_media_client.cc (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/and... media/mojo/services/android_mojo_media_client.cc:37: std::move(media_drm_storage_ptr)); #include <utility> https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/gpu... File media/mojo/services/gpu_mojo_media_client.cc (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/gpu... media/mojo/services/gpu_mojo_media_client.cc:44: std::move(media_drm_storage_ptr)); #include <utility> https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/moj... File media/mojo/services/mojo_media_drm_storage.cc (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/moj... media/mojo/services/mojo_media_drm_storage.cc:78: std::move(load_persistent_session_cb).Run(success, key_set_id, mime_type); #include <utility>
dcheng: Thanks for the comments. PTAL again! https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... File chrome/browser/media/android/cdm/media_drm_storage_factory.cc (right): https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:28: if (!web_contents) On 2017/03/29 01:17:22, dcheng wrote: > I'm curious how many of these early returns are actually needed. For example, is > it possible for this to return nothing if RenderFrameHost is not null? Well, I have the same question. But I'd rather code against the API [1] which doesn't provide any comment :) I could check the implementation but I choose not to, because I don't want to code against implementation details, which isn't recommended... https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:31: content::BrowserContext* browser_context = web_contents->GetBrowserContext(); On 2017/03/29 01:17:22, dcheng wrote: > Similarly, it doesn't seem like this should /ever/ be null. Similarly, it's not clear to me whether this could be null from the API :( https://cs.chromium.org/chromium/src/content/public/browser/web_contents.h?rc... https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:39: PrefService* pref_service = profile->GetPrefs(); On 2017/03/28 16:42:43, liberato wrote: > how does lifetime for |pref_service| work? for incognito sessions, at least, > the prefs service might be owned by the profile. I don't really know. But I am following how PrefService are used throughout the code base. I think it makes sense to assume that if a RenderFrameHost is alive, then the profile associated with it is also alive. https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:41: return; On 2017/03/27 20:28:46, xhwang_slow wrote: > On 2017/03/27 19:06:41, yucliu1 wrote: > > Are we able to launch the page if these early returns happen? If so, should we > > just disable all the persistent license request? > > All of these should never happen in practice :) > > If they do happen for whatever reason, the MojoMediaDrmStorage will get a > ConnectionError. We can make the Initialize() call asyc so that we'll know > whether it's initialized successfully. If it failed, we can reject all calls > related to persistent license. > > I'll add a TODO to handle connection error in MojoMediaDrmStorage. Done. https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:45: std::move(request)); On 2017/03/29 01:17:22, dcheng wrote: > Nit: #include <utility> Done. https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... File chrome/browser/media/android/cdm/media_drm_storage_factory.h (right): https://codereview.chromium.org/2765343003/diff/80001/chrome/browser/media/an... chrome/browser/media/android/cdm/media_drm_storage_factory.h:16: void CreateMediaDrmStorage(content::RenderFrameHost* render_frame_host, On 2017/03/28 16:42:44, liberato wrote: > i think that the answer to my question is 'leave it as is'. however, here's the > discussion that went on in my head. :) > > should this be exposed on RFH directly? all of the other interfaces are visible > in content/ already. of course, RFH would need to ask the embedder to provide > the concrete impl. > > do we believe that the interfaces in media/ are applicable to any embedder? > seems like the answer should be 'yes'. We discussed this offline. The idea of this comment is that we always register the service at the content layer, then use ContentClient to delegate the concrete work to the content embedder. It's totally doable. But currently we already have a lot of layers. Adding this indirection doesn't seem to bring us any obvious benefits. So for now I'll leave this as is, but we can always revisit this idea. https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:63: origin_ = origin.Serialize(); On 2017/03/29 01:17:23, dcheng wrote: > Rather than passing the origin over Mojo, can we just get it from the > RenderFrameHost? > > (This also means we need to close the pipe on navigation) Yeah, that's what we chat about before and proposed in the permission bug. But see my question here: https://bugs.chromium.org/p/chromium/issues/detail?id=698985#c11 as it's not super clear to me how to handle navigation etc. This whole CL is behind a flag for prototyping/development. Can we land this first and then iterate on it later? Added a TODO in media_drm_storage.h https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:69: DCHECK(!origin_.empty()); On 2017/03/29 01:17:23, dcheng wrote: > Note that we shouldn't be DCHECKing these sorts of things: it's very possible > that a malicious renderer could simply not call Initialize(), hence my comment > about initializing origin from the RFH. FWIW, the CDM (MediaDrm) is actually running in the GPU process which is trusted. But given the flexibility of Mojo Media, it's totally possible to run the CDM (MediaDrm) in a untrusted process. So your comment is right. Updated the code to explicitly check whether origin has been set. https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.h (right): https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.h:28: ~MediaDrmStorageImpl() final; On 2017/03/29 01:17:23, dcheng wrote: > Alternatively, the entire class can be marked final: > > class MediaDrmStorageImpl final : public ... { Done. But then do I still need to mark each overridden method as final or override? The style guide doesn't say anything about this... https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... File media/mojo/interfaces/media_drm_storage.mojom (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/media_drm_storage.mojom:21: string session_id, array<uint8> key_set_id, string mime_type) On 2017/03/29 01:17:23, dcheng wrote: > One alternative is to package key_set_id and mime_type in a mojo struct. That's what I started with :) But with only two values in the struct, I found it's actually making code more complex so I gave up. Please let me know if feel strong about this. https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/media_drm_storage.mojom:28: => (bool success, array<uint8> key_set_id, string mime_type); On 2017/03/29 01:17:23, dcheng wrote: > Then here we can just return SessionData? here, and avoid the need for bool > success + the two members ditto. I actually like to have |success| here to be consistent with other methods. But I agree if we use the struct then it's not necessary. https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/and... File media/mojo/services/android_mojo_media_client.cc (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/and... media/mojo/services/android_mojo_media_client.cc:37: std::move(media_drm_storage_ptr)); On 2017/03/29 01:17:23, dcheng wrote: > #include <utility> Done. https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/gpu... File media/mojo/services/gpu_mojo_media_client.cc (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/services/gpu... media/mojo/services/gpu_mojo_media_client.cc:44: std::move(media_drm_storage_ptr)); On 2017/03/29 01:17:23, dcheng wrote: > #include <utility> Done.
https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:63: origin_ = origin.Serialize(); On 2017/03/29 20:47:25, xhwang_slow wrote: > On 2017/03/29 01:17:23, dcheng wrote: > > Rather than passing the origin over Mojo, can we just get it from the > > RenderFrameHost? > > > > (This also means we need to close the pipe on navigation) > > Yeah, that's what we chat about before and proposed in the permission bug. But > see my question here: > https://bugs.chromium.org/p/chromium/issues/detail?id=698985#c11 > as it's not super clear to me how to handle navigation etc. > > This whole CL is behind a flag for prototyping/development. Can we land this > first and then iterate on it later? > > Added a TODO in media_drm_storage.h I think I'm missing something: the GPU process is calling this interface? How does it know the origin? https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... File media/mojo/interfaces/media_drm_storage.mojom (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/media_drm_storage.mojom:21: string session_id, array<uint8> key_set_id, string mime_type) On 2017/03/29 20:47:25, xhwang_slow wrote: > On 2017/03/29 01:17:23, dcheng wrote: > > One alternative is to package key_set_id and mime_type in a mojo struct. > > That's what I started with :) > > But with only two values in the struct, I found it's actually making code more > complex so I gave up. Please let me know if feel strong about this. Hm... why did it make it more complex? Once we have the struct here, can't we just pass around session_data.key_id and session_data.mime_type directly? https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/media_drm_storage.mojom:28: => (bool success, array<uint8> key_set_id, string mime_type); On 2017/03/29 20:47:25, xhwang_slow wrote: > On 2017/03/29 01:17:23, dcheng wrote: > > Then here we can just return SessionData? here, and avoid the need for bool > > success + the two members > > ditto. I actually like to have |success| here to be consistent with other > methods. But I agree if we use the struct then it's not necessary. Really, the primary value is here: then we only need a single return value, and we can guarantee that the result is consistent.
comments-only https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:63: origin_ = origin.Serialize(); On 2017/03/29 21:15:12, dcheng wrote: > On 2017/03/29 20:47:25, xhwang_slow wrote: > > On 2017/03/29 01:17:23, dcheng wrote: > > > Rather than passing the origin over Mojo, can we just get it from the > > > RenderFrameHost? > > > > > > (This also means we need to close the pipe on navigation) > > > > Yeah, that's what we chat about before and proposed in the permission bug. But > > see my question here: > > https://bugs.chromium.org/p/chromium/issues/detail?id=698985#c11 > > as it's not super clear to me how to handle navigation etc. > > > > This whole CL is behind a flag for prototyping/development. Can we land this > > first and then iterate on it later? > > > > Added a TODO in media_drm_storage.h > > I think I'm missing something: the GPU process is calling this interface? How > does it know the origin? We have many out-of-process media components. For example, hardware decoders running in the GPU process, or CDM running in the PPAPI process. Even though they run in a separate process, they still belong to the media pipeline running in the render process, which belongs to a frame (with an origin). When we create a CDM, we pass the origin from the render process to the remote process (e.g. GPU). I agree that the |origin| cannot be trusted, which is why I chat with you about browser side origin validation, or getting the origin from the browser directly. https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... File media/mojo/interfaces/media_drm_storage.mojom (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/media_drm_storage.mojom:21: string session_id, array<uint8> key_set_id, string mime_type) On 2017/03/29 21:15:12, dcheng wrote: > On 2017/03/29 20:47:25, xhwang_slow wrote: > > On 2017/03/29 01:17:23, dcheng wrote: > > > One alternative is to package key_set_id and mime_type in a mojo struct. > > > > That's what I started with :) > > > > But with only two values in the struct, I found it's actually making code more > > complex so I gave up. Please let me know if feel strong about this. > > Hm... why did it make it more complex? Once we have the struct here, can't we > just pass around session_data.key_id and session_data.mime_type directly? Sure, let me give it another try. https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/media_drm_storage.mojom:28: => (bool success, array<uint8> key_set_id, string mime_type); On 2017/03/29 21:15:12, dcheng wrote: > On 2017/03/29 20:47:25, xhwang_slow wrote: > > On 2017/03/29 01:17:23, dcheng wrote: > > > Then here we can just return SessionData? here, and avoid the need for bool > > > success + the two members > > > > ditto. I actually like to have |success| here to be consistent with other > > methods. But I agree if we use the struct then it's not necessary. > > Really, the primary value is here: then we only need a single return value, and > we can guarantee that the result is consistent. Acknowledged.
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
dcheng: I updated the CL to use a struct for the session data. PTAnotherL! Thanks! https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... File media/mojo/interfaces/media_drm_storage.mojom (right): https://codereview.chromium.org/2765343003/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/media_drm_storage.mojom:21: string session_id, array<uint8> key_set_id, string mime_type) On 2017/03/29 21:30:44, xhwang_slow wrote: > On 2017/03/29 21:15:12, dcheng wrote: > > On 2017/03/29 20:47:25, xhwang_slow wrote: > > > On 2017/03/29 01:17:23, dcheng wrote: > > > > One alternative is to package key_set_id and mime_type in a mojo struct. > > > > > > That's what I started with :) > > > > > > But with only two values in the struct, I found it's actually making code > more > > > complex so I gave up. Please let me know if feel strong about this. > > > > Hm... why did it make it more complex? Once we have the struct here, can't we > > just pass around session_data.key_id and session_data.mime_type directly? > > Sure, let me give it another try. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... components/cdm/browser/media_drm_storage_impl.cc:63: origin_ = origin.Serialize(); On 2017/03/29 21:30:44, xhwang_slow wrote: > On 2017/03/29 21:15:12, dcheng wrote: > > On 2017/03/29 20:47:25, xhwang_slow wrote: > > > On 2017/03/29 01:17:23, dcheng wrote: > > > > Rather than passing the origin over Mojo, can we just get it from the > > > > RenderFrameHost? > > > > > > > > (This also means we need to close the pipe on navigation) > > > > > > Yeah, that's what we chat about before and proposed in the permission bug. > But > > > see my question here: > > > https://bugs.chromium.org/p/chromium/issues/detail?id=698985#c11 > > > as it's not super clear to me how to handle navigation etc. > > > > > > This whole CL is behind a flag for prototyping/development. Can we land this > > > first and then iterate on it later? > > > > > > Added a TODO in media_drm_storage.h > > > > I think I'm missing something: the GPU process is calling this interface? How > > does it know the origin? > > We have many out-of-process media components. For example, hardware decoders > running in the GPU process, or CDM running in the PPAPI process. Even though > they run in a separate process, they still belong to the media pipeline running > in the render process, which belongs to a frame (with an origin). When we create > a CDM, we pass the origin from the render process to the remote process (e.g. > GPU). > > I agree that the |origin| cannot be trusted, which is why I chat with you about > browser side origin validation, or getting the origin from the browser directly. There are some complexities with plumbing here, but how about: - Have the factory create the impl with an origin from GetLastCommittedOrigin(). - Use WebContentsObserver::DidFinishNavigation(), and delete the impl (and the observer) if it's not a same-page navigation This should address the concerns about the untrusted origin being sent from the renderer, as well as the concerns about the origin becoming stale. (I know this is somewhat experimental, but I'm hoping we can get a good pattern for this set up)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
On 2017/03/30 00:29:15, dcheng wrote: > https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... > File components/cdm/browser/media_drm_storage_impl.cc (right): > > https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... > components/cdm/browser/media_drm_storage_impl.cc:63: origin_ = > origin.Serialize(); > On 2017/03/29 21:30:44, xhwang_slow wrote: > > On 2017/03/29 21:15:12, dcheng wrote: > > > On 2017/03/29 20:47:25, xhwang_slow wrote: > > > > On 2017/03/29 01:17:23, dcheng wrote: > > > > > Rather than passing the origin over Mojo, can we just get it from the > > > > > RenderFrameHost? > > > > > > > > > > (This also means we need to close the pipe on navigation) > > > > > > > > Yeah, that's what we chat about before and proposed in the permission bug. > > But > > > > see my question here: > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=698985#c11 > > > > as it's not super clear to me how to handle navigation etc. > > > > > > > > This whole CL is behind a flag for prototyping/development. Can we land > this > > > > first and then iterate on it later? > > > > > > > > Added a TODO in media_drm_storage.h > > > > > > I think I'm missing something: the GPU process is calling this interface? > How > > > does it know the origin? > > > > We have many out-of-process media components. For example, hardware decoders > > running in the GPU process, or CDM running in the PPAPI process. Even though > > they run in a separate process, they still belong to the media pipeline > running > > in the render process, which belongs to a frame (with an origin). When we > create > > a CDM, we pass the origin from the render process to the remote process (e.g. > > GPU). > > > > I agree that the |origin| cannot be trusted, which is why I chat with you > about > > browser side origin validation, or getting the origin from the browser > directly. > > There are some complexities with plumbing here, but how about: > > - Have the factory create the impl with an origin from GetLastCommittedOrigin(). > - Use WebContentsObserver::DidFinishNavigation(), and delete the impl (and the > observer) if it's not a same-page navigation > > This should address the concerns about the untrusted origin being sent from the > renderer, as well as the concerns about the origin becoming stale. > > (I know this is somewhat experimental, but I'm hoping we can get a good pattern > for this set up) Thanks for the suggestions! Let me try this a bit.
On 2017/03/30 04:41:45, xhwang_slow wrote: > On 2017/03/30 00:29:15, dcheng wrote: > > > https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... > > File components/cdm/browser/media_drm_storage_impl.cc (right): > > > > > https://codereview.chromium.org/2765343003/diff/80001/components/cdm/browser/... > > components/cdm/browser/media_drm_storage_impl.cc:63: origin_ = > > origin.Serialize(); > > On 2017/03/29 21:30:44, xhwang_slow wrote: > > > On 2017/03/29 21:15:12, dcheng wrote: > > > > On 2017/03/29 20:47:25, xhwang_slow wrote: > > > > > On 2017/03/29 01:17:23, dcheng wrote: > > > > > > Rather than passing the origin over Mojo, can we just get it from the > > > > > > RenderFrameHost? > > > > > > > > > > > > (This also means we need to close the pipe on navigation) > > > > > > > > > > Yeah, that's what we chat about before and proposed in the permission > bug. > > > But > > > > > see my question here: > > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=698985#c11 > > > > > as it's not super clear to me how to handle navigation etc. > > > > > > > > > > This whole CL is behind a flag for prototyping/development. Can we land > > this > > > > > first and then iterate on it later? > > > > > > > > > > Added a TODO in media_drm_storage.h > > > > > > > > I think I'm missing something: the GPU process is calling this interface? > > How > > > > does it know the origin? > > > > > > We have many out-of-process media components. For example, hardware decoders > > > running in the GPU process, or CDM running in the PPAPI process. Even though > > > they run in a separate process, they still belong to the media pipeline > > running > > > in the render process, which belongs to a frame (with an origin). When we > > create > > > a CDM, we pass the origin from the render process to the remote process > (e.g. > > > GPU). > > > > > > I agree that the |origin| cannot be trusted, which is why I chat with you > > about > > > browser side origin validation, or getting the origin from the browser > > directly. > > > > There are some complexities with plumbing here, but how about: > > > > - Have the factory create the impl with an origin from > GetLastCommittedOrigin(). > > - Use WebContentsObserver::DidFinishNavigation(), and delete the impl (and the > > observer) if it's not a same-page navigation > > > > This should address the concerns about the untrusted origin being sent from > the > > renderer, as well as the concerns about the origin becoming stale. > > > > (I know this is somewhat experimental, but I'm hoping we can get a good > pattern > > for this set up) > > Thanks for the suggestions! Let me try this a bit. Done. But I have some question about WebContentsObserver and let's chat about that. Otherwise, PTAL again.
dcheng: As discussed, I added RenderFrameDeleted(). PTAL!
rebase only
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2765343003/diff/240001/chrome/browser/media/a... File chrome/browser/media/android/cdm/media_drm_storage_factory.cc (right): https://codereview.chromium.org/2765343003/diff/240001/chrome/browser/media/a... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:43: DVLOG(1) << __func__ << ": Profile not available."; Per chromium-dev, I think we should be DCHECKing the conditions from 30-42. https://codereview.chromium.org/2765343003/diff/240001/chrome/browser/media/a... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:48: if (!pref_service) { I strongly suspect this one should be a DCHECK as well. This code is all //chrome specific, and it seems reasonable that we should be DCHECKing that these are true (it's not commented, but it should be--and we shouldn't be writing code that makes it seem like these invariants don't hold) https://codereview.chromium.org/2765343003/diff/240001/components/cdm/browser... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/240001/components/cdm/browser... components/cdm/browser/media_drm_storage_impl.cc:79: if (!origin_.IsSameOriginWith(origin)) { Nit: as mentioned yesterday, let's remove the origin check here (it doesn't add anything) Where possible, we want to try to rely on the Mojo pipe itself to provide the authentication/identity for the pipe: by virtue of the renderer having this pipe, we /know/ this impl must have been created for |origin_|. https://codereview.chromium.org/2765343003/diff/240001/components/cdm/browser... components/cdm/browser/media_drm_storage_impl.cc:161: if (!origin_.IsSameOriginWith(url::Origin(navigation_handle->GetURL()))) { 1) I think this should be checking that navigation_handle->GetRenderFrameHost() == render_frame_host_ 2) If it is equal, I think we should be unconditionally closing. https://codereview.chromium.org/2765343003/diff/240001/components/cdm/browser... components/cdm/browser/media_drm_storage_impl.cc:171: Observe(nullptr); This is unnecessary: WebContentsObserver unobserves in its destructor.
comments addressed
dcheng: all done with one question. Thanks! https://codereview.chromium.org/2765343003/diff/240001/chrome/browser/media/a... File chrome/browser/media/android/cdm/media_drm_storage_factory.cc (right): https://codereview.chromium.org/2765343003/diff/240001/chrome/browser/media/a... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:43: DVLOG(1) << __func__ << ": Profile not available."; On 2017/03/31 18:01:56, dcheng wrote: > Per chromium-dev, I think we should be DCHECKing the conditions from 30-42. Thanks for starting that discussion. Done! https://codereview.chromium.org/2765343003/diff/240001/chrome/browser/media/a... chrome/browser/media/android/cdm/media_drm_storage_factory.cc:48: if (!pref_service) { On 2017/03/31 18:01:56, dcheng wrote: > I strongly suspect this one should be a DCHECK as well. > > This code is all //chrome specific, and it seems reasonable that we should be > DCHECKing that these are true (it's not commented, but it should be--and we > shouldn't be writing code that makes it seem like these invariants don't hold) Done. https://codereview.chromium.org/2765343003/diff/240001/components/cdm/browser... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/240001/components/cdm/browser... components/cdm/browser/media_drm_storage_impl.cc:79: if (!origin_.IsSameOriginWith(origin)) { On 2017/03/31 18:01:56, dcheng wrote: > Nit: as mentioned yesterday, let's remove the origin check here (it doesn't add > anything) > > Where possible, we want to try to rely on the Mojo pipe itself to provide the > authentication/identity for the pipe: by virtue of the renderer having this > pipe, we /know/ this impl must have been created for |origin_|. Done. https://codereview.chromium.org/2765343003/diff/240001/components/cdm/browser... components/cdm/browser/media_drm_storage_impl.cc:161: if (!origin_.IsSameOriginWith(url::Origin(navigation_handle->GetURL()))) { On 2017/03/31 18:01:56, dcheng wrote: > 1) I think this should be checking that navigation_handle->GetRenderFrameHost() > == render_frame_host_ > 2) If it is equal, I think we should be unconditionally closing. Done. But is it possible that after navigation we reuse the RenderFrameHost, but the origin has been changed? It will not cause crash, but could cause unexpected behavior. https://codereview.chromium.org/2765343003/diff/240001/components/cdm/browser... components/cdm/browser/media_drm_storage_impl.cc:171: Observe(nullptr); On 2017/03/31 18:01:56, dcheng wrote: > This is unnecessary: WebContentsObserver unobserves in its destructor. Done. I actually saw this pattern in a few places. We should all clean them up and maybe DCHECK for nullptr to avoid misuse.
xhwang@chromium.org changed reviewers: + gab@chromium.org, rockot@chromium.org
gab: Please OWNERS review the trivial change in chrome/browser/prefs/browser_prefs.cc and the new deps on 'components/prefs' in components/cdm/browser/DEPS. rockot: Please OWNERS review the new deps on 'mojo/public/cpp/bindings' in components/cdm/browser/DEPS.
https://codereview.chromium.org/2765343003/diff/260001/components/cdm/browser... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/260001/components/cdm/browser... components/cdm/browser/media_drm_storage_impl.cc:154: if (navigation_handle->GetRenderFrameHost() != render_frame_host_) { This should be ==, not != (which I think should answer your question)
dcheng: Thanks! PTAL again https://codereview.chromium.org/2765343003/diff/260001/components/cdm/browser... File components/cdm/browser/media_drm_storage_impl.cc (right): https://codereview.chromium.org/2765343003/diff/260001/components/cdm/browser... components/cdm/browser/media_drm_storage_impl.cc:154: if (navigation_handle->GetRenderFrameHost() != render_frame_host_) { On 2017/04/01 00:32:03, dcheng wrote: > This should be ==, not != (which I think should answer your question) Ah, now I know what you mean! Done.
LGTM
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rebase only
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gab / rockot: kindly ping :) gab: Please OWNERS review the trivial change in chrome/browser/prefs/browser_prefs.cc and the new deps on 'components/prefs' in components/cdm/browser/DEPS. rockot: Please OWNERS review the new deps on 'mojo/public/cpp/bindings' in components/cdm/browser/DEPS.
On 2017/04/03 16:33:13, xhwang_slow wrote: > gab / rockot: kindly ping :) > > gab: Please OWNERS review the trivial change in > chrome/browser/prefs/browser_prefs.cc and the new deps on 'components/prefs' in > components/cdm/browser/DEPS. rubberstamp lgtm > > rockot: Please OWNERS review the new deps on 'mojo/public/cpp/bindings' in > components/cdm/browser/DEPS.
On 2017/04/03 16:44:19, gab wrote: > On 2017/04/03 16:33:13, xhwang_slow wrote: > > gab / rockot: kindly ping :) > > > > gab: Please OWNERS review the trivial change in > > chrome/browser/prefs/browser_prefs.cc and the new deps on 'components/prefs' > in > > components/cdm/browser/DEPS. > > rubberstamp lgtm > > > > > rockot: Please OWNERS review the new deps on 'mojo/public/cpp/bindings' in > > components/cdm/browser/DEPS. components/cdm/browser/DEPS LGTM
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yucliu@chromium.org, liberato@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2765343003/#ps300001 (title: "rebase only")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1491242848259270,
"parent_rev": "ca2623e17b5c45400485a2ceb08545baa9f09c4f", "commit_rev":
"116acb96458241e0db329600538a40ae9c3d1abd"}
Message was sent while issue was closed.
Description was changed from
==========
media: Add MediaDrmStorage
MediaDrmStorage is an interface that provides persistent storage for
MediaDrmBridge. This is needed for per-origin provisioning, and for
persistent license support.
The real storage is implemented using PrefService, and all the read and
write operations happen in the browser process. This makes it easier to
manage concurrent access to the storage by multiple MediaDrmBridge
instances, and by code implementing related features, e.g. clearing
media licenses in "Clear Browsing Data".
A mojom MediaDrmStorage interface is added to connect MediaDrmBridge
(typically running in the GPU process) and the PrefService running in
the browser process. It strictly maps the C++ MediaDrmStorage interface.
Here's a summary of the stack:
MediaDrmBridge (Java) ->
MediaDrmStorageBridge (JNI) ->
MediaDrmStorage (C++ interface) ->
MojoMediaDrmStorage (C++ interface implementation) ->
media::mojom::MojoMediaDrm (mojo interface) ->
------ GPU process ------
mojo IPC
------ Browser process ------
MediaDrmStorageImpl (mojo interface implementation) ->
PrefService
Note that there's a mojo Preferences service (/services/preferences/).
I prototyped it and talked with the owners. It provides some cool
features, but is hard to code against when there are multiple clients
of the preference. In our case, we could have multiple MediaDrmBridge
instances running in the GPU process and UI code clearing media
licenses in the Browser process accessing the preference at the same
time. Plumbing everything to the browser process makes it much easier
to avoid tricky race/synchronization problems, and is recommended by
Preferences owners.
BUG=493521
==========
to
==========
media: Add MediaDrmStorage
MediaDrmStorage is an interface that provides persistent storage for
MediaDrmBridge. This is needed for per-origin provisioning, and for
persistent license support.
The real storage is implemented using PrefService, and all the read and
write operations happen in the browser process. This makes it easier to
manage concurrent access to the storage by multiple MediaDrmBridge
instances, and by code implementing related features, e.g. clearing
media licenses in "Clear Browsing Data".
A mojom MediaDrmStorage interface is added to connect MediaDrmBridge
(typically running in the GPU process) and the PrefService running in
the browser process. It strictly maps the C++ MediaDrmStorage interface.
Here's a summary of the stack:
MediaDrmBridge (Java) ->
MediaDrmStorageBridge (JNI) ->
MediaDrmStorage (C++ interface) ->
MojoMediaDrmStorage (C++ interface implementation) ->
media::mojom::MojoMediaDrm (mojo interface) ->
------ GPU process ------
mojo IPC
------ Browser process ------
MediaDrmStorageImpl (mojo interface implementation) ->
PrefService
Note that there's a mojo Preferences service (/services/preferences/).
I prototyped it and talked with the owners. It provides some cool
features, but is hard to code against when there are multiple clients
of the preference. In our case, we could have multiple MediaDrmBridge
instances running in the GPU process and UI code clearing media
licenses in the Browser process accessing the preference at the same
time. Plumbing everything to the browser process makes it much easier
to avoid tricky race/synchronization problems, and is recommended by
Preferences owners.
BUG=493521
Review-Url: https://codereview.chromium.org/2765343003
Cr-Commit-Position: refs/heads/master@{#461479}
Committed:
https://chromium.googlesource.com/chromium/src/+/116acb96458241e0db329600538a...
==========
Message was sent while issue was closed.
Committed patchset #15 (id:300001) as https://chromium.googlesource.com/chromium/src/+/116acb96458241e0db329600538a... |
