Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4832002:
Add PluginDataRemover. (Closed)
|
Created:
10 years, 1 month ago by Bernhard Bauer Modified:
9 years, 7 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, stuartmorgan+watch_chromium.org, ben+cc_chromium.org, danno Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd PluginDataRemover.
BUG=58235
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66615
Patch Set 1 #
Total comments: 6
Patch Set 2 : review #Patch Set 3 : fix race condition #Patch Set 4 : foo #
Messages
Total messages: 19 (0 generated)
Please review. http://codereview.chromium.org/4832002/diff/1/third_party/npapi/bindings/npapi.h File third_party/npapi/bindings/npapi.h (right): http://codereview.chromium.org/4832002/diff/1/third_party/npapi/bindings/npap... third_party/npapi/bindings/npapi.h:829: void NP_LOADDS NPP_LostFocus(NPP instance); I added these functions so the offsets in the NPFunctions struct correspond to what the Flash plugin uses. If we don't make use of the API in the host, it shouldn't be a problem, right?
http://codereview.chromium.org/4832002/diff/1/third_party/npapi/bindings/npapi.h File third_party/npapi/bindings/npapi.h (right): http://codereview.chromium.org/4832002/diff/1/third_party/npapi/bindings/npap... third_party/npapi/bindings/npapi.h:133: #define NP_VERSION_MINOR 25 Please don't make direct changes to this file. It's based on an upstream repository, I did a lot of work to get them mostly in sync, and it should not be made to diverge again with a very good reason. If you need a more recent version, you should update from upstream: http://code.google.com/p/npapi-headers first in a different change. If you don't want to do that, I can do it later today. http://codereview.chromium.org/4832002/diff/1/third_party/npapi/bindings/npfu... File third_party/npapi/bindings/npfunctions.h (right): http://codereview.chromium.org/4832002/diff/1/third_party/npapi/bindings/npfu... third_party/npapi/bindings/npfunctions.h:148: NPP_ClearSiteDataProcPtr clearsitedata; This is a serious problem; it collides with an accepted, upstream change. If this is what Flash's structure looks like, then they need to rebuild using newer headers. It's worse than that though; this will break other browsers, and us in the future, because new NPAPI functions will be added at the end and collide. Under no circumstances should Adobe be changing this struct unilaterally and without getting that change upstreamed.
http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... File chrome/browser/plugin_data_remover.cc (right): http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... chrome/browser/plugin_data_remover.cc:51: static int id = ChildProcessInfo::GenerateChildProcessUniqueId(); i think there could be race conditions if two of these objects are created right after each other? why not just have an id that's constant per object? http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.cc File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.c... chrome/browser/plugin_service.cc:229: if (plugin_path.value().empty()) curious, any reason to change this logic? it seems redundant to have this check since GetPluginInfoByPath will return null. if the dcheck/notreached can fire in normal circumstances, then perhaps no need to have it. if it does fire, when does that happen? http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h File chrome/plugin/plugin_channel.h (right): http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h#... chrome/plugin/plugin_channel.h:43: bool ClearSiteData(uint64 flags, this should be private. also, seems unnecessary to have this, why not just move this code to OnClearSiteData?
oh, i also defer to stuartmorgan for the npapi header stuff
On Fri, Nov 12, 2010 at 21:44, <jam@chromium.org> wrote: > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... > File chrome/browser/plugin_data_remover.cc (right): > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... > chrome/browser/plugin_data_remover.cc:51: static int id = > ChildProcessInfo::GenerateChildProcessUniqueId(); > i think there could be race conditions if two of these objects are > created right after each other? why not just have an id that's constant > per object? The point of the ID here is to identify the process requesting a connection so we can reuse an existing one, so keeping the ID around would create a very minor speedup the next time around. I don't think two PluginDataRemovers will ever exist at the same time, and if they would, they would just request one unnecessary ID (GenerateChildProcessUniqueId itself is thread-safe). > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.cc > File chrome/browser/plugin_service.cc (right): > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.c... > chrome/browser/plugin_service.cc:229: if (plugin_path.value().empty()) > curious, any reason to change this logic? it seems redundant to have > this check since GetPluginInfoByPath will return null. if the > dcheck/notreached can fire in normal circumstances, then perhaps no need > to have it. if it does fire, when does that happen? If the path is empty, FindPluginProcess will DCHECK before GetPluginInfoByPath is called, and I'd prefer to return NULL here so this can be handled in the Client. This will happen if the requested plug-in is disabled or not found, which should be checked before calling OpenChannelToPlugin, but could in theory change in between. > http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h > File chrome/plugin/plugin_channel.h (right): > > http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h#... > chrome/plugin/plugin_channel.h:43: bool ClearSiteData(uint64 flags, > this should be private. also, seems unnecessary to have this, why not > just move this code to OnClearSiteData? I think it's a bit clearer to have one method which does the work (and which returns a bool), and one that's basically glue to the IPC layer. The same pattern is used for OnGenerateRouteID / GenerateRouteID. > > http://codereview.chromium.org/4832002/ >
On Fri, Nov 12, 2010 at 5:23 PM, Bernhard Bauer <bauerb@chromium.org> wrote: > On Fri, Nov 12, 2010 at 21:44, <jam@chromium.org> wrote: > > > > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... > > File chrome/browser/plugin_data_remover.cc (right): > > > > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... > > chrome/browser/plugin_data_remover.cc:51: static int id = > > ChildProcessInfo::GenerateChildProcessUniqueId(); > > i think there could be race conditions if two of these objects are > > created right after each other? why not just have an id that's constant > > per object? > > The point of the ID here is to identify the process requesting a > connection so we can reuse an existing one, so keeping the ID around > would create a very minor speedup the next time around. I don't think > two PluginDataRemovers will ever exist at the same time, and if they > would, they would just request one unnecessary ID > (GenerateChildProcessUniqueId itself is thread-safe). > The id is also used to name the child process. That's why it's a monotonically increasing number for renderers, instead of the process id which is what we originally had. For race condition when one object (renderer/PluginDataRemover etc) has gone away while the plugin process is still closing down the IPC channel, and another connects with the same ID, the IPC channel creation in the browser process will fail since the named pipe already exists. > > > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.cc > > File chrome/browser/plugin_service.cc (right): > > > > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.c... > > chrome/browser/plugin_service.cc:229: if (plugin_path.value().empty()) > > curious, any reason to change this logic? it seems redundant to have > > this check since GetPluginInfoByPath will return null. if the > > dcheck/notreached can fire in normal circumstances, then perhaps no need > > to have it. if it does fire, when does that happen? > > If the path is empty, FindPluginProcess will DCHECK before > GetPluginInfoByPath is called, and I'd prefer to return NULL here so > this can be handled in the Client. This will happen if the requested > plug-in is disabled or not found, which should be checked before > calling OpenChannelToPlugin, but could in theory change in between. > just take out the DCHECK then, if it's something that can happen. DCHECKs (or really, should have been a NOTIMPLEMENTED() in this case) are put in to catch a situation which shouldn't happen. It seems that in this case it can happen due to things like disabled plugins which weren't there when that code was written. > > > > http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h > > File chrome/plugin/plugin_channel.h (right): > > > > > http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h#... > > chrome/plugin/plugin_channel.h:43: bool ClearSiteData(uint64 flags, > > this should be private. also, seems unnecessary to have this, why not > > just move this code to OnClearSiteData? > > I think it's a bit clearer to have one method which does the work (and > which returns a bool), and one that's basically glue to the IPC layer. > The same pattern is used for OnGenerateRouteID / GenerateRouteID. > OnGenerateRouteId is done that way because a number of other places call it. I don't see this pattern in other places in the code. The way I look at it is, if we switched all the code to do this pattern, would it improve things, or will it just lead to busy work for each new message? In this case I don't see the improvement, and so I prefer that we don't add unnecessary functions. > > > > http://codereview.chromium.org/4832002/ > > >
On Fri, Nov 12, 2010 at 5:54 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Fri, Nov 12, 2010 at 5:23 PM, Bernhard Bauer <bauerb@chromium.org>wrote: > >> On Fri, Nov 12, 2010 at 21:44, <jam@chromium.org> wrote: >> > >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... >> > File chrome/browser/plugin_data_remover.cc (right): >> > >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... >> > chrome/browser/plugin_data_remover.cc:51: static int id = >> > ChildProcessInfo::GenerateChildProcessUniqueId(); >> > i think there could be race conditions if two of these objects are >> > created right after each other? why not just have an id that's constant >> > per object? >> >> The point of the ID here is to identify the process requesting a >> connection so we can reuse an existing one, so keeping the ID around >> would create a very minor speedup the next time around. I don't think >> two PluginDataRemovers will ever exist at the same time, and if they >> would, they would just request one unnecessary ID >> (GenerateChildProcessUniqueId itself is thread-safe). >> > > The id is also used to name the child process. > this should read "child process ipc channel" That's why it's a monotonically increasing number for renderers, instead of > the process id which is what we originally had. For race condition when one > object (renderer/PluginDataRemover etc) has gone away while the plugin > process is still closing down the IPC channel, and another connects with the > same ID, the IPC channel creation in the browser process will fail since the > named pipe already exists. > >> >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.cc >> > File chrome/browser/plugin_service.cc (right): >> > >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.c... >> > chrome/browser/plugin_service.cc:229: if (plugin_path.value().empty()) >> > curious, any reason to change this logic? it seems redundant to have >> > this check since GetPluginInfoByPath will return null. if the >> > dcheck/notreached can fire in normal circumstances, then perhaps no need >> > to have it. if it does fire, when does that happen? >> >> If the path is empty, FindPluginProcess will DCHECK before >> GetPluginInfoByPath is called, and I'd prefer to return NULL here so >> this can be handled in the Client. This will happen if the requested >> plug-in is disabled or not found, which should be checked before >> calling OpenChannelToPlugin, but could in theory change in between. >> > > just take out the DCHECK then, if it's something that can happen. DCHECKs > (or really, should have been a NOTIMPLEMENTED() in this case) are put in to > catch a situation which shouldn't happen. It seems that in this case it can > happen due to things like disabled plugins which weren't there when that > code was written. > >> >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h >> > File chrome/plugin/plugin_channel.h (right): >> > >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h#... >> > chrome/plugin/plugin_channel.h:43: bool ClearSiteData(uint64 flags, >> > this should be private. also, seems unnecessary to have this, why not >> > just move this code to OnClearSiteData? >> >> I think it's a bit clearer to have one method which does the work (and >> which returns a bool), and one that's basically glue to the IPC layer. >> The same pattern is used for OnGenerateRouteID / GenerateRouteID. >> > > OnGenerateRouteId is done that way because a number of other places call > it. > > I don't see this pattern in other places in the code. The way I look at it > is, if we switched all the code to do this pattern, would it improve things, > or will it just lead to busy work for each new message? In this case I > don't see the improvement, and so I prefer that we don't add unnecessary > functions. > > >> > >> > http://codereview.chromium.org/4832002/ >> > >> > >
On Sat, Nov 13, 2010 at 02:54, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Fri, Nov 12, 2010 at 5:23 PM, Bernhard Bauer <bauerb@chromium.org> wrote: >> >> On Fri, Nov 12, 2010 at 21:44, <jam@chromium.org> wrote: >> > >> > >> > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... >> > File chrome/browser/plugin_data_remover.cc (right): >> > >> > >> > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... >> > chrome/browser/plugin_data_remover.cc:51: static int id = >> > ChildProcessInfo::GenerateChildProcessUniqueId(); >> > i think there could be race conditions if two of these objects are >> > created right after each other? why not just have an id that's constant >> > per object? >> >> The point of the ID here is to identify the process requesting a >> connection so we can reuse an existing one, so keeping the ID around >> would create a very minor speedup the next time around. I don't think >> two PluginDataRemovers will ever exist at the same time, and if they >> would, they would just request one unnecessary ID >> (GenerateChildProcessUniqueId itself is thread-safe). > > The id is also used to name the child process. That's why it's a > monotonically increasing number for renderers, instead of the process id > which is what we originally had. For race condition when one object > (renderer/PluginDataRemover etc) has gone away while the plugin process is > still closing down the IPC channel, and another connects with the same ID, > the IPC channel creation in the browser process will fail since the named > pipe already exists. Trying to understand here, couldn't/wouldn't a different client in the same process (another PluginDataRemover in the browser process or another ResourceMessageFilter in the same renderer) reuse the same IPC channel and not connect a new one? If one client is torn down while another one connects, wouldn't that cause the same problem in the renderer? >> > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.cc >> > File chrome/browser/plugin_service.cc (right): >> > >> > >> > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.c... >> > chrome/browser/plugin_service.cc:229: if (plugin_path.value().empty()) >> > curious, any reason to change this logic? it seems redundant to have >> > this check since GetPluginInfoByPath will return null. if the >> > dcheck/notreached can fire in normal circumstances, then perhaps no need >> > to have it. if it does fire, when does that happen? >> >> If the path is empty, FindPluginProcess will DCHECK before >> GetPluginInfoByPath is called, and I'd prefer to return NULL here so >> this can be handled in the Client. This will happen if the requested >> plug-in is disabled or not found, which should be checked before >> calling OpenChannelToPlugin, but could in theory change in between. > > just take out the DCHECK then, if it's something that can happen. DCHECKs > (or really, should have been a NOTIMPLEMENTED() in this case) are put in to > catch a situation which shouldn't happen. It seems that in this case it can > happen due to things like disabled plugins which weren't there when that > code was written. >> >> > >> > http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h >> > File chrome/plugin/plugin_channel.h (right): >> > >> > >> > http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h#... >> > chrome/plugin/plugin_channel.h:43: bool ClearSiteData(uint64 flags, >> > this should be private. also, seems unnecessary to have this, why not >> > just move this code to OnClearSiteData? >> >> I think it's a bit clearer to have one method which does the work (and >> which returns a bool), and one that's basically glue to the IPC layer. >> The same pattern is used for OnGenerateRouteID / GenerateRouteID. > > OnGenerateRouteId is done that way because a number of other places call > it. > I don't see this pattern in other places in the code. The way I look at it > is, if we switched all the code to do this pattern, would it improve things, > or will it just lead to busy work for each new message? In this case I > don't see the improvement, and so I prefer that we don't add unnecessary > functions. >> >> > >> > http://codereview.chromium.org/4832002/ >> > > >
On Fri, Nov 12, 2010 at 6:18 PM, Bernhard Bauer <bauerb@chromium.org> wrote: > On Sat, Nov 13, 2010 at 02:54, John Abd-El-Malek <jam@chromium.org> wrote: > > > > > > On Fri, Nov 12, 2010 at 5:23 PM, Bernhard Bauer <bauerb@chromium.org> > wrote: > >> > >> On Fri, Nov 12, 2010 at 21:44, <jam@chromium.org> wrote: > >> > > >> > > >> > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... > >> > File chrome/browser/plugin_data_remover.cc (right): > >> > > >> > > >> > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... > >> > chrome/browser/plugin_data_remover.cc:51: static int id = > >> > ChildProcessInfo::GenerateChildProcessUniqueId(); > >> > i think there could be race conditions if two of these objects are > >> > created right after each other? why not just have an id that's > constant > >> > per object? > >> > >> The point of the ID here is to identify the process requesting a > >> connection so we can reuse an existing one, so keeping the ID around > >> would create a very minor speedup the next time around. I don't think > >> two PluginDataRemovers will ever exist at the same time, and if they > >> would, they would just request one unnecessary ID > >> (GenerateChildProcessUniqueId itself is thread-safe). > > > > The id is also used to name the child process. That's why it's a > > monotonically increasing number for renderers, instead of the process id > > which is what we originally had. For race condition when one object > > (renderer/PluginDataRemover etc) has gone away while the plugin process > is > > still closing down the IPC channel, and another connects with the same > ID, > > the IPC channel creation in the browser process will fail since the named > > pipe already exists. > > Trying to understand here, couldn't/wouldn't a different client in the > same process (another PluginDataRemover in the browser process or > another ResourceMessageFilter in the same renderer) reuse the same IPC > channel and not connect a new one? If one client is torn down while > another one connects, wouldn't that cause the same problem in the > renderer? > The race condition is that the UI thread in the plugin process doesn't think it has an open channel to the renderer/brower, but the task it posted to the IO thread to actually close the named pipe hasn't run yet. If during this brief period of time the browser-side code tries to create a named pipe with that name, it will fail. > > >> > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.cc > >> > File chrome/browser/plugin_service.cc (right): > >> > > >> > > >> > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.c... > >> > chrome/browser/plugin_service.cc:229: if (plugin_path.value().empty()) > >> > curious, any reason to change this logic? it seems redundant to have > >> > this check since GetPluginInfoByPath will return null. if the > >> > dcheck/notreached can fire in normal circumstances, then perhaps no > need > >> > to have it. if it does fire, when does that happen? > >> > >> If the path is empty, FindPluginProcess will DCHECK before > >> GetPluginInfoByPath is called, and I'd prefer to return NULL here so > >> this can be handled in the Client. This will happen if the requested > >> plug-in is disabled or not found, which should be checked before > >> calling OpenChannelToPlugin, but could in theory change in between. > > > > just take out the DCHECK then, if it's something that can happen. > DCHECKs > > (or really, should have been a NOTIMPLEMENTED() in this case) are put in > to > > catch a situation which shouldn't happen. It seems that in this case it > can > > happen due to things like disabled plugins which weren't there when that > > code was written. > >> > >> > > >> > > http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h > >> > File chrome/plugin/plugin_channel.h (right): > >> > > >> > > >> > > http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h#... > >> > chrome/plugin/plugin_channel.h:43: bool ClearSiteData(uint64 flags, > >> > this should be private. also, seems unnecessary to have this, why not > >> > just move this code to OnClearSiteData? > >> > >> I think it's a bit clearer to have one method which does the work (and > >> which returns a bool), and one that's basically glue to the IPC layer. > >> The same pattern is used for OnGenerateRouteID / GenerateRouteID. > > > > OnGenerateRouteId is done that way because a number of other places call > > it. > > I don't see this pattern in other places in the code. The way I look at > it > > is, if we switched all the code to do this pattern, would it improve > things, > > or will it just lead to busy work for each new message? In this case I > > don't see the improvement, and so I prefer that we don't add unnecessary > > functions. > >> > >> > > >> > http://codereview.chromium.org/4832002/ > >> > > > > > >
On Mon, Nov 15, 2010 at 9:47 AM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Fri, Nov 12, 2010 at 6:18 PM, Bernhard Bauer <bauerb@chromium.org>wrote: > >> On Sat, Nov 13, 2010 at 02:54, John Abd-El-Malek <jam@chromium.org> >> wrote: >> > >> > >> > On Fri, Nov 12, 2010 at 5:23 PM, Bernhard Bauer <bauerb@chromium.org> >> wrote: >> >> >> >> On Fri, Nov 12, 2010 at 21:44, <jam@chromium.org> wrote: >> >> > >> >> > >> >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... >> >> > File chrome/browser/plugin_data_remover.cc (right): >> >> > >> >> > >> >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... >> >> > chrome/browser/plugin_data_remover.cc:51: static int id = >> >> > ChildProcessInfo::GenerateChildProcessUniqueId(); >> >> > i think there could be race conditions if two of these objects are >> >> > created right after each other? why not just have an id that's >> constant >> >> > per object? >> >> >> >> The point of the ID here is to identify the process requesting a >> >> connection so we can reuse an existing one, so keeping the ID around >> >> would create a very minor speedup the next time around. I don't think >> >> two PluginDataRemovers will ever exist at the same time, and if they >> >> would, they would just request one unnecessary ID >> >> (GenerateChildProcessUniqueId itself is thread-safe). >> > >> > The id is also used to name the child process. That's why it's a >> > monotonically increasing number for renderers, instead of the process id >> > which is what we originally had. For race condition when one object >> > (renderer/PluginDataRemover etc) has gone away while the plugin process >> is >> > still closing down the IPC channel, and another connects with the same >> ID, >> > the IPC channel creation in the browser process will fail since the >> named >> > pipe already exists. >> >> Trying to understand here, couldn't/wouldn't a different client in the >> same process (another PluginDataRemover in the browser process or >> another ResourceMessageFilter in the same renderer) reuse the same IPC >> channel and not connect a new one? If one client is torn down while >> another one connects, wouldn't that cause the same problem in the >> renderer? >> > > The race condition is that the UI thread in the plugin process doesn't > think it has an open channel to the renderer/brower, but the task it posted > to the IO thread to actually close the named pipe hasn't run yet. If during > this brief period of time the browser-side code tries to create a named pipe > with that name, it will fail. > btw see ipc_channel_proxy.cc for more details > >> >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.cc >> >> > File chrome/browser/plugin_service.cc (right): >> >> > >> >> > >> >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.c... >> >> > chrome/browser/plugin_service.cc:229: if >> (plugin_path.value().empty()) >> >> > curious, any reason to change this logic? it seems redundant to have >> >> > this check since GetPluginInfoByPath will return null. if the >> >> > dcheck/notreached can fire in normal circumstances, then perhaps no >> need >> >> > to have it. if it does fire, when does that happen? >> >> >> >> If the path is empty, FindPluginProcess will DCHECK before >> >> GetPluginInfoByPath is called, and I'd prefer to return NULL here so >> >> this can be handled in the Client. This will happen if the requested >> >> plug-in is disabled or not found, which should be checked before >> >> calling OpenChannelToPlugin, but could in theory change in between. >> > >> > just take out the DCHECK then, if it's something that can happen. >> DCHECKs >> > (or really, should have been a NOTIMPLEMENTED() in this case) are put in >> to >> > catch a situation which shouldn't happen. It seems that in this case it >> can >> > happen due to things like disabled plugins which weren't there when that >> > code was written. >> >> >> >> > >> >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h >> >> > File chrome/plugin/plugin_channel.h (right): >> >> > >> >> > >> >> > >> http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h#... >> >> > chrome/plugin/plugin_channel.h:43: bool ClearSiteData(uint64 flags, >> >> > this should be private. also, seems unnecessary to have this, why >> not >> >> > just move this code to OnClearSiteData? >> >> >> >> I think it's a bit clearer to have one method which does the work (and >> >> which returns a bool), and one that's basically glue to the IPC layer. >> >> The same pattern is used for OnGenerateRouteID / GenerateRouteID. >> > >> > OnGenerateRouteId is done that way because a number of other places call >> > it. >> > I don't see this pattern in other places in the code. The way I look at >> it >> > is, if we switched all the code to do this pattern, would it improve >> things, >> > or will it just lead to busy work for each new message? In this case I >> > don't see the improvement, and so I prefer that we don't add unnecessary >> > functions. >> >> >> >> > >> >> > http://codereview.chromium.org/4832002/ >> >> > >> > >> > >> > >
On 2010/11/15 17:47:52, John Abd-El-Malek wrote: > On Mon, Nov 15, 2010 at 9:47 AM, John Abd-El-Malek <mailto:jam@chromium.org> wrote: > > > > > > > On Fri, Nov 12, 2010 at 6:18 PM, Bernhard Bauer <bauerb@chromium.org>wrote: > > > >> On Sat, Nov 13, 2010 at 02:54, John Abd-El-Malek <mailto:jam@chromium.org> > >> wrote: > >> > > >> > > >> > On Fri, Nov 12, 2010 at 5:23 PM, Bernhard Bauer <mailto:bauerb@chromium.org> > >> wrote: > >> >> > >> >> On Fri, Nov 12, 2010 at 21:44, <mailto:jam@chromium.org> wrote: > >> >> > > >> >> > > >> >> > > >> > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... > >> >> > File chrome/browser/plugin_data_remover.cc (right): > >> >> > > >> >> > > >> >> > > >> > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... > >> >> > chrome/browser/plugin_data_remover.cc:51: static int id = > >> >> > ChildProcessInfo::GenerateChildProcessUniqueId(); > >> >> > i think there could be race conditions if two of these objects are > >> >> > created right after each other? why not just have an id that's > >> constant > >> >> > per object? > >> >> > >> >> The point of the ID here is to identify the process requesting a > >> >> connection so we can reuse an existing one, so keeping the ID around > >> >> would create a very minor speedup the next time around. I don't think > >> >> two PluginDataRemovers will ever exist at the same time, and if they > >> >> would, they would just request one unnecessary ID > >> >> (GenerateChildProcessUniqueId itself is thread-safe). > >> > > >> > The id is also used to name the child process. That's why it's a > >> > monotonically increasing number for renderers, instead of the process id > >> > which is what we originally had. For race condition when one object > >> > (renderer/PluginDataRemover etc) has gone away while the plugin process > >> is > >> > still closing down the IPC channel, and another connects with the same > >> ID, > >> > the IPC channel creation in the browser process will fail since the > >> named > >> > pipe already exists. > >> > >> Trying to understand here, couldn't/wouldn't a different client in the > >> same process (another PluginDataRemover in the browser process or > >> another ResourceMessageFilter in the same renderer) reuse the same IPC > >> channel and not connect a new one? If one client is torn down while > >> another one connects, wouldn't that cause the same problem in the > >> renderer? > >> > > > > The race condition is that the UI thread in the plugin process doesn't > > think it has an open channel to the renderer/brower, but the task it posted > > to the IO thread to actually close the named pipe hasn't run yet. If during > > this brief period of time the browser-side code tries to create a named pipe > > with that name, it will fail. > > > > btw see ipc_channel_proxy.cc for more details Ok... sorry if I'm being dumb here (and: done, BTW), but does that mean that this is mitigated in the renderer case by using IPC::ChannelProxy? > > > > >> >> > > >> > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.cc > >> >> > File chrome/browser/plugin_service.cc (right): > >> >> > > >> >> > > >> >> > > >> > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.c... > >> >> > chrome/browser/plugin_service.cc:229: if > >> (plugin_path.value().empty()) > >> >> > curious, any reason to change this logic? it seems redundant to have > >> >> > this check since GetPluginInfoByPath will return null. if the > >> >> > dcheck/notreached can fire in normal circumstances, then perhaps no > >> need > >> >> > to have it. if it does fire, when does that happen? > >> >> > >> >> If the path is empty, FindPluginProcess will DCHECK before > >> >> GetPluginInfoByPath is called, and I'd prefer to return NULL here so > >> >> this can be handled in the Client. This will happen if the requested > >> >> plug-in is disabled or not found, which should be checked before > >> >> calling OpenChannelToPlugin, but could in theory change in between. > >> > > >> > just take out the DCHECK then, if it's something that can happen. > >> DCHECKs > >> > (or really, should have been a NOTIMPLEMENTED() in this case) are put in > >> to > >> > catch a situation which shouldn't happen. It seems that in this case it > >> can > >> > happen due to things like disabled plugins which weren't there when that > >> > code was written. OK, done. > >> >> > >> >> > > >> >> > > >> http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h > >> >> > File chrome/plugin/plugin_channel.h (right): > >> >> > > >> >> > > >> >> > > >> > http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h#... > >> >> > chrome/plugin/plugin_channel.h:43: bool ClearSiteData(uint64 flags, > >> >> > this should be private. also, seems unnecessary to have this, why > >> not > >> >> > just move this code to OnClearSiteData? > >> >> > >> >> I think it's a bit clearer to have one method which does the work (and > >> >> which returns a bool), and one that's basically glue to the IPC layer. > >> >> The same pattern is used for OnGenerateRouteID / GenerateRouteID. > >> > > >> > OnGenerateRouteId is done that way because a number of other places call > >> > it. > >> > I don't see this pattern in other places in the code. The way I look at > >> it > >> > is, if we switched all the code to do this pattern, would it improve > >> things, > >> > or will it just lead to busy work for each new message? In this case I > >> > don't see the improvement, and so I prefer that we don't add unnecessary > >> > functions. Done. I also removed the NPAPI header changes, so right now NPP_ClearSiteData does nothing. Changing that once we have the right header and corresponding plug-in shouldn't be too hard. > >> >> > >> >> > > >> >> > http://codereview.chromium.org/4832002/ > >> >> > > >> > > >> > > >> > > > >
On Mon, Nov 15, 2010 at 10:23 AM, <bauerb@chromium.org> wrote: > On 2010/11/15 17:47:52, John Abd-El-Malek wrote: > >> On Mon, Nov 15, 2010 at 9:47 AM, John Abd-El-Malek <mailto: >> jam@chromium.org> >> > wrote: > > > >> > >> > On Fri, Nov 12, 2010 at 6:18 PM, Bernhard Bauer <bauerb@chromium.org >> >wrote: >> > >> >> On Sat, Nov 13, 2010 at 02:54, John Abd-El-Malek <mailto: >> jam@chromium.org> >> >> >> wrote: >> >> > >> >> > >> >> > On Fri, Nov 12, 2010 at 5:23 PM, Bernhard Bauer >> > <mailto:bauerb@chromium.org> > >> >> wrote: >> >> >> >> >> >> >> On Fri, Nov 12, 2010 at 21:44, <mailto:jam@chromium.org> wrote: >> >> >> > >> >> >> > >> >> >> > >> >> >> > > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... > >> >> >> > File chrome/browser/plugin_data_remover.cc (right): >> >> >> > >> >> >> > >> >> >> > >> >> >> > > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_data_remo... > >> >> >> > chrome/browser/plugin_data_remover.cc:51: static int id = >> >> >> > ChildProcessInfo::GenerateChildProcessUniqueId(); >> >> >> > i think there could be race conditions if two of these objects are >> >> >> > created right after each other? why not just have an id that's >> >> constant >> >> >> > per object? >> >> >> >> >> >> The point of the ID here is to identify the process requesting a >> >> >> connection so we can reuse an existing one, so keeping the ID around >> >> >> would create a very minor speedup the next time around. I don't >> think >> >> >> two PluginDataRemovers will ever exist at the same time, and if they >> >> >> would, they would just request one unnecessary ID >> >> >> (GenerateChildProcessUniqueId itself is thread-safe). >> >> > >> >> > The id is also used to name the child process. That's why it's a >> >> > monotonically increasing number for renderers, instead of the process >> id >> >> > which is what we originally had. For race condition when one object >> >> > (renderer/PluginDataRemover etc) has gone away while the plugin >> process >> >> is >> >> > still closing down the IPC channel, and another connects with the >> same >> >> ID, >> >> > the IPC channel creation in the browser process will fail since the >> >> named >> >> > pipe already exists. >> >> >> >> Trying to understand here, couldn't/wouldn't a different client in the >> >> same process (another PluginDataRemover in the browser process or >> >> another ResourceMessageFilter in the same renderer) reuse the same IPC >> >> channel and not connect a new one? If one client is torn down while >> >> another one connects, wouldn't that cause the same problem in the >> >> renderer? >> >> >> > >> > The race condition is that the UI thread in the plugin process doesn't >> > think it has an open channel to the renderer/brower, but the task it >> posted >> > to the IO thread to actually close the named pipe hasn't run yet. If >> during >> > this brief period of time the browser-side code tries to create a named >> pipe >> > with that name, it will fail. >> > >> > > btw see ipc_channel_proxy.cc for more details >> > > Ok... sorry if I'm being dumb here (and: done, BTW), but does that mean > that > this is mitigated in the renderer case by using IPC::ChannelProxy? it's mitigated by always using unique ids that are never reused. > > > > > >> >> >> > >> >> >> >> http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.cc >> >> >> > File chrome/browser/plugin_service.cc (right): >> >> >> > >> >> >> > >> >> >> > >> >> >> > > > http://codereview.chromium.org/4832002/diff/1/chrome/browser/plugin_service.c... > >> >> >> > chrome/browser/plugin_service.cc:229: if >> >> (plugin_path.value().empty()) >> >> >> > curious, any reason to change this logic? it seems redundant to >> have >> >> >> > this check since GetPluginInfoByPath will return null. if the >> >> >> > dcheck/notreached can fire in normal circumstances, then perhaps >> no >> >> need >> >> >> > to have it. if it does fire, when does that happen? >> >> >> >> >> >> If the path is empty, FindPluginProcess will DCHECK before >> >> >> GetPluginInfoByPath is called, and I'd prefer to return NULL here so >> >> >> this can be handled in the Client. This will happen if the requested >> >> >> plug-in is disabled or not found, which should be checked before >> >> >> calling OpenChannelToPlugin, but could in theory change in between. >> >> > >> >> > just take out the DCHECK then, if it's something that can happen. >> >> DCHECKs >> >> > (or really, should have been a NOTIMPLEMENTED() in this case) are put >> in >> >> to >> >> > catch a situation which shouldn't happen. It seems that in this case >> it >> >> can >> >> > happen due to things like disabled plugins which weren't there when >> that >> >> > code was written. >> > > OK, done. > > > >> >> >> >> >> > >> >> >> > >> >> >> > > http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h > >> >> >> > File chrome/plugin/plugin_channel.h (right): >> >> >> > >> >> >> > >> >> >> > >> >> >> > > > http://codereview.chromium.org/4832002/diff/1/chrome/plugin/plugin_channel.h#... > >> >> >> > chrome/plugin/plugin_channel.h:43: bool ClearSiteData(uint64 >> flags, >> >> >> > this should be private. also, seems unnecessary to have this, why >> >> not >> >> >> > just move this code to OnClearSiteData? >> >> >> >> >> >> I think it's a bit clearer to have one method which does the work >> (and >> >> >> which returns a bool), and one that's basically glue to the IPC >> layer. >> >> >> The same pattern is used for OnGenerateRouteID / GenerateRouteID. >> >> > >> >> > OnGenerateRouteId is done that way because a number of other places >> call >> >> > it. >> >> > I don't see this pattern in other places in the code. The way I look >> at >> >> it >> >> > is, if we switched all the code to do this pattern, would it improve >> >> things, >> >> > or will it just lead to busy work for each new message? In this case >> I >> >> > don't see the improvement, and so I prefer that we don't add >> unnecessary >> >> > functions. >> > > Done. > > I also removed the NPAPI header changes, so right now NPP_ClearSiteData > does > nothing. Changing that once we have the right header and corresponding > plug-in > shouldn't be too hard. > > > >> >> >> >> >> > >> >> >> > http://codereview.chromium.org/4832002/ >> >> >> > >> >> > >> >> > >> >> >> > >> > >> > > > > http://codereview.chromium.org/4832002/ >
On Mon, Nov 15, 2010 at 19:31, John Abd-El-Malek <jam@chromium.org> wrote: > it's mitigated by always using unique ids that are never reused. Well, they are reused by a different ResourceMessageFilter for the same renderer process, but as long as the RenderProcessHost they get their ID from is still around, the connection in the plugin process isn't closed, right?
On Mon, Nov 15, 2010 at 10:42 AM, Bernhard Bauer <bauerb@chromium.org>wrote: > On Mon, Nov 15, 2010 at 19:31, John Abd-El-Malek <jam@chromium.org> wrote: > > it's mitigated by always using unique ids that are never reused. > > Well, they are reused by a different ResourceMessageFilter for the > same renderer process, but as long as the RenderProcessHost they get > their ID from is still around, the connection in the plugin process > isn't closed, right? > ah, sorry I thought you were talking about the channel between browser-renderer. not sure what you mean by different ResourceMessageFilter? There's only one RMF per renderer process.
On Mon, Nov 15, 2010 at 20:41, John Abd-El-Malek <jam@chromium.org> wrote: > > On Mon, Nov 15, 2010 at 10:42 AM, Bernhard Bauer <bauerb@chromium.org> > wrote: >> >> On Mon, Nov 15, 2010 at 19:31, John Abd-El-Malek <jam@chromium.org> wrote: >> > it's mitigated by always using unique ids that are never reused. >> >> Well, they are reused by a different ResourceMessageFilter for the >> same renderer process, but as long as the RenderProcessHost they get >> their ID from is still around, the connection in the plugin process >> isn't closed, right? > > ah, sorry I thought you were talking about the channel between > browser-renderer. > not sure what you mean by different ResourceMessageFilter? There's only one > RMF per renderer process. And I almost thought I had it ;-) Okay, does every single call to OpenChannelToPlugin use a different ID? If so, why do we store the map of opened channels at all?
On Mon, Nov 15, 2010 at 12:53 PM, Bernhard Bauer <bauerb@chromium.org>wrote: > On Mon, Nov 15, 2010 at 20:41, John Abd-El-Malek <jam@chromium.org> wrote: > > > > On Mon, Nov 15, 2010 at 10:42 AM, Bernhard Bauer <bauerb@chromium.org> > > wrote: > >> > >> On Mon, Nov 15, 2010 at 19:31, John Abd-El-Malek <jam@chromium.org> > wrote: > >> > it's mitigated by always using unique ids that are never reused. > >> > >> Well, they are reused by a different ResourceMessageFilter for the > >> same renderer process, but as long as the RenderProcessHost they get > >> their ID from is still around, the connection in the plugin process > >> isn't closed, right? > > > > ah, sorry I thought you were talking about the channel between > > browser-renderer. > > not sure what you mean by different ResourceMessageFilter? There's only > one > > RMF per renderer process. > > And I almost thought I had it ;-) > > Okay, does every single call to OpenChannelToPlugin use a different > ID? If so, why do we store the map of opened channels at all? > no, the channel name between a renderer and a plugin process is always the same (see PluginChannel::GetPluginChannel which generates it).
On Mon, Nov 15, 2010 at 1:32 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Mon, Nov 15, 2010 at 12:53 PM, Bernhard Bauer <bauerb@chromium.org>wrote: > >> On Mon, Nov 15, 2010 at 20:41, John Abd-El-Malek <jam@chromium.org> >> wrote: >> > >> > On Mon, Nov 15, 2010 at 10:42 AM, Bernhard Bauer <bauerb@chromium.org> >> > wrote: >> >> >> >> On Mon, Nov 15, 2010 at 19:31, John Abd-El-Malek <jam@chromium.org> >> wrote: >> >> > it's mitigated by always using unique ids that are never reused. >> >> >> >> Well, they are reused by a different ResourceMessageFilter for the >> >> same renderer process, but as long as the RenderProcessHost they get >> >> their ID from is still around, the connection in the plugin process >> >> isn't closed, right? >> > >> > ah, sorry I thought you were talking about the channel between >> > browser-renderer. >> > not sure what you mean by different ResourceMessageFilter? There's only >> one >> > RMF per renderer process. >> >> And I almost thought I had it ;-) >> >> Okay, does every single call to OpenChannelToPlugin use a different >> ID? If so, why do we store the map of opened channels at all? >> > > no, the channel name between a renderer and a plugin process is always the > same (see PluginChannel::GetPluginChannel which generates it). > to expand a bit :) it's always the same if the plugin process is long-lived. if the plugin process goes away, then it'll be a different name.
On 2010/11/15 21:33:21, John Abd-El-Malek wrote: > On Mon, Nov 15, 2010 at 1:32 PM, John Abd-El-Malek <mailto:jam@chromium.org> wrote: > > > > > > > On Mon, Nov 15, 2010 at 12:53 PM, Bernhard Bauer <bauerb@chromium.org>wrote: > > > >> On Mon, Nov 15, 2010 at 20:41, John Abd-El-Malek <mailto:jam@chromium.org> > >> wrote: > >> > > >> > On Mon, Nov 15, 2010 at 10:42 AM, Bernhard Bauer <mailto:bauerb@chromium.org> > >> > wrote: > >> >> > >> >> On Mon, Nov 15, 2010 at 19:31, John Abd-El-Malek <mailto:jam@chromium.org> > >> wrote: > >> >> > it's mitigated by always using unique ids that are never reused. > >> >> > >> >> Well, they are reused by a different ResourceMessageFilter for the > >> >> same renderer process, but as long as the RenderProcessHost they get > >> >> their ID from is still around, the connection in the plugin process > >> >> isn't closed, right? > >> > > >> > ah, sorry I thought you were talking about the channel between > >> > browser-renderer. > >> > not sure what you mean by different ResourceMessageFilter? There's only > >> one > >> > RMF per renderer process. > >> > >> And I almost thought I had it ;-) > >> > >> Okay, does every single call to OpenChannelToPlugin use a different > >> ID? If so, why do we store the map of opened channels at all? > >> > > > > no, the channel name between a renderer and a plugin process is always the > > same (see PluginChannel::GetPluginChannel which generates it). > > > > to expand a bit :) it's always the same if the plugin process is > long-lived. if the plugin process goes away, then it'll be a different > name. (Grr, Rietveld swallowed the last two mails) Okay, I think I get it :-) I removed the |static| from the identifier, BTW. Does the rest of the CL LGTY?
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
