Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
128773002:
Restart plugin loading only if the plugin list has actually become stale. (Closed)
|
Created:
6 years, 11 months ago by Bernhard Bauer Modified:
6 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRestart plugin loading only if the plugin list has actually become stale.
BUG=171404
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247252
Patch Set 1 #Patch Set 2 : x #Patch Set 3 : x #
Total comments: 9
Patch Set 4 : review #
Total comments: 9
Patch Set 5 : review #Patch Set 6 : fix test #
Messages
Total messages: 17 (0 generated)
Please review.
https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... File content/browser/plugin_loader_posix.cc (right): https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... content/browser/plugin_loader_posix.cc:40: PluginList::Singleton()->PrepareForPluginLoading(); Why can't this just be RefreshPlugins? https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... File content/browser/plugin_loader_posix.h (left): https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... content/browser/plugin_loader_posix.h:56: scoped_refptr<base::MessageLoopProxy> target_loop, You're changing the calling semantics in terms of which thread |callback| is run on. Can you now document on which thread |callback| is run? https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... File content/browser/plugin_loader_posix.h (right): https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... content/browser/plugin_loader_posix.h:72: void GetPluginsWrapper( Please comment. https://codereview.chromium.org/128773002/diff/150001/content/common/plugin_l... File content/common/plugin_list.h (right): https://codereview.chromium.org/128773002/diff/150001/content/common/plugin_l... content/common/plugin_list.h:143: // loading plugins with a different instance of this class. Returns false if This is a singleton, how would you use a different instance?
https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... File content/browser/plugin_loader_posix.cc (right): https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... content/browser/plugin_loader_posix.cc:40: PluginList::Singleton()->PrepareForPluginLoading(); On 2014/01/21 15:17:22, rsesek wrote: > Why can't this just be RefreshPlugins? RefreshPlugins() marks the plugin list as invalidated (i.e. NEEDS_REFRESH). If we are here, then the plugin list is already in that state. What we do here is set the state to REFRESHING, so that future calls to RefreshPlugins() will set it to NEEDS_REFRESH again. https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... File content/browser/plugin_loader_posix.h (left): https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... content/browser/plugin_loader_posix.h:56: scoped_refptr<base::MessageLoopProxy> target_loop, On 2014/01/21 15:17:22, rsesek wrote: > You're changing the calling semantics in terms of which thread |callback| is run > on. Can you now document on which thread |callback| is run? Done. https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... File content/browser/plugin_loader_posix.h (right): https://codereview.chromium.org/128773002/diff/150001/content/browser/plugin_... content/browser/plugin_loader_posix.h:72: void GetPluginsWrapper( On 2014/01/21 15:17:22, rsesek wrote: > Please comment. Done. https://codereview.chromium.org/128773002/diff/150001/content/common/plugin_l... File content/common/plugin_list.h (right): https://codereview.chromium.org/128773002/diff/150001/content/common/plugin_l... content/common/plugin_list.h:143: // loading plugins with a different instance of this class. Returns false if On 2014/01/21 15:17:22, rsesek wrote: > This is a singleton, how would you use a different instance? In a different process? I copied the term from line 136, assuming that this is the accepted not-mentioning-PluginLoaderPosix description of PluginLoaderPosix.
https://codereview.chromium.org/128773002/diff/150001/content/common/plugin_l... File content/common/plugin_list.h (right): https://codereview.chromium.org/128773002/diff/150001/content/common/plugin_l... content/common/plugin_list.h:143: // loading plugins with a different instance of this class. Returns false if On 2014/01/21 15:48:25, Bernhard Bauer wrote: > On 2014/01/21 15:17:22, rsesek wrote: > > This is a singleton, how would you use a different instance? > > In a different process? I copied the term from line 136, assuming that this is > the accepted not-mentioning-PluginLoaderPosix description of PluginLoaderPosix. That's not immediately clear, but it makes sense. https://codereview.chromium.org/128773002/diff/270001/content/browser/plugin_... File content/browser/plugin_loader_posix.cc (right): https://codereview.chromium.org/128773002/diff/270001/content/browser/plugin_... content/browser/plugin_loader_posix.cc:146: const std::vector<WebPluginInfo>& /* plugins */) { nit: I don't think we typically omit parameters https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... File content/common/plugin_list.cc (right): https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... content/common/plugin_list.cc:178: if (loading_state_ == LOADING_STATE_UP_TO_DATE) I think this is kind of a weird API; it maybe sets something on an unexposed internal condition. Since this check is only used internally, maybe this method should just return void and just set loading_state_ instead. I defer to jam@ though.
https://codereview.chromium.org/128773002/diff/270001/content/browser/plugin_... File content/browser/plugin_loader_posix.cc (right): https://codereview.chromium.org/128773002/diff/270001/content/browser/plugin_... content/browser/plugin_loader_posix.cc:146: const std::vector<WebPluginInfo>& /* plugins */) { On 2014/01/21 20:58:18, rsesek wrote: > nit: I don't think we typically omit parameters Hmm... the style guide does say that unused parameters should be commented out (http://goo.gl/LN96S2)/. In this case in particular, I want to make it clear that the value passed to us shouldn't be used. Rename it to |plugins_unused|? https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... File content/common/plugin_list.cc (right): https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... content/common/plugin_list.cc:178: if (loading_state_ == LOADING_STATE_UP_TO_DATE) On 2014/01/21 20:58:18, rsesek wrote: > I think this is kind of a weird API; it maybe sets something on an unexposed > internal condition. Since this check is only used internally, maybe this method > should just return void and just set loading_state_ instead. I defer to jam@ > though. Well, what happens to the state internally is not part of the API. This is really just the initial part of LoadPlugins() extracted to a separate method, so we can reuse it if we load plugins asynchronously.
John, can you take a look? https://codereview.chromium.org/128773002/diff/270001/content/browser/plugin_... File content/browser/plugin_loader_posix.cc (right): https://codereview.chromium.org/128773002/diff/270001/content/browser/plugin_... content/browser/plugin_loader_posix.cc:146: const std::vector<WebPluginInfo>& /* plugins */) { On 2014/01/21 23:02:11, Bernhard Bauer wrote: > On 2014/01/21 20:58:18, rsesek wrote: > > nit: I don't think we typically omit parameters > > Hmm... the style guide does say that unused parameters should be commented out > (http://goo.gl/LN96S2%29/. In this case in particular, I want to make it clear > that the value passed to us shouldn't be used. Rename it to |plugins_unused|? Renamed.
https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... File content/common/plugin_list.cc (right): https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... content/common/plugin_list.cc:178: if (loading_state_ == LOADING_STATE_UP_TO_DATE) On 2014/01/21 23:02:11, Bernhard Bauer wrote: > On 2014/01/21 20:58:18, rsesek wrote: > > I think this is kind of a weird API; it maybe sets something on an unexposed > > internal condition. Since this check is only used internally, maybe this > method > > should just return void and just set loading_state_ instead. I defer to jam@ > > though. > > Well, what happens to the state internally is not part of the API. This is > really just the initial part of LoadPlugins() extracted to a separate method, so > we can reuse it if we load plugins asynchronously. yes it's a bid odd to see this method that the caller has to call before LoadPlugins. Can we get rid of it or hide it so that it's a private implementation of this class?
https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... File content/common/plugin_list.cc (right): https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... content/common/plugin_list.cc:178: if (loading_state_ == LOADING_STATE_UP_TO_DATE) On 2014/01/23 01:43:09, jam wrote: > On 2014/01/21 23:02:11, Bernhard Bauer wrote: > > On 2014/01/21 20:58:18, rsesek wrote: > > > I think this is kind of a weird API; it maybe sets something on an unexposed > > > internal condition. Since this check is only used internally, maybe this > > method > > > should just return void and just set loading_state_ instead. I defer to jam@ > > > though. > > > > Well, what happens to the state internally is not part of the API. This is > > really just the initial part of LoadPlugins() extracted to a separate method, > so > > we can reuse it if we load plugins asynchronously. > > yes it's a bid odd to see this method that the caller has to call before > LoadPlugins. Can we get rid of it or hide it so that it's a private > implementation of this class? LoadPlugins() does call this method internally. A caller would either call LoadPlugins(), or call PrepareForPluginLoading(), load the plugins themselves, then call SetPlugins(). You and Robert seem to be concerned about two different things. Robert's concern was the internal check based on the state; I can indeed get rid of this and just set the state unconditionally. I just wanted to extract a common method to make out-of-process plugin loading more similar to in-process. Your concern is that callers have to call an additional method. There doesn't really seem to be a way around that, as we have to know when plugin loading starts (otherwise we can't distinguish the case where the plugin list is invalidated after we have starting loading them from the case where it is invalidated before). One thing I could possibly do is to have the client instead set a LoadPluginsCallback that would be called whenever the PluginList wants to load plugins, but that would mean that PluginList would now have to queue up the callbacks. Alternatively, I could move the state transition into the call to GetPluginsNoRefresh() (which we call before), but that would make the contract of that method really weird. SetPlugins() would be easier to get rid of: PrepareForPluginLoading could return a callback bound to it, to be called by the client with the list of plugins.
https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... File content/common/plugin_list.cc (right): https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... content/common/plugin_list.cc:178: if (loading_state_ == LOADING_STATE_UP_TO_DATE) On 2014/01/23 17:35:31, Bernhard Bauer wrote: > On 2014/01/23 01:43:09, jam wrote: > > On 2014/01/21 23:02:11, Bernhard Bauer wrote: > > > On 2014/01/21 20:58:18, rsesek wrote: > > > > I think this is kind of a weird API; it maybe sets something on an > unexposed > > > > internal condition. Since this check is only used internally, maybe this > > > method > > > > should just return void and just set loading_state_ instead. I defer to > jam@ > > > > though. > > > > > > Well, what happens to the state internally is not part of the API. This is > > > really just the initial part of LoadPlugins() extracted to a separate > method, > > so > > > we can reuse it if we load plugins asynchronously. > > > > yes it's a bid odd to see this method that the caller has to call before > > LoadPlugins. Can we get rid of it or hide it so that it's a private > > implementation of this class? > > LoadPlugins() does call this method internally. A caller would either call > LoadPlugins(), or call PrepareForPluginLoading(), load the plugins themselves, > then call SetPlugins(). > > You and Robert seem to be concerned about two different things. Robert's concern > was the internal check based on the state; I can indeed get rid of this and just > set the state unconditionally. I just wanted to extract a common method to make > out-of-process plugin loading more similar to in-process. > > Your concern is that callers have to call an additional method. There doesn't > really seem to be a way around that, as we have to know when plugin loading > starts (otherwise we can't distinguish the case where the plugin list is > invalidated after we have starting loading them from the case where it is > invalidated before). One thing I could possibly do is to have the client instead > set a LoadPluginsCallback that would be called whenever the PluginList wants to > load plugins, but that would mean that PluginList would now have to queue up the > callbacks. Alternatively, I could move the state transition into the call to > GetPluginsNoRefresh() (which we call before), but that would make the contract > of that method really weird. > > SetPlugins() would be easier to get rid of: PrepareForPluginLoading could return > a callback bound to it, to be called by the client with the list of plugins. We don't need to support refreshing the plugins if they get invalidated while we were fetching them. That adds a bit of complexity for not much benefit. For the single-renderer case, kPluginsRefreshThresholdInSeconds means that we will really almost always have loaded the plugins before we invalidate the list. For the multiple renderers case, if one renderer invalidated to cause a refresh, and another invalidate, then that is a race condition and the prior invalidation is good enough.
https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... File content/common/plugin_list.cc (right): https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... content/common/plugin_list.cc:178: if (loading_state_ == LOADING_STATE_UP_TO_DATE) On 2014/01/23 17:57:47, jam wrote: > On 2014/01/23 17:35:31, Bernhard Bauer wrote: > > On 2014/01/23 01:43:09, jam wrote: > > > On 2014/01/21 23:02:11, Bernhard Bauer wrote: > > > > On 2014/01/21 20:58:18, rsesek wrote: > > > > > I think this is kind of a weird API; it maybe sets something on an > > unexposed > > > > > internal condition. Since this check is only used internally, maybe this > > > > method > > > > > should just return void and just set loading_state_ instead. I defer to > > jam@ > > > > > though. > > > > > > > > Well, what happens to the state internally is not part of the API. This is > > > > really just the initial part of LoadPlugins() extracted to a separate > > method, > > > so > > > > we can reuse it if we load plugins asynchronously. > > > > > > yes it's a bid odd to see this method that the caller has to call before > > > LoadPlugins. Can we get rid of it or hide it so that it's a private > > > implementation of this class? > > > > LoadPlugins() does call this method internally. A caller would either call > > LoadPlugins(), or call PrepareForPluginLoading(), load the plugins themselves, > > then call SetPlugins(). > > > > You and Robert seem to be concerned about two different things. Robert's > concern > > was the internal check based on the state; I can indeed get rid of this and > just > > set the state unconditionally. I just wanted to extract a common method to > make > > out-of-process plugin loading more similar to in-process. > > > > Your concern is that callers have to call an additional method. There doesn't > > really seem to be a way around that, as we have to know when plugin loading > > starts (otherwise we can't distinguish the case where the plugin list is > > invalidated after we have starting loading them from the case where it is > > invalidated before). One thing I could possibly do is to have the client > instead > > set a LoadPluginsCallback that would be called whenever the PluginList wants > to > > load plugins, but that would mean that PluginList would now have to queue up > the > > callbacks. Alternatively, I could move the state transition into the call to > > GetPluginsNoRefresh() (which we call before), but that would make the contract > > of that method really weird. > > > > SetPlugins() would be easier to get rid of: PrepareForPluginLoading could > return > > a callback bound to it, to be called by the client with the list of plugins. > > We don't need to support refreshing the plugins if they get invalidated while we > were fetching them. That adds a bit of complexity for not much benefit. For the > single-renderer case, kPluginsRefreshThresholdInSeconds means that we will > really almost always have loaded the plugins before we invalidate the list. For > the multiple renderers case, if one renderer invalidated to cause a refresh, and > another invalidate, then that is a race condition and the prior invalidation is > good enough. Renderers aren't the only thing that can invalidate the plugin list. This is not just an unlikely edge case; see http://crbug.com/117561 for the bug that prompted the initial change.
On 2014/01/23 18:17:46, Bernhard Bauer wrote: > https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... > File content/common/plugin_list.cc (right): > > https://codereview.chromium.org/128773002/diff/270001/content/common/plugin_l... > content/common/plugin_list.cc:178: if (loading_state_ == > LOADING_STATE_UP_TO_DATE) > On 2014/01/23 17:57:47, jam wrote: > > On 2014/01/23 17:35:31, Bernhard Bauer wrote: > > > On 2014/01/23 01:43:09, jam wrote: > > > > On 2014/01/21 23:02:11, Bernhard Bauer wrote: > > > > > On 2014/01/21 20:58:18, rsesek wrote: > > > > > > I think this is kind of a weird API; it maybe sets something on an > > > unexposed > > > > > > internal condition. Since this check is only used internally, maybe > this > > > > > method > > > > > > should just return void and just set loading_state_ instead. I defer > to > > > jam@ > > > > > > though. > > > > > > > > > > Well, what happens to the state internally is not part of the API. This > is > > > > > really just the initial part of LoadPlugins() extracted to a separate > > > method, > > > > so > > > > > we can reuse it if we load plugins asynchronously. > > > > > > > > yes it's a bid odd to see this method that the caller has to call before > > > > LoadPlugins. Can we get rid of it or hide it so that it's a private > > > > implementation of this class? > > > > > > LoadPlugins() does call this method internally. A caller would either call > > > LoadPlugins(), or call PrepareForPluginLoading(), load the plugins > themselves, > > > then call SetPlugins(). > > > > > > You and Robert seem to be concerned about two different things. Robert's > > concern > > > was the internal check based on the state; I can indeed get rid of this and > > just > > > set the state unconditionally. I just wanted to extract a common method to > > make > > > out-of-process plugin loading more similar to in-process. > > > > > > Your concern is that callers have to call an additional method. There > doesn't > > > really seem to be a way around that, as we have to know when plugin loading > > > starts (otherwise we can't distinguish the case where the plugin list is > > > invalidated after we have starting loading them from the case where it is > > > invalidated before). One thing I could possibly do is to have the client > > instead > > > set a LoadPluginsCallback that would be called whenever the PluginList wants > > to > > > load plugins, but that would mean that PluginList would now have to queue up > > the > > > callbacks. Alternatively, I could move the state transition into the call to > > > GetPluginsNoRefresh() (which we call before), but that would make the > contract > > > of that method really weird. > > > > > > SetPlugins() would be easier to get rid of: PrepareForPluginLoading could > > return > > > a callback bound to it, to be called by the client with the list of plugins. > > > > We don't need to support refreshing the plugins if they get invalidated while > we > > were fetching them. That adds a bit of complexity for not much benefit. For > the > > single-renderer case, kPluginsRefreshThresholdInSeconds means that we will > > really almost always have loaded the plugins before we invalidate the list. > For > > the multiple renderers case, if one renderer invalidated to cause a refresh, > and > > another invalidate, then that is a race condition and the prior invalidation > is > > good enough. > > Renderers aren't the only thing that can invalidate the plugin list. This is not > just an unlikely edge case; see http://crbug.com/117561 for the bug that > prompted the initial change. I'm not following exactly which things you're referring to. I'm referring to the fact that if the plugin list is invalidated, whether that's from a renderer or extension code, and we're already fetching from disk, no need to start another fetch and the given callback can be called when the existing load finishes. If there's something wrong with this specific logic, please describe it in more detail.
On 2014/01/23 21:17:47, jam wrote: > I'm not following exactly which things you're referring to. I'm referring to the > fact that if the plugin list is invalidated, whether that's from a renderer or > extension code, and we're already fetching from disk, no need to start another > fetch and the given callback can be called when the existing load finishes. If > there's something wrong with this specific logic, please describe it in more > detail. Take the following sequence of calls on PluginList: 1. RemoveExtraPluginPath(plugin) 2. RefreshPlugins() 3. GetPlugins() 4. AddExtraPluginPath(plugin) 5. RefreshPlugins() 6. GetPlugins() (This is what happens if an extension bundling a plugin is reloaded: First it is disabled, removing the plugin, then it's enabled again, adding the plugin. The GetPlugins() calls are from the renderers.) In step 3 we start loading plugins, and the loaded list of plugins will not contain |plugin|, so the first request to GetPlugins() will be fulfilled without |plugin|. If plugin loading doesn't finish by the time we reach step 6, we will fulfil that request with the same list of plugins that does not contain |plugin|, even though the plugin was added before step 6. Also, because we cache the result, all subsequent requests to GetPlugins() will return that list too. We have effectively lost |plugin|.
On 2014/01/23 21:48:52, Bernhard Bauer wrote: > On 2014/01/23 21:17:47, jam wrote: > > I'm not following exactly which things you're referring to. I'm referring to > the > > fact that if the plugin list is invalidated, whether that's from a renderer or > > extension code, and we're already fetching from disk, no need to start another > > fetch and the given callback can be called when the existing load finishes. If > > there's something wrong with this specific logic, please describe it in more > > detail. > > Take the following sequence of calls on PluginList: > 1. RemoveExtraPluginPath(plugin) > 2. RefreshPlugins() > 3. GetPlugins() > 4. AddExtraPluginPath(plugin) > 5. RefreshPlugins() > 6. GetPlugins() > > (This is what happens if an extension bundling a plugin is reloaded: First it is > disabled, removing the plugin, then it's enabled again, adding the plugin. The > GetPlugins() calls are from the renderers.) > > In step 3 we start loading plugins, and the loaded list of plugins will not > contain |plugin|, so the first request to GetPlugins() will be fulfilled without > |plugin|. If plugin loading doesn't finish by the time we reach step 6, we will > fulfil that request with the same list of plugins that does not contain > |plugin|, even though the plugin was added before step 6. Also, because we cache > the result, all subsequent requests to GetPlugins() will return that list too. > We have effectively lost |plugin|. I see, ok lgtm. I guess I shouldn't nit so much since this code path is only used on Mac now and will go away when we drop NPAPI support soon.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bauerb@chromium.org/128773002/330001
Retried try job too often on mac_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bauerb@chromium.org/128773002/700001
Message was sent while issue was closed.
Change committed as 247252 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
