Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23828007:
Check whether the Pepper CDM is available before adding the key system. (Closed)
|
Created:
7 years, 3 months ago by ddorwin Modified:
7 years, 3 months ago CC:
chromium-reviews, stuartmorgan+watch_chromium.org, feature-media-reviews_chromium.org, xhwang, shadi, anandc Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCheck whether the Pepper CDM is available before adding the key system.
This adds a synchronous IPC from chrome_key_systems.cc to the browser process to
check with the PluginService.
BUG=224793
TEST=existing IsTypeSupported browser tests and new negative tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223719
Patch Set 1 #
Total comments: 9
Patch Set 2 : review feedback #Patch Set 3 : rebase only #Patch Set 4 : moved functions - no logic change #Patch Set 5 : Changed to check for Pepper plugins only and not use PluginList; added tests #
Total comments: 10
Patch Set 6 : review feedback #Patch Set 7 : rebase only #
Total comments: 4
Patch Set 8 : updated comments #
Total comments: 4
Patch Set 9 : Use GetInternalPlugins(); breaks External Clear Key #
Total comments: 2
Patch Set 10 : Fix bug that broke the test CDM. #
Messages
Total messages: 25 (0 generated)
The tests depend on John's CL. PTAL at the code for now.
https://codereview.chromium.org/23828007/diff/1/chrome/common/render_messages.h File chrome/common/render_messages.h (right): https://codereview.chromium.org/23828007/diff/1/chrome/common/render_messages... chrome/common/render_messages.h:509: IPC_SYNC_MESSAGE_CONTROL1_1(ChromeViewHostMsg_IsSupportingPluginAvailable, hmm... this name is a bit ambiguous but I'm having a hard time thinking of a better alternative WDYT about Plugin{Exists,Registered}WithMimeType? it conveys the mime-type-ness and also that it's an existence check https://codereview.chromium.org/23828007/diff/1/chrome/common/render_messages... chrome/common/render_messages.h:510: std::string /* mime_type */, indent is off https://codereview.chromium.org/23828007/diff/1/chrome/renderer/media/chrome_... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23828007/diff/1/chrome/renderer/media/chrome_... chrome/renderer/media/chrome_key_systems.cc:86: bool is_available = false; do you want cache this value?
looking good other than scherkus's comments and a nit. https://codereview.chromium.org/23828007/diff/1/chrome/browser/plugins/plugin... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/23828007/diff/1/chrome/browser/plugins/plugin... chrome/browser/plugins/plugin_info_message_filter.cc:68: GURL(), mime_type, false, &matching_plugins, NULL); indent
I found that PluginList, which was the underlying source of data, only has (the internal) plugins actually registered when they have been used. Therefor, I moved the logic to the PluginService. For simplicity and because it's all we care about, I limit the check to Pepper plugins. The existing positive tests and new negative tests verify the behavior. https://codereview.chromium.org/23828007/diff/1/chrome/browser/plugins/plugin... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/23828007/diff/1/chrome/browser/plugins/plugin... chrome/browser/plugins/plugin_info_message_filter.cc:68: GURL(), mime_type, false, &matching_plugins, NULL); On 2013/09/11 22:49:28, xhwang wrote: > indent Done. https://codereview.chromium.org/23828007/diff/1/chrome/common/render_messages.h File chrome/common/render_messages.h (right): https://codereview.chromium.org/23828007/diff/1/chrome/common/render_messages... chrome/common/render_messages.h:509: IPC_SYNC_MESSAGE_CONTROL1_1(ChromeViewHostMsg_IsSupportingPluginAvailable, On 2013/09/11 22:34:41, scherkus wrote: > hmm... this name is a bit ambiguous but I'm having a hard time thinking of a > better alternative > > WDYT about Plugin{Exists,Registered}WithMimeType? it conveys the mime-type-ness > and also that it's an existence check Good suggestions. WDYT of IsPluginRegisteredForMimeType? I think that's exactly what we're checking. (I actually had to add "Pepper" due to the logic change. https://codereview.chromium.org/23828007/diff/1/chrome/common/render_messages... chrome/common/render_messages.h:510: std::string /* mime_type */, On 2013/09/11 22:34:41, scherkus wrote: > indent is off Done. https://codereview.chromium.org/23828007/diff/1/chrome/renderer/media/chrome_... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23828007/diff/1/chrome/renderer/media/chrome_... chrome/renderer/media/chrome_key_systems.cc:86: bool is_available = false; On 2013/09/11 22:34:41, scherkus wrote: > do you want cache this value? It's not really necessary. This should only happen once per renderer process since it is invoked by KeySystems, which is a lazily-instantiated singleton. That's a bit of implementation knowledge that this code probably shouldn't have, but adding static values just adds complexity for no real gain. Note that this is not called for every canPlayType() call since this code is run to _add_ key systems. WDYT? P.S. If we were going to cache this value, it would probably need to be at line 104.
lgtm w/ nits https://codereview.chromium.org/23828007/diff/1/chrome/renderer/media/chrome_... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23828007/diff/1/chrome/renderer/media/chrome_... chrome/renderer/media/chrome_key_systems.cc:86: bool is_available = false; On 2013/09/16 17:17:58, ddorwin wrote: > On 2013/09/11 22:34:41, scherkus wrote: > > do you want cache this value? > > It's not really necessary. This should only happen once per renderer process > since it is invoked by KeySystems, which is a lazily-instantiated singleton. > That's a bit of implementation knowledge that this code probably shouldn't have, > but adding static values just adds complexity for no real gain. Note that this > is not called for every canPlayType() call since this code is run to _add_ key > systems. WDYT? > > P.S. If we were going to cache this value, it would probably need to be at line > 104. seems fine as is https://codereview.chromium.org/23828007/diff/22001/chrome/common/render_mess... File chrome/common/render_messages.h (right): https://codereview.chromium.org/23828007/diff/22001/chrome/common/render_mess... chrome/common/render_messages.h:508: // (i.e. by checking Content Settings). ditto on grammar (see my other comment) https://codereview.chromium.org/23828007/diff/22001/content/browser/plugin_se... File content/browser/plugin_service_impl.cc (right): https://codereview.chromium.org/23828007/diff/22001/content/browser/plugin_se... content/browser/plugin_service_impl.cc:686: for (size_t i = 0; i < ppapi_plugins_.size(); i++) { nit: ++i (rest of this file is consistent with that usage) https://codereview.chromium.org/23828007/diff/22001/content/browser/plugin_se... content/browser/plugin_service_impl.cc:687: const std::vector<WebPluginMimeType> & mime_types = nit: remove space before & https://codereview.chromium.org/23828007/diff/22001/content/browser/plugin_se... content/browser/plugin_service_impl.cc:689: for (size_t j = 0; j < mime_types.size(); j++) { nit: ++j https://codereview.chromium.org/23828007/diff/22001/content/public/browser/pl... File content/public/browser/plugin_service.h (right): https://codereview.chromium.org/23828007/diff/22001/content/public/browser/pl... content/public/browser/plugin_service.h:107: // (i.e. by checking Content Settings). grammar nit: did you mean to use i.e. ("that is" / "in other words") or e.g., ("for example")? the former provides an explicit clarification, the latter provides one of many possible examples i.e., if Content Settings isn't the only reason why plugins can't be instantiated then I'd recommend using e.g., ;)
jam, please review content/browser/. (Note: This is the desktop version of https://codereview.chromium.org/23513055/.) bauerb, please review chrome/browser/plugins/ jschuh, please review the new IPC in render_messages.h. https://codereview.chromium.org/23828007/diff/22001/chrome/common/render_mess... File chrome/common/render_messages.h (right): https://codereview.chromium.org/23828007/diff/22001/chrome/common/render_mess... chrome/common/render_messages.h:508: // (i.e. by checking Content Settings). On 2013/09/16 17:42:05, scherkus wrote: > ditto on grammar (see my other comment) Done. https://codereview.chromium.org/23828007/diff/22001/content/browser/plugin_se... File content/browser/plugin_service_impl.cc (right): https://codereview.chromium.org/23828007/diff/22001/content/browser/plugin_se... content/browser/plugin_service_impl.cc:686: for (size_t i = 0; i < ppapi_plugins_.size(); i++) { On 2013/09/16 17:42:05, scherkus wrote: > nit: ++i (rest of this file is consistent with that usage) Done. https://codereview.chromium.org/23828007/diff/22001/content/browser/plugin_se... content/browser/plugin_service_impl.cc:687: const std::vector<WebPluginMimeType> & mime_types = On 2013/09/16 17:42:05, scherkus wrote: > nit: remove space before & Done. https://codereview.chromium.org/23828007/diff/22001/content/browser/plugin_se... content/browser/plugin_service_impl.cc:689: for (size_t j = 0; j < mime_types.size(); j++) { On 2013/09/16 17:42:05, scherkus wrote: > nit: ++j Done. https://codereview.chromium.org/23828007/diff/22001/content/public/browser/pl... File content/public/browser/plugin_service.h (right): https://codereview.chromium.org/23828007/diff/22001/content/public/browser/pl... content/public/browser/plugin_service.h:107: // (i.e. by checking Content Settings). On 2013/09/16 17:42:05, scherkus wrote: > grammar nit: did you mean to use i.e. ("that is" / "in other words") or e.g., > ("for example")? > > the former provides an explicit clarification, the latter provides one of many > possible examples > > i.e., if Content Settings isn't the only reason why plugins can't be > instantiated then I'd recommend using e.g., ;) Done.
LGTM with nits addressed: https://codereview.chromium.org/23828007/diff/40001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/23828007/diff/40001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:159: bool expect_adapter_exists = true) { Default arguments aren't allowed per the style guide. https://codereview.chromium.org/23828007/diff/40001/content/public/browser/pl... File content/public/browser/plugin_service.h (right): https://codereview.chromium.org/23828007/diff/40001/content/public/browser/pl... content/public/browser/plugin_service.h:107: // (e.g. by checking Content Settings). Not sure whether we want to mention content settings here at all, as that's not a content thing. Maybe just mention that the embedder has the last word whether a plugin will actually be loaded?
ipc security lgtm.
jam, PTAL at content/. https://codereview.chromium.org/23828007/diff/40001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/23828007/diff/40001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:159: bool expect_adapter_exists = true) { On 2013/09/16 19:16:03, Bernhard Bauer wrote: > Default arguments aren't allowed per the style guide. It seems this falls under the specific exception for localized use since this is a local class. I could make it a local function in a different CL. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments https://codereview.chromium.org/23828007/diff/40001/content/public/browser/pl... File content/public/browser/plugin_service.h (right): https://codereview.chromium.org/23828007/diff/40001/content/public/browser/pl... content/public/browser/plugin_service.h:107: // (e.g. by checking Content Settings). On 2013/09/16 19:16:03, Bernhard Bauer wrote: > Not sure whether we want to mention content settings here at all, as that's not > a content thing. Maybe just mention that the embedder has the last word whether > a plugin will actually be loaded? Done. I changed it to two generic examples. I think these examples are useful to understanding how it should work.
https://codereview.chromium.org/23828007/diff/49001/content/public/browser/pl... File content/public/browser/plugin_service.h (right): https://codereview.chromium.org/23828007/diff/49001/content/public/browser/pl... content/public/browser/plugin_service.h:108: virtual bool IsPepperPluginRegisteredForMimeType( seems that you can avoid changing content, since chrome is the one that told content about which pepper plugins are available. so why do you need to go back into content to ask it information which you gave it? if it's because that chrome doesn't keep track of this (not sure?) then you can get this data from the GetPlugins callback?
jam, buaerb, scherkus: PTAL at the discussion in PS8. This PS is simpler since it reuses PluginService::GetInternalPlugins(), but it breaks our test CDM. We'll either need to: a) Make the test CDM an internal plugin or b) Revert this PS and fix RegisterInternalPlugins(). https://codereview.chromium.org/23828007/diff/49001/content/public/browser/pl... File content/public/browser/plugin_service.h (right): https://codereview.chromium.org/23828007/diff/49001/content/public/browser/pl... content/public/browser/plugin_service.h:108: virtual bool IsPepperPluginRegisteredForMimeType( On 2013/09/17 00:27:57, jam wrote: > seems that you can avoid changing content, since chrome is the one that told > content about which pepper plugins are available. so why do you need to go back > into content to ask it information which you gave it? if it's because that > chrome doesn't keep track of this (not sure?) then you can get this data from > the GetPlugins callback? Yes, it's because chrome doesn't keep track (and plugins are registered from multiple locations that all uses content's PluginService as the central point. For example, component-installed plugins: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/com... tl;dr: I found that PluginServiceImpl::pepper_plugins_ doesn't provide what I needed on all platforms and I need to use PluginService::GetInternalPlugins(), so I _don't_ need to change content, but I do need to call back into content. However, see the next paragraph. Problem: Using PluginService::GetInternalPlugins() means we can no longer register our test CDM (External Clear Key) on the command line because it is not internal and only appears in PluginServiceImpl::pepper_plugins_. I guess we can solve that by making it an internal plugin that is normally not present because it can't be found. This might add a slight delay to Chrome startup (equivalent to that of any other internal Pepper plugin). Other options: a) Add a command line flag for the test CDM (just optimizes out the File::Exists check in the normal case) b) Make PluginService::RegisterInternalPlugins() add PLUGIN_TYPE_PEPPER_* plugins to pepper_plugins_ and revert back to Patch Set 8, which does require content changes. Details: PluginList::plugins_list_ (and now I've found PluginServiceImpl::pepper_plugins_) don't include (all) the plugins I care about. It appears only PluginList::internal_plugins_ does. I've switched the implementation to use this. GetPlugins() "Asynchronously loads plugins if necessary and then calls back..." In Patch Set 1, I was trying to use GetPluginInfoArray(), but, like I believe GetPlugins() does, it uses PluginList, which only returns information if the plugin has been loaded (or is NPAPI/external?) and is thus in plugins_list_. It doesn't check internal_plugins_. Since we want to query availability before loading, this wouldn't work. Specifically, manual tests failed sometime (though I didn't know exactly why at the time) and the browser tests were failing. Unfortunately, there are several lists of plugins that aren't all in agreement. PluginList::plugins_list_ appears to include only loaded plugins (and maybe NPAPI?). PluginServiceImpl::pepper_plugins_ includes packaged Pepper Plugins but not component-installed plugins. PluginList::internal_plugins_ includes all internal plugins, which I believe includes all Pepper Plugins, including component-installed ones. There is also PepperPluginRegistry, which doesn't do what one would expect (crbug.com/277039).
Added a third option. On 2013/09/17 01:41:18, ddorwin wrote: > jam, buaerb, scherkus: PTAL at the discussion in PS8. > > This PS is simpler since it reuses PluginService::GetInternalPlugins(), but it > breaks our test CDM. We'll either need to: > a) Make the test CDM an internal plugin or > b) Revert this PS and fix RegisterInternalPlugins(). c) Add PluginService::AddPepperPlugin() to add plugins to ppapi_plugins_ and fix the component installers to also call this method. (c) may be better but seems brittle and probably not much better than (b), though (b) may affect assumptions in other parts of the code.
jam, bauerb, scherkus: Please review the changes in PS9 (see below for details). xhwang, shadi, anandc: FYI on changes to testing. Our tests that register the Widevine CDM on component platforms (Win & Mac) will also break with this change. i think the best and most isolated solution is to add --enable-cdms-for-test to chrome_switches (needs to be passed to renderer too) and have it a) register the test CDM and b) register the non-component Widevine CDM on component platforms. (a) will also ensure the test CDM is registered in the same way with the same permissions as real CDMs. If anyone objects, please let me know ASAP. Otherwise, I'll begin implementing the switch in a separate CL (to land before this one). On 2013/09/17 01:46:09, ddorwin wrote: > Added a third option. > On 2013/09/17 01:41:18, ddorwin wrote: > > jam, buaerb, scherkus: PTAL at the discussion in PS8. > > > > This PS is simpler since it reuses PluginService::GetInternalPlugins(), but it > > breaks our test CDM. We'll either need to: > > a) Make the test CDM an internal plugin or > > b) Revert this PS and fix RegisterInternalPlugins(). > c) Add PluginService::AddPepperPlugin() to add plugins to ppapi_plugins_ and > fix the component installers to also call this method. > > (c) may be better but seems brittle and probably not much better than (b), > though (b) may affect assumptions in other parts of the code.
xhwang, shadi, anandc: FYI on changes to testing. https://codereview.chromium.org/23828007/diff/55001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/23828007/diff/55001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:856: // Since this test fixture does not register the CDMs on the command line, the This test will work by not setting the new switch. https://codereview.chromium.org/23828007/diff/55001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:884: // Since this test fixture does not register the CDMs on the command line, the This test will no longer be possible. We'd have to rename the files, which seems like a bad idea, or somehow override the lookup path.
On 2013/09/17 04:04:04, ddorwin wrote: > jam, bauerb, scherkus: Please review the changes in PS9 (see below for details). > > xhwang, shadi, anandc: FYI on changes to testing. > > Our tests that register the Widevine CDM on component platforms (Win & Mac) will > also break with this change. > > i think the best and most isolated solution is to add --enable-cdms-for-test to > chrome_switches (needs to be passed to renderer too) and have it a) register the > test CDM and b) register the non-component Widevine CDM on component platforms. > (a) will also ensure the test CDM is registered in the same way with the same > permissions as real CDMs. > > If anyone objects, please let me know ASAP. Otherwise, I'll begin implementing > the switch in a separate CL (to land before this one). I see, thanks for the detailed explanations. at this point, i'm fine with either PS 8 or 9 with the other cl that you describe. Agree that it is preferable if the test CDM is registered the same way as the non-test one lgtm
https://codereview.chromium.org/23828007/diff/49001/content/public/browser/pl... File content/public/browser/plugin_service.h (right): https://codereview.chromium.org/23828007/diff/49001/content/public/browser/pl... content/public/browser/plugin_service.h:108: virtual bool IsPepperPluginRegisteredForMimeType( On 2013/09/17 01:41:18, ddorwin wrote: > On 2013/09/17 00:27:57, jam wrote: > > seems that you can avoid changing content, since chrome is the one that told > > content about which pepper plugins are available. so why do you need to go > back > > into content to ask it information which you gave it? if it's because that > > chrome doesn't keep track of this (not sure?) then you can get this data from > > the GetPlugins callback? > > Yes, it's because chrome doesn't keep track (and plugins are registered from > multiple locations that all uses content's PluginService as the central point. > For example, component-installed plugins: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/com... For locations in chrome/ that register plug-ins on the PluginService, we *could* of course also store the information somewhere in chrome/. But plug-ins added on the command line are registered directly in PluginServiceImpl. > tl;dr: I found that PluginServiceImpl::pepper_plugins_ doesn't provide what I > needed on all platforms and I need to use PluginService::GetInternalPlugins(), > so I _don't_ need to change content, but I do need to call back into content. > However, see the next paragraph. > > Problem: Using PluginService::GetInternalPlugins() means we can no longer > register our test CDM (External Clear Key) on the command line because it is not > internal and only appears in PluginServiceImpl::pepper_plugins_. I guess we can > solve that by making it an internal plugin that is normally not present because > it can't be found. This might add a slight delay to Chrome startup (equivalent > to that of any other internal Pepper plugin). Why is that? If we register an internal plugin, we don't need to look at the file on disk. > Other options: a) Add a command > line flag for the test CDM (just optimizes out the File::Exists check in the > normal case) b) Make PluginService::RegisterInternalPlugins() add > PLUGIN_TYPE_PEPPER_* plugins to pepper_plugins_ and revert back to Patch Set 8, > which does require content changes. > > Details: > PluginList::plugins_list_ (and now I've found > PluginServiceImpl::pepper_plugins_) don't include (all) the plugins I care > about. It appears only PluginList::internal_plugins_ does. I've switched the > implementation to use this. > > GetPlugins() "Asynchronously loads plugins if necessary and then calls back..." > In Patch Set 1, I was trying to use GetPluginInfoArray(), but, like I believe > GetPlugins() does, it uses PluginList, which only returns information if the > plugin has been loaded (or is NPAPI/external?) and is thus in plugins_list_. It > doesn't check internal_plugins_. Since we want to query availability before > loading, this wouldn't work. Specifically, manual tests failed sometime (though > I didn't know exactly why at the time) and the browser tests were failing. > > Unfortunately, there are several lists of plugins that aren't all in agreement. > PluginList::plugins_list_ appears to include only loaded plugins (and maybe > NPAPI?). PluginServiceImpl::pepper_plugins_ includes packaged Pepper Plugins but > not component-installed plugins. PluginList::internal_plugins_ includes all > internal plugins, which I believe includes all Pepper Plugins, including > component-installed ones. There is also PepperPluginRegistry, which doesn't do > what one would expect (crbug.com/277039). What exactly do you mean by "loaded plugins"? GetPluginInfoArray() will return all plugins that PluginList knows about. i.e. for which it has a WebPluginInfo. The WebPluginInfo usually comes from the .so/.dll/.plugin on disk, but can also be provided manually for internal plugins. The registered Pepper plugins (i.e. the ones added by ChromeContentBrowserClient or coming from the command line) are also registered as internal plugins AFAIK (see https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/pl...).
https://codereview.chromium.org/23828007/diff/49001/content/public/browser/pl... File content/public/browser/plugin_service.h (right): https://codereview.chromium.org/23828007/diff/49001/content/public/browser/pl... content/public/browser/plugin_service.h:108: virtual bool IsPepperPluginRegisteredForMimeType( On 2013/09/17 14:52:57, Bernhard Bauer wrote: > On 2013/09/17 01:41:18, ddorwin wrote: > > On 2013/09/17 00:27:57, jam wrote: > > > seems that you can avoid changing content, since chrome is the one that told > > > content about which pepper plugins are available. so why do you need to go > > back > > > into content to ask it information which you gave it? if it's because that > > > chrome doesn't keep track of this (not sure?) then you can get this data > from > > > the GetPlugins callback? > > > > Yes, it's because chrome doesn't keep track (and plugins are registered from > > multiple locations that all uses content's PluginService as the central point. > > For example, component-installed plugins: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/com... > > For locations in chrome/ that register plug-ins on the PluginService, we *could* > of course also store the information somewhere in chrome/. But plug-ins added on > the command line are registered directly in PluginServiceImpl. > > > tl;dr: I found that PluginServiceImpl::pepper_plugins_ doesn't provide what I > > needed on all platforms and I need to use PluginService::GetInternalPlugins(), > > so I _don't_ need to change content, but I do need to call back into content. > > However, see the next paragraph. > > > > Problem: Using PluginService::GetInternalPlugins() means we can no longer > > register our test CDM (External Clear Key) on the command line because it is > not > > internal and only appears in PluginServiceImpl::pepper_plugins_. I guess we > can > > solve that by making it an internal plugin that is normally not present > because > > it can't be found. This might add a slight delay to Chrome startup (equivalent > > to that of any other internal Pepper plugin). > > Why is that? If we register an internal plugin, we don't need to look at the > file on disk. We don't need to load it, but we make sure PathExists: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro... > > > Other options: a) Add a command > > line flag for the test CDM (just optimizes out the File::Exists check in the > > normal case) b) Make PluginService::RegisterInternalPlugins() add > > PLUGIN_TYPE_PEPPER_* plugins to pepper_plugins_ and revert back to Patch Set > 8, > > which does require content changes. > > > > Details: > > PluginList::plugins_list_ (and now I've found > > PluginServiceImpl::pepper_plugins_) don't include (all) the plugins I care > > about. It appears only PluginList::internal_plugins_ does. I've switched the > > implementation to use this. > > > > GetPlugins() "Asynchronously loads plugins if necessary and then calls > back..." > > In Patch Set 1, I was trying to use GetPluginInfoArray(), but, like I believe > > GetPlugins() does, it uses PluginList, which only returns information if the > > plugin has been loaded (or is NPAPI/external?) and is thus in plugins_list_. > It > > doesn't check internal_plugins_. Since we want to query availability before > > loading, this wouldn't work. Specifically, manual tests failed sometime > (though > > I didn't know exactly why at the time) and the browser tests were failing. > > > > Unfortunately, there are several lists of plugins that aren't all in > agreement. > > PluginList::plugins_list_ appears to include only loaded plugins (and maybe > > NPAPI?). PluginServiceImpl::pepper_plugins_ includes packaged Pepper Plugins > but > > not component-installed plugins. PluginList::internal_plugins_ includes all > > internal plugins, which I believe includes all Pepper Plugins, including > > component-installed ones. There is also PepperPluginRegistry, which doesn't do > > what one would expect (crbug.com/277039). > > What exactly do you mean by "loaded plugins"? GetPluginInfoArray() will return > all plugins that PluginList knows about. i.e. for which it has a WebPluginInfo. > The WebPluginInfo usually comes from the .so/.dll/.plugin on disk, but can also > be provided manually for internal plugins. The registered Pepper plugins (i.e. > the ones added by ChromeContentBrowserClient or coming from the command line) > are also registered as internal plugins AFAIK (see > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/pl...). Those plugins are registered as internal plugins, which means they are added to internal_plugins_. GetPluginInfoArray() only reads plugins_list_. plugins_list_ appears to be populated by LoadPlugins(). It calls GetPluginPathsToLoad(), which uses extra_plugin_paths_, to which the internal plugins were added. So GetPluginInfoArray() will return the internal Pepper plugins, but only after LoadPlugins() has been called (again?). When is LoadPlugins() called? It doesn't appear to be called after internal plugin registration until a plugin is needed, at least on Linux.
On Tue, Sep 17, 2013 at 10:19 AM, <ddorwin@chromium.org> wrote: > > https://codereview.chromium.**org/23828007/diff/49001/** > content/public/browser/plugin_**service.h<https://codereview.chromium.org/23828007/diff/49001/content/public/browser/plugin_service.h> > File content/public/browser/plugin_**service.h (right): > > https://codereview.chromium.**org/23828007/diff/49001/** > content/public/browser/plugin_**service.h#newcode108<https://codereview.chromium.org/23828007/diff/49001/content/public/browser/plugin_service.h#newcode108> > content/public/browser/plugin_**service.h:108: virtual bool > IsPepperPluginRegisteredForMim**eType( > On 2013/09/17 14:52:57, Bernhard Bauer wrote: > >> On 2013/09/17 01:41:18, ddorwin wrote: >> > On 2013/09/17 00:27:57, jam wrote: >> > > seems that you can avoid changing content, since chrome is the one >> > that told > >> > > content about which pepper plugins are available. so why do you >> > need to go > >> > back >> > > into content to ask it information which you gave it? if it's >> > because that > >> > > chrome doesn't keep track of this (not sure?) then you can get >> > this data > >> from >> > > the GetPlugins callback? >> > >> > Yes, it's because chrome doesn't keep track (and plugins are >> > registered from > >> > multiple locations that all uses content's PluginService as the >> > central point. > >> > For example, component-installed plugins: >> > >> > > https://code.google.com/p/**chromium/codesearch#chromium/** > src/chrome/browser/component_**updater/pepper_flash_** > component_installer.cc&sq=**package:chromium&type=cs&l=** > 204&q=RegisterInternalPlugin<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/component_updater/pepper_flash_component_installer.cc&sq=package:chromium&type=cs&l=204&q=RegisterInternalPlugin> > > For locations in chrome/ that register plug-ins on the PluginService, >> > we *could* > >> of course also store the information somewhere in chrome/. But >> > plug-ins added on > >> the command line are registered directly in PluginServiceImpl. >> > > > tl;dr: I found that PluginServiceImpl::pepper_**plugins_ doesn't >> > provide what I > >> > needed on all platforms and I need to use >> > PluginService::**GetInternalPlugins(), > >> > so I _don't_ need to change content, but I do need to call back into >> > content. > >> > However, see the next paragraph. >> > >> > Problem: Using PluginService::**GetInternalPlugins() means we can no >> > longer > >> > register our test CDM (External Clear Key) on the command line >> > because it is > >> not >> > internal and only appears in PluginServiceImpl::pepper_**plugins_. I >> > guess we > >> can >> > solve that by making it an internal plugin that is normally not >> > present > >> because >> > it can't be found. This might add a slight delay to Chrome startup >> > (equivalent > >> > to that of any other internal Pepper plugin). >> > > Why is that? If we register an internal plugin, we don't need to look >> > at the > >> file on disk. >> > > We don't need to load it, but we make sure PathExists: > https://code.google.com/p/**chromium/codesearch#chromium/** > src/chrome/common/chrome_**content_client.cc&q=**ComputeBuiltInPlugins%** > 20PathExists&sq=package:**chromium&type=cs&l=130<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chrome_content_client.cc&q=ComputeBuiltInPlugins%20PathExists&sq=package:chromium&type=cs&l=130> Ah, I see. I think Pepper plugins added on the command line are also registered as internal plugins though (in fact, all Pepper plugins are), and we don't check the file for those. > > Other options: a) Add a command >> > line flag for the test CDM (just optimizes out the File::Exists >> > check in the > >> > normal case) b) Make PluginService::**RegisterInternalPlugins() add >> > PLUGIN_TYPE_PEPPER_* plugins to pepper_plugins_ and revert back to >> > Patch Set > >> 8, >> > which does require content changes. >> > >> > Details: >> > PluginList::plugins_list_ (and now I've found >> > PluginServiceImpl::pepper_**plugins_) don't include (all) the plugins >> > I care > >> > about. It appears only PluginList::internal_plugins_ does. I've >> > switched the > >> > implementation to use this. >> > >> > GetPlugins() "Asynchronously loads plugins if necessary and then >> > calls > >> back..." >> > In Patch Set 1, I was trying to use GetPluginInfoArray(), but, like >> > I believe > >> > GetPlugins() does, it uses PluginList, which only returns >> > information if the > >> > plugin has been loaded (or is NPAPI/external?) and is thus in >> > plugins_list_. > >> It >> > doesn't check internal_plugins_. Since we want to query availability >> > before > >> > loading, this wouldn't work. Specifically, manual tests failed >> > sometime > >> (though >> > I didn't know exactly why at the time) and the browser tests were >> > failing. > >> > >> > Unfortunately, there are several lists of plugins that aren't all in >> agreement. >> > PluginList::plugins_list_ appears to include only loaded plugins >> > (and maybe > >> > NPAPI?). PluginServiceImpl::pepper_**plugins_ includes packaged Pepper >> > Plugins > >> but >> > not component-installed plugins. PluginList::internal_plugins_ >> > includes all > >> > internal plugins, which I believe includes all Pepper Plugins, >> > including > >> > component-installed ones. There is also PepperPluginRegistry, which >> > doesn't do > >> > what one would expect (crbug.com/277039). >> > > What exactly do you mean by "loaded plugins"? GetPluginInfoArray() >> > will return > >> all plugins that PluginList knows about. i.e. for which it has a >> > WebPluginInfo. > >> The WebPluginInfo usually comes from the .so/.dll/.plugin on disk, but >> > can also > >> be provided manually for internal plugins. The registered Pepper >> > plugins (i.e. > >> the ones added by ChromeContentBrowserClient or coming from the >> > command line) > >> are also registered as internal plugins AFAIK (see >> > > https://code.google.com/p/**chromium/codesearch#chromium/** > src/content/browser/plugin_**service_impl.cc&sq=package:** > chromium&rcl=1379378696&l=651<https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/plugin_service_impl.cc&sq=package:chromium&rcl=1379378696&l=651> > )**. > > Those plugins are registered as internal plugins, which means they are > added to internal_plugins_. GetPluginInfoArray() only reads > plugins_list_. > > plugins_list_ appears to be populated by LoadPlugins(). It calls > GetPluginPathsToLoad(), which uses extra_plugin_paths_, to which the > internal plugins were added. So GetPluginInfoArray() will return the > internal Pepper plugins, but only after LoadPlugins() has been called > (again?). > > When is LoadPlugins() called? It doesn't appear to be called after > internal plugin registration until a plugin is needed, at least on > Linux. > That's correct. LoadPlugins() is called lazily, to avoid unnecessary disk access. GetPluginInfoArray() triggers a LoadPlugins() though. > https://codereview.chromium.**org/23828007/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
On 2013/09/17 17:35:40, Bernhard Bauer wrote: > On Tue, Sep 17, 2013 at 10:19 AM, <mailto:ddorwin@chromium.org> wrote: > > > > > https://codereview.chromium.**org/23828007/diff/49001/** > > > content/public/browser/plugin_**service.h<https://codereview.chromium.org/23828007/diff/49001/content/public/browser/plugin_service.h> > > File content/public/browser/plugin_**service.h (right): > > > > https://codereview.chromium.**org/23828007/diff/49001/** > > > content/public/browser/plugin_**service.h#newcode108<https://codereview.chromium.org/23828007/diff/49001/content/public/browser/plugin_service.h#newcode108> > > content/public/browser/plugin_**service.h:108: virtual bool > > IsPepperPluginRegisteredForMim**eType( > > On 2013/09/17 14:52:57, Bernhard Bauer wrote: > > > >> On 2013/09/17 01:41:18, ddorwin wrote: > >> > On 2013/09/17 00:27:57, jam wrote: > >> > > seems that you can avoid changing content, since chrome is the one > >> > > that told > > > >> > > content about which pepper plugins are available. so why do you > >> > > need to go > > > >> > back > >> > > into content to ask it information which you gave it? if it's > >> > > because that > > > >> > > chrome doesn't keep track of this (not sure?) then you can get > >> > > this data > > > >> from > >> > > the GetPlugins callback? > >> > > >> > Yes, it's because chrome doesn't keep track (and plugins are > >> > > registered from > > > >> > multiple locations that all uses content's PluginService as the > >> > > central point. > > > >> > For example, component-installed plugins: > >> > > >> > > > > https://code.google.com/p/**chromium/codesearch#chromium/** > > src/chrome/browser/component_**updater/pepper_flash_** > > component_installer.cc&sq=**package:chromium&type=cs&l=** > > > 204&q=RegisterInternalPlugin<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/component_updater/pepper_flash_component_installer.cc&sq=package:chromium&type=cs&l=204&q=RegisterInternalPlugin> > > > > For locations in chrome/ that register plug-ins on the PluginService, > >> > > we *could* > > > >> of course also store the information somewhere in chrome/. But > >> > > plug-ins added on > > > >> the command line are registered directly in PluginServiceImpl. > >> > > > > > tl;dr: I found that PluginServiceImpl::pepper_**plugins_ doesn't > >> > > provide what I > > > >> > needed on all platforms and I need to use > >> > > PluginService::**GetInternalPlugins(), > > > >> > so I _don't_ need to change content, but I do need to call back into > >> > > content. > > > >> > However, see the next paragraph. > >> > > >> > Problem: Using PluginService::**GetInternalPlugins() means we can no > >> > > longer > > > >> > register our test CDM (External Clear Key) on the command line > >> > > because it is > > > >> not > >> > internal and only appears in PluginServiceImpl::pepper_**plugins_. I > >> > > guess we > > > >> can > >> > solve that by making it an internal plugin that is normally not > >> > > present > > > >> because > >> > it can't be found. This might add a slight delay to Chrome startup > >> > > (equivalent > > > >> > to that of any other internal Pepper plugin). > >> > > > > Why is that? If we register an internal plugin, we don't need to look > >> > > at the > > > >> file on disk. > >> > > > > We don't need to load it, but we make sure PathExists: > > https://code.google.com/p/**chromium/codesearch#chromium/** > > src/chrome/common/chrome_**content_client.cc&q=**ComputeBuiltInPlugins%** > > > 20PathExists&sq=package:**chromium&type=cs&l=130<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chrome_content_client.cc&q=ComputeBuiltInPlugins%20PathExists&sq=package:chromium&type=cs&l=130> > > > Ah, I see. I think Pepper plugins added on the command line are also > registered as internal plugins though (in fact, all Pepper plugins are), > and we don't check the file for those. Looking at the code again, that appears to be correct. Let me look again at why this wasn't working. Note that not all Pepper plugins are registered as PPAPI plugins - namely the components. That might be a bug. > > > > > > Other options: a) Add a command > >> > line flag for the test CDM (just optimizes out the File::Exists > >> > > check in the > > > >> > normal case) b) Make PluginService::**RegisterInternalPlugins() add > >> > PLUGIN_TYPE_PEPPER_* plugins to pepper_plugins_ and revert back to > >> > > Patch Set > > > >> 8, > >> > which does require content changes. > >> > > >> > Details: > >> > PluginList::plugins_list_ (and now I've found > >> > PluginServiceImpl::pepper_**plugins_) don't include (all) the plugins > >> > > I care > > > >> > about. It appears only PluginList::internal_plugins_ does. I've > >> > > switched the > > > >> > implementation to use this. > >> > > >> > GetPlugins() "Asynchronously loads plugins if necessary and then > >> > > calls > > > >> back..." > >> > In Patch Set 1, I was trying to use GetPluginInfoArray(), but, like > >> > > I believe > > > >> > GetPlugins() does, it uses PluginList, which only returns > >> > > information if the > > > >> > plugin has been loaded (or is NPAPI/external?) and is thus in > >> > > plugins_list_. > > > >> It > >> > doesn't check internal_plugins_. Since we want to query availability > >> > > before > > > >> > loading, this wouldn't work. Specifically, manual tests failed > >> > > sometime > > > >> (though > >> > I didn't know exactly why at the time) and the browser tests were > >> > > failing. > > > >> > > >> > Unfortunately, there are several lists of plugins that aren't all in > >> agreement. > >> > PluginList::plugins_list_ appears to include only loaded plugins > >> > > (and maybe > > > >> > NPAPI?). PluginServiceImpl::pepper_**plugins_ includes packaged Pepper > >> > > Plugins > > > >> but > >> > not component-installed plugins. PluginList::internal_plugins_ > >> > > includes all > > > >> > internal plugins, which I believe includes all Pepper Plugins, > >> > > including > > > >> > component-installed ones. There is also PepperPluginRegistry, which > >> > > doesn't do > > > >> > what one would expect (crbug.com/277039). > >> > > > > What exactly do you mean by "loaded plugins"? GetPluginInfoArray() > >> > > will return > > > >> all plugins that PluginList knows about. i.e. for which it has a > >> > > WebPluginInfo. > > > >> The WebPluginInfo usually comes from the .so/.dll/.plugin on disk, but > >> > > can also > > > >> be provided manually for internal plugins. The registered Pepper > >> > > plugins (i.e. > > > >> the ones added by ChromeContentBrowserClient or coming from the > >> > > command line) > > > >> are also registered as internal plugins AFAIK (see > >> > > > > https://code.google.com/p/**chromium/codesearch#chromium/** > > src/content/browser/plugin_**service_impl.cc&sq=package:** > > > chromium&rcl=1379378696&l=651<https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/plugin_service_impl.cc&sq=package:chromium&rcl=1379378696&l=651> > > )**. > > > > Those plugins are registered as internal plugins, which means they are > > added to internal_plugins_. GetPluginInfoArray() only reads > > plugins_list_. > > > > plugins_list_ appears to be populated by LoadPlugins(). It calls > > GetPluginPathsToLoad(), which uses extra_plugin_paths_, to which the > > internal plugins were added. So GetPluginInfoArray() will return the > > internal Pepper plugins, but only after LoadPlugins() has been called > > (again?). > > > > When is LoadPlugins() called? It doesn't appear to be called after > > internal plugin registration until a plugin is needed, at least on > > Linux. > > > > That's correct. LoadPlugins() is called lazily, to avoid unnecessary disk > access. GetPluginInfoArray() triggers a LoadPlugins() though. Based on experiments, GetPluginInfoArray() does not appear to trigger LoadPlugins(). This is because the use_stale _pointer_ is not NULL: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/plu... Is this a bug? Should it be checking the dereferenced value? I've found that GetPlugins() does cause LoadPlugins() to be called and is called in various places. In the browser_tests, it's after 20 seconds with this stack: [0x7f10f5ed3ea5] content::PluginServiceImpl::GetPlugins() [0x000001211a63] MetricsService::OnInitTaskGotHardwareClass() If I check the dererferenced value, LoadPlugins() is called in the browser_test, BUT it crashes in WillLoadPluginsCallback: CHECK(false) << "Plugin loading should happen out-of-process."; So, there may be a bug in GetPluginInfoArray(), but fixing it causes browser_tests to break. If that's not a bug, then GetPluginInfoArray() will not force plugins to load like you expected. > > > > > https://codereview.chromium.**org/23828007/%3Chttps://codereview.chromium.org...> > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Tue, Sep 17, 2013 at 8:32 PM, <ddorwin@chromium.org> wrote: > Based on experiments, GetPluginInfoArray() does not appear to trigger > LoadPlugins(). This is because the use_stale _pointer_ is not NULL: > https://code.google.com/p/**chromium/codesearch#chromium/** > src/content/common/plugin_**list.cc&q=use_stale%**20GetPluginInfoArray&sq= > **package:chromium&type=cs&l=321<https://code.google.com/p/chromium/codesearch#chromium/src/content/common/plugin_list.cc&q=use_stale%20GetPluginInfoArray&sq=package:chromium&type=cs&l=321> > Is this a bug? Should it be checking the dereferenced value? No, this is on purpose. You can pass in NULL to indicate that you always want the plug-in list to be loaded, or non-NULL if you are okay with possibly getting a stale version, in which case a flag will be written to it that says whether the plug-in list was stale or not. I've found that GetPlugins() does cause LoadPlugins() to be called and is > called > in various places. In the browser_tests, it's after 20 seconds with this > stack: > [0x7f10f5ed3ea5] content::PluginServiceImpl::**GetPlugins() > [0x000001211a63] MetricsService::**OnInitTaskGotHardwareClass() > > If I check the dererferenced value, LoadPlugins() is called in the > browser_test, > BUT it crashes in WillLoadPluginsCallback: > CHECK(false) << "Plugin loading should happen out-of-process."; > > So, there may be a bug in GetPluginInfoArray(), but fixing it causes > browser_tests to break. > If that's not a bug, then GetPluginInfoArray() will not force plugins to > load > like you expected. > Yes, I think what we do in these cases is first call PluginService::GetPlugins() (to force loading the plug-in list), then GetPluginInfo (so we know that the plug-in list is not older than the time we called GetPlugins()). > > https://codereview.chromium.**org/23828007/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Good news: We do register command line Pepper plugins as internal plugins, so PS9 can work for the test CDM. There was a bug (fixed in PS10) that just happened to not affect the Widevine CDM. So, I will land this CL, and we don't need a new flag to enable test CDMs. We may consider that in the future for other reasons, though. Thanks Bernhard for your help. On 2013/09/17 18:51:14, Bernhard Bauer wrote: > On Tue, Sep 17, 2013 at 8:32 PM, <mailto:ddorwin@chromium.org> wrote: > > If that's not a bug, then GetPluginInfoArray() will not force plugins to > > load > > like you expected. > > > > Yes, I think what we do in these cases is first call > PluginService::GetPlugins() (to force loading the plug-in list), then > GetPluginInfo (so we know that the plug-in list is not older than the time > we called GetPlugins()). This is totally unrelated to this CL, but would it be better to make this part of and/or more explicit in GetPluginInfo* in PluginList/PluginService?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ddorwin@chromium.org/23828007/67001
On Tue, Sep 17, 2013 at 9:53 PM, <ddorwin@chromium.org> wrote: > Good news: We do register command line Pepper plugins as internal plugins, > so > PS9 can work for the test CDM. There was a bug (fixed in PS10) that just > happened to not affect the Widevine CDM. > > So, I will land this CL, and we don't need a new flag to enable test CDMs. > We > may consider that in the future for other reasons, though. > > Thanks Bernhard for your help. > > On 2013/09/17 18:51:14, Bernhard Bauer wrote: > > On Tue, Sep 17, 2013 at 8:32 PM, <mailto:ddorwin@chromium.org> wrote: >> > If that's not a bug, then GetPluginInfoArray() will not force plugins to >> > load >> > like you expected. >> > >> > > Yes, I think what we do in these cases is first call >> PluginService::GetPlugins() (to force loading the plug-in list), then >> GetPluginInfo (so we know that the plug-in list is not older than the time >> we called GetPlugins()). >> > > This is totally unrelated to this CL, but would it be better to make this > part > of and/or more explicit in GetPluginInfo* in PluginList/PluginService? > Yes! I thought we already had that in there. > https://codereview.chromium.**org/23828007/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Change committed as 223719 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
