Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2582463003:
media: Verify CDM Host files (Closed)
|
Created:
4 years ago by xhwang Modified:
3 years, 11 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, eme-reviews_chromium.org, ddorwin, Haoming Chen, Rintaro Kuroiwa Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Verify CDM host files
This change provides the Content Decryption Module (CDM) a way to verify
the host files, the CDM adapter and the CDM itself.
GetCdmHostFilePath() provides a list of host files.
CdmHostFile opens the pair of a file and its corresponding signature
file. CdmHostFiles manages all files (and signature files) opened for
verification.
On platforms that use the Zygote, files are opened in the Zygote process.
After a child process is forked, if it's not a ppapi process, all files
will be immediately closed.
On other platforms, files are opened after the ppapi process is
launched, but before the sandbox is sealed.
On all platforms, if the ppapi process hosts a CDM, VerifyHostFiles()
will be called with the files managed in CdmHostFiles. Then all opened
files will be closed.
A browser test is added to make sure files are opened correctly and are
passed in VerifyHostFiles() as expected.
BUG=658036
TEST=New browser_tests added.
Review-Url: https://codereview.chromium.org/2582463003
Cr-Commit-Position: refs/heads/master@{#446260}
Committed: https://chromium.googlesource.com/chromium/src/+/785a834775112c711cda038e3ae51a01c3a029dc
Patch Set 1 #
Total comments: 55
Patch Set 2 : rebase only #Patch Set 3 : Polished! #
Total comments: 21
Patch Set 4 : comments addressed #
Total comments: 15
Patch Set 5 : kerrnel/tinskip's comments addressed/replied #Patch Set 6 : rebase only #Patch Set 7 : renames based on new API #Patch Set 8 : VerifyFiles() now returns a boolean. #
Total comments: 8
Patch Set 9 : comments addressed #
Total comments: 4
Patch Set 10 : comments addressed #Messages
Total messages: 87 (38 generated)
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...
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org, hmchen@chromium.org, jrummell@chromium.org, kerrnel@chromium.org, tinskip@chromium.org
This is a prototype CL, so it definitely needs more polish (and tests). But, before I go further, please help check that I am on the right track. With that, please do a high level review on the design and structure of code. kerrnel: Please review from Chrome security's perspective, especially the changes in content/app/content_main_runner.cc content/ppapi_plugin/ppapi_thread.cc content/zygote/zygote_main_linux.cc tinskip / hmchen: Please review from the CDM's perspective ddorwin / jrummell: Please review from EME's perspective https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... File media/cdm/content_decryption_module_export.h (right): https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... media/cdm/content_decryption_module_export.h:6: #define CDM_CONTENT_DECRYPTION_MODULE_EXPORT_H_ This will be shared by cdm.h and cdm_ext.h. https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... File media/cdm/content_decryption_module_ext.h (right): https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... media/cdm/content_decryption_module_ext.h:6: #define CDM_CONTENT_DECRYPTION_MODULE_EXT_H_ Adding this new API in a different file since this API will be dynamically found (using GetFunctionPointerFromNativeLibrary()). Other existing APIs in media/cdm/api/content_decryption_module.h are statically called by the adapter. In the future, when we switch to use mojo, all APIs will be dynamically found.
https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... content/ppapi_plugin/ppapi_thread.cc:458: cdm_host_files->VerifyFiles(library.get(), path); tinskip / hmchen: This will be called right after INITIALIZE_CDM_MODULE() and before CreateCdmInstance().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... content/ppapi_plugin/ppapi_thread.cc:358: std::unique_ptr<CdmHostFiles> cdm_host_files; Use a local instance such that if we return early for any error, all files will be closed automatically.
Initial comments after a quick review. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_file.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_file.cc:40: new CdmHostFile(file_path, std::move(file), std::move(sig_file))); I think Chromium is moving to base::MakeUnique<CdmHostFile>(file_path, std::move(file), std::move(sig_file)); https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_file.h (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_file.h:28: cdm::CdmHostFile TakePlatformFile(); Does this take ownership of the File objects? or a copy? Should it be a unique_ptr<> too? https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.cc:59: base::GetNativeLibraryName(cdm_file_name)); Is this needed? kClearKeyCdmAdapterFileName returns "clearkeycdmadapter.dll", so I don't think another .dll is needed. https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... File media/cdm/content_decryption_module_ext.h (right): https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... media/cdm/content_decryption_module_ext.h:24: #elif defined(OS_POSIX) Why not just #else? That way it should compile everywhere. https://codereview.chromium.org/2582463003/diff/1/media/cdm/ppapi/external_cl... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/2582463003/diff/1/media/cdm/ppapi/external_cl... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:282: } How about trying write() and checking that it fails?
This seems to be on the right track, but I asked a few questions so I can dig deeper into the implementation from the Chrome side. Thanks. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.h (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:28: #if defined(OS_POSIX) && !defined(OS_NACL) && !defined(OS_MACOSX) && \ Is it simpler to just say defined(OS_LINUX)? Do another other systems use a zygote? Note that OS_LINUX is true for OS_CHROMEOS. https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... content/ppapi_plugin/ppapi_thread.cc:362: cdm_host_files = OpenCdmHostFiles(path); Am I missing something? Where is this defined?
Answered Greg's questions https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.h (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:28: #if defined(OS_POSIX) && !defined(OS_NACL) && !defined(OS_MACOSX) && \ On 2016/12/16 22:10:41, Greg K wrote: > Is it simpler to just say defined(OS_LINUX)? Do another other systems use a > zygote? Note that OS_LINUX is true for OS_CHROMEOS. Good question. This was what I originally did. But I wasn't sure due to this code: https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?rcl=0... I was about to ask you about this but then I "felt" I found the solution, which is where this code is coming/copied from: https://cs.chromium.org/chromium/src/base/metrics/field_trial.cc?rcl=0&l=26 https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... content/ppapi_plugin/ppapi_thread.cc:362: cdm_host_files = OpenCdmHostFiles(path); On 2016/12/16 22:10:41, Greg K wrote: > Am I missing something? Where is this defined? oops, this is some old code that I didn't update. This should be cdm_host_files = CdmHostFiles::Create(path);
https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.h (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:28: #if defined(OS_POSIX) && !defined(OS_NACL) && !defined(OS_MACOSX) && \ On 2016/12/16 22:30:15, xhwang wrote: > On 2016/12/16 22:10:41, Greg K wrote: > > Is it simpler to just say defined(OS_LINUX)? Do another other systems use a > > zygote? Note that OS_LINUX is true for OS_CHROMEOS. > > Good question. This was what I originally did. But I wasn't sure due to this > code: > > https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?rcl=0... > > I was about to ask you about this but then I "felt" I found the solution, which > is where this code is coming/copied from: > > https://cs.chromium.org/chromium/src/base/metrics/field_trial.cc?rcl=0&l=26 You're right. I looked at the definition of OS_POSIX and it's a lot more complicated than just Linux, and Mac: // For access to standard POSIXish features, use OS_POSIX instead of a // more specific macro. #if defined(OS_MACOSX) || defined(OS_LINUX) || defined(OS_FREEBSD) || \ defined(OS_NETBSD) || defined(OS_OPENBSD) || defined(OS_SOLARIS) || \ defined(OS_ANDROID) || defined(OS_OPENBSD) || defined(OS_SOLARIS) || \ defined(OS_ANDROID) || defined(OS_NACL) || defined(OS_QNX) #define OS_POSIX 1 #endif https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... content/ppapi_plugin/ppapi_thread.cc:362: cdm_host_files = OpenCdmHostFiles(path); On 2016/12/16 22:30:15, xhwang wrote: > On 2016/12/16 22:10:41, Greg K wrote: > > Am I missing something? Where is this defined? > > oops, this is some old code that I didn't update. This should be > > cdm_host_files = CdmHostFiles::Create(path); Thanks, I'll take a look now.
https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.h (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:28: #if defined(OS_POSIX) && !defined(OS_NACL) && !defined(OS_MACOSX) && \ On 2016/12/16 22:34:41, Greg K wrote: > On 2016/12/16 22:30:15, xhwang wrote: > > On 2016/12/16 22:10:41, Greg K wrote: > > > Is it simpler to just say defined(OS_LINUX)? Do another other systems use a > > > zygote? Note that OS_LINUX is true for OS_CHROMEOS. > > > > Good question. This was what I originally did. But I wasn't sure due to this > > code: > > > > > https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?rcl=0... > > > > I was about to ask you about this but then I "felt" I found the solution, > which > > is where this code is coming/copied from: > > > > https://cs.chromium.org/chromium/src/base/metrics/field_trial.cc?rcl=0&l=26 > > You're right. I looked at the definition of OS_POSIX and it's a lot more > complicated than just Linux, and Mac: > > // For access to standard POSIXish features, use OS_POSIX instead of a > // more specific macro. > #if defined(OS_MACOSX) || defined(OS_LINUX) || defined(OS_FREEBSD) || \ > defined(OS_NETBSD) || defined(OS_OPENBSD) || defined(OS_SOLARIS) || \ > defined(OS_ANDROID) || defined(OS_OPENBSD) || defined(OS_SOLARIS) || \ > defined(OS_ANDROID) || defined(OS_NACL) || defined(OS_QNX) > #define OS_POSIX 1 > #endif Does it make sense to have an official USE_ZYGOTE macro/buildflag somewhere?
kindly post-holiday ping!
https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.h (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:73: // Opens CDM specific files for the CDM adapter registered at newline https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:92: bool IsCdm(const base::FilePath& cdm_adapter_path); Add some comments on this function. https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... content/ppapi_plugin/ppapi_thread.cc:458: cdm_host_files->VerifyFiles(library.get(), path); On 2016/12/16 01:08:27, xhwang wrote: > tinskip / hmchen: This will be called right after INITIALIZE_CDM_MODULE() and > before CreateCdmInstance(). Acknowledged. https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... content/ppapi_plugin/ppapi_thread.cc:458: cdm_host_files->VerifyFiles(library.get(), path); The interface name on the design doc (https://docs.google.com/document/d/1XXZzRqudB0fQC9q-ZKLUW7NksMMFV-CVzuwXjMhQb...) is "InitVmp", not "VerifyFiles". Which name should we use? https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... File media/cdm/content_decryption_module_ext.h (right): https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... media/cdm/content_decryption_module_ext.h:8: #if defined(WIN32) s/WIN32/OS_WIN? https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... media/cdm/content_decryption_module_ext.h:20: #if defined(WIN32) ditto
On a high level, this looks like a reasonable approach. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_file.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_file.cc:32: base::File sig_file(sig_file_path, On OS X we'll need to locate the signature file in the Resources folder. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.cc:36: const char* GetCdmFileName(const base::FilePath& cdm_adapter_file_name) { How is the CDM library file name different from the adapter file name? https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.cc:159: if (TakePlatformFiles(cdm_adapter_path, &cdm_host_files)) I'm not sure what's going on here, so a comment might be better. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.h (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:28: #if defined(OS_POSIX) && !defined(OS_NACL) && !defined(OS_MACOSX) && \ On 2016/12/16 22:37:06, xhwang wrote: > On 2016/12/16 22:34:41, Greg K wrote: > > On 2016/12/16 22:30:15, xhwang wrote: > > > On 2016/12/16 22:10:41, Greg K wrote: > > > > Is it simpler to just say defined(OS_LINUX)? Do another other systems use > a > > > > zygote? Note that OS_LINUX is true for OS_CHROMEOS. > > > > > > Good question. This was what I originally did. But I wasn't sure due to this > > > code: > > > > > > > > > https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?rcl=0... > > > > > > I was about to ask you about this but then I "felt" I found the solution, > > which > > > is where this code is coming/copied from: > > > > > > https://cs.chromium.org/chromium/src/base/metrics/field_trial.cc?rcl=0&l=26 > > > > You're right. I looked at the definition of OS_POSIX and it's a lot more > > complicated than just Linux, and Mac: > > > > // For access to standard POSIXish features, use OS_POSIX instead of a > > // more specific macro. > > #if defined(OS_MACOSX) || defined(OS_LINUX) || defined(OS_FREEBSD) || \ > > defined(OS_NETBSD) || defined(OS_OPENBSD) || defined(OS_SOLARIS) || \ > > defined(OS_ANDROID) || defined(OS_OPENBSD) || defined(OS_SOLARIS) || \ > > defined(OS_ANDROID) || defined(OS_NACL) || defined(OS_QNX) > > #define OS_POSIX 1 > > #endif > > Does it make sense to have an official USE_ZYGOTE macro/buildflag somewhere? We may want to define that flag but you probably don't want to do that as part of this CL. If it breaks anything it'll double the troubleshooting. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:54: // CDM |library|. If unexpected error happens, all files will be closed. What do you mean by "calling VerifyCdmFiles() on the CDM |library|"?
https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_file.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_file.cc:32: base::File sig_file(sig_file_path, On 2017/01/09 21:45:14, Greg K wrote: > On OS X we'll need to locate the signature file in the Resources folder. Specifically in the Chrome Framework. https://codereview.chromium.org/2582463003/diff/1/media/cdm/ppapi/external_cl... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/2582463003/diff/1/media/cdm/ppapi/external_cl... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:269: LOG(ERROR) << "Path " << i << ": " << cdm_host_files[i].file_path; Why is this necessary? Let the CDM figure out if there are any issues with the files?
rebase only
Thanks for the feedback! I replied to the comments (with a lot of will-do). Next step I'll split the CL, remove hacks polish them and send out individual CLs for detailed review. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_file.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_file.cc:32: base::File sig_file(sig_file_path, On 2017/01/09 23:49:30, tinskip1 wrote: > On 2017/01/09 21:45:14, Greg K wrote: > > On OS X we'll need to locate the signature file in the Resources folder. > > Specifically in the Chrome Framework. Good point. Will do. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_file.cc:40: new CdmHostFile(file_path, std::move(file), std::move(sig_file))); On 2016/12/16 20:21:30, jrummell wrote: > I think Chromium is moving to base::MakeUnique<CdmHostFile>(file_path, > std::move(file), std::move(sig_file)); Good point. Will do. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_file.h (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_file.h:28: cdm::CdmHostFile TakePlatformFile(); On 2016/12/16 20:21:30, jrummell wrote: > Does this take ownership of the File objects? or a copy? > > Should it be a unique_ptr<> too? I can't use a unique_ptr since I need to push the cdm::CdmHostFile into a vector. That being said I found this confusing as well. I'll see how I can improve it. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.cc:36: const char* GetCdmFileName(const base::FilePath& cdm_adapter_file_name) { On 2017/01/09 21:45:14, Greg K wrote: > How is the CDM library file name different from the adapter file name? For example: Widevine CDM adapter file name: libwidevinecdmadapter.so Widevine CDM file name: libwidevinecdm.so That being said, the point of this function is that we don't care about how the files are named. We could strip the "adapter" part to get the CDM file name but I felt that's more hacky. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.cc:59: base::GetNativeLibraryName(cdm_file_name)); On 2016/12/16 20:21:30, jrummell wrote: > Is this needed? kClearKeyCdmAdapterFileName returns "clearkeycdmadapter.dll", so > I don't think another .dll is needed. GetCdmFileName() returns kClearKeyCdmLibraryName which would be "clearkeycdm" for Clear Key CDM. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.cc:159: if (TakePlatformFiles(cdm_adapter_path, &cdm_host_files)) On 2017/01/09 21:45:14, Greg K wrote: > I'm not sure what's going on here, so a comment might be better. Will do. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.h (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:28: #if defined(OS_POSIX) && !defined(OS_NACL) && !defined(OS_MACOSX) && \ On 2017/01/09 21:45:15, Greg K wrote: > On 2016/12/16 22:37:06, xhwang wrote: > > On 2016/12/16 22:34:41, Greg K wrote: > > > On 2016/12/16 22:30:15, xhwang wrote: > > > > On 2016/12/16 22:10:41, Greg K wrote: > > > > > Is it simpler to just say defined(OS_LINUX)? Do another other systems > use > > a > > > > > zygote? Note that OS_LINUX is true for OS_CHROMEOS. > > > > > > > > Good question. This was what I originally did. But I wasn't sure due to > this > > > > code: > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?rcl=0... > > > > > > > > I was about to ask you about this but then I "felt" I found the solution, > > > which > > > > is where this code is coming/copied from: > > > > > > > > > https://cs.chromium.org/chromium/src/base/metrics/field_trial.cc?rcl=0&l=26 > > > > > > You're right. I looked at the definition of OS_POSIX and it's a lot more > > > complicated than just Linux, and Mac: > > > > > > // For access to standard POSIXish features, use OS_POSIX instead of a > > > // more specific macro. > > > #if defined(OS_MACOSX) || defined(OS_LINUX) || defined(OS_FREEBSD) || \ > > > defined(OS_NETBSD) || defined(OS_OPENBSD) || defined(OS_SOLARIS) || \ > > > defined(OS_ANDROID) || defined(OS_OPENBSD) || defined(OS_SOLARIS) || \ > > > defined(OS_ANDROID) || defined(OS_NACL) || defined(OS_QNX) > > > #define OS_POSIX 1 > > > #endif > > > > Does it make sense to have an official USE_ZYGOTE macro/buildflag somewhere? > > We may want to define that flag but you probably don't want to do that as part > of this CL. If it breaks anything it'll double the troubleshooting. okay, then I'll keep this as is. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:54: // CDM |library|. If unexpected error happens, all files will be closed. On 2017/01/09 21:45:15, Greg K wrote: > What do you mean by "calling VerifyCdmFiles() on the CDM |library|"? oops, it should be "calling VerifyHostFiles() exported by the CDM |library|". Does this answer your question? https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:73: // Opens CDM specific files for the CDM adapter registered at On 2017/01/04 22:23:39, hmchen1 wrote: > newline Will do. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:92: bool IsCdm(const base::FilePath& cdm_adapter_path); On 2017/01/04 22:23:38, hmchen1 wrote: > Add some comments on this function. Will do. https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... content/ppapi_plugin/ppapi_thread.cc:458: cdm_host_files->VerifyFiles(library.get(), path); On 2017/01/04 22:23:39, hmchen1 wrote: > The interface name on the design doc > (https://docs.google.com/document/d/1XXZzRqudB0fQC9q-ZKLUW7NksMMFV-CVzuwXjMhQb...) > is "InitVmp", not "VerifyFiles". > > Which name should we use? Nobody would be able to understand what VMP is. So I'd like a simpler name to describe what we do here. https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... File media/cdm/content_decryption_module_ext.h (right): https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... media/cdm/content_decryption_module_ext.h:8: #if defined(WIN32) On 2017/01/04 22:23:39, hmchen1 wrote: > s/WIN32/OS_WIN? This interface is intended to be used independent of Chromium, e.g. by other non-Chromium based browsers. So we should not depend on Chromium defines like OS_WIN. https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... media/cdm/content_decryption_module_ext.h:20: #if defined(WIN32) On 2017/01/04 22:23:39, hmchen1 wrote: > ditto Acknowledged. https://codereview.chromium.org/2582463003/diff/1/media/cdm/content_decryptio... media/cdm/content_decryption_module_ext.h:24: #elif defined(OS_POSIX) On 2016/12/16 20:21:30, jrummell wrote: > Why not just #else? That way it should compile everywhere. I am not sure whether the file descriptor is an integer on all other platforms. So even though it'll "compile", it might not work. I'd rather it to fail early (at compile time). Also, this is consistent with base::PlatformFile https://cs.chromium.org/chromium/src/base/files/file.h?rcl=1483993878&l=35 https://codereview.chromium.org/2582463003/diff/1/media/cdm/ppapi/external_cl... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/2582463003/diff/1/media/cdm/ppapi/external_cl... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:269: LOG(ERROR) << "Path " << i << ": " << cdm_host_files[i].file_path; On 2017/01/09 23:49:30, tinskip1 wrote: > Why is this necessary? Let the CDM figure out if there are any issues with the > files? This is only for my own logging. Will remove. https://codereview.chromium.org/2582463003/diff/1/media/cdm/ppapi/external_cl... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:282: } On 2016/12/16 20:21:30, jrummell wrote: > How about trying write() and checking that it fails? Good point. Will do.
Description was changed from ========== media: Verify CDM Host NOT FOR REVIEW. PROTOTYPE ONLY. BUG=658036 TEST=Manually tested. Will enable auto test later. ========== to ========== media: Verify CDM host files This change provides the Content Decryption Module (CDM) a way to verify the host files, the CDM adapter and the CDM itself. GetCdmHostFilePath() provides a list of host files. CdmHostFile opens the pair of a file and its corresponding signature file. CdmHostFiles manages all files (and signature files) opened for verification. On platforms that use the Zygote, files are opened in the Zygote process. After a child process is forked, if it's not a ppapi process, all files will be immediately closed. On other platforms, files are opened after the ppapi process is launched, but before the sandbox is sealed. On all platforms, if the ppapi process hosts a CDM, VerifyHostFiles() will be called with the files managed in CdmHostFiles. A browser test is added to make sure files are opened correctly and are passed in VerifyHostFiles() as expected. BUG=658036 TEST=New browser_tests added. ==========
xhwang@chromium.org changed reviewers: - ddorwin@chromium.org, hmchen@chromium.org
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_file.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_file.cc:32: base::File sig_file(sig_file_path, On 2017/01/12 20:15:02, xhwang wrote: > On 2017/01/09 23:49:30, tinskip1 wrote: > > On 2017/01/09 21:45:14, Greg K wrote: > > > On OS X we'll need to locate the signature file in the Resources folder. > > > > Specifically in the Chrome Framework. > > Good point. Will do. On Mac we are more interested in "Google Chrome Framework" under "/Applications/Google Chrome.app/Contents/Versions/55.0.2883.95/Google Chrome Framework.framework". Do I also need to put the sig file under Google Chrome Framework.framework/Resources? https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_file.cc:40: new CdmHostFile(file_path, std::move(file), std::move(sig_file))); On 2017/01/12 20:15:02, xhwang wrote: > On 2016/12/16 20:21:30, jrummell wrote: > > I think Chromium is moving to base::MakeUnique<CdmHostFile>(file_path, > > std::move(file), std::move(sig_file)); > > Good point. Will do. Actually, now I remember why I didn't use MakeUnique: the constructor is private so MakeUnique can't access it. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_file.h (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_file.h:28: cdm::CdmHostFile TakePlatformFile(); On 2017/01/12 20:15:02, xhwang wrote: > On 2016/12/16 20:21:30, jrummell wrote: > > Does this take ownership of the File objects? or a copy? > > > > Should it be a unique_ptr<> too? > > I can't use a unique_ptr since I need to push the cdm::CdmHostFile into a > vector. That being said I found this confusing as well. I'll see how I can > improve it. Added comments. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.cc:159: if (TakePlatformFiles(cdm_adapter_path, &cdm_host_files)) On 2017/01/12 20:15:02, xhwang wrote: > On 2017/01/09 21:45:14, Greg K wrote: > > I'm not sure what's going on here, so a comment might be better. > > Will do. Done. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... File content/common/media/cdm_host_files.h (right): https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:54: // CDM |library|. If unexpected error happens, all files will be closed. On 2017/01/12 20:15:02, xhwang wrote: > On 2017/01/09 21:45:15, Greg K wrote: > > What do you mean by "calling VerifyCdmFiles() on the CDM |library|"? > > oops, it should be "calling VerifyHostFiles() exported by the CDM |library|". > Does this answer your question? Done. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:73: // Opens CDM specific files for the CDM adapter registered at On 2017/01/12 20:15:02, xhwang wrote: > On 2017/01/04 22:23:39, hmchen1 wrote: > > newline > > Will do. Done. https://codereview.chromium.org/2582463003/diff/1/content/common/media/cdm_ho... content/common/media/cdm_host_files.h:92: bool IsCdm(const base::FilePath& cdm_adapter_path); On 2017/01/12 20:15:02, xhwang wrote: > On 2017/01/04 22:23:38, hmchen1 wrote: > > Add some comments on this function. > > Will do. Done. https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2582463003/diff/1/content/ppapi_plugin/ppapi_... content/ppapi_plugin/ppapi_thread.cc:362: cdm_host_files = OpenCdmHostFiles(path); On 2016/12/16 22:34:41, Greg K wrote: > On 2016/12/16 22:30:15, xhwang wrote: > > On 2016/12/16 22:10:41, Greg K wrote: > > > Am I missing something? Where is this defined? > > > > oops, this is some old code that I didn't update. This should be > > > > cdm_host_files = CdmHostFiles::Create(path); > > Thanks, I'll take a look now. Done. https://codereview.chromium.org/2582463003/diff/1/media/cdm/ppapi/external_cl... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/2582463003/diff/1/media/cdm/ppapi/external_cl... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:269: LOG(ERROR) << "Path " << i << ": " << cdm_host_files[i].file_path; On 2017/01/12 20:15:02, xhwang wrote: > On 2017/01/09 23:49:30, tinskip1 wrote: > > Why is this necessary? Let the CDM figure out if there are any issues with the > > files? > > This is only for my own logging. Will remove. Done. https://codereview.chromium.org/2582463003/diff/1/media/cdm/ppapi/external_cl... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:282: } On 2017/01/12 20:15:02, xhwang wrote: > On 2016/12/16 20:21:30, jrummell wrote: > > How about trying write() and checking that it fails? > > Good point. Will do. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tinskip1 / jrummell /Greg K: The CL is now updated and bots are all green. PTAL! ddorwin / hmchen1 / rkuroiwa: FYI Changes since last review: - Addressed all comments. - Fixed various deps/include/ifdef issues. - Added content_client.h and chrome/ changes to populate the list of common host files. - Added a new media switch kIgnoreMissingCdmHostFileForTesting so that we can test most of the functionality in browser_tests (where some host/sig files do not exist). - Added a browser_tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== media: Verify CDM host files This change provides the Content Decryption Module (CDM) a way to verify the host files, the CDM adapter and the CDM itself. GetCdmHostFilePath() provides a list of host files. CdmHostFile opens the pair of a file and its corresponding signature file. CdmHostFiles manages all files (and signature files) opened for verification. On platforms that use the Zygote, files are opened in the Zygote process. After a child process is forked, if it's not a ppapi process, all files will be immediately closed. On other platforms, files are opened after the ppapi process is launched, but before the sandbox is sealed. On all platforms, if the ppapi process hosts a CDM, VerifyHostFiles() will be called with the files managed in CdmHostFiles. A browser test is added to make sure files are opened correctly and are passed in VerifyHostFiles() as expected. BUG=658036 TEST=New browser_tests added. ========== to ========== media: Verify CDM host files This change provides the Content Decryption Module (CDM) a way to verify the host files, the CDM adapter and the CDM itself. GetCdmHostFilePath() provides a list of host files. CdmHostFile opens the pair of a file and its corresponding signature file. CdmHostFiles manages all files (and signature files) opened for verification. On platforms that use the Zygote, files are opened in the Zygote process. After a child process is forked, if it's not a ppapi process, all files will be immediately closed. On other platforms, files are opened after the ppapi process is launched, but before the sandbox is sealed. On all platforms, if the ppapi process hosts a CDM, VerifyHostFiles() will be called with the files managed in CdmHostFiles. Then all opened files will be closed. A browser test is added to make sure files are opened correctly and are passed in VerifyHostFiles() as expected. BUG=658036 TEST=New browser_tests added. ==========
xhwang@chromium.org changed reviewers: + alexmos@chromium.org, sky@chromium.org
alexmos: Please OWNERS review content/* changes except for content/common/media/* sky: Please OWNERS review chrome/* changes. Despite the size of the CL, the content/ and chrome/ changes for review should be pretty straightforward. I didn't split this into smaller CLs because other reviewers have already reviewed the smaller prototype CL in December (see PS1). Let me know if you feel strongly about this. For more background, see BUG for a link to the design doc. If it looks good I'd like to land this asap. Your prompt response would be highly appreciated!
lgtm w/nits https://codereview.chromium.org/2582463003/diff/40001/chrome/common/BUILD.gn File chrome/common/BUILD.gn (right): https://codereview.chromium.org/2582463003/diff/40001/chrome/common/BUILD.gn#... chrome/common/BUILD.gn:176: "//chrome/common:version_header", Is this needed? It appears you add it on line 540, so I don't think it's needed here. However, if changes to other files require this, then I would think that 540 is redundant. https://codereview.chromium.org/2582463003/diff/40001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/2582463003/diff/40001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:32: #include "chrome/common/media/cdm_host_file_path.h" This is only needed if BUILDFLAG(ENABLE_PEPPER_CDMS). Since there is already a bunch of includes conditionally added below, this should be part of it. https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... File content/common/media/cdm_host_files.cc (right): https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... content/common/media/cdm_host_files.cc:185: // the CDM explicitly and pass the CDM |library| in this call. Does this comment still apply? https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... content/common/media/cdm_host_files.cc:314: cdm_host_files->push_back(file->TakePlatformFile()); Should reserve() be called on |cdm_host_files| first, since you know the sizes of common_files_ and cdm_specific_files? It was done in cdm_host_file_path.cc. I know that it's a small number of files, so it may not make any noticeable difference, but I wonder if it's a good idea or not. (Similarly on lines 286-287, you know the vector needs space for 2 elements.)
comments addressed
jrummell's comments addressed https://codereview.chromium.org/2582463003/diff/40001/chrome/common/BUILD.gn File chrome/common/BUILD.gn (right): https://codereview.chromium.org/2582463003/diff/40001/chrome/common/BUILD.gn#... chrome/common/BUILD.gn:176: "//chrome/common:version_header", On 2017/01/18 21:46:45, jrummell wrote: > Is this needed? It appears you add it on line 540, so I don't think it's needed > here. However, if changes to other files require this, then I would think that > 540 is redundant. Good catch. I forgot to remove this line. Done. https://codereview.chromium.org/2582463003/diff/40001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/2582463003/diff/40001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:32: #include "chrome/common/media/cdm_host_file_path.h" On 2017/01/18 21:46:45, jrummell wrote: > This is only needed if BUILDFLAG(ENABLE_PEPPER_CDMS). Since there is already a > bunch of includes conditionally added below, this should be part of it. Done. https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... File content/common/media/cdm_host_files.cc (right): https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... content/common/media/cdm_host_files.cc:185: // the CDM explicitly and pass the CDM |library| in this call. On 2017/01/18 21:46:45, jrummell wrote: > Does this comment still apply? Good catch. Removed. https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... content/common/media/cdm_host_files.cc:314: cdm_host_files->push_back(file->TakePlatformFile()); On 2017/01/18 21:46:45, jrummell wrote: > Should reserve() be called on |cdm_host_files| first, since you know the sizes > of common_files_ and cdm_specific_files? It was done in cdm_host_file_path.cc. I > know that it's a small number of files, so it may not make any noticeable > difference, but I wonder if it's a good idea or not. (Similarly on lines > 286-287, you know the vector needs space for 2 elements.) Done.
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/2582463003/diff/60001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/2582463003/diff/60001/chrome/browser/media/en... chrome/browser/media/encrypted_media_browsertest.cc:62: const char kExternalClearKeyVerifyHostFilesTestKeySystem[] = Suggest: Shorten from VerifyHostFiles to VerifyHost all around. https://codereview.chromium.org/2582463003/diff/60001/chrome/common/media/cdm... File chrome/common/media/cdm_host_file_path.cc (right): https://codereview.chromium.org/2582463003/diff/60001/chrome/common/media/cdm... chrome/common/media/cdm_host_file_path.cc:40: FILE_PATH_LITERAL("chrome.exe")}; Can we get the actual name of the executable to deal with renaming? https://codereview.chromium.org/2582463003/diff/60001/content/common/media/cd... File content/common/media/cdm_host_files.cc (right): https://codereview.chromium.org/2582463003/diff/60001/content/common/media/cd... content/common/media/cdm_host_files.cc:161: static const char kVerifyHostFilesFuncName[] = "VerifyHostFiles"; This function name should probably be versioned just like the other entry points. https://codereview.chromium.org/2582463003/diff/60001/content/common/media/cd... content/common/media/cdm_host_files.cc:207: verify_host_files_func(cdm_host_files.data(), cdm_host_files.size()); Check return value. Fail if false. https://codereview.chromium.org/2582463003/diff/60001/content/common/media/cd... File content/common/media/cdm_host_files.h (right): https://codereview.chromium.org/2582463003/diff/60001/content/common/media/cd... content/common/media/cdm_host_files.h:62: // Verifies |cdm_adapter_path| CDM files by calling VerifyHostFiles() exported Again, perhaps VerifyHost?
Some comments here. I don't see any scripts to generate the .sig files during the build? https://codereview.chromium.org/2582463003/diff/40001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/2582463003/diff/40001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:562: AddCdmHostFilePaths(cdm_host_file_paths); Not sure, but should this be namespaced? Otherwise I expect it to be defined in this file. https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... File content/common/media/cdm_host_file.h (right): https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... content/common/media/cdm_host_file.h:35: cdm::HostFile TakePlatformFile(); cdm::HostFile and CdmHostFile seems confusing to read, but maybe the other reviewers are OK with the naming. https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... File content/common/media/cdm_host_files.cc (right): https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... content/common/media/cdm_host_files.cc:51: base::FilePath GetSigFilePath(const base::FilePath& file_path) { Isn't this already defined elsewhere? https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... content/common/media/cdm_host_files.cc:209: verify_host_files_func(cdm_host_files.data(), cdm_host_files.size()); The service is sandboxed when this is called, right? https://codereview.chromium.org/2582463003/diff/40001/media/cdm/ppapi/externa... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/2582463003/diff/40001/media/cdm/ppapi/externa... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:293: int bytes_written = file.Write(0, kDataToWrite, 1); Isn't it extremely dangerous to check that a file isn't writable by actually writing to it?
kerrnel: Chrome sig files should be available on Windows now. We are still working on generating/bundling sig files on other platforms. I addressed/replied kerrnel and tinskip's comments. PTAL again! https://codereview.chromium.org/2582463003/diff/40001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/2582463003/diff/40001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:562: AddCdmHostFilePaths(cdm_host_file_paths); On 2017/01/19 04:13:07, Greg K wrote: > Not sure, but should this be namespaced? Otherwise I expect it to be defined in > this file. Well, a lot of chrome/ classes (e.g. ChromeContentClient in this file) are not namespaced. So typically I don't use "chrome" namespace even in chrome/. That being said, code search tells me that "chrome" namespace is actually used in a lot of files. So I'll use it :) Done. https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... File content/common/media/cdm_host_file.h (right): https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... content/common/media/cdm_host_file.h:35: cdm::HostFile TakePlatformFile(); On 2017/01/19 04:13:07, Greg K wrote: > cdm::HostFile and CdmHostFile seems confusing to read, but maybe the other > reviewers are OK with the naming. Acknowledged. https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... File content/common/media/cdm_host_files.cc (right): https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... content/common/media/cdm_host_files.cc:51: base::FilePath GetSigFilePath(const base::FilePath& file_path) { On 2017/01/19 04:13:07, Greg K wrote: > Isn't this already defined elsewhere? Right, it's also defined in chrome/common/media/cdm_host_file_path.cc. However, content/ cannot depend on chrome/, and chrome/ can only depend on content/public/. I felt it would be silly to make this a content public API. I could move this to media/, but then it'll seem odd this is not directly used in media/. Since this helper is so tiny, I felt this duplication is not too bad. That's why I left a TODO and plan to deal with it later. WDYT? https://codereview.chromium.org/2582463003/diff/40001/content/common/media/cd... content/common/media/cdm_host_files.cc:209: verify_host_files_func(cdm_host_files.data(), cdm_host_files.size()); On 2017/01/19 04:13:07, Greg K wrote: > The service is sandboxed when this is called, right? Yes. See the call site in ppapi_thread.cc. https://codereview.chromium.org/2582463003/diff/40001/media/cdm/ppapi/externa... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/2582463003/diff/40001/media/cdm/ppapi/externa... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:293: int bytes_written = file.Write(0, kDataToWrite, 1); On 2017/01/19 04:13:07, Greg K wrote: > Isn't it extremely dangerous to check that a file isn't writable by actually > writing to it? Good point. At least on Linux, we should be able to use fcntl (fd, F_GETFL) to get the file access mode. That'll involve some base/file/ change so I'll leave it to another CL. For now, I replace this check with a TODO. https://codereview.chromium.org/2582463003/diff/60001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/2582463003/diff/60001/chrome/browser/media/en... chrome/browser/media/encrypted_media_browsertest.cc:62: const char kExternalClearKeyVerifyHostFilesTestKeySystem[] = On 2017/01/19 03:42:46, tinskip1 wrote: > Suggest: Shorten from VerifyHostFiles to VerifyHost all around. The API change has been landed in https://chromiumcodereview.appspot.com/2631463003/. I wish I got this suggestion earlier :) I'll send another CL (with a DEPS roll) to change the API if you feel strongly about this. https://codereview.chromium.org/2582463003/diff/60001/chrome/common/media/cdm... File chrome/common/media/cdm_host_file_path.cc (right): https://codereview.chromium.org/2582463003/diff/60001/chrome/common/media/cdm... chrome/common/media/cdm_host_file_path.cc:40: FILE_PATH_LITERAL("chrome.exe")}; On 2017/01/19 03:42:47, tinskip1 wrote: > Can we get the actual name of the executable to deal with renaming? It's possible. I thought about using PathService::Get(FILE_EXE, exe_path) to get the executable path but didn't do it for the following reasons: - The current code is consistent with existing binary integrity analyzer code at https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/incident_re... - I still need to hard code chrome.dll and chrome_child.dll. - PathService::Get(FILE_EXE, exe_path) could return "<path>/browser_tests" when running browser tests, which gets a bit confusing. https://codereview.chromium.org/2582463003/diff/60001/content/common/media/cd... File content/common/media/cdm_host_files.cc (right): https://codereview.chromium.org/2582463003/diff/60001/content/common/media/cd... content/common/media/cdm_host_files.cc:161: static const char kVerifyHostFilesFuncName[] = "VerifyHostFiles"; On 2017/01/19 03:42:47, tinskip1 wrote: > This function name should probably be versioned just like the other entry > points. Agreed that if we change the API we need a different (possibly versioned) name. But do we want to add version for the initial function name? In other words, we can always add versions later, or use something like VerifyHostFilesEx() later. https://codereview.chromium.org/2582463003/diff/60001/content/common/media/cd... content/common/media/cdm_host_files.cc:207: verify_host_files_func(cdm_host_files.data(), cdm_host_files.size()); On 2017/01/19 03:42:47, tinskip1 wrote: > Check return value. Fail if false. The current API doesn't return a boolean: https://cs.chromium.org/chromium/src/media/cdm/api/content_decryption_module_... There's really nothing Chromium can do when it fails... We'll still load the CDM and use it even if VerifyHostFiles() fails (either synchronously or asynchronously), right? https://codereview.chromium.org/2582463003/diff/60001/content/common/media/cd... File content/common/media/cdm_host_files.h (right): https://codereview.chromium.org/2582463003/diff/60001/content/common/media/cd... content/common/media/cdm_host_files.h:62: // Verifies |cdm_adapter_path| CDM files by calling VerifyHostFiles() exported On 2017/01/19 03:42:47, tinskip1 wrote: > Again, perhaps VerifyHost? ditto
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.
Best time for these changes is now, before initial release. Thanks. https://codereview.chromium.org/2582463003/diff/60001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/2582463003/diff/60001/chrome/browser/media/en... chrome/browser/media/encrypted_media_browsertest.cc:62: const char kExternalClearKeyVerifyHostFilesTestKeySystem[] = On 2017/01/19 08:30:51, xhwang_slow wrote: > On 2017/01/19 03:42:46, tinskip1 wrote: > > Suggest: Shorten from VerifyHostFiles to VerifyHost all around. > > The API change has been landed in > https://chromiumcodereview.appspot.com/2631463003/. I wish I got this suggestion > earlier :) I'll send another CL (with a DEPS roll) to change the API if you feel > strongly about this. Well, "verifyhostfiles" IMO gives away what we're doing. We'd rather not do that. Also, we ought to version. Returning at least a bool is a good idea. Please let's make the changes; they are rather minimal IMO. https://codereview.chromium.org/2582463003/diff/60001/chrome/common/media/cdm... File chrome/common/media/cdm_host_file_path.cc (right): https://codereview.chromium.org/2582463003/diff/60001/chrome/common/media/cdm... chrome/common/media/cdm_host_file_path.cc:40: FILE_PATH_LITERAL("chrome.exe")}; On 2017/01/19 08:30:51, xhwang_slow wrote: > On 2017/01/19 03:42:47, tinskip1 wrote: > > Can we get the actual name of the executable to deal with renaming? > > It's possible. I thought about using PathService::Get(FILE_EXE, exe_path) to get > the executable path but didn't do it for the following reasons: > > - The current code is consistent with existing binary integrity analyzer code at > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/incident_re... > > - I still need to hard code chrome.dll and chrome_child.dll. > > - PathService::Get(FILE_EXE, exe_path) could return "<path>/browser_tests" when > running browser tests, which gets a bit confusing. I'm just concerned about getting a bunch of VMP validation failures because users might rename chrome.exe. We can leave for now and see how things go.
https://codereview.chromium.org/2582463003/diff/60001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/2582463003/diff/60001/chrome/browser/media/en... chrome/browser/media/encrypted_media_browsertest.cc:62: const char kExternalClearKeyVerifyHostFilesTestKeySystem[] = On 2017/01/19 17:36:38, tinskip1 wrote: > On 2017/01/19 08:30:51, xhwang_slow wrote: > > On 2017/01/19 03:42:46, tinskip1 wrote: > > > Suggest: Shorten from VerifyHostFiles to VerifyHost all around. > > > > The API change has been landed in > > https://chromiumcodereview.appspot.com/2631463003/. I wish I got this > suggestion > > earlier :) I'll send another CL (with a DEPS roll) to change the API if you > feel > > strongly about this. > > Well, "verifyhostfiles" IMO gives away what we're doing. We'd rather not do > that. > > Also, we ought to version. I can do these two. > Returning at least a bool is a good idea. What do you expect Chromium do when the return value is false? > Please let's make the changes; they are rather minimal IMO.
https://codereview.chromium.org/2582463003/diff/60001/chrome/common/media/cdm... File chrome/common/media/cdm_host_file_path.cc (right): https://codereview.chromium.org/2582463003/diff/60001/chrome/common/media/cdm... chrome/common/media/cdm_host_file_path.cc:40: FILE_PATH_LITERAL("chrome.exe")}; On 2017/01/19 17:36:38, tinskip1 wrote: > On 2017/01/19 08:30:51, xhwang_slow wrote: > > On 2017/01/19 03:42:47, tinskip1 wrote: > > > Can we get the actual name of the executable to deal with renaming? > > > > It's possible. I thought about using PathService::Get(FILE_EXE, exe_path) to > get > > the executable path but didn't do it for the following reasons: > > > > - The current code is consistent with existing binary integrity analyzer code > at > > > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/incident_re... > > > > - I still need to hard code chrome.dll and chrome_child.dll. > > > > - PathService::Get(FILE_EXE, exe_path) could return "<path>/browser_tests" > when > > running browser tests, which gets a bit confusing. > > I'm just concerned about getting a bunch of VMP validation failures because > users might rename chrome.exe. We can leave for now and see how things go. In that case do you expect the user also rename the sig file? I don't feel this is the use case we want to support.
https://codereview.chromium.org/2582463003/diff/60001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/2582463003/diff/60001/chrome/browser/media/en... chrome/browser/media/encrypted_media_browsertest.cc:62: const char kExternalClearKeyVerifyHostFilesTestKeySystem[] = On 2017/01/19 17:46:06, xhwang_slow wrote: > On 2017/01/19 17:36:38, tinskip1 wrote: > > On 2017/01/19 08:30:51, xhwang_slow wrote: > > > On 2017/01/19 03:42:46, tinskip1 wrote: > > > > Suggest: Shorten from VerifyHostFiles to VerifyHost all around. > > > > > > The API change has been landed in > > > https://chromiumcodereview.appspot.com/2631463003/. I wish I got this > > suggestion > > > earlier :) I'll send another CL (with a DEPS roll) to change the API if you > > feel > > > strongly about this. > > > > Well, "verifyhostfiles" IMO gives away what we're doing. We'd rather not do > > that. > > > > Also, we ought to version. > > I can do these two. > > > Returning at least a bool is a good idea. > > What do you expect Chromium do when the return value is false? > > > Please let's make the changes; they are rather minimal IMO. > Fail as if it could not load the CDM.
On 2017/01/19 18:02:17, tinskip1 wrote: > https://codereview.chromium.org/2582463003/diff/60001/chrome/browser/media/en... > File chrome/browser/media/encrypted_media_browsertest.cc (right): > > https://codereview.chromium.org/2582463003/diff/60001/chrome/browser/media/en... > chrome/browser/media/encrypted_media_browsertest.cc:62: const char > kExternalClearKeyVerifyHostFilesTestKeySystem[] = > On 2017/01/19 17:46:06, xhwang_slow wrote: > > On 2017/01/19 17:36:38, tinskip1 wrote: > > > On 2017/01/19 08:30:51, xhwang_slow wrote: > > > > On 2017/01/19 03:42:46, tinskip1 wrote: > > > > > Suggest: Shorten from VerifyHostFiles to VerifyHost all around. > > > > > > > > The API change has been landed in > > > > https://chromiumcodereview.appspot.com/2631463003/. I wish I got this > > > suggestion > > > > earlier :) I'll send another CL (with a DEPS roll) to change the API if > you > > > feel > > > > strongly about this. > > > > > > Well, "verifyhostfiles" IMO gives away what we're doing. We'd rather not do > > > that. > > > > > > Also, we ought to version. > > > > I can do these two. > > > > > Returning at least a bool is a good idea. > > > > What do you expect Chromium do when the return value is false? > > > > > Please let's make the changes; they are rather minimal IMO. > > > > Fail as if it could not load the CDM. Let's talk about this further offline.
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...
tinskip's comments addressed. Sorry I have to rebase to get the new API. PTAL again!
lgtm
Now I get approval from media/CDM owners. Greg: Please review from security's perspective. alexmos: Please OWNERS review content/* changes except for content/common/media/* sky: Please OWNERS review chrome/* changes. Despite the size of the CL, the content/ and chrome/ changes for review should be pretty straightforward. I didn't split this into smaller CLs because other reviewers have already reviewed the smaller prototype CL in December (see PS1). Let me know if you feel strongly about this. For more background, see BUG for a link to the design doc. Thank you all!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sky@chromium.org changed reviewers: + jam@chromium.org - sky@chromium.org
sky->jam
https://codereview.chromium.org/2582463003/diff/140001/content/browser/ppapi_... File content/browser/ppapi_plugin_process_host.cc (right): https://codereview.chromium.org/2582463003/diff/140001/content/browser/ppapi_... content/browser/ppapi_plugin_process_host.cc:404: switches::kIgnoreMissingCdmHostFileForTesting, this isn't a testing only flag anymore then? https://codereview.chromium.org/2582463003/diff/140001/content/public/common/... File content/public/common/content_client.h (right): https://codereview.chromium.org/2582463003/diff/140001/content/public/common/... content/public/common/content_client.h:98: virtual void AddContentDecryptionModuleHostFilePaths( why isn't this just combined with the above method?
comments addressed
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...
jam: Thanks for the comments. PTAL again! https://codereview.chromium.org/2582463003/diff/140001/content/browser/ppapi_... File content/browser/ppapi_plugin_process_host.cc (right): https://codereview.chromium.org/2582463003/diff/140001/content/browser/ppapi_... content/browser/ppapi_plugin_process_host.cc:404: switches::kIgnoreMissingCdmHostFileForTesting, On 2017/01/23 17:45:44, jam wrote: > this isn't a testing only flag anymore then? Agreed. Though the purpose of adding such a flag is for testing, it can be enabled by non-testing code. I updated the switch name by dropping the "ForTesting" part. https://codereview.chromium.org/2582463003/diff/140001/content/public/common/... File content/public/common/content_client.h (right): https://codereview.chromium.org/2582463003/diff/140001/content/public/common/... content/public/common/content_client.h:98: virtual void AddContentDecryptionModuleHostFilePaths( On 2017/01/23 17:45:45, jam wrote: > why isn't this just combined with the above method? They serve different purposes. But I guess for the sake of this file, simpler API is preferred. Done.
https://codereview.chromium.org/2582463003/diff/140001/content/browser/ppapi_... File content/browser/ppapi_plugin_process_host.cc (right): https://codereview.chromium.org/2582463003/diff/140001/content/browser/ppapi_... content/browser/ppapi_plugin_process_host.cc:404: switches::kIgnoreMissingCdmHostFileForTesting, On 2017/01/23 23:16:09, xhwang_slow wrote: > On 2017/01/23 17:45:44, jam wrote: > > this isn't a testing only flag anymore then? > > Agreed. Though the purpose of adding such a flag is for testing, it can be > enabled by non-testing code. > > I updated the switch name by dropping the "ForTesting" part. Can you help me understand this more? I see you removed the ForTesting in the name, but the comment is there. If this is only for tests, why is this file passing this flag instead of only the test harness (content_browsertests?). Do we really want production code to ignore this in pepper processes? https://codereview.chromium.org/2582463003/diff/160001/chrome/common/media/cd... File chrome/common/media/cdm_host_file_path.cc (right): https://codereview.chromium.org/2582463003/diff/160001/chrome/common/media/cd... chrome/common/media/cdm_host_file_path.cc:110: AddCdmHostFilePathsWin(cdm_host_file_paths); nit: name the method the same on all platforms and then you can just have one call here. you can even just have one method definition and move the ifdef inside of it. https://codereview.chromium.org/2582463003/diff/160001/content/browser/zygote... File content/browser/zygote_host/zygote_communication_linux.cc (right): https://codereview.chromium.org/2582463003/diff/160001/content/browser/zygote... content/browser/zygote_host/zygote_communication_linux.cc:264: // Need to tell CdmHostFile(s) to ignore missing CDM host files in tests. ditto
LGTM https://codereview.chromium.org/2582463003/diff/140001/content/common/media/c... File content/common/media/cdm_host_file.cc (right): https://codereview.chromium.org/2582463003/diff/140001/content/common/media/c... content/common/media/cdm_host_file.cc:27: std::unique_ptr<CdmHostFile> CdmHostFile::Create( nit: // static
comments addressed
jam: PTAL again! https://codereview.chromium.org/2582463003/diff/140001/content/browser/ppapi_... File content/browser/ppapi_plugin_process_host.cc (right): https://codereview.chromium.org/2582463003/diff/140001/content/browser/ppapi_... content/browser/ppapi_plugin_process_host.cc:404: switches::kIgnoreMissingCdmHostFileForTesting, On 2017/01/24 00:39:24, jam wrote: > On 2017/01/23 23:16:09, xhwang_slow wrote: > > On 2017/01/23 17:45:44, jam wrote: > > > this isn't a testing only flag anymore then? > > > > Agreed. Though the purpose of adding such a flag is for testing, it can be > > enabled by non-testing code. > > > > I updated the switch name by dropping the "ForTesting" part. > > Can you help me understand this more? I see you removed the ForTesting in the > name, but the comment is there. If this is only for tests, why is this file > passing this flag instead of only the test harness (content_browsertests?). Do > we really want production code to ignore this in pepper processes? err, my bad. I should drop the "in tests" part from this comment. The flag will also work in production. This flag will be used in two cases: - In the browser_tests added as part of this CL (not in content_browsertests). - By developers to test the functionality even when some host files are missing (e.g. when running a Chromium build). https://codereview.chromium.org/2582463003/diff/140001/content/common/media/c... File content/common/media/cdm_host_file.cc (right): https://codereview.chromium.org/2582463003/diff/140001/content/common/media/c... content/common/media/cdm_host_file.cc:27: std::unique_ptr<CdmHostFile> CdmHostFile::Create( On 2017/01/24 00:40:14, alexmos wrote: > nit: // static Done. https://codereview.chromium.org/2582463003/diff/160001/chrome/common/media/cd... File chrome/common/media/cdm_host_file_path.cc (right): https://codereview.chromium.org/2582463003/diff/160001/chrome/common/media/cd... chrome/common/media/cdm_host_file_path.cc:110: AddCdmHostFilePathsWin(cdm_host_file_paths); On 2017/01/24 00:39:24, jam wrote: > nit: name the method the same on all platforms and then you can just have one > call here. you can even just have one method definition and move the ifdef > inside of it. Done. https://codereview.chromium.org/2582463003/diff/160001/content/browser/zygote... File content/browser/zygote_host/zygote_communication_linux.cc (right): https://codereview.chromium.org/2582463003/diff/160001/content/browser/zygote... content/browser/zygote_host/zygote_communication_linux.cc:264: // Need to tell CdmHostFile(s) to ignore missing CDM host files in tests. On 2017/01/24 00:39:24, jam wrote: > ditto Done.
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.
jam and kerrnel: Kindly ping! :)
From a security standpoint I care most that: 1) verification happens once the process is sandboxed 2) the open file descriptors are closed in any non-CDM processes I see that happening, so LGTM. https://codereview.chromium.org/2582463003/diff/40001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/2582463003/diff/40001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:562: AddCdmHostFilePaths(cdm_host_file_paths); On 2017/01/19 08:30:50, xhwang_slow wrote: > On 2017/01/19 04:13:07, Greg K wrote: > > Not sure, but should this be namespaced? Otherwise I expect it to be defined > in > > this file. > > Well, a lot of chrome/ classes (e.g. ChromeContentClient in this file) are not > namespaced. So typically I don't use "chrome" namespace even in chrome/. That > being said, code search tells me that "chrome" namespace is actually used in a > lot of files. So I'll use it :) > > Done. Yeah I guess the owners can decide this one, was just pointing it out. https://codereview.chromium.org/2582463003/diff/40001/media/cdm/ppapi/externa... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/2582463003/diff/40001/media/cdm/ppapi/externa... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:293: int bytes_written = file.Write(0, kDataToWrite, 1); On 2017/01/19 08:30:51, xhwang_slow wrote: > On 2017/01/19 04:13:07, Greg K wrote: > > Isn't it extremely dangerous to check that a file isn't writable by actually > > writing to it? > > Good point. At least on Linux, we should be able to use fcntl (fd, F_GETFL) to > get the file access mode. That'll involve some base/file/ change so I'll leave > it to another CL. For now, I replace this check with a TODO. Yeah please make that a TODO, I'm not comfortable actually trying to write to a file.
https://codereview.chromium.org/2582463003/diff/40001/media/cdm/ppapi/externa... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/2582463003/diff/40001/media/cdm/ppapi/externa... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:293: int bytes_written = file.Write(0, kDataToWrite, 1); On 2017/01/25 06:15:32, Greg K wrote: > On 2017/01/19 08:30:51, xhwang_slow wrote: > > On 2017/01/19 04:13:07, Greg K wrote: > > > Isn't it extremely dangerous to check that a file isn't writable by actually > > > writing to it? > > > > Good point. At least on Linux, we should be able to use fcntl (fd, F_GETFL) to > > get the file access mode. That'll involve some base/file/ change so I'll leave > > it to another CL. For now, I replace this check with a TODO. > > Yeah please make that a TODO, I'm not comfortable actually trying to write to a > file. Thanks for the review!!! A TODO is already added in the latest PS.
lgtm, sorry for the delay as I missed your reply (ping me on IM if I don't respond in a few hours)
Thank you all for the through review!
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jrummell@chromium.org, tinskip@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2582463003/#ps180001 (title: "comments addressed")
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
Try jobs failed on following builders: 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
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": 180001, "attempt_start_ts": 1485410368132560,
"parent_rev": "2edd5a809a0b0f1fc1f10a121b8c66a690feecd1", "commit_rev":
"785a834775112c711cda038e3ae51a01c3a029dc"}
Message was sent while issue was closed.
Description was changed from ========== media: Verify CDM host files This change provides the Content Decryption Module (CDM) a way to verify the host files, the CDM adapter and the CDM itself. GetCdmHostFilePath() provides a list of host files. CdmHostFile opens the pair of a file and its corresponding signature file. CdmHostFiles manages all files (and signature files) opened for verification. On platforms that use the Zygote, files are opened in the Zygote process. After a child process is forked, if it's not a ppapi process, all files will be immediately closed. On other platforms, files are opened after the ppapi process is launched, but before the sandbox is sealed. On all platforms, if the ppapi process hosts a CDM, VerifyHostFiles() will be called with the files managed in CdmHostFiles. Then all opened files will be closed. A browser test is added to make sure files are opened correctly and are passed in VerifyHostFiles() as expected. BUG=658036 TEST=New browser_tests added. ========== to ========== media: Verify CDM host files This change provides the Content Decryption Module (CDM) a way to verify the host files, the CDM adapter and the CDM itself. GetCdmHostFilePath() provides a list of host files. CdmHostFile opens the pair of a file and its corresponding signature file. CdmHostFiles manages all files (and signature files) opened for verification. On platforms that use the Zygote, files are opened in the Zygote process. After a child process is forked, if it's not a ppapi process, all files will be immediately closed. On other platforms, files are opened after the ppapi process is launched, but before the sandbox is sealed. On all platforms, if the ppapi process hosts a CDM, VerifyHostFiles() will be called with the files managed in CdmHostFiles. Then all opened files will be closed. A browser test is added to make sure files are opened correctly and are passed in VerifyHostFiles() as expected. BUG=658036 TEST=New browser_tests added. Review-Url: https://codereview.chromium.org/2582463003 Cr-Commit-Position: refs/heads/master@{#446260} Committed: https://chromium.googlesource.com/chromium/src/+/785a834775112c711cda038e3ae5... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/785a834775112c711cda038e3ae5... |
