Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5278001:
Add a preference to clear Flash LSO data in the Clear Browsing Data dialog. (Closed)
|
Created:
10 years, 1 month ago by Bernhard Bauer Modified:
9 years, 7 months ago CC:
chromium-reviews, ncarter (slow), ben+cc_chromium.org, Raghu Simha, idana, jam, stuartmorgan+watch_chromium.org, tim (not reviewing), darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a preference to clear Flash LSO data in the Clear Browsing Data dialog.
The preference defaults to false, so without UI it should do nothing.
BUG=58235
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69268
Patch Set 1 #
Total comments: 14
Patch Set 2 : rebview #Patch Set 3 : sync 'n' fix #Patch Set 4 : move clearing lso data on shutdown to browser_shutdown #Patch Set 5 : formatting #
Total comments: 3
Patch Set 6 : review #Patch Set 7 : Update pref if a plugin is enabled/disabled #Patch Set 8 : foo #Patch Set 9 : sync #Patch Set 10 : sync #
Messages
Total messages: 29 (0 generated)
Please review. John: There aren't many plug-in related changes in here, so I just cc'd you.
http://codereview.chromium.org/5278001/diff/1/chrome/browser/browser_process_... File chrome/browser/browser_process_impl.cc (right): http://codereview.chromium.org/5278001/diff/1/chrome/browser/browser_process_... chrome/browser/browser_process_impl.cc:131: MessageLoop::current()->Run(); i think this is the wrong place to do this. Can't you guarantee that when the plugin service terminates, all plugin processes are gone? Then you should trigger the removal of LSOs during the destruction of the plugin service. This clear local state stuff here is really just a bad hack that will go away soon. http://codereview.chromium.org/5278001/diff/1/chrome/browser/browsing_data_re... File chrome/browser/browsing_data_remover.cc (right): http://codereview.chromium.org/5278001/diff/1/chrome/browser/browsing_data_re... chrome/browser/browsing_data_remover.cc:517: remove empty line http://codereview.chromium.org/5278001/diff/1/chrome/browser/plugin_data_remo... File chrome/browser/plugin_data_remover_helper.cc (right): http://codereview.chromium.org/5278001/diff/1/chrome/browser/plugin_data_remo... chrome/browser/plugin_data_remover_helper.cc:58: }; disallow copy and assign http://codereview.chromium.org/5278001/diff/1/chrome/browser/plugin_data_remo... File chrome/browser/plugin_data_remover_helper.h (right): http://codereview.chromium.org/5278001/diff/1/chrome/browser/plugin_data_remo... chrome/browser/plugin_data_remover_helper.h:19: no empty line http://codereview.chromium.org/5278001/diff/1/chrome/browser/plugin_data_remo... chrome/browser/plugin_data_remover_helper.h:40: }; disallow copy and assign http://codereview.chromium.org/5278001/diff/1/chrome/common/pref_names.h File chrome/common/pref_names.h (right): http://codereview.chromium.org/5278001/diff/1/chrome/common/pref_names.h#newc... chrome/common/pref_names.h:170: extern const char kDeleteLSOData[]; what is this used for? http://codereview.chromium.org/5278001/diff/1/chrome/common/pref_names.h#newc... chrome/common/pref_names.h:171: extern const char kClearPluginLSODataEnabled[]; why do you need this pref? There's no code to change it, but you have code that detects whether a suitable flash is installed?
http://codereview.chromium.org/5278001/diff/1/chrome/browser/browser_process_... File chrome/browser/browser_process_impl.cc (right): http://codereview.chromium.org/5278001/diff/1/chrome/browser/browser_process_... chrome/browser/browser_process_impl.cc:131: MessageLoop::current()->Run(); On 2010/11/24 19:03:07, jochen wrote: > i think this is the wrong place to do this. Can't you guarantee that when the > plugin service terminates, all plugin processes are gone? Then you should > trigger the removal of LSOs during the destruction of the plugin service. PluginDataRemover needs the PluginService in order to connect to Flash and call the clear data API on it, so I can't start the removing in the destructor. http://codereview.chromium.org/5278001/diff/1/chrome/browser/browsing_data_re... File chrome/browser/browsing_data_remover.cc (right): http://codereview.chromium.org/5278001/diff/1/chrome/browser/browsing_data_re... chrome/browser/browsing_data_remover.cc:517: On 2010/11/24 19:03:07, jochen wrote: > remove empty line Done. http://codereview.chromium.org/5278001/diff/1/chrome/browser/plugin_data_remo... File chrome/browser/plugin_data_remover_helper.cc (right): http://codereview.chromium.org/5278001/diff/1/chrome/browser/plugin_data_remo... chrome/browser/plugin_data_remover_helper.cc:58: }; On 2010/11/24 19:03:07, jochen wrote: > disallow copy and assign Done. http://codereview.chromium.org/5278001/diff/1/chrome/browser/plugin_data_remo... File chrome/browser/plugin_data_remover_helper.h (right): http://codereview.chromium.org/5278001/diff/1/chrome/browser/plugin_data_remo... chrome/browser/plugin_data_remover_helper.h:19: On 2010/11/24 19:03:07, jochen wrote: > no empty line Done. http://codereview.chromium.org/5278001/diff/1/chrome/browser/plugin_data_remo... chrome/browser/plugin_data_remover_helper.h:40: }; On 2010/11/24 19:03:07, jochen wrote: > disallow copy and assign Done. http://codereview.chromium.org/5278001/diff/1/chrome/common/pref_names.h File chrome/common/pref_names.h (right): http://codereview.chromium.org/5278001/diff/1/chrome/common/pref_names.h#newc... chrome/common/pref_names.h:170: extern const char kDeleteLSOData[]; On 2010/11/24 19:03:07, jochen wrote: > what is this used for? It's used in the Clear Browsing Data dialog (the other prefs above are for the other types of data). http://codereview.chromium.org/5278001/diff/1/chrome/common/pref_names.h#newc... chrome/common/pref_names.h:171: extern const char kClearPluginLSODataEnabled[]; On 2010/11/24 19:03:07, jochen wrote: > why do you need this pref? There's no code to change it, but you have code that > detects whether a suitable flash is installed? PluginDataRemoverHelper updates this preference. It's instantiated by the pref UI in order to disable checkboxes when no suitable plugin is installed. Because the list of plugins could change at any time, and this check is asynchronous, we can't keep the pref in sync all the time, so we just update the enabled state when the options dialog is shown, and persist it for the next time.
After (offline) discussions with Jochen, I moved the code to delete LSO data on shutdown to browser_shutdown::Shutdown(). Darin/John, Jochen had some concerns about possible race conditions where a plug-in would set LSO data after the hook at shutdown has run. When would be the best time to run the hook to avoid this?
We should be able to know when all plugin processes are shutdown. /me looks to jam for the details. -Darin On Mon, Nov 29, 2010 at 4:28 AM, <bauerb@chromium.org> wrote: > After (offline) discussions with Jochen, I moved the code to delete LSO > data on > shutdown to browser_shutdown::Shutdown(). > > Darin/John, Jochen had some concerns about possible race conditions where a > plug-in would set LSO data after the hook at shutdown has run. When would > be the > best time to run the hook to avoid this? > > > http://codereview.chromium.org/5278001/ >
Oh... but what if Flash is running in a separate browser? Doesn't it use a common location for all instances, across all browsers? -Darin On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher <darin@chromium.org> wrote: > We should be able to know when all plugin processes are shutdown. /me > looks to jam for the details. > > -Darin > > > > On Mon, Nov 29, 2010 at 4:28 AM, <bauerb@chromium.org> wrote: > >> After (offline) discussions with Jochen, I moved the code to delete LSO >> data on >> shutdown to browser_shutdown::Shutdown(). >> >> Darin/John, Jochen had some concerns about possible race conditions where >> a >> plug-in would set LSO data after the hook at shutdown has run. When would >> be the >> best time to run the hook to avoid this? >> >> >> http://codereview.chromium.org/5278001/ >> > >
http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... File chrome/browser/browser_shutdown.cc (right): http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... chrome/browser/browser_shutdown.cc:142: remover.StartRemoving(base::Time(), new MessageLoop::QuitTask()); I have a big concern about blocking our shutdown to wait on creating a Flash process. That could be pretty slow for various reasons (i.e. system is overloaded, Flash loads slowly). If a user is waiting to restart Chrome again (say because they shutdown Chrome for an auto-update), and try to start Chrome manually while this is happening, it won't work. They might shutdown Chrome processes manually which would be unfortunate as it could lead to profile corruption. Running a nested message loop also seems kind of scary, it's really hard to predict the side-effects of reentrancy at this point in shutdown. If the user has the kClearPluginLSODataOnExit pref set, why not just call Flash's ClearLSO function each time before we shutdown the Flash process? That way there's no slowing down of shutdown, which is critical, and it would avoid any race conditions that you mention. http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... File chrome/browser/plugin_data_remover.cc (right): http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... chrome/browser/plugin_data_remover.cc:22: const std::string min_flash_version = "10.2"; these globals don't seem to match our global naming convention, i.e. they look like local variables. I suggest prefixing them with "g_". we also avoid having any globals that use types with constructors/destructors. the reasons are that it slows down startup/shutdown slightly (see davemoore's crusade to remove them for chrome OS perf reasons, and also why we have base::LazyInstance), and also there's a limit to how many objects the CRT can keep track of. can you make the strings just be const char*? http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... File chrome/browser/plugin_service.cc (right): http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... chrome/browser/plugin_service.cc:216: if (plugin_path.value().empty()) why is this needed? Won't FindPluginProcess return null in this case?
On Mon, Nov 29, 2010 at 9:14 AM, <jam@chromium.org> wrote: > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... > File chrome/browser/browser_shutdown.cc (right): > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... > chrome/browser/browser_shutdown.cc:142: > remover.StartRemoving(base::Time(), new MessageLoop::QuitTask()); > I have a big concern about blocking our shutdown to wait on creating a > Flash process. That could be pretty slow for various reasons (i.e. > system is overloaded, Flash loads slowly). If a user is waiting to > restart Chrome again (say because they shutdown Chrome for an > auto-update), and try to start Chrome manually while this is happening, > it won't work. They might shutdown Chrome processes manually which > would be unfortunate as it could lead to profile corruption. > > Running a nested message loop also seems kind of scary, it's really hard > to predict the side-effects of reentrancy at this point in shutdown. > > If the user has the kClearPluginLSODataOnExit pref set, why not just > call Flash's ClearLSO function each time before we shutdown the Flash > process? just to be clear, I mean this decision should be made in the plugin process. the browser asynchronously updates the flash process, if it's running, on changes for this pref, and also of course tells it the value when it starts. then code in PluginProcess can call the Flash function depending on the cached value. > That way there's no slowing down of shutdown, which is critical, and it would avoid any race conditions that you mention. > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... > File chrome/browser/plugin_data_remover.cc (right): > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... > chrome/browser/plugin_data_remover.cc:22: const std::string > min_flash_version = "10.2"; > these globals don't seem to match our global naming convention, i.e. > they look like local variables. I suggest prefixing them with "g_". > > we also avoid having any globals that use types with > constructors/destructors. the reasons are that it slows down > startup/shutdown slightly (see davemoore's crusade to remove them for > chrome OS perf reasons, and also why we have base::LazyInstance), and > also there's a limit to how many objects the CRT can keep track of. can > you make the strings just be const char*? > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... > File chrome/browser/plugin_service.cc (right): > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... > chrome/browser/plugin_service.cc:216: if (plugin_path.value().empty()) > why is this needed? Won't FindPluginProcess return null in this case? > > > http://codereview.chromium.org/5278001/ >
On Mon, Nov 29, 2010 at 9:21 AM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Mon, Nov 29, 2010 at 9:14 AM, <jam@chromium.org> wrote: > >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... >> File chrome/browser/browser_shutdown.cc (right): >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... >> chrome/browser/browser_shutdown.cc:142: >> remover.StartRemoving(base::Time(), new MessageLoop::QuitTask()); >> I have a big concern about blocking our shutdown to wait on creating a >> Flash process. That could be pretty slow for various reasons (i.e. >> system is overloaded, Flash loads slowly). If a user is waiting to >> restart Chrome again (say because they shutdown Chrome for an >> auto-update), and try to start Chrome manually while this is happening, >> it won't work. They might shutdown Chrome processes manually which >> would be unfortunate as it could lead to profile corruption. >> >> Running a nested message loop also seems kind of scary, it's really hard >> to predict the side-effects of reentrancy at this point in shutdown. >> >> If the user has the kClearPluginLSODataOnExit pref set, why not just >> call Flash's ClearLSO function each time before we shutdown the Flash >> process? > > > just to be clear, I mean this decision should be made in the plugin > process. the browser asynchronously updates the flash process, if it's > running, on changes for this pref, and also of course tells it the value > when it starts. then code in PluginProcess can call the Flash function > depending on the cached value. > > >> That way there's no slowing down of shutdown, which is > > critical, and it would avoid any race conditions that you mention. >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... >> File chrome/browser/plugin_data_remover.cc (right): >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... >> chrome/browser/plugin_data_remover.cc:22: const std::string >> min_flash_version = "10.2"; >> these globals don't seem to match our global naming convention, i.e. >> they look like local variables. I suggest prefixing them with "g_". >> >> we also avoid having any globals that use types with >> constructors/destructors. the reasons are that it slows down >> startup/shutdown slightly (see davemoore's crusade to remove them for >> chrome OS perf reasons, and also why we have base::LazyInstance), and >> also there's a limit to how many objects the CRT can keep track of. can >> you make the strings just be const char*? >> > btw sorry for not catching this in the original review :) > >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... >> File chrome/browser/plugin_service.cc (right): >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... >> chrome/browser/plugin_service.cc:216: if (plugin_path.value().empty()) >> why is this needed? Won't FindPluginProcess return null in this case? >> >> >> http://codereview.chromium.org/5278001/ >> > >
On 2010/11/29 17:07:34, darin wrote: > Oh... but what if Flash is running in a separate browser? Doesn't it use a > common location for all instances, across all browsers? > > -Darin Shouldn't it be the responsibility of the plug-in if it implements the API to make sure that it can delete data while it's running in another browser? > On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher <mailto:darin@chromium.org> wrote: > > > We should be able to know when all plugin processes are shutdown. /me > > looks to jam for the details. > > > > -Darin > > > > > > > > On Mon, Nov 29, 2010 at 4:28 AM, <mailto:bauerb@chromium.org> wrote: > > > >> After (offline) discussions with Jochen, I moved the code to delete LSO > >> data on > >> shutdown to browser_shutdown::Shutdown(). > >> > >> Darin/John, Jochen had some concerns about possible race conditions where > >> a > >> plug-in would set LSO data after the hook at shutdown has run. When would > >> be the > >> best time to run the hook to avoid this? > >> > >> > >> http://codereview.chromium.org/5278001/ > >> > > > >
On 2010/11/29 17:28:16, John Abd-El-Malek wrote: > On Mon, Nov 29, 2010 at 9:21 AM, John Abd-El-Malek <mailto:jam@chromium.org> wrote: > > > > > > > On Mon, Nov 29, 2010 at 9:14 AM, <mailto:jam@chromium.org> wrote: > > > >> > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... > >> File chrome/browser/browser_shutdown.cc (right): > >> > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... > >> chrome/browser/browser_shutdown.cc:142: > >> remover.StartRemoving(base::Time(), new MessageLoop::QuitTask()); > >> I have a big concern about blocking our shutdown to wait on creating a > >> Flash process. That could be pretty slow for various reasons (i.e. > >> system is overloaded, Flash loads slowly). If a user is waiting to > >> restart Chrome again (say because they shutdown Chrome for an > >> auto-update), and try to start Chrome manually while this is happening, > >> it won't work. They might shutdown Chrome processes manually which > >> would be unfortunate as it could lead to profile corruption. > >> > >> Running a nested message loop also seems kind of scary, it's really hard > >> to predict the side-effects of reentrancy at this point in shutdown. This is called after the message loop in BrowserMain has run, so it's not nested. > >> If the user has the kClearPluginLSODataOnExit pref set, why not just > >> call Flash's ClearLSO function each time before we shutdown the Flash > >> process? IIRC, plugin processes are also shut down after 30 seconds if the plug-in is not used. Do plugin processes even get notified if the browser is shut down or are they just killed if the browser process exits? > > just to be clear, I mean this decision should be made in the plugin > > process. the browser asynchronously updates the flash process, if it's > > running, on changes for this pref, and also of course tells it the value > > when it starts. then code in PluginProcess can call the Flash function > > depending on the cached value. > > > > > >> That way there's no slowing down of shutdown, which is > > > > critical, and it would avoid any race conditions that you mention. > >> > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... > >> File chrome/browser/plugin_data_remover.cc (right): > >> > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... > >> chrome/browser/plugin_data_remover.cc:22: const std::string > >> min_flash_version = "10.2"; > >> these globals don't seem to match our global naming convention, i.e. > >> they look like local variables. I suggest prefixing them with "g_". > >> > >> we also avoid having any globals that use types with > >> constructors/destructors. the reasons are that it slows down > >> startup/shutdown slightly (see davemoore's crusade to remove them for > >> chrome OS perf reasons, and also why we have base::LazyInstance), and > >> also there's a limit to how many objects the CRT can keep track of. can > >> you make the strings just be const char*? > > btw sorry for not catching this in the original review :) Uh, sure. Sorry for forgetting it ;-) > > > > > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... > >> File chrome/browser/plugin_service.cc (right): > >> > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... > >> chrome/browser/plugin_service.cc:216: if (plugin_path.value().empty()) > >> why is this needed? Won't FindPluginProcess return null in this case? It will, which means that we try to create a new plugin process with an empty path, which will DCHECK a couple of lines further down. > >> > >> > >> http://codereview.chromium.org/5278001/ > >> > > > >
On Mon, Nov 29, 2010 at 9:44 AM, <bauerb@chromium.org> wrote: > On 2010/11/29 17:28:16, John Abd-El-Malek wrote: > >> On Mon, Nov 29, 2010 at 9:21 AM, John Abd-El-Malek <mailto: >> jam@chromium.org> >> > wrote: > > > > >> > >> > On Mon, Nov 29, 2010 at 9:14 AM, <mailto:jam@chromium.org> wrote: >> > >> >> >> >> >> >> >> > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... > >> >> File chrome/browser/browser_shutdown.cc (right): >> >> >> >> >> >> >> > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... > >> >> chrome/browser/browser_shutdown.cc:142: >> >> remover.StartRemoving(base::Time(), new MessageLoop::QuitTask()); >> >> I have a big concern about blocking our shutdown to wait on creating a >> >> Flash process. That could be pretty slow for various reasons (i.e. >> >> system is overloaded, Flash loads slowly). If a user is waiting to >> >> restart Chrome again (say because they shutdown Chrome for an >> >> auto-update), and try to start Chrome manually while this is happening, >> >> it won't work. They might shutdown Chrome processes manually which >> >> would be unfortunate as it could lead to profile corruption. >> >> >> >> Running a nested message loop also seems kind of scary, it's really >> hard >> >> to predict the side-effects of reentrancy at this point in shutdown. >> > > This is called after the message loop in BrowserMain has run, so it's not > nested. I was more worried about some objects already being destructed at this point, and tasks running that need them. i.e. looking through ~BrowserProcessImpl, all the other thread have already gone along with a bunch of other objects. > > >> If the user has the kClearPluginLSODataOnExit pref set, why not just >> >> call Flash's ClearLSO function each time before we shutdown the Flash >> >> process? >> > > IIRC, plugin processes are also shut down after 30 seconds if the plug-in > is not > used. > right. Do we need the LSO data to only be cleared when the browser shutdown, or can we do it more often? > > Do plugin processes even get notified if the browser is shut down or are > they > just killed if the browser process exits? Any running plugin processes will die in one of two ways, depending on timing. either they'll notice that all their renderer-channels have been closed, and thus will drop their ref count and wait the 30 seconds (although it's unlikely that the browser shutdown will have taken this long), or they'll notice that their browser channel has been closed, and will immediately close. > > > > just to be clear, I mean this decision should be made in the plugin >> > process. the browser asynchronously updates the flash process, if it's >> > running, on changes for this pref, and also of course tells it the value >> > when it starts. then code in PluginProcess can call the Flash function >> > depending on the cached value. >> > >> > >> >> That way there's no slowing down of shutdown, which is >> > >> > critical, and it would avoid any race conditions that you mention. >> >> >> >> >> >> >> > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... > >> >> File chrome/browser/plugin_data_remover.cc (right): >> >> >> >> >> >> >> > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... > >> >> chrome/browser/plugin_data_remover.cc:22: const std::string >> >> min_flash_version = "10.2"; >> >> these globals don't seem to match our global naming convention, i.e. >> >> they look like local variables. I suggest prefixing them with "g_". >> >> >> >> we also avoid having any globals that use types with >> >> constructors/destructors. the reasons are that it slows down >> >> startup/shutdown slightly (see davemoore's crusade to remove them for >> >> chrome OS perf reasons, and also why we have base::LazyInstance), and >> >> also there's a limit to how many objects the CRT can keep track of. can >> >> you make the strings just be const char*? >> > > btw sorry for not catching this in the original review :) >> > > Uh, sure. Sorry for forgetting it ;-) > > > > > > >> >> >> >> >> > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... > >> >> File chrome/browser/plugin_service.cc (right): >> >> >> >> >> >> >> > > > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... > >> >> chrome/browser/plugin_service.cc:216: if (plugin_path.value().empty()) >> >> why is this needed? Won't FindPluginProcess return null in this case? >> > > It will, which means that we try to create a new plugin process with an > empty > path, which will DCHECK a couple of lines further down. we should just remove that dcheck then, if that condition is now possible, instead of adding code to work around it. > > > >> >> >> >> >> http://codereview.chromium.org/5278001/ >> >> >> > >> > >> > > > > http://codereview.chromium.org/5278001/ >
On Mon, Nov 29, 2010 at 19:28, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Mon, Nov 29, 2010 at 9:44 AM, <bauerb@chromium.org> wrote: >> >> On 2010/11/29 17:28:16, John Abd-El-Malek wrote: >>> >>> On Mon, Nov 29, 2010 at 9:21 AM, John Abd-El-Malek >>> <mailto:jam@chromium.org> >> >> wrote: >> >>> > >>> > >>> > On Mon, Nov 29, 2010 at 9:14 AM, <mailto:jam@chromium.org> wrote: >>> > >>> >> >>> >> >>> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... >>> >>> >> File chrome/browser/browser_shutdown.cc (right): >>> >> >>> >> >>> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... >>> >>> >> chrome/browser/browser_shutdown.cc:142: >>> >> remover.StartRemoving(base::Time(), new MessageLoop::QuitTask()); >>> >> I have a big concern about blocking our shutdown to wait on creating a >>> >> Flash process. That could be pretty slow for various reasons (i.e. >>> >> system is overloaded, Flash loads slowly). If a user is waiting to >>> >> restart Chrome again (say because they shutdown Chrome for an >>> >> auto-update), and try to start Chrome manually while this is >>> >> happening, >>> >> it won't work. They might shutdown Chrome processes manually which >>> >> would be unfortunate as it could lead to profile corruption. >>> >> >>> >> Running a nested message loop also seems kind of scary, it's really >>> >> hard >>> >> to predict the side-effects of reentrancy at this point in shutdown. >> >> This is called after the message loop in BrowserMain has run, so it's not >> nested. > > I was more worried about some objects already being destructed at this > point, and tasks running that need them. i.e. looking through > ~BrowserProcessImpl, all the other thread have already gone along with a > bunch of other objects. The BrowserProcessImpl object is deleted by browser_shutdown::Shutdown() after my code, so the threads should still be there (apart from them, I only need the PluginService, which is still around as well). >>> >> If the user has the kClearPluginLSODataOnExit pref set, why not just >>> >> call Flash's ClearLSO function each time before we shutdown the Flash >>> >> process? >> >> IIRC, plugin processes are also shut down after 30 seconds if the plug-in >> is not >> used. > > right. Do we need the LSO data to only be cleared when the browser > shutdown, or can we do it more often? I think that would create very strange behavior. Whether a plugin process is running or not should be totally transparent to the user, and having stored data disappear in the middle of a browser session would be very confusing. >> >> Do plugin processes even get notified if the browser is shut down or are >> they >> just killed if the browser process exits? > > Any running plugin processes will die in one of two ways, depending on > timing. either they'll notice that all their renderer-channels have been > closed, and thus will drop their ref count and wait the 30 seconds (although > it's unlikely that the browser shutdown will have taken this long), or > they'll notice that their browser channel has been closed, and will > immediately close. So... do we know that they won't be killed when the browser process exits? If so, that would mean that we wouldn't need for the NPP_ClearSiteData call to finish before quitting the browser, so we could do it asynchronously. We'd still need to start a Flash process if it is not running, but we could pass it a command line flag to call NPP_ClearSiteData and exit immediately afterwards. Does that sound reasonable? >>> > just to be clear, I mean this decision should be made in the plugin >>> > process. the browser asynchronously updates the flash process, if it's >>> > running, on changes for this pref, and also of course tells it the >>> > value >>> > when it starts. then code in PluginProcess can call the Flash function >>> > depending on the cached value. >>> > >>> > >>> >> That way there's no slowing down of shutdown, which is >>> > >>> > critical, and it would avoid any race conditions that you mention. >>> >> >>> >> >>> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... >>> >>> >> File chrome/browser/plugin_data_remover.cc (right): >>> >> >>> >> >>> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... >>> >>> >> chrome/browser/plugin_data_remover.cc:22: const std::string >>> >> min_flash_version = "10.2"; >>> >> these globals don't seem to match our global naming convention, i.e. >>> >> they look like local variables. I suggest prefixing them with "g_". >>> >> >>> >> we also avoid having any globals that use types with >>> >> constructors/destructors. the reasons are that it slows down >>> >> startup/shutdown slightly (see davemoore's crusade to remove them for >>> >> chrome OS perf reasons, and also why we have base::LazyInstance), and >>> >> also there's a limit to how many objects the CRT can keep track of. >>> >> can >>> >> you make the strings just be const char*? >> >>> btw sorry for not catching this in the original review :) >> >> Uh, sure. Sorry for forgetting it ;-) >> >> >> >>> > >>> >> >>> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... >>> >>> >> File chrome/browser/plugin_service.cc (right): >>> >> >>> >> >>> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... >>> >>> >> chrome/browser/plugin_service.cc:216: if (plugin_path.value().empty()) >>> >> why is this needed? Won't FindPluginProcess return null in this case? >> >> It will, which means that we try to create a new plugin process with an >> empty >> path, which will DCHECK a couple of lines further down. > > we should just remove that dcheck then, if that condition is now possible, > instead of adding code to work around it. >> >>> >> >>> >> >>> >> http://codereview.chromium.org/5278001/ >>> >> >>> > >>> > >> >> >> >> http://codereview.chromium.org/5278001/ > >
On Mon, Nov 29, 2010 at 12:16 PM, Bernhard Bauer <bauerb@chromium.org> wrote: > On Mon, Nov 29, 2010 at 19:28, John Abd-El-Malek <jam@chromium.org> wrote: > > > > > > On Mon, Nov 29, 2010 at 9:44 AM, <bauerb@chromium.org> wrote: > >> > >> On 2010/11/29 17:28:16, John Abd-El-Malek wrote: > >>> > >>> On Mon, Nov 29, 2010 at 9:21 AM, John Abd-El-Malek > >>> <mailto:jam@chromium.org> > >> > >> wrote: > >> > >>> > > >>> > > >>> > On Mon, Nov 29, 2010 at 9:14 AM, <mailto:jam@chromium.org> wrote: > >>> > > >>> >> > >>> >> > >>> >> > >> > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... > >>> > >>> >> File chrome/browser/browser_shutdown.cc (right): > >>> >> > >>> >> > >>> >> > >> > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... > >>> > >>> >> chrome/browser/browser_shutdown.cc:142: > >>> >> remover.StartRemoving(base::Time(), new MessageLoop::QuitTask()); > >>> >> I have a big concern about blocking our shutdown to wait on creating > a > >>> >> Flash process. That could be pretty slow for various reasons (i.e. > >>> >> system is overloaded, Flash loads slowly). If a user is waiting to > >>> >> restart Chrome again (say because they shutdown Chrome for an > >>> >> auto-update), and try to start Chrome manually while this is > >>> >> happening, > >>> >> it won't work. They might shutdown Chrome processes manually which > >>> >> would be unfortunate as it could lead to profile corruption. > >>> >> > >>> >> Running a nested message loop also seems kind of scary, it's really > >>> >> hard > >>> >> to predict the side-effects of reentrancy at this point in shutdown. > >> > >> This is called after the message loop in BrowserMain has run, so it's > not > >> nested. > > > > I was more worried about some objects already being destructed at this > > point, and tasks running that need them. i.e. looking through > > ~BrowserProcessImpl, all the other thread have already gone along with a > > bunch of other objects. > > The BrowserProcessImpl object is deleted by > browser_shutdown::Shutdown() after my code, so the threads should > still be there (apart from them, I only need the PluginService, which > is still around as well). > ah, I incorrectly read that code, nm > > >>> >> If the user has the kClearPluginLSODataOnExit pref set, why not just > >>> >> call Flash's ClearLSO function each time before we shutdown the > Flash > >>> >> process? > >> > >> IIRC, plugin processes are also shut down after 30 seconds if the > plug-in > >> is not > >> used. > > > > right. Do we need the LSO data to only be cleared when the browser > > shutdown, or can we do it more often? > > I think that would create very strange behavior. Whether a plugin > process is running or not should be totally transparent to the user, > and having stored data disappear in the middle of a browser session > would be very confusing. > I agree that would be confusing. Can we delete the data on the first run of the Flash process after Chrome was shutdown? Chrome shutdown time is something we work really hard to keep small for the reasons above. > > >> > >> Do plugin processes even get notified if the browser is shut down or are > >> they > >> just killed if the browser process exits? > > > > Any running plugin processes will die in one of two ways, depending on > > timing. either they'll notice that all their renderer-channels have been > > closed, and thus will drop their ref count and wait the 30 seconds > (although > > it's unlikely that the browser shutdown will have taken this long), or > > they'll notice that their browser channel has been closed, and will > > immediately close. > > So... do we know that they won't be killed when the browser process > exits? It's a race condition based on if the plugin process notices that the browser process has gone away (second condition above) before the IPC message for the clear LSO is received. If the IPC message is received first, then things will be ok. If ChildThread::OnChannelError() gets called first though, the IPC message won't be processed. > If so, that would mean that we wouldn't need for the > NPP_ClearSiteData call to finish before quitting the browser, so we > could do it asynchronously. We'd still need to start a Flash process > if it is not running, but we could pass it a command line flag to call > NPP_ClearSiteData and exit immediately afterwards. > If the plugin process is busy though, this could interfere with auto-updating. Adding Carlos to get his opinion. The issue is that if the browser process has shutdown, and there is one plugin process running that has a lock on the binaries, if the user restarts Chrome then the new browser process might not be able to move the binaries. Carlos, wdyt? > > Does that sound reasonable? > > >>> > just to be clear, I mean this decision should be made in the plugin > >>> > process. the browser asynchronously updates the flash process, if > it's > >>> > running, on changes for this pref, and also of course tells it the > >>> > value > >>> > when it starts. then code in PluginProcess can call the Flash > function > >>> > depending on the cached value. > >>> > > >>> > > >>> >> That way there's no slowing down of shutdown, which is > >>> > > >>> > critical, and it would avoid any race conditions that you mention. > >>> >> > >>> >> > >>> >> > >> > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... > >>> > >>> >> File chrome/browser/plugin_data_remover.cc (right): > >>> >> > >>> >> > >>> >> > >> > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... > >>> > >>> >> chrome/browser/plugin_data_remover.cc:22: const std::string > >>> >> min_flash_version = "10.2"; > >>> >> these globals don't seem to match our global naming convention, i.e. > >>> >> they look like local variables. I suggest prefixing them with "g_". > >>> >> > >>> >> we also avoid having any globals that use types with > >>> >> constructors/destructors. the reasons are that it slows down > >>> >> startup/shutdown slightly (see davemoore's crusade to remove them > for > >>> >> chrome OS perf reasons, and also why we have base::LazyInstance), > and > >>> >> also there's a limit to how many objects the CRT can keep track of. > >>> >> can > >>> >> you make the strings just be const char*? > >> > >>> btw sorry for not catching this in the original review :) > >> > >> Uh, sure. Sorry for forgetting it ;-) > >> > >> > >> > >>> > > >>> >> > >>> >> > >> > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... > >>> > >>> >> File chrome/browser/plugin_service.cc (right): > >>> >> > >>> >> > >>> >> > >> > >> > >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... > >>> > >>> >> chrome/browser/plugin_service.cc:216: if > (plugin_path.value().empty()) > >>> >> why is this needed? Won't FindPluginProcess return null in this > case? > >> > >> It will, which means that we try to create a new plugin process with an > >> empty > >> path, which will DCHECK a couple of lines further down. > > > > we should just remove that dcheck then, if that condition is now > possible, > > instead of adding code to work around it. > >> > >>> >> > >>> >> > >>> >> http://codereview.chromium.org/5278001/ > >>> >> > >>> > > >>> > > >> > >> > >> > >> http://codereview.chromium.org/5278001/ > > > > > >
On Mon, Nov 29, 2010 at 21:42, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Mon, Nov 29, 2010 at 12:16 PM, Bernhard > Bauer <bauerb@chromium.org> wrote: >> >> On Mon, Nov 29, 2010 at 19:28, John Abd-El-Malek <jam@chromium.org> wrote: >> > >> > >> > On Mon, Nov 29, 2010 at 9:44 AM, <bauerb@chromium.org> wrote: >> >> >> >> On 2010/11/29 17:28:16, John Abd-El-Malek wrote: >> >>> >> >>> On Mon, Nov 29, 2010 at 9:21 AM, John Abd-El-Malek >> >>> <mailto:jam@chromium.org> >> >> >> >> wrote: >> >> >> >>> > >> >>> > >> >>> > On Mon, Nov 29, 2010 at 9:14 AM, <mailto:jam@chromium.org> wrote: >> >>> > >> >>> >> >> >>> >> >> >>> >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shutdown.cc >> >>> >> >>> >> File chrome/browser/browser_shutdown.cc (right): >> >>> >> >> >>> >> >> >>> >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shutdown.cc#newcode142 >> >>> >> >>> >> chrome/browser/browser_shutdown.cc:142: >> >>> >> remover.StartRemoving(base::Time(), new MessageLoop::QuitTask()); >> >>> >> I have a big concern about blocking our shutdown to wait on >> >>> >> creating a >> >>> >> Flash process. That could be pretty slow for various reasons (i.e. >> >>> >> system is overloaded, Flash loads slowly). If a user is waiting to >> >>> >> restart Chrome again (say because they shutdown Chrome for an >> >>> >> auto-update), and try to start Chrome manually while this is >> >>> >> happening, >> >>> >> it won't work. They might shutdown Chrome processes manually which >> >>> >> would be unfortunate as it could lead to profile corruption. >> >>> >> >> >>> >> Running a nested message loop also seems kind of scary, it's really >> >>> >> hard >> >>> >> to predict the side-effects of reentrancy at this point in >> >>> >> shutdown. >> >> >> >> This is called after the message loop in BrowserMain has run, so it's >> >> not >> >> nested. >> > >> > I was more worried about some objects already being destructed at this >> > point, and tasks running that need them. i.e. looking through >> > ~BrowserProcessImpl, all the other thread have already gone along with a >> > bunch of other objects. >> >> The BrowserProcessImpl object is deleted by >> browser_shutdown::Shutdown() after my code, so the threads should >> still be there (apart from them, I only need the PluginService, which >> is still around as well). > > ah, I incorrectly read that code, nm > >> >> >>> >> If the user has the kClearPluginLSODataOnExit pref set, why not >> >>> >> just >> >>> >> call Flash's ClearLSO function each time before we shutdown the >> >>> >> Flash >> >>> >> process? >> >> >> >> IIRC, plugin processes are also shut down after 30 seconds if the >> >> plug-in >> >> is not >> >> used. >> > >> > right. Do we need the LSO data to only be cleared when the browser >> > shutdown, or can we do it more often? >> >> I think that would create very strange behavior. Whether a plugin >> process is running or not should be totally transparent to the user, >> and having stored data disappear in the middle of a browser session >> would be very confusing. > > I agree that would be confusing. > Can we delete the data on the first run of the Flash process after Chrome > was shutdown? Chrome shutdown time is something we work really hard to keep > small for the reasons above. That would still leave the data for other browsers. I think the correct solution is to note when it's only an idle shutdown and not delete the data in that case. >> >> >> >> >> Do plugin processes even get notified if the browser is shut down or >> >> are >> >> they >> >> just killed if the browser process exits? >> > >> > Any running plugin processes will die in one of two ways, depending on >> > timing. either they'll notice that all their renderer-channels have >> > been >> > closed, and thus will drop their ref count and wait the 30 seconds >> > (although >> > it's unlikely that the browser shutdown will have taken this long), or >> > they'll notice that their browser channel has been closed, and will >> > immediately close. >> >> So... do we know that they won't be killed when the browser process >> exits? > > It's a race condition based on if the plugin process notices that the > browser process has gone away (second condition above) before the IPC > message for the clear LSO is received. If the IPC message is received > first, then things will be ok. If ChildThread::OnChannelError() gets called > first though, the IPC message won't be processed. But if we do what you suggested earlier and send the plugin a message to clear data on shutdown when the preference changes / on startup, we avoid that race condition. >> >> If so, that would mean that we wouldn't need for the >> NPP_ClearSiteData call to finish before quitting the browser, so we >> could do it asynchronously. We'd still need to start a Flash process >> if it is not running, but we could pass it a command line flag to call >> NPP_ClearSiteData and exit immediately afterwards. > > If the plugin process is busy though, this could interfere with > auto-updating. Adding Carlos to get his opinion. > The issue is that if the browser process has shutdown, and there is one > plugin process running that has a lock on the binaries, if the user restarts > Chrome then the new browser process might not be able to move the binaries. > Carlos, wdyt? >> >> Does that sound reasonable? >> >> >>> > just to be clear, I mean this decision should be made in the plugin >> >>> > process. the browser asynchronously updates the flash process, if >> >>> > it's >> >>> > running, on changes for this pref, and also of course tells it the >> >>> > value >> >>> > when it starts. then code in PluginProcess can call the Flash >> >>> > function >> >>> > depending on the cached value. >> >>> > >> >>> > >> >>> >> That way there's no slowing down of shutdown, which is >> >>> > >> >>> > critical, and it would avoid any race conditions that you mention. >> >>> >> >> >>> >> >> >>> >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_remover.cc >> >>> >> >>> >> File chrome/browser/plugin_data_remover.cc (right): >> >>> >> >> >>> >> >> >>> >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_remover.cc#newcode22 >> >>> >> >>> >> chrome/browser/plugin_data_remover.cc:22: const std::string >> >>> >> min_flash_version = "10.2"; >> >>> >> these globals don't seem to match our global naming convention, >> >>> >> i.e. >> >>> >> they look like local variables. I suggest prefixing them with >> >>> >> "g_". >> >>> >> >> >>> >> we also avoid having any globals that use types with >> >>> >> constructors/destructors. the reasons are that it slows down >> >>> >> startup/shutdown slightly (see davemoore's crusade to remove them >> >>> >> for >> >>> >> chrome OS perf reasons, and also why we have base::LazyInstance), >> >>> >> and >> >>> >> also there's a limit to how many objects the CRT can keep track of. >> >>> >> can >> >>> >> you make the strings just be const char*? >> >> >> >>> btw sorry for not catching this in the original review :) >> >> >> >> Uh, sure. Sorry for forgetting it ;-) >> >> >> >> >> >> >> >>> > >> >>> >> >> >>> >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_service.cc >> >>> >> >>> >> File chrome/browser/plugin_service.cc (right): >> >>> >> >> >>> >> >> >>> >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_service.cc#newcode216 >> >>> >> >>> >> chrome/browser/plugin_service.cc:216: if >> >>> >> (plugin_path.value().empty()) >> >>> >> why is this needed? Won't FindPluginProcess return null in this >> >>> >> case? >> >> >> >> It will, which means that we try to create a new plugin process with an >> >> empty >> >> path, which will DCHECK a couple of lines further down. >> > >> > we should just remove that dcheck then, if that condition is now >> > possible, >> > instead of adding code to work around it. >> >> >> >>> >> >> >>> >> >> >>> >> http://codereview.chromium.org/5278001/ >> >>> >> >> >>> > >> >>> > >> >> >> >> >> >> >> >> http://codereview.chromium.org/5278001/ >> > >> > >> >
On Mon, Nov 29, 2010 at 12:56 PM, Bernhard Bauer <bauerb@chromium.org>wrote: > On Mon, Nov 29, 2010 at 21:42, John Abd-El-Malek <jam@chromium.org> wrote: > > > > > > On Mon, Nov 29, 2010 at 12:16 PM, Bernhard > > Bauer <bauerb@chromium.org> wrote: > >> > >> On Mon, Nov 29, 2010 at 19:28, John Abd-El-Malek <jam@chromium.org> > wrote: > >> > > >> > > >> > On Mon, Nov 29, 2010 at 9:44 AM, <bauerb@chromium.org> wrote: > >> >> > >> >> On 2010/11/29 17:28:16, John Abd-El-Malek wrote: > >> >>> > >> >>> On Mon, Nov 29, 2010 at 9:21 AM, John Abd-El-Malek > >> >>> <mailto:jam@chromium.org> > >> >> > >> >> wrote: > >> >> > >> >>> > > >> >>> > > >> >>> > On Mon, Nov 29, 2010 at 9:14 AM, <mailto:jam@chromium.org> wrote: > >> >>> > > >> >>> >> > >> >>> >> > >> >>> >> > >> >> > >> >> > >> > >> >> >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... > >> >>> > >> >>> >> File chrome/browser/browser_shutdown.cc (right): > >> >>> >> > >> >>> >> > >> >>> >> > >> >> > >> >> > >> > >> >> >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/browser_shut... > >> >>> > >> >>> >> chrome/browser/browser_shutdown.cc:142: > >> >>> >> remover.StartRemoving(base::Time(), new MessageLoop::QuitTask()); > >> >>> >> I have a big concern about blocking our shutdown to wait on > >> >>> >> creating a > >> >>> >> Flash process. That could be pretty slow for various reasons > (i.e. > >> >>> >> system is overloaded, Flash loads slowly). If a user is waiting > to > >> >>> >> restart Chrome again (say because they shutdown Chrome for an > >> >>> >> auto-update), and try to start Chrome manually while this is > >> >>> >> happening, > >> >>> >> it won't work. They might shutdown Chrome processes manually > which > >> >>> >> would be unfortunate as it could lead to profile corruption. > >> >>> >> > >> >>> >> Running a nested message loop also seems kind of scary, it's > really > >> >>> >> hard > >> >>> >> to predict the side-effects of reentrancy at this point in > >> >>> >> shutdown. > >> >> > >> >> This is called after the message loop in BrowserMain has run, so it's > >> >> not > >> >> nested. > >> > > >> > I was more worried about some objects already being destructed at this > >> > point, and tasks running that need them. i.e. looking through > >> > ~BrowserProcessImpl, all the other thread have already gone along with > a > >> > bunch of other objects. > >> > >> The BrowserProcessImpl object is deleted by > >> browser_shutdown::Shutdown() after my code, so the threads should > >> still be there (apart from them, I only need the PluginService, which > >> is still around as well). > > > > ah, I incorrectly read that code, nm > > > >> > >> >>> >> If the user has the kClearPluginLSODataOnExit pref set, why not > >> >>> >> just > >> >>> >> call Flash's ClearLSO function each time before we shutdown the > >> >>> >> Flash > >> >>> >> process? > >> >> > >> >> IIRC, plugin processes are also shut down after 30 seconds if the > >> >> plug-in > >> >> is not > >> >> used. > >> > > >> > right. Do we need the LSO data to only be cleared when the browser > >> > shutdown, or can we do it more often? > >> > >> I think that would create very strange behavior. Whether a plugin > >> process is running or not should be totally transparent to the user, > >> and having stored data disappear in the middle of a browser session > >> would be very confusing. > > > > I agree that would be confusing. > > Can we delete the data on the first run of the Flash process after Chrome > > was shutdown? Chrome shutdown time is something we work really hard to > keep > > small for the reasons above. > > That would still leave the data for other browsers. I think the > correct solution is to note when it's only an idle shutdown and not > delete the data in that case. > If we do that though, we'll still have the possibility of gating Chrome's shutdown on starting and running Flash. > > >> > >> >> > >> >> Do plugin processes even get notified if the browser is shut down or > >> >> are > >> >> they > >> >> just killed if the browser process exits? > >> > > >> > Any running plugin processes will die in one of two ways, depending on > >> > timing. either they'll notice that all their renderer-channels have > >> > been > >> > closed, and thus will drop their ref count and wait the 30 seconds > >> > (although > >> > it's unlikely that the browser shutdown will have taken this long), or > >> > they'll notice that their browser channel has been closed, and will > >> > immediately close. > >> > >> So... do we know that they won't be killed when the browser process > >> exits? > > > > It's a race condition based on if the plugin process notices that the > > browser process has gone away (second condition above) before the IPC > > message for the clear LSO is received. If the IPC message is received > > first, then things will be ok. If ChildThread::OnChannelError() gets > called > > first though, the IPC message won't be processed. > > But if we do what you suggested earlier and send the plugin a message > to clear data on shutdown when the preference changes / on startup, we > avoid that race condition. > > >> > >> If so, that would mean that we wouldn't need for the > >> NPP_ClearSiteData call to finish before quitting the browser, so we > >> could do it asynchronously. We'd still need to start a Flash process > >> if it is not running, but we could pass it a command line flag to call > >> NPP_ClearSiteData and exit immediately afterwards. > > > > If the plugin process is busy though, this could interfere with > > auto-updating. Adding Carlos to get his opinion. > > The issue is that if the browser process has shutdown, and there is one > > plugin process running that has a lock on the binaries, if the user > restarts > > Chrome then the new browser process might not be able to move the > binaries. > > Carlos, wdyt? > >> > >> Does that sound reasonable? > >> > >> >>> > just to be clear, I mean this decision should be made in the > plugin > >> >>> > process. the browser asynchronously updates the flash process, if > >> >>> > it's > >> >>> > running, on changes for this pref, and also of course tells it the > >> >>> > value > >> >>> > when it starts. then code in PluginProcess can call the Flash > >> >>> > function > >> >>> > depending on the cached value. > >> >>> > > >> >>> > > >> >>> >> That way there's no slowing down of shutdown, which is > >> >>> > > >> >>> > critical, and it would avoid any race conditions that you mention. > >> >>> >> > >> >>> >> > >> >>> >> > >> >> > >> >> > >> > >> >> >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... > >> >>> > >> >>> >> File chrome/browser/plugin_data_remover.cc (right): > >> >>> >> > >> >>> >> > >> >>> >> > >> >> > >> >> > >> > >> >> >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_data_... > >> >>> > >> >>> >> chrome/browser/plugin_data_remover.cc:22: const std::string > >> >>> >> min_flash_version = "10.2"; > >> >>> >> these globals don't seem to match our global naming convention, > >> >>> >> i.e. > >> >>> >> they look like local variables. I suggest prefixing them with > >> >>> >> "g_". > >> >>> >> > >> >>> >> we also avoid having any globals that use types with > >> >>> >> constructors/destructors. the reasons are that it slows down > >> >>> >> startup/shutdown slightly (see davemoore's crusade to remove them > >> >>> >> for > >> >>> >> chrome OS perf reasons, and also why we have base::LazyInstance), > >> >>> >> and > >> >>> >> also there's a limit to how many objects the CRT can keep track > of. > >> >>> >> can > >> >>> >> you make the strings just be const char*? > >> >> > >> >>> btw sorry for not catching this in the original review :) > >> >> > >> >> Uh, sure. Sorry for forgetting it ;-) > >> >> > >> >> > >> >> > >> >>> > > >> >>> >> > >> >>> >> > >> >> > >> >> > >> > >> >> >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... > >> >>> > >> >>> >> File chrome/browser/plugin_service.cc (right): > >> >>> >> > >> >>> >> > >> >>> >> > >> >> > >> >> > >> > >> >> >> > http://codereview.chromium.org/5278001/diff/22018/chrome/browser/plugin_servi... > >> >>> > >> >>> >> chrome/browser/plugin_service.cc:216: if > >> >>> >> (plugin_path.value().empty()) > >> >>> >> why is this needed? Won't FindPluginProcess return null in this > >> >>> >> case? > >> >> > >> >> It will, which means that we try to create a new plugin process with > an > >> >> empty > >> >> path, which will DCHECK a couple of lines further down. > >> > > >> > we should just remove that dcheck then, if that condition is now > >> > possible, > >> > instead of adding code to work around it. > >> >> > >> >>> >> > >> >>> >> > >> >>> >> http://codereview.chromium.org/5278001/ > >> >>> >> > >> >>> > > >> >>> > > >> >> > >> >> > >> >> > >> >> http://codereview.chromium.org/5278001/ > >> > > >> > > >> > > >
On Mon, Nov 29, 2010 at 9:31 AM, <bauerb@chromium.org> wrote: > On 2010/11/29 17:07:34, darin wrote: > >> Oh... but what if Flash is running in a separate browser? Doesn't it use >> a >> common location for all instances, across all browsers? >> > > -Darin >> > > Shouldn't it be the responsibility of the plug-in if it implements the API > to > make sure that it can delete data while it's running in another browser? > Yes, sorry... I forgot that deletion of this data was performed by invoking a NPAPI method. Given that, I agree with John that we should not block browser shutdown for this. -Darin > > On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher <mailto:darin@chromium.org> >> > wrote: > > > We should be able to know when all plugin processes are shutdown. /me >> > looks to jam for the details. >> > >> > -Darin >> > >> > >> > >> > On Mon, Nov 29, 2010 at 4:28 AM, <mailto:bauerb@chromium.org> wrote: >> > >> >> After (offline) discussions with Jochen, I moved the code to delete LSO >> >> data on >> >> shutdown to browser_shutdown::Shutdown(). >> >> >> >> Darin/John, Jochen had some concerns about possible race conditions >> where >> >> a >> >> plug-in would set LSO data after the hook at shutdown has run. When >> would >> >> be the >> >> best time to run the hook to avoid this? >> >> >> >> >> >> http://codereview.chromium.org/5278001/ >> >> >> > >> > >> > > > > http://codereview.chromium.org/5278001/ >
another idea: can we get the filename from the Flash plugin when we're using it, and delete it directly if the user has this option set? that way we wouldn't need to wait on a process spin-up On Mon, Nov 29, 2010 at 3:45 PM, Darin Fisher <darin@chromium.org> wrote: > > > On Mon, Nov 29, 2010 at 9:31 AM, <bauerb@chromium.org> wrote: > >> On 2010/11/29 17:07:34, darin wrote: >> >>> Oh... but what if Flash is running in a separate browser? Doesn't it use >>> a >>> common location for all instances, across all browsers? >>> >> >> -Darin >>> >> >> Shouldn't it be the responsibility of the plug-in if it implements the API >> to >> make sure that it can delete data while it's running in another browser? >> > > Yes, sorry... I forgot that deletion of this data was performed by invoking > a > NPAPI method. > > Given that, I agree with John that we should not block browser shutdown > for this. > > -Darin > > >> >> On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher <mailto:darin@chromium.org >>> > >>> >> wrote: >> >> > We should be able to know when all plugin processes are shutdown. /me >>> > looks to jam for the details. >>> > >>> > -Darin >>> > >>> > >>> > >>> > On Mon, Nov 29, 2010 at 4:28 AM, <mailto:bauerb@chromium.org> wrote: >>> > >>> >> After (offline) discussions with Jochen, I moved the code to delete >>> LSO >>> >> data on >>> >> shutdown to browser_shutdown::Shutdown(). >>> >> >>> >> Darin/John, Jochen had some concerns about possible race conditions >>> where >>> >> a >>> >> plug-in would set LSO data after the hook at shutdown has run. When >>> would >>> >> be the >>> >> best time to run the hook to avoid this? >>> >> >>> >> >>> >> http://codereview.chromium.org/5278001/ >>> >> >>> > >>> > >>> >> >> >> >> http://codereview.chromium.org/5278001/ >> > >
On Tue, Nov 30, 2010 at 01:18, John Abd-El-Malek <jam@chromium.org> wrote: > another idea: can we get the filename from the Flash plugin when we're using > it, and delete it directly if the user has this option set? that way we > wouldn't need to wait on a process spin-up I thought about that too, but it'd likely involve lots of guesswork, and we don't want to end up deleting the wrong file. This is clearly more complicated than originally anticipated, so I removed the part deleting LSO data at shutdown from the CL. Any comments about the rest? Can we schedule a VC to talk about what to do at shutdown tomorrow-ish? > On Mon, Nov 29, 2010 at 3:45 PM, Darin Fisher <darin@chromium.org> wrote: >> >> >> On Mon, Nov 29, 2010 at 9:31 AM, <bauerb@chromium.org> wrote: >>> >>> On 2010/11/29 17:07:34, darin wrote: >>>> >>>> Oh... but what if Flash is running in a separate browser? Doesn't it >>>> use a >>>> common location for all instances, across all browsers? >>> >>>> -Darin >>> >>> Shouldn't it be the responsibility of the plug-in if it implements the >>> API to >>> make sure that it can delete data while it's running in another browser? >> >> Yes, sorry... I forgot that deletion of this data was performed by >> invoking a >> NPAPI method. >> Given that, I agree with John that we should not block browser shutdown >> for this. >> -Darin >> >>>> >>>> On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher >>>> <mailto:darin@chromium.org> >>> >>> wrote: >>> >>>> > We should be able to know when all plugin processes are shutdown. /me >>>> > looks to jam for the details. >>>> > >>>> > -Darin >>>> > >>>> > >>>> > >>>> > On Mon, Nov 29, 2010 at 4:28 AM, <mailto:bauerb@chromium.org> wrote: >>>> > >>>> >> After (offline) discussions with Jochen, I moved the code to delete >>>> >> LSO >>>> >> data on >>>> >> shutdown to browser_shutdown::Shutdown(). >>>> >> >>>> >> Darin/John, Jochen had some concerns about possible race conditions >>>> >> where >>>> >> a >>>> >> plug-in would set LSO data after the hook at shutdown has run. When >>>> >> would >>>> >> be the >>>> >> best time to run the hook to avoid this? >>>> >> >>>> >> >>>> >> http://codereview.chromium.org/5278001/ >>>> >> >>>> > >>>> > >>> >>> >>> >>> http://codereview.chromium.org/5278001/ >> > >
one more: the locations of the LSOs on disk are well known: http://en.wikipedia.org/wiki/Local_Shared_Object#File_locations, we can just delete them directly On Mon, Nov 29, 2010 at 4:18 PM, John Abd-El-Malek <jam@chromium.org> wrote: > another idea: can we get the filename from the Flash plugin when we're > using it, and delete it directly if the user has this option set? that way > we wouldn't need to wait on a process spin-up > > On Mon, Nov 29, 2010 at 3:45 PM, Darin Fisher <darin@chromium.org> wrote: > >> >> >> On Mon, Nov 29, 2010 at 9:31 AM, <bauerb@chromium.org> wrote: >> >>> On 2010/11/29 17:07:34, darin wrote: >>> >>>> Oh... but what if Flash is running in a separate browser? Doesn't it >>>> use a >>>> common location for all instances, across all browsers? >>>> >>> >>> -Darin >>>> >>> >>> Shouldn't it be the responsibility of the plug-in if it implements the >>> API to >>> make sure that it can delete data while it's running in another browser? >>> >> >> Yes, sorry... I forgot that deletion of this data was performed by >> invoking a >> NPAPI method. >> >> Given that, I agree with John that we should not block browser shutdown >> for this. >> >> -Darin >> >> >>> >>> On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher <mailto: >>>> darin@chromium.org> >>>> >>> wrote: >>> >>> > We should be able to know when all plugin processes are shutdown. /me >>>> > looks to jam for the details. >>>> > >>>> > -Darin >>>> > >>>> > >>>> > >>>> > On Mon, Nov 29, 2010 at 4:28 AM, <mailto:bauerb@chromium.org> wrote: >>>> > >>>> >> After (offline) discussions with Jochen, I moved the code to delete >>>> LSO >>>> >> data on >>>> >> shutdown to browser_shutdown::Shutdown(). >>>> >> >>>> >> Darin/John, Jochen had some concerns about possible race conditions >>>> where >>>> >> a >>>> >> plug-in would set LSO data after the hook at shutdown has run. When >>>> would >>>> >> be the >>>> >> best time to run the hook to avoid this? >>>> >> >>>> >> >>>> >> http://codereview.chromium.org/5278001/ >>>> >> >>>> > >>>> > >>>> >>> >>> >>> >>> http://codereview.chromium.org/5278001/ >>> >> >> >
after our discussion, I am ok with starting the process if it's not running as long as we don't block waiting on it. can we get UMA stats added for how often we have to start the process at shutdown to do this? that way we can track this number and reconsider if it's not just a few users.. On Wed, Dec 1, 2010 at 9:03 AM, John Abd-El-Malek <jam@chromium.org> wrote: > one more: the locations of the LSOs on disk are well known: > http://en.wikipedia.org/wiki/Local_Shared_Object#File_locations, we can > just delete them directly > > > On Mon, Nov 29, 2010 at 4:18 PM, John Abd-El-Malek <jam@chromium.org>wrote: > >> another idea: can we get the filename from the Flash plugin when we're >> using it, and delete it directly if the user has this option set? that way >> we wouldn't need to wait on a process spin-up >> >> On Mon, Nov 29, 2010 at 3:45 PM, Darin Fisher <darin@chromium.org> wrote: >> >>> >>> >>> On Mon, Nov 29, 2010 at 9:31 AM, <bauerb@chromium.org> wrote: >>> >>>> On 2010/11/29 17:07:34, darin wrote: >>>> >>>>> Oh... but what if Flash is running in a separate browser? Doesn't it >>>>> use a >>>>> common location for all instances, across all browsers? >>>>> >>>> >>>> -Darin >>>>> >>>> >>>> Shouldn't it be the responsibility of the plug-in if it implements the >>>> API to >>>> make sure that it can delete data while it's running in another browser? >>>> >>> >>> Yes, sorry... I forgot that deletion of this data was performed by >>> invoking a >>> NPAPI method. >>> >>> Given that, I agree with John that we should not block browser shutdown >>> for this. >>> >>> -Darin >>> >>> >>>> >>>> On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher <mailto: >>>>> darin@chromium.org> >>>>> >>>> wrote: >>>> >>>> > We should be able to know when all plugin processes are shutdown. >>>>> /me >>>>> > looks to jam for the details. >>>>> > >>>>> > -Darin >>>>> > >>>>> > >>>>> > >>>>> > On Mon, Nov 29, 2010 at 4:28 AM, <mailto:bauerb@chromium.org> wrote: >>>>> > >>>>> >> After (offline) discussions with Jochen, I moved the code to delete >>>>> LSO >>>>> >> data on >>>>> >> shutdown to browser_shutdown::Shutdown(). >>>>> >> >>>>> >> Darin/John, Jochen had some concerns about possible race conditions >>>>> where >>>>> >> a >>>>> >> plug-in would set LSO data after the hook at shutdown has run. When >>>>> would >>>>> >> be the >>>>> >> best time to run the hook to avoid this? >>>>> >> >>>>> >> >>>>> >> http://codereview.chromium.org/5278001/ >>>>> >> >>>>> > >>>>> > >>>>> >>>> >>>> >>>> >>>> http://codereview.chromium.org/5278001/ >>>> >>> >>> >> >
On Wed, Dec 1, 2010 at 20:11, John Abd-El-Malek <jam@chromium.org> wrote: > after our discussion, I am ok with starting the process if it's not running > as long as we don't block waiting on it. So, my current plan (implemented, but not uploaded yet) is the following: * If the Flash process is already running, we just send the ClearPluginData_Msg and don't wait for its result. * If the process is not running, we start it and pass it a command line switch to call the NPP_ClearSiteData method at startup, and we don't wait for it to start up either. > can we get UMA stats added for how often we have to start the process at > shutdown to do this? that way we can track this number and reconsider if > it's not just a few users.. I've added UMA time histograms for each of the three methods (waiting for the call to finish, just sending the message, and just starting the process), which should give us absolute numbers as well as timing information, right? > > On Wed, Dec 1, 2010 at 9:03 AM, John Abd-El-Malek <jam@chromium.org> wrote: >> >> one more: the locations of the LSOs on disk are well >> known: http://en.wikipedia.org/wiki/Local_Shared_Object#File_locations, we >> can just delete them directly >> >> On Mon, Nov 29, 2010 at 4:18 PM, John Abd-El-Malek <jam@chromium.org> >> wrote: >>> >>> another idea: can we get the filename from the Flash plugin when we're >>> using it, and delete it directly if the user has this option set? that way >>> we wouldn't need to wait on a process spin-up >>> On Mon, Nov 29, 2010 at 3:45 PM, Darin Fisher <darin@chromium.org> wrote: >>>> >>>> >>>> On Mon, Nov 29, 2010 at 9:31 AM, <bauerb@chromium.org> wrote: >>>>> >>>>> On 2010/11/29 17:07:34, darin wrote: >>>>>> >>>>>> Oh... but what if Flash is running in a separate browser? Doesn't it >>>>>> use a >>>>>> common location for all instances, across all browsers? >>>>> >>>>>> -Darin >>>>> >>>>> Shouldn't it be the responsibility of the plug-in if it implements the >>>>> API to >>>>> make sure that it can delete data while it's running in another >>>>> browser? >>>> >>>> Yes, sorry... I forgot that deletion of this data was performed by >>>> invoking a >>>> NPAPI method. >>>> Given that, I agree with John that we should not block browser shutdown >>>> for this. >>>> -Darin >>>> >>>>>> >>>>>> On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher >>>>>> <mailto:darin@chromium.org> >>>>> >>>>> wrote: >>>>> >>>>>> > We should be able to know when all plugin processes are shutdown. >>>>>> > /me >>>>>> > looks to jam for the details. >>>>>> > >>>>>> > -Darin >>>>>> > >>>>>> > >>>>>> > >>>>>> > On Mon, Nov 29, 2010 at 4:28 AM, <mailto:bauerb@chromium.org> wrote: >>>>>> > >>>>>> >> After (offline) discussions with Jochen, I moved the code to delete >>>>>> >> LSO >>>>>> >> data on >>>>>> >> shutdown to browser_shutdown::Shutdown(). >>>>>> >> >>>>>> >> Darin/John, Jochen had some concerns about possible race conditions >>>>>> >> where >>>>>> >> a >>>>>> >> plug-in would set LSO data after the hook at shutdown has run. When >>>>>> >> would >>>>>> >> be the >>>>>> >> best time to run the hook to avoid this? >>>>>> >> >>>>>> >> >>>>>> >> http://codereview.chromium.org/5278001/ >>>>>> >> >>>>>> > >>>>>> > >>>>> >>>>> >>>>> >>>>> http://codereview.chromium.org/5278001/ >>>> >>> >> > >
On 2010/12/02 23:32:11, Bernhard Bauer wrote: > On Wed, Dec 1, 2010 at 20:11, John Abd-El-Malek <mailto:jam@chromium.org> wrote: > > after our discussion, I am ok with starting the process if it's not running > > as long as we don't block waiting on it. > > So, my current plan (implemented, but not uploaded yet) is the following: > > * If the Flash process is already running, we just send the > ClearPluginData_Msg and don't wait for its result. > * If the process is not running, we start it and pass it a command > line switch to call the NPP_ClearSiteData method at startup, and we > don't wait for it to start up either. Okay, I uploaded it at http://codereview.chromium.org/5579002/. Unfortunately, it turns out the browser process does kill all its child processes when it shuts down the IO thread, so that won't work :-( > > can we get UMA stats added for how often we have to start the process at > > shutdown to do this? that way we can track this number and reconsider if > > it's not just a few users.. > > I've added UMA time histograms for each of the three methods (waiting > for the call to finish, just sending the message, and just starting > the process), which should give us absolute numbers as well as timing > information, right? > > > > > On Wed, Dec 1, 2010 at 9:03 AM, John Abd-El-Malek <mailto:jam@chromium.org> wrote: > >> > >> one more: the locations of the LSOs on disk are well > >> known: http://en.wikipedia.org/wiki/Local_Shared_Object#File_locations, we > >> can just delete them directly > >> > >> On Mon, Nov 29, 2010 at 4:18 PM, John Abd-El-Malek <mailto:jam@chromium.org> > >> wrote: > >>> > >>> another idea: can we get the filename from the Flash plugin when we're > >>> using it, and delete it directly if the user has this option set? that way > >>> we wouldn't need to wait on a process spin-up > >>> On Mon, Nov 29, 2010 at 3:45 PM, Darin Fisher <mailto:darin@chromium.org> wrote: > >>>> > >>>> > >>>> On Mon, Nov 29, 2010 at 9:31 AM, <mailto:bauerb@chromium.org> wrote: > >>>>> > >>>>> On 2010/11/29 17:07:34, darin wrote: > >>>>>> > >>>>>> Oh... but what if Flash is running in a separate browser? Doesn't it > >>>>>> use a > >>>>>> common location for all instances, across all browsers? > >>>>> > >>>>>> -Darin > >>>>> > >>>>> Shouldn't it be the responsibility of the plug-in if it implements the > >>>>> API to > >>>>> make sure that it can delete data while it's running in another > >>>>> browser? > >>>> > >>>> Yes, sorry... I forgot that deletion of this data was performed by > >>>> invoking a > >>>> NPAPI method. > >>>> Given that, I agree with John that we should not block browser shutdown > >>>> for this. > >>>> -Darin > >>>> > >>>>>> > >>>>>> On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher > >>>>>> <mailto:darin@chromium.org> > >>>>> > >>>>> wrote: > >>>>> > >>>>>> > We should be able to know when all plugin processes are shutdown. > >>>>>> > /me > >>>>>> > looks to jam for the details. > >>>>>> > > >>>>>> > -Darin > >>>>>> > > >>>>>> > > >>>>>> > > >>>>>> > On Mon, Nov 29, 2010 at 4:28 AM, <mailto:bauerb@chromium.org> wrote: > >>>>>> > > >>>>>> >> After (offline) discussions with Jochen, I moved the code to delete > >>>>>> >> LSO > >>>>>> >> data on > >>>>>> >> shutdown to browser_shutdown::Shutdown(). > >>>>>> >> > >>>>>> >> Darin/John, Jochen had some concerns about possible race conditions > >>>>>> >> where > >>>>>> >> a > >>>>>> >> plug-in would set LSO data after the hook at shutdown has run. When > >>>>>> >> would > >>>>>> >> be the > >>>>>> >> best time to run the hook to avoid this? > >>>>>> >> > >>>>>> >> > >>>>>> >> http://codereview.chromium.org/5278001/ > >>>>>> >> > >>>>>> > > >>>>>> > > >>>>> > >>>>> > >>>>> > >>>>> http://codereview.chromium.org/5278001/ > >>>> > >>> > >> > > > >
John/Darin, any thoughts? I currently don't see any other way than starting up the Flash process if necessary, and waiting for the NPP_ClearSiteData call to complete :-( Tomorrow I'm going to look into starting to clear data as soon as possible on shutdown; that might alleviate it a little bit. Bernhard. On Fri, Dec 3, 2010 at 18:47, <bauerb@chromium.org> wrote: > On 2010/12/02 23:32:11, Bernhard Bauer wrote: >> >> On Wed, Dec 1, 2010 at 20:11, John Abd-El-Malek <mailto:jam@chromium.org> wrote: >> > after our discussion, I am ok with starting the process if it's not >> > running >> > as long as we don't block waiting on it. > >> So, my current plan (implemented, but not uploaded yet) is the following: > >> * If the Flash process is already running, we just send the >> ClearPluginData_Msg and don't wait for its result. >> * If the process is not running, we start it and pass it a command >> line switch to call the NPP_ClearSiteData method at startup, and we >> don't wait for it to start up either. > > Okay, I uploaded it at http://codereview.chromium.org/5579002/. > > Unfortunately, it turns out the browser process does kill all its child > processes when it shuts down the IO thread, so that won't work :-( > >> > can we get UMA stats added for how often we have to start the process at >> > shutdown to do this? that way we can track this number and >> > reconsider if >> > it's not just a few users.. > >> I've added UMA time histograms for each of the three methods (waiting >> for the call to finish, just sending the message, and just starting >> the process), which should give us absolute numbers as well as timing >> information, right? > >> > >> > On Wed, Dec 1, 2010 at 9:03 AM, John Abd-El-Malek >> > <mailto:jam@chromium.org> wrote: >> >> >> >> one more: the locations of the LSOs on disk are well >> >> > > known: http://en.wikipedia.org/wiki/Local_Shared_Object#File_locations, > we >> >> >> can just delete them directly >> >> >> >> On Mon, Nov 29, 2010 at 4:18 PM, John Abd-El-Malek <mailto:jam@chromium.org> >> >> wrote: >> >>> >> >>> another idea: can we get the filename from the Flash plugin when we're >> >>> using it, and delete it directly if the user has this option set? that way >> >>> we wouldn't need to wait on a process spin-up >> >>> On Mon, Nov 29, 2010 at 3:45 PM, Darin Fisher >> >>> <mailto:darin@chromium.org> wrote: >> >>>> >> >>>> >> >>>> On Mon, Nov 29, 2010 at 9:31 AM, <mailto:bauerb@chromium.org> wrote: >> >>>>> >> >>>>> On 2010/11/29 17:07:34, darin wrote: >> >>>>>> >> >>>>>> Oh... but what if Flash is running in a separate browser? >> >>>>>> Doesn't it >> >>>>>> use a >> >>>>>> common location for all instances, across all browsers? >> >>>>> >> >>>>>> -Darin >> >>>>> >> >>>>> Shouldn't it be the responsibility of the plug-in if it implements >> >>>>> the >> >>>>> API to >> >>>>> make sure that it can delete data while it's running in another >> >>>>> browser? >> >>>> >> >>>> Yes, sorry... I forgot that deletion of this data was performed by >> >>>> invoking a >> >>>> NPAPI method. >> >>>> Given that, I agree with John that we should not block browser >> >>>> shutdown >> >>>> for this. >> >>>> -Darin >> >>>> >> >>>>>> >> >>>>>> On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher >> >>>>>> <mailto:darin@chromium.org> >> >>>>> >> >>>>> wrote: >> >>>>> >> >>>>>> > We should be able to know when all plugin processes are shutdown. >> >>>>>> > /me >> >>>>>> > looks to jam for the details. >> >>>>>> > >> >>>>>> > -Darin >> >>>>>> > >> >>>>>> > >> >>>>>> > >> >>>>>> > On Mon, Nov 29, 2010 at 4:28 AM, <mailto:bauerb@chromium.org> >> >>>>>> > wrote: >> >>>>>> > >> >>>>>> >> After (offline) discussions with Jochen, I moved the code to >> >>>>>> >> delete >> >>>>>> >> LSO >> >>>>>> >> data on >> >>>>>> >> shutdown to browser_shutdown::Shutdown(). >> >>>>>> >> >> >>>>>> >> Darin/John, Jochen had some concerns about possible race >> >>>>>> >> conditions >> >>>>>> >> where >> >>>>>> >> a >> >>>>>> >> plug-in would set LSO data after the hook at shutdown has run. >> >>>>>> >> When >> >>>>>> >> would >> >>>>>> >> be the >> >>>>>> >> best time to run the hook to avoid this? >> >>>>>> >> >> >>>>>> >> >> >>>>>> >> http://codereview.chromium.org/5278001/ >> >>>>>> >> >> >>>>>> > >> >>>>>> > >> >>>>> >> >>>>> >> >>>>> >> >>>>> http://codereview.chromium.org/5278001/ >> >>>> >> >>> >> >> >> > >> > > > > > http://codereview.chromium.org/5278001/ >
That sounds a little better yes. Did you track the code where it shuts down all child processes? I guess we can make an exception for Flash, but that seems to carry its own set of problems as well (in case Flash is really hung). On Sun, Dec 5, 2010 at 8:23 AM, Bernhard Bauer <bauerb@chromium.org> wrote: > John/Darin, any thoughts? I currently don't see any other way than > starting up the Flash process if necessary, and waiting for the > NPP_ClearSiteData call to complete :-( > > Tomorrow I'm going to look into starting to clear data as soon as > possible on shutdown; that might alleviate it a little bit. > > Bernhard. > > On Fri, Dec 3, 2010 at 18:47, <bauerb@chromium.org> wrote: > > On 2010/12/02 23:32:11, Bernhard Bauer wrote: > >> > >> On Wed, Dec 1, 2010 at 20:11, John Abd-El-Malek <mailto: > jam@chromium.org> wrote: > >> > after our discussion, I am ok with starting the process if it's not > >> > running > >> > as long as we don't block waiting on it. > > > >> So, my current plan (implemented, but not uploaded yet) is the > following: > > > >> * If the Flash process is already running, we just send the > >> ClearPluginData_Msg and don't wait for its result. > >> * If the process is not running, we start it and pass it a command > >> line switch to call the NPP_ClearSiteData method at startup, and we > >> don't wait for it to start up either. > > > > Okay, I uploaded it at http://codereview.chromium.org/5579002/. > > > > Unfortunately, it turns out the browser process does kill all its child > > processes when it shuts down the IO thread, so that won't work :-( > > > >> > can we get UMA stats added for how often we have to start the process > at > >> > shutdown to do this? that way we can track this number and > >> > reconsider if > >> > it's not just a few users.. > > > >> I've added UMA time histograms for each of the three methods (waiting > >> for the call to finish, just sending the message, and just starting > >> the process), which should give us absolute numbers as well as timing > >> information, right? > > > >> > > >> > On Wed, Dec 1, 2010 at 9:03 AM, John Abd-El-Malek > >> > <mailto:jam@chromium.org> wrote: > >> >> > >> >> one more: the locations of the LSOs on disk are well > >> >> > > > > known: > http://en.wikipedia.org/wiki/Local_Shared_Object#File_locations, > > we > >> > >> >> can just delete them directly > >> >> > >> >> On Mon, Nov 29, 2010 at 4:18 PM, John Abd-El-Malek <mailto: > jam@chromium.org> > >> >> wrote: > >> >>> > >> >>> another idea: can we get the filename from the Flash plugin > when we're > >> >>> using it, and delete it directly if the user has this option set? > that way > >> >>> we wouldn't need to wait on a process spin-up > >> >>> On Mon, Nov 29, 2010 at 3:45 PM, Darin Fisher > >> >>> <mailto:darin@chromium.org> wrote: > >> >>>> > >> >>>> > >> >>>> On Mon, Nov 29, 2010 at 9:31 AM, <mailto:bauerb@chromium.org> > wrote: > >> >>>>> > >> >>>>> On 2010/11/29 17:07:34, darin wrote: > >> >>>>>> > >> >>>>>> Oh... but what if Flash is running in a separate browser? > >> >>>>>> Doesn't it > >> >>>>>> use a > >> >>>>>> common location for all instances, across all browsers? > >> >>>>> > >> >>>>>> -Darin > >> >>>>> > >> >>>>> Shouldn't it be the responsibility of the plug-in if it implements > >> >>>>> the > >> >>>>> API to > >> >>>>> make sure that it can delete data while it's running in another > >> >>>>> browser? > >> >>>> > >> >>>> Yes, sorry... I forgot that deletion of this data was performed by > >> >>>> invoking a > >> >>>> NPAPI method. > >> >>>> Given that, I agree with John that we should not block browser > >> >>>> shutdown > >> >>>> for this. > >> >>>> -Darin > >> >>>> > >> >>>>>> > >> >>>>>> On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher > >> >>>>>> <mailto:darin@chromium.org> > >> >>>>> > >> >>>>> wrote: > >> >>>>> > >> >>>>>> > We should be able to know when all plugin processes are > shutdown. > >> >>>>>> > /me > >> >>>>>> > looks to jam for the details. > >> >>>>>> > > >> >>>>>> > -Darin > >> >>>>>> > > >> >>>>>> > > >> >>>>>> > > >> >>>>>> > On Mon, Nov 29, 2010 at 4:28 AM, <mailto:bauerb@chromium.org> > >> >>>>>> > wrote: > >> >>>>>> > > >> >>>>>> >> After (offline) discussions with Jochen, I moved the code to > >> >>>>>> >> delete > >> >>>>>> >> LSO > >> >>>>>> >> data on > >> >>>>>> >> shutdown to browser_shutdown::Shutdown(). > >> >>>>>> >> > >> >>>>>> >> Darin/John, Jochen had some concerns about possible race > >> >>>>>> >> conditions > >> >>>>>> >> where > >> >>>>>> >> a > >> >>>>>> >> plug-in would set LSO data after the hook at shutdown has run. > >> >>>>>> >> When > >> >>>>>> >> would > >> >>>>>> >> be the > >> >>>>>> >> best time to run the hook to avoid this? > >> >>>>>> >> > >> >>>>>> >> > >> >>>>>> >> http://codereview.chromium.org/5278001/ > >> >>>>>> >> > >> >>>>>> > > >> >>>>>> > > >> >>>>> > >> >>>>> > >> >>>>> > >> >>>>> http://codereview.chromium.org/5278001/ > >> >>>> > >> >>> > >> >> > >> > > >> > > > > > > > > > http://codereview.chromium.org/5278001/ > > >
I'm okay with deferring browser shutdown until the plugin process has finished. Since this feature is not enabled by default, and we imagine few users enabling it, I think we can tolerate slower shutdown for the benefit of simpler code. -Darin On Sun, Dec 5, 2010 at 11:32 PM, John Abd-El-Malek <jam@chromium.org> wrote: > That sounds a little better yes. Did you track the code where it shuts > down all child processes? I guess we can make an exception for Flash, but > that seems to carry its own set of problems as well (in case Flash is really > hung). > > > On Sun, Dec 5, 2010 at 8:23 AM, Bernhard Bauer <bauerb@chromium.org>wrote: > >> John/Darin, any thoughts? I currently don't see any other way than >> starting up the Flash process if necessary, and waiting for the >> NPP_ClearSiteData call to complete :-( >> >> Tomorrow I'm going to look into starting to clear data as soon as >> possible on shutdown; that might alleviate it a little bit. >> >> Bernhard. >> >> On Fri, Dec 3, 2010 at 18:47, <bauerb@chromium.org> wrote: >> > On 2010/12/02 23:32:11, Bernhard Bauer wrote: >> >> >> >> On Wed, Dec 1, 2010 at 20:11, John Abd-El-Malek <mailto: >> jam@chromium.org> wrote: >> >> > after our discussion, I am ok with starting the process if it's not >> >> > running >> >> > as long as we don't block waiting on it. >> > >> >> So, my current plan (implemented, but not uploaded yet) is the >> following: >> > >> >> * If the Flash process is already running, we just send the >> >> ClearPluginData_Msg and don't wait for its result. >> >> * If the process is not running, we start it and pass it a command >> >> line switch to call the NPP_ClearSiteData method at startup, and we >> >> don't wait for it to start up either. >> > >> > Okay, I uploaded it at http://codereview.chromium.org/5579002/. >> > >> > Unfortunately, it turns out the browser process does kill all its child >> > processes when it shuts down the IO thread, so that won't work :-( >> > >> >> > can we get UMA stats added for how often we have to start the process >> at >> >> > shutdown to do this? that way we can track this number and >> >> > reconsider if >> >> > it's not just a few users.. >> > >> >> I've added UMA time histograms for each of the three methods (waiting >> >> for the call to finish, just sending the message, and just starting >> >> the process), which should give us absolute numbers as well as timing >> >> information, right? >> > >> >> > >> >> > On Wed, Dec 1, 2010 at 9:03 AM, John Abd-El-Malek >> >> > <mailto:jam@chromium.org> wrote: >> >> >> >> >> >> one more: the locations of the LSOs on disk are well >> >> >> >> > >> > known: >> http://en.wikipedia.org/wiki/Local_Shared_Object#File_locations, >> > we >> >> >> >> >> can just delete them directly >> >> >> >> >> >> On Mon, Nov 29, 2010 at 4:18 PM, John Abd-El-Malek <mailto: >> jam@chromium.org> >> >> >> wrote: >> >> >>> >> >> >>> another idea: can we get the filename from the Flash plugin >> when we're >> >> >>> using it, and delete it directly if the user has this option set? >> that way >> >> >>> we wouldn't need to wait on a process spin-up >> >> >>> On Mon, Nov 29, 2010 at 3:45 PM, Darin Fisher >> >> >>> <mailto:darin@chromium.org> wrote: >> >> >>>> >> >> >>>> >> >> >>>> On Mon, Nov 29, 2010 at 9:31 AM, <mailto:bauerb@chromium.org> >> wrote: >> >> >>>>> >> >> >>>>> On 2010/11/29 17:07:34, darin wrote: >> >> >>>>>> >> >> >>>>>> Oh... but what if Flash is running in a separate browser? >> >> >>>>>> Doesn't it >> >> >>>>>> use a >> >> >>>>>> common location for all instances, across all browsers? >> >> >>>>> >> >> >>>>>> -Darin >> >> >>>>> >> >> >>>>> Shouldn't it be the responsibility of the plug-in if it >> implements >> >> >>>>> the >> >> >>>>> API to >> >> >>>>> make sure that it can delete data while it's running in another >> >> >>>>> browser? >> >> >>>> >> >> >>>> Yes, sorry... I forgot that deletion of this data was performed by >> >> >>>> invoking a >> >> >>>> NPAPI method. >> >> >>>> Given that, I agree with John that we should not block browser >> >> >>>> shutdown >> >> >>>> for this. >> >> >>>> -Darin >> >> >>>> >> >> >>>>>> >> >> >>>>>> On Mon, Nov 29, 2010 at 9:06 AM, Darin Fisher >> >> >>>>>> <mailto:darin@chromium.org> >> >> >>>>> >> >> >>>>> wrote: >> >> >>>>> >> >> >>>>>> > We should be able to know when all plugin processes are >> shutdown. >> >> >>>>>> > /me >> >> >>>>>> > looks to jam for the details. >> >> >>>>>> > >> >> >>>>>> > -Darin >> >> >>>>>> > >> >> >>>>>> > >> >> >>>>>> > >> >> >>>>>> > On Mon, Nov 29, 2010 at 4:28 AM, <mailto:bauerb@chromium.org> >> >> >>>>>> > wrote: >> >> >>>>>> > >> >> >>>>>> >> After (offline) discussions with Jochen, I moved the code to >> >> >>>>>> >> delete >> >> >>>>>> >> LSO >> >> >>>>>> >> data on >> >> >>>>>> >> shutdown to browser_shutdown::Shutdown(). >> >> >>>>>> >> >> >> >>>>>> >> Darin/John, Jochen had some concerns about possible race >> >> >>>>>> >> conditions >> >> >>>>>> >> where >> >> >>>>>> >> a >> >> >>>>>> >> plug-in would set LSO data after the hook at shutdown has >> run. >> >> >>>>>> >> When >> >> >>>>>> >> would >> >> >>>>>> >> be the >> >> >>>>>> >> best time to run the hook to avoid this? >> >> >>>>>> >> >> >> >>>>>> >> >> >> >>>>>> >> http://codereview.chromium.org/5278001/ >> >> >>>>>> >> >> >> >>>>>> > >> >> >>>>>> > >> >> >>>>> >> >> >>>>> >> >> >>>>> >> >> >>>>> http://codereview.chromium.org/5278001/ >> >> >>>> >> >> >>> >> >> >> >> >> > >> >> > >> > >> > >> > >> > http://codereview.chromium.org/5278001/ >> > >> > >
On Mon, Dec 6, 2010 at 8:54 AM, Darin Fisher <darin@chromium.org> wrote: > I'm okay with deferring browser shutdown until the plugin process has > finished. Since this feature is not enabled by default, and we imagine few > users enabling it, I think we can tolerate slower shutdown for the benefit > of simpler code. We should put in a UMA histogram on this to see how long we delay shutdown, and also how many people have this enabled. Brett
John, can you take a look at this CL?
lgtm, let's keep a close eye over the UMA stat :) On Thu, Dec 9, 2010 at 5:49 AM, <bauerb@chromium.org> wrote: > John, can you take a look at this CL? > > > http://codereview.chromium.org/5278001/ > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
