Move ProxyConfigService construction onto the IO thread.

Move ProxyConfigService construction onto the IO thread.

BUG=146421

[Philippe, 2012-10-18 09:22:38]

Thanks Paul.

I tested your change on Android in our downstream repository. With the
adjustment I suggested (see my comment in proxy_service.cc), it works.

http://codereview.chromium.org/10912132/diff/67011/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/10912132/diff/67011/net/proxy/proxy_service.cc...
net/proxy/proxy_service.cc:1350: glib_thread_task_runner,
If you swap the two message loops here for consistency with
ProxyConfigServiceLinux (i.e. (IO, UI) -> (UI, IO)), don't forget to swap the
parameters as well in the ProxyConfigServiceAndroid's constructor.
Right now the UI message loop is being used instead of the IO loop and vice
versa :)

While I'm here too, are you sure 'glib' thread is the preferred name for the UI
thread here and in PCSAndroid (on non-Linux platforms)?

[pauljensen, 2012-10-19 02:20:43]

Will, I finally think I've finished off this CL.  I'm adding you to the
reviewers list as you were the one that asked me to address the bug, but feel
free to delegate the reviewing to others.

This originally started off as just trying to remove
URLRequestContextBuilder::set_proxy_config_service() but you expanded it to
moving creation of all ProxyConfigService's to the IO thread.  I consulted
pliard@ and sleevi@ for help with the JNI stuff for ProxyConfigServiceAndroid.
 I consulted battre@ and mnissler@ and kuan@ for help with
PrefProxyConfigTrackerImpl.

I certainly learned a lot from this.  The shutdown of ProxyConfigServiceLinux
was really fragile previously; fixing this took a lot of time as I had to do a
lot of investigating the shutdown behavior of Chrome...and the shell and service
incarnations too.

[willchan no longer on Chromium, 2012-10-19 05:36:19]

Nice. I'm going to send this review to Matt, but I'll have a look myself
tomorrow to make sure it's in good shape overall, and let him handle the
details. Anything involving shutdown is dicey, so I'll make sure to look at
that.

[Philippe, 2012-10-19 09:35:25]

I'm adding Ryan too for ProxyConfigServiceAndroid.

http://codereview.chromium.org/10912132/diff/65039/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service.h (right):

http://codereview.chromium.org/10912132/diff/65039/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service.h:64: 
Nit: extra blank line.

http://codereview.chromium.org/10912132/diff/65039/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10912132/diff/65039/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:192: void StartTearDown() {
Sorry, I didn't see at first that this was called on the JNI thread. I'm
concerned about creating the Java non-stateless instance on the Network thread
and then destroying it on the JNI thread.

What do you think?

I think this could be addressed by keeping the initial design (i.e. all the JNI
calls performed on the JNI thread).
You would have to add an extra Init() method called on the network thread that
would post SetupJNI() on the JNI thread.
You can also merge SetupJNI() and FetchInitialConfig() into a InitOnJNIThread()
method which would setup JNI then fetch the initial config.

How does that sound?

http://codereview.chromium.org/10912132/diff/65039/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10912132/diff/65039/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:41: // Called from Java (on glib
thread) to signal that the proxy settings
Maybe you should keep JNI here and in the cc file. What do you think? We don't
use glib on Android.

[digit1, 2012-10-19 10:32:46]

http://codereview.chromium.org/10912132/diff/69015/chrome/service/net/service...
File chrome/service/net/service_url_request_context.h (right):

http://codereview.chromium.org/10912132/diff/69015/chrome/service/net/service...
chrome/service/net/service_url_request_context.h:73:
scoped_refptr<base::SingleThreadTaskRunner> glib_task_runner_;
I believe that mentioning glib in source files and headers that are not 100%
specific to the Linux port is misleading.

It would be nuch better if you could name this differently (e.g.
ui_task_runner).

http://codereview.chromium.org/10912132/diff/69015/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service.h (right):

http://codereview.chromium.org/10912132/diff/69015/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service.h:66: // Called on the Glib thread to indicate
now is the time for shutdown work
Again, this file is not 100% specific to Linux, so please avoid mentioning Glib
here. I also believe there is a policy of not mentioning the UI thread
explicitely in low-level layers like base/ and net/, so maybe talk about the
"main thread" here instead.

http://codereview.chromium.org/10912132/diff/69015/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10912132/diff/69015/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:164: base::SequencedTaskRunner*
glib_task_runner,
same remarks in this file.

http://codereview.chromium.org/10912132/diff/69015/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10912132/diff/69015/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:41: // Called from Java (on glib
thread) to signal that the proxy settings
As philippe said, Android doesn't use Glibc at all, so these comments and
variable name changes are misleading. Can you fix this? Use "main thread" or
keep "jni thread" since this is under net/. Thanks.

http://codereview.chromium.org/10912132/diff/69015/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/10912132/diff/69015/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_linux.h:213: // must be called from the UI
thread.
On the other hand, this comment is probably perfectly fine and informative :)

http://codereview.chromium.org/10912132/diff/69015/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/10912132/diff/69015/net/proxy/proxy_service.h#...
net/proxy/proxy_service.h:240: base::SingleThreadTaskRunner*
glib_thread_task_runner,
Probably use main_thread_task_runner here.

http://codereview.chromium.org/10912132/diff/69015/net/tools/get_server_time/...
File net/tools/get_server_time/get_server_time.cc (right):

http://codereview.chromium.org/10912132/diff/69015/net/tools/get_server_time/...
net/tools/get_server_time/get_server_time.cc:44: #if defined(OS_LINUX) ||
defined(OS_OPENBSD)
Any reason to include OS_OPENBSD here? It looks like when this macro is defined,
only g_type_init() is called. Apart from that, all the Glibc-specific code is
protected inside mere #ifdef OS_LINUX ... #endif block and don't seem to be
compiled for BSD builds.

[pauljensen, 2012-10-19 16:00:13]

I addressed digit1 and pliard's comments.

http://codereview.chromium.org/10912132/diff/65039/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10912132/diff/65039/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:192: void StartTearDown() {
On 2012/10/19 09:35:25, Philippe wrote:
> Sorry, I didn't see at first that this was called on the JNI thread. I'm
> concerned about creating the Java non-stateless instance on the Network thread
> and then destroying it on the JNI thread.
> 
> What do you think?
> 
> I think this could be addressed by keeping the initial design (i.e. all the
JNI
> calls performed on the JNI thread).
> You would have to add an extra Init() method called on the network thread that
> would post SetupJNI() on the JNI thread.
> You can also merge SetupJNI() and FetchInitialConfig() into a
InitOnJNIThread()
> method which would setup JNI then fetch the initial config.
> 
> How does that sound?

I wondered about all this JNI thread interaction and discussed with sleevi and
yourself in an e-mail thread on 9/13/12.  

With regards to stopping the notifications on the UI thread: The notifications
from Java to C++ will always arrive on the UI thread and because of this I feel
we need to disable them on the same thread to keep things simple and avoid
races.  This is especially simple because StartTearDown() arrives on the UI
thread.

With regards to commencing the notifications (via SetupJNI()) on the network
thread:  Since the ProxyConfigServices are now created on the network thread it
seemed simplest to do it right on the same thread rather than post over to the
other thread.  sleevi said tedchoc said this was OK.  I guess I figured since
the notifications will arrive on the UI thread no matter where I call
Java_ProxyChangeListener_start() there wasn't must point to posting from one
thread to another to call the same function.

With regards to querying the proxy info from the networking thread:  This again
made things simpler by avoiding the previous PostTask() in FetchInitialConfig().
 My impression is that getting the initial proxy config quickly is also
important so this helps.

[willchan no longer on Chromium, 2012-10-19 16:28:02]

+eroman

Eric will be interested in the ProxyConfigService stuff. Or maybe he's hoping he
doesn't have to be interested anymore. Regardless, probably should loop him in
:)

[Philippe, 2012-10-19 16:34:12]

http://codereview.chromium.org/10912132/diff/65039/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10912132/diff/65039/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:192: void StartTearDown() {
On 2012/10/19 16:00:13, pauljensen wrote:
> On 2012/10/19 09:35:25, Philippe wrote:
> > Sorry, I didn't see at first that this was called on the JNI thread. I'm
> > concerned about creating the Java non-stateless instance on the Network
thread
> > and then destroying it on the JNI thread.
> > 
> > What do you think?
> > 
> > I think this could be addressed by keeping the initial design (i.e. all the
> JNI
> > calls performed on the JNI thread).
> > You would have to add an extra Init() method called on the network thread
that
> > would post SetupJNI() on the JNI thread.
> > You can also merge SetupJNI() and FetchInitialConfig() into a
> InitOnJNIThread()
> > method which would setup JNI then fetch the initial config.
> > 
> > How does that sound?
> 
> I wondered about all this JNI thread interaction and discussed with sleevi and
> yourself in an e-mail thread on 9/13/12.  
> 
> With regards to stopping the notifications on the UI thread: The notifications
> from Java to C++ will always arrive on the UI thread and because of this I
feel
> we need to disable them on the same thread to keep things simple and avoid
> races.  This is especially simple because StartTearDown() arrives on the UI
> thread.
> 
> With regards to commencing the notifications (via SetupJNI()) on the network
> thread:  Since the ProxyConfigServices are now created on the network thread
it
> seemed simplest to do it right on the same thread rather than post over to the
> other thread.  sleevi said tedchoc said this was OK.  I guess I figured since
> the notifications will arrive on the UI thread no matter where I call
> Java_ProxyChangeListener_start() there wasn't must point to posting from one
> thread to another to call the same function.
> 
> With regards to querying the proxy info from the networking thread:  This
again
> made things simpler by avoiding the previous PostTask() in
FetchInitialConfig().
>  My impression is that getting the initial proxy config quickly is also
> important so this helps.

I agree that destruction should happen on the UI thread (as before).

I was just concerned about possible races on the Java side (see
ProxyChangeListener.java) due to non-synchronized concurrent accesses (UI vs I/O
thread) to |mNativePtr|, |mProxyReceiver|, |mContext|.
It is probably fine in practice with the current Java-side implementation but
that could change later.

I agree too that your design minimizes the amount of PostTask() and makes the
initial proxy config available as soon as possible which is good.

[Ryan Sleevi, 2012-10-19 17:56:43]

So I only looked at the net/ code, and I think I've tried to capture my
high-level design concern in the comment below.

As Will said, shutdown is very hard to get right, and from looking at the code
outside of net/, I can tell it's going to take very careful review. However, I
think it *may* be simplified by changing some of the net/ implementation
details, which is why I didn't review the rest too carefully.

http://codereview.chromium.org/10912132/diff/88001/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service.h (right):

http://codereview.chromium.org/10912132/diff/88001/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service.h:67: // to post shutdown tasks to other browser
threads.
nit: comment layering violation within net/ "to other browser threads".
BrowserThread is a content/ concept. I'd suggest just saying "threads"

From a high-level perspective though, I'm concerned about introducing the idea
of browser/process shutdown into the net/ layer. It would be much nicer if the
shutdown semantics in net/ just focused on the object lifetimes, rather than the
overall process lifetimes. The fact that there is only one PCS per-process - or
whatever other such - shouldn't alter these semantics.

Most importantly, I think it introduces a real design flaw, in that it forces
every ProxyConfigService to consider living on two threads (the Network thread,
where it's created, and the main thread, where this can be called). I'm really
opposed to this, and tried to work very hard during the Android changes to try
and separate out the objects and give them distinct and clear lifetimes.

[willchan no longer on Chromium, 2012-10-19 18:06:36]

http://codereview.chromium.org/10912132/diff/88001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/10912132/diff/88001/chrome/browser/io_thread.h...
chrome/browser/io_thread.h:147: // Called on the UI thread to indicate now is
the time for shutdown work that
Can you explain why this hook needs to exist? Why can't you use
IOThread::~IOThread()?

Note that the IOThread *object* lives on the UI thread.

http://codereview.chromium.org/10912132/diff/88001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10912132/diff/88001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:209: // Used on the UI thread to
return a pointer to the Profile.
This is too powerful. We should try to keep the scope narrow. The Profile object
contains too many dependencies. Please restructure the callback so it doesn't
return a raw Profile*, but only exactly what you need.

I am still looking, but you should make sure to handle shutdown properly. What
happens when this gets posted from the IO thread to the UI thread while the
profile (let's say an incognito profile) is being destroyed? Do we get a use
after free? Be very very careful :)

[eroman, 2012-10-19 18:53:27]

I have only skimmed through this change, but I don't generally like moving the
multithreaded destruction semantics into net (i.e.
ProxyConfigService::StartTearDown).

There is a tension here between wanting to make things easier for network
library embedders, and making things cleaner for Chrome.

Once upon a time, the Mac and Windows system ProxyConfigServices fit cleanly
into net and it made sense for that code to be re-usable and live in net
library.

However at this point, I cringe to see all the dependencies that need to be
plumbed down into net to support the "system" proxy settings for
android/linux/ios/cros.

I don't want to derail this thread or undermine the great work that you have
been doing! However I question the goal of simplifying things for the non-Chrome
embedders, in this case.

In my opinion, what we should be doing instead is tearing out all of the
ProxyConfigService implementations from net. 

The net library would now only expose the interface. I think we already have a
critical mass of ugliness resulting from having tried to implement those in net.

Under this model, the linux/cros/ios/etc.. implementations would live in
chrome's source tree, and as such would have direct access to the threads and
platform-sepcific goop they need. Destruction and initialization could be
customized as needed without jumping through hoops in net (all net would have is
a IO-thread only interface).

The code to select and create the appropriate config service would live in
Chrome rather than net. This would come at the cost of making it harder for
external embedders to re-use those services.

I would argue that there is already enough setup complexity to using them
correctly from net (i.e. need to pass in all the right threads, and understand
glib interactions), that we can't really give embedders a clean factory method
anyway. I would rather embedders have to re-engineer these details if they want
to support the above platforms, if it means keeping the chrome case tight.

You have looked at this more than me, so would be interested to hear your take
on it!

[Ryan Sleevi, 2012-10-19 18:57:16]

On Fri, Oct 19, 2012 at 11:53 AM,  <eroman@chromium.org> wrote:
> I have only skimmed through this change, but I don't generally like moving
> the
> multithreaded destruction semantics into net (i.e.
> ProxyConfigService::StartTearDown).
>
> There is a tension here between wanting to make things easier for network
> library embedders, and making things cleaner for Chrome.
>
> Once upon a time, the Mac and Windows system ProxyConfigServices fit cleanly
> into net and it made sense for that code to be re-usable and live in net
> library.
>
> However at this point, I cringe to see all the dependencies that need to be
> plumbed down into net to support the "system" proxy settings for
> android/linux/ios/cros.
>
> I don't want to derail this thread or undermine the great work that you have
> been doing! However I question the goal of simplifying things for the
> non-Chrome
> embedders, in this case.
>
> In my opinion, what we should be doing instead is tearing out all of the
> ProxyConfigService implementations from net.
>
> The net library would now only expose the interface. I think we already have
> a
> critical mass of ugliness resulting from having tried to implement those in
> net.
>
> Under this model, the linux/cros/ios/etc.. implementations would live in
> chrome's source tree, and as such would have direct access to the threads
> and
> platform-sepcific goop they need. Destruction and initialization could be
> customized as needed without jumping through hoops in net (all net would
> have is
> a IO-thread only interface).
>
> The code to select and create the appropriate config service would live in
> Chrome rather than net. This would come at the cost of making it harder for
> external embedders to re-use those services.
>
> I would argue that there is already enough setup complexity to using them
> correctly from net (i.e. need to pass in all the right threads, and
> understand
> glib interactions), that we can't really give embedders a clean factory
> method
> anyway. I would rather embedders have to re-engineer these details if they
> want
> to support the above platforms, if it means keeping the chrome case tight.
>
> You have looked at this more than me, so would be interested to hear your
> take
> on it!

+1, although I would suggest perhaps content/, rather than chrome/ ?

I think content/ should have all the necessary primitives (IO Threads,
Browser Threads, awareness of net/ URLRequestContexts-etc). The
problem with moving it all the way up to chrome/ is that it forces
embedders to re-do the entire implementation (since we discourage
embedding at the chrome/ layer)

Just my $.02

[pauljensen, 2012-10-19 22:34:28]

The construction and destruction semantics for ProxyConfigServiceLinux aren't
simple and probably never will be.  I can talk more about this if you want.  If
we think its too ugly for embedders with or without my CL then we should
probably move the linux, android and iOS ProxyConfigServices out of net/.  I
thinks its probably too ugly with or without my CL.  When I say ugliness I mean
complexity that users must be aware of to safely use it.

Ugliness without my CL:
1. must be constructed on "main" (UI) thread
2. must be passed handle to file thread
3. must be destructed on network thread
4. destruction is really fragile
Ugliness with my CL:
1. constructor must be passed handle to "main" (UI) thread and file thread (This
could go away once moved to content/)
2. StartTearDown() must be called at a certain time

We really need to get the KDE part of ProxyConfigServiceLinux off the file
thread.  This will be a significant decrease in ugliness.  szym has been working
on this and I've been talking to him about all this.  He'll probably comment on
this CL soon.

I feel shutdown in Chrome is a bit scary and have a couple ideas if anyone wants
to hear.

http://codereview.chromium.org/10912132/diff/88001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/10912132/diff/88001/chrome/browser/io_thread.h...
chrome/browser/io_thread.h:147: // Called on the UI thread to indicate now is
the time for shutdown work that
On 2012/10/19 18:06:36, willchan wrote:
> Can you explain why this hook needs to exist? Why can't you use
> IOThread::~IOThread()?
> 
> Note that the IOThread *object* lives on the UI thread.

~IOThread() is called after every thread except the UI thread is gone which is
too late to stop notifications for SettingGetterImplKDE which arrive on the file
thread.

http://codereview.chromium.org/10912132/diff/88001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10912132/diff/88001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:209: // Used on the UI thread to
return a pointer to the Profile.
On 2012/10/19 18:06:36, willchan wrote:
> This is too powerful. We should try to keep the scope narrow. The Profile
object
> contains too many dependencies. Please restructure the callback so it doesn't
> return a raw Profile*, but only exactly what you need.

Sorry, I was just copying how ChromeCookieMonsterDelegate gets a pointer to the
Profile.  I could instead replace it with a Callback that just takes a
ChromeProxyConfigService argument and doesn't return anything.

> 
> I am still looking, but you should make sure to handle shutdown properly. What
> happens when this gets posted from the IO thread to the UI thread while the
> profile (let's say an incognito profile) is being destroyed? Do we get a use
> after free? Be very very careful :)

It uses GetProfileOnUI() which checks with IsValidProfile() to make sure its
still alive.

[szym, 2012-10-20 18:35:54]

I second eroman's suggestion to decouple ProxyConfigService from the lifetime of
the network thread,
but I'd suggest moving ProxyConfigService into the NetworkChangeNotifier, while
keeping it in net/.

NCN has it's own issues (|if (g_XXX) g_XXX->YYY()| is not really thread-safe),
but if we nailed
the lifetime there, then it is a nice nexus for the Observers and Notifiers to
meet. No object
on the network thread really needs to own those "ConfigServices" which hook into
the system.

There's a number of places where ProxyConfigServiceFixed is used to override the
configuration
but I think ProxyService could expose a method to allow such override directly
when needed.

On 2012/10/19 22:34:28, pauljensen wrote:
> The construction and destruction semantics for ProxyConfigServiceLinux aren't
> simple and probably never will be.  I can talk more about this if you want. 
If
> we think its too ugly for embedders with or without my CL then we should
> probably move the linux, android and iOS ProxyConfigServices out of net/.  I
> thinks its probably too ugly with or without my CL.  When I say ugliness I
mean
> complexity that users must be aware of to safely use it.
> 
> Ugliness without my CL:
> 1. must be constructed on "main" (UI) thread
> 2. must be passed handle to file thread
> 3. must be destructed on network thread
> 4. destruction is really fragile
> Ugliness with my CL:
> 1. constructor must be passed handle to "main" (UI) thread and file thread
(This
> could go away once moved to content/)
> 2. StartTearDown() must be called at a certain time
> 
> We really need to get the KDE part of ProxyConfigServiceLinux off the file
> thread.  This will be a significant decrease in ugliness.  szym has been
working
> on this and I've been talking to him about all this.  He'll probably comment
on
> this CL soon.

SettingGetterImplKDE implements inotify-based file watcher, even though there
already
is one: base::files::FilePathWatcher (however imperfect). It also relies on a
file
thread for blocking operations, when we could use a WorkerPool instead. I
started
sketching a refactoring which replaces the use of file_loop with those two
components.

> I feel shutdown in Chrome is a bit scary and have a couple ideas if anyone
wants
> to hear.
> 
> http://codereview.chromium.org/10912132/diff/88001/chrome/browser/io_thread.h
> File chrome/browser/io_thread.h (right):
> 
>
http://codereview.chromium.org/10912132/diff/88001/chrome/browser/io_thread.h...
> chrome/browser/io_thread.h:147: // Called on the UI thread to indicate now is
> the time for shutdown work that
> On 2012/10/19 18:06:36, willchan wrote:
> > Can you explain why this hook needs to exist? Why can't you use
> > IOThread::~IOThread()?
> > 
> > Note that the IOThread *object* lives on the UI thread.
> 
> ~IOThread() is called after every thread except the UI thread is gone which is
> too late to stop notifications for SettingGetterImplKDE which arrive on the
file
> thread.
> 
>
http://codereview.chromium.org/10912132/diff/88001/chrome/browser/profiles/pr...
> File chrome/browser/profiles/profile_io_data.h (right):
> 
>
http://codereview.chromium.org/10912132/diff/88001/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile_io_data.h:209: // Used on the UI thread to
> return a pointer to the Profile.
> On 2012/10/19 18:06:36, willchan wrote:
> > This is too powerful. We should try to keep the scope narrow. The Profile
> object
> > contains too many dependencies. Please restructure the callback so it
doesn't
> > return a raw Profile*, but only exactly what you need.
> 
> Sorry, I was just copying how ChromeCookieMonsterDelegate gets a pointer to
the
> Profile.  I could instead replace it with a Callback that just takes a
> ChromeProxyConfigService argument and doesn't return anything.
> 
> > 
> > I am still looking, but you should make sure to handle shutdown properly.
What
> > happens when this gets posted from the IO thread to the UI thread while the
> > profile (let's say an incognito profile) is being destroyed? Do we get a use
> > after free? Be very very careful :)
> 
> It uses GetProfileOnUI() which checks with IsValidProfile() to make sure its
> still alive.

[Ryan Sleevi, 2012-10-20 19:15:53]

Using a WorkerPool, rather than the explicit TaskRunner/MessageLoop system
of today, seems like needless overhead creating and destroying threads. It
also brings up a whole host of new threading issues regarding joinability
that would be nice to avoid.
On Oct 20, 2012 11:35 AM, <szym@chromium.org> wrote:

> I second eroman's suggestion to decouple ProxyConfigService from the
> lifetime of
> the network thread,
> but I'd suggest moving ProxyConfigService into the NetworkChangeNotifier,
> while
> keeping it in net/.
>
> NCN has it's own issues (|if (g_XXX) g_XXX->YYY()| is not really
> thread-safe),
> but if we nailed
> the lifetime there, then it is a nice nexus for the Observers and
> Notifiers to
> meet. No object
> on the network thread really needs to own those "ConfigServices" which
> hook into
> the system.
>
> There's a number of places where ProxyConfigServiceFixed is used to
> override the
> configuration
> but I think ProxyService could expose a method to allow such override
> directly
> when needed.
>
> On 2012/10/19 22:34:28, pauljensen wrote:
>
>> The construction and destruction semantics for ProxyConfigServiceLinux
>> aren't
>> simple and probably never will be.  I can talk more about this if you
>> want.
>>
> If
>
>> we think its too ugly for embedders with or without my CL then we should
>> probably move the linux, android and iOS ProxyConfigServices out of net/.
>>  I
>> thinks its probably too ugly with or without my CL.  When I say ugliness I
>>
> mean
>
>> complexity that users must be aware of to safely use it.
>>
>
>  Ugliness without my CL:
>> 1. must be constructed on "main" (UI) thread
>> 2. must be passed handle to file thread
>> 3. must be destructed on network thread
>> 4. destruction is really fragile
>> Ugliness with my CL:
>> 1. constructor must be passed handle to "main" (UI) thread and file thread
>>
> (This
>
>> could go away once moved to content/)
>> 2. StartTearDown() must be called at a certain time
>>
>
>  We really need to get the KDE part of ProxyConfigServiceLinux off the file
>> thread.  This will be a significant decrease in ugliness.  szym has been
>>
> working
>
>> on this and I've been talking to him about all this.  He'll probably
>> comment
>>
> on
>
>> this CL soon.
>>
>
> SettingGetterImplKDE implements inotify-based file watcher, even though
> there
> already
> is one: base::files::FilePathWatcher (however imperfect). It also relies
> on a
> file
> thread for blocking operations, when we could use a WorkerPool instead. I
> started
> sketching a refactoring which replaces the use of file_loop with those two
> components.
>
>  I feel shutdown in Chrome is a bit scary and have a couple ideas if anyone
>>
> wants
>
>> to hear.
>>
>
>  http://codereview.chromium.**org/10912132/diff/88001/**
>>
chrome/browser/io_thread.h<http://codereview.chromium.org/10912132/diff/88001/chrome/browser/io_thread.h>
>> File chrome/browser/io_thread.h (right):
>>
>
>
> http://codereview.chromium.**org/10912132/diff/88001/**
>
chrome/browser/io_thread.h#**newcode147<http://codereview.chromium.org/10912132/diff/88001/chrome/browser/io_thread.h#newcode147>
>
>> chrome/browser/io_thread.h:**147: // Called on the UI thread to indicate
>> now is
>> the time for shutdown work that
>> On 2012/10/19 18:06:36, willchan wrote:
>> > Can you explain why this hook needs to exist? Why can't you use
>> > IOThread::~IOThread()?
>> >
>> > Note that the IOThread *object* lives on the UI thread.
>>
>
>  ~IOThread() is called after every thread except the UI thread is gone
>> which is
>> too late to stop notifications for SettingGetterImplKDE which arrive on
>> the
>>
> file
>
>> thread.
>>
>
>
> http://codereview.chromium.**org/10912132/diff/88001/**
>
chrome/browser/profiles/**profile_io_data.h<http://codereview.chromium.org/10912132/diff/88001/chrome/browser/profiles/profile_io_data.h>
>
>> File chrome/browser/profiles/**profile_io_data.h (right):
>>
>
>
> http://codereview.chromium.**org/10912132/diff/88001/**
>
chrome/browser/profiles/**profile_io_data.h#newcode209<http://codereview.chromium.org/10912132/diff/88001/chrome/browser/profiles/profile_io_data.h#newcode209>
>
>> chrome/browser/profiles/**profile_io_data.h:209: // Used on the UI
>> thread to
>> return a pointer to the Profile.
>> On 2012/10/19 18:06:36, willchan wrote:
>> > This is too powerful. We should try to keep the scope narrow. The
>> Profile
>> object
>> > contains too many dependencies. Please restructure the callback so it
>>
> doesn't
>
>> > return a raw Profile*, but only exactly what you need.
>>
>
>  Sorry, I was just copying how ChromeCookieMonsterDelegate gets a pointer
>> to
>>
> the
>
>> Profile.  I could instead replace it with a Callback that just takes a
>> ChromeProxyConfigService argument and doesn't return anything.
>>
>
>  >
>> > I am still looking, but you should make sure to handle shutdown
>> properly.
>>
> What
>
>> > happens when this gets posted from the IO thread to the UI thread while
>> the
>> > profile (let's say an incognito profile) is being destroyed? Do we get
>> a use
>> > after free? Be very very careful :)
>>
>
>  It uses GetProfileOnUI() which checks with IsValidProfile() to make sure
>> its
>> still alive.
>>
>
>
>
http://codereview.chromium.**org/10912132/<http://codereview.chromium.org/109...
>

[digit1, 2013-04-26 14:14:03]

Hello, is this CL still needed? The latest patch is 6 months old so I assume
not. If so, could you close it? Thanks.