Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3717005:
This adds a plugin selection policy for selecting allowed plugins (Closed)
|
Created:
10 years, 2 months ago by Greg Spencer (Chromium) Modified:
9 years, 7 months ago CC:
chromium-reviews, ben+cc_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, stuartmorgan+watch_chromium.org, Paweł Hajdan Jr., davemoore+watch_chromium.org, jam Visibility:
Public. |
DescriptionThis adds a plugin selection policy for selecting which plugin is
allowed for a particular domain.
It is only used on ChromeOS. It reads from a file that is installed
in a known location on ChromeOS, and uses that as it's policy.
When there are multiple plugins supporting the same mime-type, the
appropriate plugin file to load is now selected based on policy.
BUG=http://crosbug.com/7403
TEST=ran new unit test, tested on device.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=62679
Patch Set 1 #
Total comments: 8
Patch Set 2 : Test changes #Patch Set 3 : changed ASSERT_TRUE to ASSERT_GT #Patch Set 4 : Reworked so policy is only in browser, etc. #Patch Set 5 : merging to TOT, reverting gypi sorts #Patch Set 6 : some cleanup #Patch Set 7 : Fixing tasks #
Total comments: 18
Patch Set 8 : finally ready, I hope. #Patch Set 9 : uploading after merge to TOT #
Total comments: 1
Patch Set 10 : fix win build #
Total comments: 1
Messages
Total messages: 29 (0 generated)
file on both the browser IO thread is really frowned upon for performance reason. this is why we have the file thread. it's also really bad to read this file off disk each time a RenderView is created, considering this file never changes. also i'm curious how this works on chrome os, is that file path allowed through the sandbox? we've been very careful to not do file access on threads that are involved on the UI (i.e. browser UI/IO threads, renderer main thread) to keep chrome responsive. please maintain this. On Tue, Oct 12, 2010 at 3:41 PM, <gspencer@chromium.org> wrote: > Reviewers: piman, > > Description: > This adds a plugin selection policy for selecting which plugin is > allowed for a particular domain. > > It is only used on ChromeOS. It reads from a file that is installed > in a known location on ChromeOS, and uses that as it's policy. > > When there are multiple plugins supporting the same mime-type, the > appropriate plugin file to load is now selected based on policy. > > BUG=http://crosbug.com/7403 > TEST=ran new unit test, tested on device. > > Please review this at http://codereview.chromium.org/3717005/show > > Affected files: > M chrome/browser/plugin_service.h > M chrome/browser/plugin_service.cc > M chrome/chrome_common.gypi > M chrome/chrome_tests.gypi > A chrome/common/plugin_selection_policy.h > A chrome/common/plugin_selection_policy.cc > A chrome/common/plugin_selection_policy_unittest.cc > M chrome/renderer/render_view.h > M chrome/renderer/render_view.cc > > >
http://codereview.chromium.org/3717005/diff/1/2 File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/3717005/diff/1/2#newcode63 chrome/browser/plugin_service.cc:63: // reads from a policy file. Shouldn't this be the FILE thread ? http://codereview.chromium.org/3717005/diff/1/2#newcode280 chrome/browser/plugin_service.cc:280: int allowed_index = plugin_selection_policy_->FindFirstAllowed(url, info); Isn't there a race condition ? Since the initialization is done on a different thread, this could be run before it's initialized, couldn't it ? I'm particularly worried about a mismatch between the renderer and the browser... http://codereview.chromium.org/3717005/diff/1/6 File chrome/common/plugin_selection_policy.cc (right): http://codereview.chromium.org/3717005/diff/1/6#newcode32 chrome/common/plugin_selection_policy.cc:32: "/usr/share/chromeos-assets/flash/flapper"; flapper -> plugin_policy ? http://codereview.chromium.org/3717005/diff/1/9 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/3717005/diff/1/9#newcode478 chrome/renderer/render_view.cc:478: plugin_selection_policy_->Init(); How does this work from inside the sandbox ?
On 2010/10/12 22:55:06, John Abd-El-Malek wrote: > file on both the browser IO thread is really frowned upon for performance > reason. this is why we have the file thread. Sorry, I thought the IO thread *was* the thread to do file IO on. It's called the IO thread after all, and it appeared to me to be used for that purpose in other places. I must have been mistaken. I'll use the FILE thread instead. > it's also really bad to read this file off disk each time a RenderView is > created, considering this file never changes. also i'm curious how this > works on chrome os, is that file path allowed through the sandbox? Good points, I'll try and find another way. Any suggestions? > we've been very careful to not do file access on threads that are involved > on the UI (i.e. browser UI/IO threads, renderer main thread) to keep chrome > responsive. please maintain this. I was attempting to maintain it by using the IO thread (because I thought that's what it was for). The last thing I want to do is make chrome janky.
On Tue, Oct 12, 2010 at 4:10 PM, <gspencer@chromium.org> wrote: > On 2010/10/12 22:55:06, John Abd-El-Malek wrote: > >> file on both the browser IO thread is really frowned upon for performance >> reason. this is why we have the file thread. >> > > Sorry, I thought the IO thread *was* the thread to do file IO on. > It's called the IO thread after all, and it appeared to me to be used for > that purpose in other places. yeah, the name is a little confusing. it's for networking IO only. see http://dev.chromium.org/developers/design-documents/threading for background If you find any examples, please let me know :) This shouldn't be happening. > I must have been mistaken. > > I'll use the FILE thread instead. > > it's also really bad to read this file off disk each time a RenderView is >> created, considering this file never changes. also i'm curious how this >> works on chrome os, is that file path allowed through the sandbox? >> > > Good points, I'll try and find another way. Any suggestions? > Well, I think this logic should all be in the browser, and if the plugin doesn't load, no need to fallback. this way we don't complicate the code for a situation that will never happened in a released build (i.e. out-of-sync flash plugin). But Antoine disagrees with me. > > we've been very careful to not do file access on threads that are involved >> on the UI (i.e. browser UI/IO threads, renderer main thread) to keep >> chrome >> responsive. please maintain this. >> > > I was attempting to maintain it by using the IO thread (because I thought > that's what it was for). The last thing I want to do is make chrome janky. > > > http://codereview.chromium.org/3717005/show >
http://codereview.chromium.org/3717005/diff/1/2 File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/3717005/diff/1/2#newcode63 chrome/browser/plugin_service.cc:63: // reads from a policy file. On 2010/10/12 23:05:29, piman wrote: > Shouldn't this be the FILE thread ? Yes, I'm well aware of that now.. :) http://codereview.chromium.org/3717005/diff/1/2#newcode280 chrome/browser/plugin_service.cc:280: int allowed_index = plugin_selection_policy_->FindFirstAllowed(url, info); On 2010/10/12 23:05:29, piman wrote: > Isn't there a race condition ? Since the initialization is done on a different > thread, this could be run before it's initialized, couldn't it ? I'm > particularly worried about a mismatch between the renderer and the browser... Well it couldn't be run before it starts to initialize. It could run before it's done, however. There is a lock, but it probably should lock the entire Init function -- right now it just locks the final swap. As for the renderer/browser sync issue, I think I have a solution to that, which involves creating a new ViewHostMsg that will let the render view have a copy of the policy that the browser loaded. That way there isn't a sandbox issue, and it can init when it starts with the same data as the browser.
On Tue, Oct 12, 2010 at 5:03 PM, <gspencer@chromium.org> wrote: > > http://codereview.chromium.org/3717005/diff/1/2 > File chrome/browser/plugin_service.cc (right): > > http://codereview.chromium.org/3717005/diff/1/2#newcode63 > chrome/browser/plugin_service.cc:63: // reads from a policy file. > On 2010/10/12 23:05:29, piman wrote: > >> Shouldn't this be the FILE thread ? >> > > Yes, I'm well aware of that now.. :) > > > http://codereview.chromium.org/3717005/diff/1/2#newcode280 > chrome/browser/plugin_service.cc:280: int allowed_index = > plugin_selection_policy_->FindFirstAllowed(url, info); > On 2010/10/12 23:05:29, piman wrote: > >> Isn't there a race condition ? Since the initialization is done on a >> > different > >> thread, this could be run before it's initialized, couldn't it ? I'm >> particularly worried about a mismatch between the renderer and the >> > browser... > > Well it couldn't be run before it starts to initialize. It could run > before it's done, however. There is a lock, but it probably should > lock the entire Init function -- right now it just locks the final swap. I don't think that's necessary (the member accesses looks safe), and I don't think it will solve the problem in the general case (I'm worried about calling FindFirstAllowed before Init is even run, which will allow plugins to skip the white/black list). > As for the renderer/browser sync issue, I think I have a solution to > that, which involves creating a new ViewHostMsg that will let the render > view have a copy of the policy that the browser loaded. That way there > isn't a sandbox issue, and it can init when it starts with the same data > as the browser. It sounds reasonable. > > > http://codereview.chromium.org/3717005/show >
On Tue, Oct 12, 2010 at 5:03 PM, <gspencer@chromium.org> wrote: > > http://codereview.chromium.org/3717005/diff/1/2 > File chrome/browser/plugin_service.cc (right): > > http://codereview.chromium.org/3717005/diff/1/2#newcode63 > chrome/browser/plugin_service.cc:63: // reads from a policy file. > On 2010/10/12 23:05:29, piman wrote: > >> Shouldn't this be the FILE thread ? >> > > Yes, I'm well aware of that now.. :) > > > http://codereview.chromium.org/3717005/diff/1/2#newcode280 > chrome/browser/plugin_service.cc:280: int allowed_index = > plugin_selection_policy_->FindFirstAllowed(url, info); > On 2010/10/12 23:05:29, piman wrote: > >> Isn't there a race condition ? Since the initialization is done on a >> > different > >> thread, this could be run before it's initialized, couldn't it ? I'm >> particularly worried about a mismatch between the renderer and the >> > browser... > > Well it couldn't be run before it starts to initialize. It could run > before it's done, however. There is a lock, but it probably should > lock the entire Init function -- right now it just locks the final swap. > > As for the renderer/browser sync issue, I think I have a solution to > that, which involves creating a new ViewHostMsg that will let the render > view have a copy of the policy that the browser loaded. That way there > isn't a sandbox issue, and it can init when it starts with the same data > as the browser. This really sounds more complicated than it needs to be. Why not just make the decision in the browser? The only reason I've heard so far is that the renderer can run normal flash if the pepper one won't run because of a pepper api mismatch. This won't happen in the field though, so why add code to support something that won't happen? > > > http://codereview.chromium.org/3717005/show >
On Tue, Oct 12, 2010 at 5:20 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Tue, Oct 12, 2010 at 5:03 PM, <gspencer@chromium.org> wrote: > >> >> http://codereview.chromium.org/3717005/diff/1/2 >> File chrome/browser/plugin_service.cc (right): >> >> http://codereview.chromium.org/3717005/diff/1/2#newcode63 >> chrome/browser/plugin_service.cc:63: // reads from a policy file. >> On 2010/10/12 23:05:29, piman wrote: >> >>> Shouldn't this be the FILE thread ? >>> >> >> Yes, I'm well aware of that now.. :) >> >> >> http://codereview.chromium.org/3717005/diff/1/2#newcode280 >> chrome/browser/plugin_service.cc:280: int allowed_index = >> plugin_selection_policy_->FindFirstAllowed(url, info); >> On 2010/10/12 23:05:29, piman wrote: >> >>> Isn't there a race condition ? Since the initialization is done on a >>> >> different >> >>> thread, this could be run before it's initialized, couldn't it ? I'm >>> particularly worried about a mismatch between the renderer and the >>> >> browser... >> >> Well it couldn't be run before it starts to initialize. It could run >> before it's done, however. There is a lock, but it probably should >> lock the entire Init function -- right now it just locks the final swap. >> >> As for the renderer/browser sync issue, I think I have a solution to >> that, which involves creating a new ViewHostMsg that will let the render >> view have a copy of the policy that the browser loaded. That way there >> isn't a sandbox issue, and it can init when it starts with the same data >> as the browser. > > > This really sounds more complicated than it needs to be. Why not just make > the decision in the browser? > How's that simpler ? What happens if the browser decides to choose the pepper plugin ? Today it's not involved in creating the pepper one, so you'd need to add at least a couple of messages for it to work. In fact, I'd rather have the opposite, get the renderer to make the choice and tell the browser. But either of those necessitate significant refactoring, because the data of which actual plugin to create (as in, its path as opposed to its mime type) gets lost as control goes to webkit. The only reason I've heard so far is that the renderer can run normal flash > if the pepper one won't run because of a pepper api mismatch. This won't > happen in the field though, so why add code to support something that won't > happen? > > >> >> http://codereview.chromium.org/3717005/show >> > >
On Tue, Oct 12, 2010 at 5:09 PM, Antoine Labour <piman@chromium.org> wrote: > I don't think that's necessary (the member accesses looks safe), and I > don't think it will solve the problem in the general case (I'm worried about > calling FindFirstAllowed before Init is even run, which will allow plugins > to skip the white/black list). > OK, I'll think some more about how this should be structured.
On Tue, Oct 12, 2010 at 5:32 PM, Antoine Labour <piman@chromium.org> wrote: > > > On Tue, Oct 12, 2010 at 5:20 PM, John Abd-El-Malek <jam@chromium.org>wrote: > >> >> >> On Tue, Oct 12, 2010 at 5:03 PM, <gspencer@chromium.org> wrote: >> >>> >>> http://codereview.chromium.org/3717005/diff/1/2 >>> File chrome/browser/plugin_service.cc (right): >>> >>> http://codereview.chromium.org/3717005/diff/1/2#newcode63 >>> chrome/browser/plugin_service.cc:63: // reads from a policy file. >>> On 2010/10/12 23:05:29, piman wrote: >>> >>>> Shouldn't this be the FILE thread ? >>>> >>> >>> Yes, I'm well aware of that now.. :) >>> >>> >>> http://codereview.chromium.org/3717005/diff/1/2#newcode280 >>> chrome/browser/plugin_service.cc:280: int allowed_index = >>> plugin_selection_policy_->FindFirstAllowed(url, info); >>> On 2010/10/12 23:05:29, piman wrote: >>> >>>> Isn't there a race condition ? Since the initialization is done on a >>>> >>> different >>> >>>> thread, this could be run before it's initialized, couldn't it ? I'm >>>> particularly worried about a mismatch between the renderer and the >>>> >>> browser... >>> >>> Well it couldn't be run before it starts to initialize. It could run >>> before it's done, however. There is a lock, but it probably should >>> lock the entire Init function -- right now it just locks the final swap. >>> >>> As for the renderer/browser sync issue, I think I have a solution to >>> that, which involves creating a new ViewHostMsg that will let the render >>> view have a copy of the policy that the browser loaded. That way there >>> isn't a sandbox issue, and it can init when it starts with the same data >>> as the browser. >> >> >> This really sounds more complicated than it needs to be. Why not just >> make the decision in the browser? >> > > How's that simpler ? What happens if the browser decides to choose the > pepper plugin ? Today it's not involved in creating the pepper one, so you'd > need to add at least a couple of messages for it to work. > I don't understand this. Doesn't RenderView just create a plugin based on the path? so if it gets a path to a pepper plugin, PepperPluginRegistry::GetInstance()->GetModule will return a module which is then used. > In fact, I'd rather have the opposite, get the renderer to make the choice > and tell the browser. > > But either of those necessitate significant refactoring, because the data > of which actual plugin to create (as in, its path as opposed to its mime > type) gets lost as control goes to webkit. > > The only reason I've heard so far is that the renderer can run normal flash >> if the pepper one won't run because of a pepper api mismatch. This won't >> happen in the field though, so why add code to support something that won't >> happen? >> > >> > >>> >>> http://codereview.chromium.org/3717005/show >>> >> >> >
Drive-by with a test comment. http://codereview.chromium.org/3717005/diff/1/8 File chrome/common/plugin_selection_policy_unittest.cc (right): http://codereview.chromium.org/3717005/diff/1/8#newcode34 chrome/common/plugin_selection_policy_unittest.cc:34: file_util::WriteFile(policy_file, contents.c_str(), contents.size()); Check return value of this.
btw, the more i think about this, the more I'm against having both the browser and renderer processes knowing about the fact that getplugininfo can return more than one plugin. this is a hack just for the beta build. the only reason i've heard so far for adding this knowledge in the renderer is as a fall-back in case we ship mismatched versions of pepper/flash. given that this won't happen for dev channel builds, i really don't think we need to make this temporary feature more complicated than it has to be. if wrong version of flash is shipped on a dev channel release of chrome os, then too bad, and pepper flash shouldn't load. On Tue, Oct 12, 2010 at 6:14 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Tue, Oct 12, 2010 at 5:32 PM, Antoine Labour <piman@chromium.org>wrote: > >> >> >> On Tue, Oct 12, 2010 at 5:20 PM, John Abd-El-Malek <jam@chromium.org>wrote: >> >>> >>> >>> On Tue, Oct 12, 2010 at 5:03 PM, <gspencer@chromium.org> wrote: >>> >>>> >>>> http://codereview.chromium.org/3717005/diff/1/2 >>>> File chrome/browser/plugin_service.cc (right): >>>> >>>> http://codereview.chromium.org/3717005/diff/1/2#newcode63 >>>> chrome/browser/plugin_service.cc:63: // reads from a policy file. >>>> On 2010/10/12 23:05:29, piman wrote: >>>> >>>>> Shouldn't this be the FILE thread ? >>>>> >>>> >>>> Yes, I'm well aware of that now.. :) >>>> >>>> >>>> http://codereview.chromium.org/3717005/diff/1/2#newcode280 >>>> chrome/browser/plugin_service.cc:280: int allowed_index = >>>> plugin_selection_policy_->FindFirstAllowed(url, info); >>>> On 2010/10/12 23:05:29, piman wrote: >>>> >>>>> Isn't there a race condition ? Since the initialization is done on a >>>>> >>>> different >>>> >>>>> thread, this could be run before it's initialized, couldn't it ? I'm >>>>> particularly worried about a mismatch between the renderer and the >>>>> >>>> browser... >>>> >>>> Well it couldn't be run before it starts to initialize. It could run >>>> before it's done, however. There is a lock, but it probably should >>>> lock the entire Init function -- right now it just locks the final swap. >>>> >>>> As for the renderer/browser sync issue, I think I have a solution to >>>> that, which involves creating a new ViewHostMsg that will let the render >>>> view have a copy of the policy that the browser loaded. That way there >>>> isn't a sandbox issue, and it can init when it starts with the same data >>>> as the browser. >>> >>> >>> This really sounds more complicated than it needs to be. Why not just >>> make the decision in the browser? >>> >> >> How's that simpler ? What happens if the browser decides to choose the >> pepper plugin ? Today it's not involved in creating the pepper one, so you'd >> need to add at least a couple of messages for it to work. >> > > I don't understand this. Doesn't RenderView just create a plugin based on > the path? so if it gets a path to a pepper plugin, > PepperPluginRegistry::GetInstance()->GetModule will return a module which is > then used. > >> In fact, I'd rather have the opposite, get the renderer to make the choice >> and tell the browser. >> >> But either of those necessitate significant refactoring, because the data >> of which actual plugin to create (as in, its path as opposed to its mime >> type) gets lost as control goes to webkit. >> >> The only reason I've heard so far is that the renderer can run normal >>> flash if the pepper one won't run because of a pepper api mismatch. This >>> won't happen in the field though, so why add code to support something that >>> won't happen? >>> >> >>> >> >>>> >>>> http://codereview.chromium.org/3717005/show >>>> >>> >>> >> >
On Wed, Oct 13, 2010 at 10:34 AM, John Abd-El-Malek <jam@chromium.org>wrote: > btw, the more i think about this, the more I'm against having both the > browser and renderer processes knowing about the fact that getplugininfo can > return more than one plugin. this is a hack just for the beta build. the > only reason i've heard so far for adding this knowledge in the renderer is > as a fall-back in case we ship mismatched versions of pepper/flash. given > that this won't happen for dev channel builds, this should read: widely shipped builds > i really don't think we need to make this temporary feature more > complicated than it has to be. if wrong version of flash is shipped on a > dev channel release of chrome os, then too bad, and pepper flash shouldn't > load. > > On Tue, Oct 12, 2010 at 6:14 PM, John Abd-El-Malek <jam@chromium.org>wrote: > >> >> >> On Tue, Oct 12, 2010 at 5:32 PM, Antoine Labour <piman@chromium.org>wrote: >> >>> >>> >>> On Tue, Oct 12, 2010 at 5:20 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>> >>>> >>>> >>>> On Tue, Oct 12, 2010 at 5:03 PM, <gspencer@chromium.org> wrote: >>>> >>>>> >>>>> http://codereview.chromium.org/3717005/diff/1/2 >>>>> File chrome/browser/plugin_service.cc (right): >>>>> >>>>> http://codereview.chromium.org/3717005/diff/1/2#newcode63 >>>>> chrome/browser/plugin_service.cc:63: // reads from a policy file. >>>>> On 2010/10/12 23:05:29, piman wrote: >>>>> >>>>>> Shouldn't this be the FILE thread ? >>>>>> >>>>> >>>>> Yes, I'm well aware of that now.. :) >>>>> >>>>> >>>>> http://codereview.chromium.org/3717005/diff/1/2#newcode280 >>>>> chrome/browser/plugin_service.cc:280: int allowed_index = >>>>> plugin_selection_policy_->FindFirstAllowed(url, info); >>>>> On 2010/10/12 23:05:29, piman wrote: >>>>> >>>>>> Isn't there a race condition ? Since the initialization is done on a >>>>>> >>>>> different >>>>> >>>>>> thread, this could be run before it's initialized, couldn't it ? I'm >>>>>> particularly worried about a mismatch between the renderer and the >>>>>> >>>>> browser... >>>>> >>>>> Well it couldn't be run before it starts to initialize. It could run >>>>> before it's done, however. There is a lock, but it probably should >>>>> lock the entire Init function -- right now it just locks the final >>>>> swap. >>>>> >>>>> As for the renderer/browser sync issue, I think I have a solution to >>>>> that, which involves creating a new ViewHostMsg that will let the >>>>> render >>>>> view have a copy of the policy that the browser loaded. That way there >>>>> isn't a sandbox issue, and it can init when it starts with the same >>>>> data >>>>> as the browser. >>>> >>>> >>>> This really sounds more complicated than it needs to be. Why not just >>>> make the decision in the browser? >>>> >>> >>> How's that simpler ? What happens if the browser decides to choose the >>> pepper plugin ? Today it's not involved in creating the pepper one, so you'd >>> need to add at least a couple of messages for it to work. >>> >> >> I don't understand this. Doesn't RenderView just create a plugin based on >> the path? so if it gets a path to a pepper plugin, >> PepperPluginRegistry::GetInstance()->GetModule will return a module which is >> then used. >> >>> In fact, I'd rather have the opposite, get the renderer to make the >>> choice and tell the browser. >>> >>> But either of those necessitate significant refactoring, because the data >>> of which actual plugin to create (as in, its path as opposed to its mime >>> type) gets lost as control goes to webkit. >>> >>> The only reason I've heard so far is that the renderer can run normal >>>> flash if the pepper one won't run because of a pepper api mismatch. This >>>> won't happen in the field though, so why add code to support something that >>>> won't happen? >>>> >>> >>>> >>> >>>>> >>>>> http://codereview.chromium.org/3717005/show >>>>> >>>> >>>> >>> >> >
On 2010/10/13 17:36:40, John Abd-El-Malek wrote: > btw, the more i think about this, the more I'm against having both the > browser and renderer processes knowing about the fact that getplugininfo can > return more than one plugin. this is a hack just for the beta build. the > only reason i've heard so far for adding this knowledge in the renderer is > as a fall-back in case we ship mismatched versions of pepper/flash. given > that this won't happen for dev channel builds, i really don't think we need > to make this temporary feature more complicated than it has to be. if wrong > version of flash is shipped on a dev channel release of chrome os, then too > bad, and pepper flash shouldn't load. One solution I can think of is to add a ViewHostMsg that is essentially a call to the PluginSelectionPolicy class (in this CL) in the browser that will return a WebPluginInfo object for the renderer to use. It might be slower because of the need for an IPC. Do you have any other proposed solutions?
http://codereview.chromium.org/3717005/diff/1/8 File chrome/common/plugin_selection_policy_unittest.cc (right): http://codereview.chromium.org/3717005/diff/1/8#newcode34 chrome/common/plugin_selection_policy_unittest.cc:34: file_util::WriteFile(policy_file, contents.c_str(), contents.size()); On 2010/10/13 07:59:04, Paweł Hajdan Jr. wrote: > Check return value of this. Fixed. I was very surprised to find that ASSERT_* (really anything that uses GTEST_FATAL_FAILURE_) does a "return;", so that any function it is embedded in must return void. Is it even valid to call these within a helper function? It doesn't seem to actually do anything other than print something. In this case, it asserts eventually anyhow because I assert that the Init succeeds in each case, and it can't succeed with an empty file path. What is the preferred way to assert from a helper function in this case? Just use CHECK?
On Wed, Oct 13, 2010 at 11:13 AM, <gspencer@chromium.org> wrote: > On 2010/10/13 17:36:40, John Abd-El-Malek wrote: > >> btw, the more i think about this, the more I'm against having both the >> browser and renderer processes knowing about the fact that getplugininfo >> can >> return more than one plugin. this is a hack just for the beta build. the >> only reason i've heard so far for adding this knowledge in the renderer is >> as a fall-back in case we ship mismatched versions of pepper/flash. given >> that this won't happen for dev channel builds, i really don't think we >> need >> >> to make this temporary feature more complicated than it has to be. if >> wrong >> version of flash is shipped on a dev channel release of chrome os, then >> too >> bad, and pepper flash shouldn't load. >> > > One solution I can think of is to add a ViewHostMsg that is essentially a > call > to the PluginSelectionPolicy class (in this CL) in the browser that will > return > a WebPluginInfo object for the renderer to use. It might be slower because > of > the need for an IPC. > > Do you have any other proposed solutions? Why not just change ViewHostMsg_GetPluginInfoArray back to ViewHostMsg_GetPluginInfo, and have the callsite in the browser use PluginSelectionPolicy? > > > http://codereview.chromium.org/3717005/show >
On 2010/10/13 19:15:49, John Abd-El-Malek wrote: > Why not just change ViewHostMsg_GetPluginInfoArray back to > ViewHostMsg_GetPluginInfo, and have the callsite in the browser use > PluginSelectionPolicy? OK, I spoke with Antoine again, and he was OK with me implementing your suggestion, so that's what I'll do.
OK, I think I've got this reworked now, so please take another look. It moves the policy into the browser (under chromeos), and changes the call to a single object request for getting PluginInfo (as it used to be). I'm not done testing it on the device, but chromeos version of chrome works on my desktop, and the unit test passes. I'll test it on the device tomorrow.
Mostly LGTM with one thing. http://codereview.chromium.org/3717005/diff/31001/32001 File chrome/browser/chromeos/plugin_selection_policy.cc (right): http://codereview.chromium.org/3717005/diff/31001/32001#newcode65 chrome/browser/chromeos/plugin_selection_policy.cc:65: AutoLock lock(lock_); So, in practice, both InitFromFile and FindFirstAllowed/IsAllowed are called from the FILE thread. I would simply remove the lock and replace it with an assert that they're all called from that thread. I think it simplifies the thread logic overall.
http://codereview.chromium.org/3717005/diff/31001/32001 File chrome/browser/chromeos/plugin_selection_policy.cc (right): http://codereview.chromium.org/3717005/diff/31001/32001#newcode59 chrome/browser/chromeos/plugin_selection_policy.cc:59: return InitFromFile(FilePath(kPluginSelectionPolicyFile)); We don't want to do any file access on the IO thread, ever, since if the disk is busy with other IO, it won't be 1-2 ms and during that time all IPC will be blocked. Given that this code is called in response to a renderer request, and those will only happen after the plugin list has been fetched on the FILE thread, it seems that as long as you call StartInit() in browser initialization, the file task will be guaranteed to run before plugins are loaded on the FILE thread, and there is no race condition. http://codereview.chromium.org/3717005/diff/31001/32001#newcode76 chrome/browser/chromeos/plugin_selection_policy.cc:76: return false; should initialized_ be set to true at line 70? because if the file is missing, or the format isn't as expected, this function will attempt ot read it each time. http://codereview.chromium.org/3717005/diff/31001/32001#newcode92 chrome/browser/chromeos/plugin_selection_policy.cc:92: TrimString(line, kWhitespace, &line); can just use base::TrimWhitespaceASCII then you don't have to define kWhitespace http://codereview.chromium.org/3717005/diff/31001/32001#newcode150 chrome/browser/chromeos/plugin_selection_policy.cc:150: // called. See comments on Init above for more info. see above, we should not have the possibility of doing file access on the IO thread, or blocking the IO thread while the FILE thread is hitting the disk (same thing as the first condition), otherwise there's no point in offloading work to the file thread. if you must use a lock, then the file thread should only use it while it's updating the shared data structure (policies_), and not while it's reading it from disk. http://codereview.chromium.org/3717005/diff/31001/32002 File chrome/browser/chromeos/plugin_selection_policy.h (right): http://codereview.chromium.org/3717005/diff/31001/32002#newcode35 chrome/browser/chromeos/plugin_selection_policy.h:35: // Posts a task to call Init on the FILE thread. this comment is off (no task is posted from that function). also, to nit a little, it's better to comment a function on how it's used, not how it's implemented, since the latter can be seen by looking at its code, and it can go out of sync. http://codereview.chromium.org/3717005/diff/31001/32004 File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/3717005/diff/31001/32004#newcode271 chrome/browser/plugin_service.cc:271: if (!info.empty() && info[allowed_index].enabled) { need to guard against allowed_index being -1 http://codereview.chromium.org/3717005/diff/31001/32006 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/3717005/diff/31001/32006#newcode770 chrome/browser/renderer_host/resource_message_filter.cc:770: // Don't do it on the IO thread. why reformat this comment? http://codereview.chromium.org/3717005/diff/31001/32006#newcode789 chrome/browser/renderer_host/resource_message_filter.cc:789: WebPluginInfo allowed_info; why not have a function in PluginService to return a WebPluginInfo, and have that function contain the logic to cal into plugin_selection_policy? then you don't need this code duplicated here and in PluginService::OpenChannelToPlugin, and we can avoid having RMF knowing anything about PluginSelectionPolicy. also, enabled can set on the webplugininfo, that way OnGotPluginInfo doesn't need to call to PSP
http://codereview.chromium.org/3717005/diff/31001/32001 File chrome/browser/chromeos/plugin_selection_policy.cc (right): http://codereview.chromium.org/3717005/diff/31001/32001#newcode65 chrome/browser/chromeos/plugin_selection_policy.cc:65: AutoLock lock(lock_); On 2010/10/14 02:43:26, piman wrote: > So, in practice, both InitFromFile and FindFirstAllowed/IsAllowed are called > from the FILE thread. I would simply remove the lock and replace it with an > assert that they're all called from that thread. I think it simplifies the > thread logic overall. Man, I could have saved myself a couple hours and a lot of complexity if I had noticed that... Duh!
http://codereview.chromium.org/3717005/diff/31001/32001 File chrome/browser/chromeos/plugin_selection_policy.cc (right): http://codereview.chromium.org/3717005/diff/31001/32001#newcode65 chrome/browser/chromeos/plugin_selection_policy.cc:65: AutoLock lock(lock_); On 2010/10/14 17:59:07, Greg Spencer (Chromium) wrote: > On 2010/10/14 02:43:26, piman wrote: > > So, in practice, both InitFromFile and FindFirstAllowed/IsAllowed are called > > from the FILE thread. I would simply remove the lock and replace it with an > > assert that they're all called from that thread. I think it simplifies the > > thread logic overall. > > Man, I could have saved myself a couple hours and a lot of complexity if I had > noticed that... Duh! > this isn't true with the current code: PluginService::OpenChannelToPlugin calls FindFirstAllowed on the IO thread
On Thu, Oct 14, 2010 at 11:13 AM, <jam@chromium.org> wrote: > Man, I could have saved myself a couple hours and a lot of complexity >> > if I had > >> noticed that... Duh! > > > > this isn't true with the current code: > PluginService::OpenChannelToPlugin calls FindFirstAllowed on the IO > thread Yeah, I just ran into that... Grr. OK, so I'll leave the lock and threading stuff in there, and just work through the issues you pointed out. -Greg.
OK, I've done the following: - Removed the call to Init from IsAllowed - Removed the lock - Made sure that all calls to PluginSelectionPolicy occur on the FILE thread. - Split up OpenChannelToPlugin into three parts and call each one in the right thread. - Eliminated the 'locale' argument from OpenChannelToPlugin, since it was never used. - Added a function GetFirstAllowedPluginInfo to the PluginService that will do the right thing to get the first allowed plugin according to the rules and return a WebPluginInfo. Let's hope that's close enough. ;) http://codereview.chromium.org/3717005/diff/31001/32001 File chrome/browser/chromeos/plugin_selection_policy.cc (right): http://codereview.chromium.org/3717005/diff/31001/32001#newcode59 chrome/browser/chromeos/plugin_selection_policy.cc:59: return InitFromFile(FilePath(kPluginSelectionPolicyFile)); I've removed the lock, and the call to Init from IsAllowed, so this shouldn't be a problem anymore. http://codereview.chromium.org/3717005/diff/31001/32001#newcode65 chrome/browser/chromeos/plugin_selection_policy.cc:65: AutoLock lock(lock_); On 2010/10/14 18:13:03, John Abd-El-Malek wrote: > this isn't true with the current code: PluginService::OpenChannelToPlugin calls > FindFirstAllowed on the IO thread OK, so now it is true. I split up OpenChannelToPlugin into three functions and call the appropriate parts on each thread. http://codereview.chromium.org/3717005/diff/31001/32001#newcode76 chrome/browser/chromeos/plugin_selection_policy.cc:76: return false; On 2010/10/14 17:36:52, John Abd-El-Malek wrote: > should initialized_ be set to true at line 70? because if the file is missing, > or the format isn't as expected, this function will attempt ot read it each > time. Fixed by not calling Init from IsAllowed anymore. http://codereview.chromium.org/3717005/diff/31001/32001#newcode92 chrome/browser/chromeos/plugin_selection_policy.cc:92: TrimString(line, kWhitespace, &line); On 2010/10/14 17:36:52, John Abd-El-Malek wrote: > can just use base::TrimWhitespaceASCII then you don't have to define kWhitespace Cool, thanks. I looked for a function like this: I'm not really sure how I missed it. http://codereview.chromium.org/3717005/diff/31001/32001#newcode150 chrome/browser/chromeos/plugin_selection_policy.cc:150: // called. See comments on Init above for more info. I removed the call to Init here, and replaced it with a DCHECK, since we're currently guaranteed to finish the init on the FILE thread before a renderer can request a plugin, so I think we're OK. http://codereview.chromium.org/3717005/diff/31001/32004 File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/3717005/diff/31001/32004#newcode271 chrome/browser/plugin_service.cc:271: if (!info.empty() && info[allowed_index].enabled) { On 2010/10/14 17:36:52, John Abd-El-Malek wrote: > need to guard against allowed_index being -1 Done. http://codereview.chromium.org/3717005/diff/31001/32006 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/3717005/diff/31001/32006#newcode770 chrome/browser/renderer_host/resource_message_filter.cc:770: // Don't do it on the IO thread. On 2010/10/14 17:36:52, John Abd-El-Malek wrote: > why reformat this comment? For fun and profit. :) (and force of habit) Reverted.
lgtm!
LGTM with the one fix for MSVC http://codereview.chromium.org/3717005/diff/39001/10008 File chrome/browser/plugin_service.h (right): http://codereview.chromium.org/3717005/diff/39001/10008#newcode44 chrome/browser/plugin_service.h:44: class WebPluginInfo; class->struct (see trybot issue)
Could you do a follow-up CL? http://codereview.chromium.org/3717005/diff/48001/30014 File chrome/browser/chromeos/plugin_selection_policy_unittest.cc (right): http://codereview.chromium.org/3717005/diff/48001/30014#newcode89 chrome/browser/chromeos/plugin_selection_policy_unittest.cc:89: return bytes_written >= 0; This should be bytes_written == contents.size().
Yep, I can do that. -Greg. On Fri, Oct 15, 2010 at 12:17 AM, <phajdan.jr@chromium.org> wrote: > Could you do a follow-up CL? > > > http://codereview.chromium.org/3717005/diff/48001/30014 > File chrome/browser/chromeos/plugin_selection_policy_unittest.cc > (right): > > http://codereview.chromium.org/3717005/diff/48001/30014#newcode89 > chrome/browser/chromeos/plugin_selection_policy_unittest.cc:89: return > bytes_written >= 0; > This should be bytes_written == contents.size(). > > > http://codereview.chromium.org/3717005/show > |
