Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1124863005:
Restrict use of hardware-secure codecs based on the RendererPreference. (Closed)
|
Created:
5 years, 7 months ago by sandersd (OOO until July 31) Modified:
5 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@setsecurity Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestrict use of hardware-secure codecs based on the RendererPreference.
This passes the value of |use_video_overlay_for_embedded_encrypted_video| from RendererPreferences to requestMediaKeySystemAccess() so that it can correctly block either non-hardware-secure codecs or hardware-secure codecs.
BUG=467779
Committed: https://crrev.com/02d9aea8003449c921c5c4b41ab67b0f554b1a72
Cr-Commit-Position: refs/heads/master@{#329329}
Patch Set 1 #Patch Set 2 : Pass result of CB into selector. #Patch Set 3 : Rename secure_codec -> secure_surface in some cases. #
Total comments: 18
Patch Set 4 : Rebase. #Patch Set 5 : Remove ifdefs. #
Total comments: 6
Patch Set 6 : Switch back to SECURE_CODECS. #
Total comments: 2
Patch Set 7 : Comment for base::Unretained(). #
Total comments: 2
Patch Set 8 : Move into anonymous namespace. #Patch Set 9 : Constructor initialization ordering. #Patch Set 10 : Enum name typo. #Patch Set 11 : Read renderer preference correctly. #Patch Set 12 : Correct unittests. #
Messages
Total messages: 52 (18 generated)
sandersd@chromium.org changed reviewers: + ddorwin@chromium.org, xhwang@chromium.org
Patchset #3 (id:40001) has been deleted
Thanks. https://codereview.chromium.org/1124863005/diff/40002/components/html_viewer/... File components/html_viewer/html_document.cc (right): https://codereview.chromium.org/1124863005/diff/40002/components/html_viewer/... components/html_viewer/html_document.cc:390: bool AllowSecureSurfaces() { This could be interrupted as doing something. Use Is... IsSecureSurfaceAllowed? https://codereview.chromium.org/1124863005/diff/40002/components/html_viewer/... components/html_viewer/html_document.cc:391: return false; Add some sort of comment. i.e. not currently supported by HTML Viewer. https://codereview.chromium.org/1124863005/diff/40002/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1124863005/diff/40002/content/renderer/render... content/renderer/render_frame_impl.h:783: bool AllowSecureSurfaces(); ditto https://codereview.chromium.org/1124863005/diff/40002/media/blink/key_system_... File media/blink/key_system_config_selector.cc (right): https://codereview.chromium.org/1124863005/diff/40002/media/blink/key_system_... media/blink/key_system_config_selector.cc:736: // preference is not enabled. This comment is specific to code outside the function. https://codereview.chromium.org/1124863005/diff/40002/media/blink/key_system_... File media/blink/key_system_config_selector.h (right): https://codereview.chromium.org/1124863005/diff/40002/media/blink/key_system_... media/blink/key_system_config_selector.h:53: #if defined(OS_ANDROID) So may ifdefs. Perhaps we should just note that this only applies to platforms that support it and should be false otherwise. We can then DCHECK(!allow) in the implementation. https://codereview.chromium.org/1124863005/diff/40002/media/blink/key_system_... media/blink/key_system_config_selector.h:54: const bool allow_secure_surfaces, is_... https://codereview.chromium.org/1124863005/diff/40002/media/blink/webencrypte... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1124863005/diff/40002/media/blink/webencrypte... media/blink/webencryptedmediaclient_impl.cc:128: const bool require_secure_surfaces, no const https://codereview.chromium.org/1124863005/diff/40002/media/blink/webencrypte... media/blink/webencryptedmediaclient_impl.cc:134: // TODO(sandersd): Pass |require_secure_surfaces| along and use it to Ideally, we would pass along something generic, such as the robustness level. Or perhaps the max_robustness_level requested. That would help us avoid all these ifdefs and maybe even remove some in this CL. WDYT?
https://codereview.chromium.org/1124863005/diff/40002/components/html_viewer/... File components/html_viewer/html_document.cc (right): https://codereview.chromium.org/1124863005/diff/40002/components/html_viewer/... components/html_viewer/html_document.cc:390: bool AllowSecureSurfaces() { On 2015/05/06 02:17:11, ddorwin wrote: > This could be interrupted as doing something. Use Is... > IsSecureSurfaceAllowed? Done. https://codereview.chromium.org/1124863005/diff/40002/components/html_viewer/... components/html_viewer/html_document.cc:391: return false; On 2015/05/06 02:17:11, ddorwin wrote: > Add some sort of comment. i.e. not currently supported by HTML Viewer. Done. https://codereview.chromium.org/1124863005/diff/40002/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1124863005/diff/40002/content/renderer/render... content/renderer/render_frame_impl.h:783: bool AllowSecureSurfaces(); On 2015/05/06 02:17:11, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1124863005/diff/40002/media/blink/key_system_... File media/blink/key_system_config_selector.cc (right): https://codereview.chromium.org/1124863005/diff/40002/media/blink/key_system_... media/blink/key_system_config_selector.cc:736: // preference is not enabled. On 2015/05/06 02:17:11, ddorwin wrote: > This comment is specific to code outside the function. Done. https://codereview.chromium.org/1124863005/diff/40002/media/blink/key_system_... File media/blink/key_system_config_selector.h (right): https://codereview.chromium.org/1124863005/diff/40002/media/blink/key_system_... media/blink/key_system_config_selector.h:53: #if defined(OS_ANDROID) On 2015/05/06 02:17:11, ddorwin wrote: > So may ifdefs. Perhaps we should just note that this only applies to platforms > that support it and should be false otherwise. We can then DCHECK(!allow) in the > implementation. Done. https://codereview.chromium.org/1124863005/diff/40002/media/blink/key_system_... media/blink/key_system_config_selector.h:54: const bool allow_secure_surfaces, On 2015/05/06 02:17:12, ddorwin wrote: > is_... Done. https://codereview.chromium.org/1124863005/diff/40002/media/blink/webencrypte... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1124863005/diff/40002/media/blink/webencrypte... media/blink/webencryptedmediaclient_impl.cc:128: const bool require_secure_surfaces, On 2015/05/06 02:17:12, ddorwin wrote: > no const Done. https://codereview.chromium.org/1124863005/diff/40002/media/blink/webencrypte... media/blink/webencryptedmediaclient_impl.cc:134: // TODO(sandersd): Pass |require_secure_surfaces| along and use it to On 2015/05/06 02:17:12, ddorwin wrote: > Ideally, we would pass along something generic, such as the robustness level. Or > perhaps the max_robustness_level requested. That would help us avoid all these > ifdefs and maybe even remove some in this CL. WDYT? This is in the same category as |allow_persistent_state| and |allow_distinctive_identifier|, except that we can't infer it from the accumulated configuration. I'm thinking we should pull all those out into a KeySystemConfiguration struct (as a Chromium counterpart to WebMediaKeySystemConfiguration). Does that seem reasonable?
LG https://codereview.chromium.org/1124863005/diff/40002/media/blink/webencrypte... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1124863005/diff/40002/media/blink/webencrypte... media/blink/webencryptedmediaclient_impl.cc:134: // TODO(sandersd): Pass |require_secure_surfaces| along and use it to On 2015/05/08 00:37:42, sandersd wrote: > On 2015/05/06 02:17:12, ddorwin wrote: > > Ideally, we would pass along something generic, such as the robustness level. > Or > > perhaps the max_robustness_level requested. That would help us avoid all these > > ifdefs and maybe even remove some in this CL. WDYT? > > This is in the same category as |allow_persistent_state| and > |allow_distinctive_identifier|, except that we can't infer it from the > accumulated configuration. I'm thinking we should pull all those out into a > KeySystemConfiguration struct (as a Chromium counterpart to > WebMediaKeySystemConfiguration). Does that seem reasonable? We can infer those from the accumulated_configuration, though, right? My point was the type/meaning of the new passed value. I don't think it matters whether it's in a struct or not. Or were you considering passing multiple items? https://codereview.chromium.org/1124863005/diff/90001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1124863005/diff/90001/content/renderer/render... content/renderer/render_frame_impl.cc:4930: return false; Must be in #else or you'll have unreachable code. https://codereview.chromium.org/1124863005/diff/90001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1124863005/diff/90001/media/base/eme_constant... media/base/eme_constants.h:132: // The configuration option prevents requiring the use of secure surfaces. This really (will in the next CL) affects the security level. Surfaces are the current implementation, but that may change in the future. If you think this is clearer for now, we can leave this, but we'll probably have to change it later. https://codereview.chromium.org/1124863005/diff/90001/media/base/eme_constant... media/base/eme_constants.h:136: SECURE_SURFACES_NOT_REQUIRABLE, Why "REQUIRABLE" instead of "ALLOWED"?
https://codereview.chromium.org/1124863005/diff/40002/media/blink/webencrypte... File media/blink/webencryptedmediaclient_impl.cc (right): https://codereview.chromium.org/1124863005/diff/40002/media/blink/webencrypte... media/blink/webencryptedmediaclient_impl.cc:134: // TODO(sandersd): Pass |require_secure_surfaces| along and use it to On 2015/05/08 03:18:31, ddorwin wrote: > On 2015/05/08 00:37:42, sandersd wrote: > > On 2015/05/06 02:17:12, ddorwin wrote: > > > Ideally, we would pass along something generic, such as the robustness > level. > > Or > > > perhaps the max_robustness_level requested. That would help us avoid all > these > > > ifdefs and maybe even remove some in this CL. WDYT? > > > > This is in the same category as |allow_persistent_state| and > > |allow_distinctive_identifier|, except that we can't infer it from the > > accumulated configuration. I'm thinking we should pull all those out into a > > KeySystemConfiguration struct (as a Chromium counterpart to > > WebMediaKeySystemConfiguration). Does that seem reasonable? > > We can infer those from the accumulated_configuration, though, right? > > My point was the type/meaning of the new passed value. I don't think it matters > whether it's in a struct or not. Or were you considering passing multiple items? Yes, I am considering passing all three of those bools in a struct, and getting rid of the inference (the plan would be to do that in the next CL, as I would be plumbing this bool through in that CL anyway). This value isn't directly convertible to a robustness value, since it is a property of the codecs used, not the configuration. It is true that you can only currently get it by requesting robustness--do we want to turn the current state into a guarantee? (The implication is that we could never support codecs that do not have non-hardware-secure modes.) https://codereview.chromium.org/1124863005/diff/90001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1124863005/diff/90001/content/renderer/render... content/renderer/render_frame_impl.cc:4930: return false; On 2015/05/08 03:18:31, ddorwin wrote: > Must be in #else or you'll have unreachable code. Done. https://codereview.chromium.org/1124863005/diff/90001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1124863005/diff/90001/media/base/eme_constant... media/base/eme_constants.h:132: // The configuration option prevents requiring the use of secure surfaces. On 2015/05/08 03:18:31, ddorwin wrote: > This really (will in the next CL) affects the security level. Surfaces are the > current implementation, but that may change in the future. > > If you think this is clearer for now, we can leave this, but we'll probably have > to change it later. Things get murky here, because there is an implicit relation between secure codecs/secure surfaces/security level. I changed to using surfaces in this CL because that seems to be the key issue to solve. In the future (especially one with secure textures) we may want to take a different approach, but I don't see this changing in the next CL. https://codereview.chromium.org/1124863005/diff/90001/media/base/eme_constant... media/base/eme_constants.h:136: SECURE_SURFACES_NOT_REQUIRABLE, On 2015/05/08 03:18:31, ddorwin wrote: > Why "REQUIRABLE" instead of "ALLOWED"? The actual use of secure surfaces is independent of this rule; secure codecs require them, but regular codecs don't block them. The key is that some codecs may not have a secure variant, and if we add them to the config, then we know we can't request L1. Without L1 we can't guarantee that secure surfaces are used; therefore the rule must be incompatible with requiring secure surfaces. The opposite value, NOT_ALLOWED, isn't accurate because secure surfaces may actually still be used.
LGTM with a new PS containing the #else fix.
On 2015/05/08 18:14:00, ddorwin wrote: > LGTM with a new PS containing the #else fix. I switched back to SECURE_CODECS on the following logic: - Currently, the use of secure codecs is synonymous with the use of SetSecurityLevel(1), so it doesn't matter if we use SECURE_CODECS or SECURITY_LEVEL_L1, but avoiding L1 in the code is a goal. - We can always infer the use of secure surfaces from the use of secure codecs. This remains true with the addition of secure textures (under reasonable guesses about how they would work). - We can't deduce the security level from the use of secure surfaces if hardware-secure-only codecs are supported or if secure textures are added. We may still need a better way to surface the resulting bool, but we can leave that for a future CL.
sandersd@chromium.org changed reviewers: + erikwright@chromium.org, nasko@chromium.org
erikwright@chromium.org: Please review changes in components/html_viewer. nasko@chromium.org: Please review changes in content/renderer.
https://codereview.chromium.org/1124863005/diff/110001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1124863005/diff/110001/content/renderer/rende... content/renderer/render_frame_impl.cc:3590: base::Unretained(this)), nit: Put a comment explaining why using base::Unretained is ok here. It isn't immediately obvious.
https://codereview.chromium.org/1124863005/diff/110001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1124863005/diff/110001/content/renderer/rende... content/renderer/render_frame_impl.cc:3590: base::Unretained(this)), On 2015/05/08 22:17:23, nasko wrote: > nit: Put a comment explaining why using base::Unretained is ok here. It isn't > immediately obvious. Done.
Thanks! LGTM
lgtm
sandersd@chromium.org changed reviewers: + sky@chromium.org - erikwright@chromium.org
sky@: Please review changes in components/html_viewer.
LGTM https://codereview.chromium.org/1124863005/diff/130001/components/html_viewer... File components/html_viewer/html_document.cc (right): https://codereview.chromium.org/1124863005/diff/130001/components/html_viewer... components/html_viewer/html_document.cc:386: bool AreSecureCodecsSupported() { nit: move this into anonymous namespace at the top of the file.
https://codereview.chromium.org/1124863005/diff/130001/components/html_viewer... File components/html_viewer/html_document.cc (right): https://codereview.chromium.org/1124863005/diff/130001/components/html_viewer... components/html_viewer/html_document.cc:386: bool AreSecureCodecsSupported() { On 2015/05/11 19:13:05, sky wrote: > nit: move this into anonymous namespace at the top of the file. Done.
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, nasko@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1124863005/#ps150001 (title: "Move into anonymous namespace.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124863005/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, nasko@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1124863005/#ps170001 (title: "Constructor initialization ordering.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124863005/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, nasko@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1124863005/#ps190001 (title: "Enum name typo.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124863005/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, nasko@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1124863005/#ps210001 (title: "Read renderer preference correctly.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124863005/210001
Message was sent while issue was closed.
Committed patchset #11 (id:210001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/491fea8c41977b1557a79cf2f53d4b60ecd159d9 Cr-Commit-Position: refs/heads/master@{#329296}
Message was sent while issue was closed.
On 2015/05/12 00:31:46, I haz the power (commit-bot) wrote: > Patchset 11 (id:??) landed as > https://crrev.com/491fea8c41977b1557a79cf2f53d4b60ecd159d9 > Cr-Commit-Position: refs/heads/master@{#329296} Cause of http://build.chromium.org/p/chromium.linux/builders/Linux%20GN%20Clobber/buil... ?
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:210001) has been created in https://codereview.chromium.org/1137993004/ by mdempsky@chromium.org. The reason for reverting is: Compile failure in KeySystemConfigSelectorTest::SelectConfig(). ../../media/blink/key_system_config_selector_unittest.cc:177:57: error: too few arguments to function call, expected 6, have 5 http://build.chromium.org/p/chromium.linux/builders/Linux%20GN%20Clobber/buil....
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, nasko@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1124863005/#ps230001 (title: "Correct unittests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124863005/230001
Message was sent while issue was closed.
Committed patchset #12 (id:230001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/02d9aea8003449c921c5c4b41ab67b0f554b1a72 Cr-Commit-Position: refs/heads/master@{#329329}
Message was sent while issue was closed.
On 2015/05/12 02:30:28, I haz the power (commit-bot) wrote: > Patchset 12 (id:??) landed as > https://crrev.com/02d9aea8003449c921c5c4b41ab67b0f554b1a72 > Cr-Commit-Position: refs/heads/master@{#329329} Patchset 12 looks good to me. No complaints about that in particular. I'm just curious if this was the proper code review procedure here? I had no idea it was possible to reuse old CL#'s to resubmit a reverted CL, so it surprises me that it was possible to do so without requiring additional LGTMs. On the other hand, had the CQ warned you that patch set 11 was broken (like it should have done), you would have been able to upload patch set 12 without additional LGTMs anyway... so maybe it shouldn't be that surprising.
Message was sent while issue was closed.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Message was sent while issue was closed.
This comes up on chromium-dev every once in and while and I've never seen consensus; some prefer new patch sets w/ PS#1 being the old change, others prefer the existing patch set if it's simply minor fixes (as in this case, few lines of unittest patch). My preference is to reuse the existing patch set if there are only _minor_ changes; so I recommended Dan just reupload to this existing patch set to avoid churn and noise.
Message was sent while issue was closed.
Anyone know why PS11 passed all bots and was allowed to commit? Are media_blink_unittests not being built on any of the trybots?
Message was sent while issue was closed.
I believe they aren't, I ran into this issue the other day but haven't had time to track it down.
Message was sent while issue was closed.
On 2015/05/12 03:27:42, DaleCurtis wrote: > I believe they aren't, I ran into this issue the other day but haven't had time > to track it down. Filed https://code.google.com/p/chromium/issues/detail?id=487300 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
