Fix gconf for the linux proxy config service....

Fix gconf for the linux proxy config service.

-Reenables fetching of settings from gconf.

-Moves all gconf access to happen from the UI thread only, (where
 the default glib main loop runs).

-Adds support for gconf notifications, avoiding having to poll the settings.

-Fixes a small initialization glitch in the unittest. Plus minor code style tweaks.

-Permanently removes gdk and glib threading initialization calls that
 were previously disabled.

-Slight reorganization of ProxyService creation to pass down the IO
 thread MessageLoop.

BUG=11111
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=16739

[sdoyon, 2009-05-06 20:04:40]

This is much inspired by eroman's excellent pseudo-code in
http://codereview.chromium.org/99126.

[Dean McNamee, 2009-05-07 09:42:05]

I just have some mostly style comments.  Eric is going to have to do the meat, I
am pretty lost in the network code :)

http://codereview.chromium.org/113043/diff/20/1010
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/113043/diff/20/1010#newcode447
Line 447: g_type_init();
Wouldn't this happen within gtk_init?  Not suggesting we shouldn't do it
explicitly, just trying to understand.

http://codereview.chromium.org/113043/diff/20/1011
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/113043/diff/20/1011#newcode58
Line 58: g_browser_process->io_thread()->message_loop()) :
This runs on the UI thread? (I would guess so, it is only safe to access
g_browser_process on the UI thread).

http://codereview.chromium.org/113043/diff/20/1007
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/113043/diff/20/1007#newcode282
Line 282: if ((client_ = gconf_client_get_default())) {
extra sent of parens here.  I guess you are trying to emphasize that you are
assigning to client_.

I would probably just move the client_ = line up before the if, and do if
(client_).

Not a big deal though.

http://codereview.chromium.org/113043/diff/20/1007#newcode458
Line 458: if (mode.compare("none") == 0) {
mode == "none" ?

http://codereview.chromium.org/113043/diff/20/1007#newcode588
Line 588: && desktop_session.compare("gnome") == 0);
desktop_session == "gnome" ?

http://codereview.chromium.org/113043/diff/20/1007#newcode623
Line 623: // whereever we can find one.
spelling?

http://codereview.chromium.org/113043/diff/20/1008
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/113043/diff/20/1008#newcode156
Line 156: MessageLoop* glib_default_loop_;
default_loop meaning the UI thread right.  Maybe just add a comment:  // UI
thread's message loop.

http://codereview.chromium.org/113043/diff/20/1009
File net/proxy/proxy_config_service_linux_unittest.cc (right):

http://codereview.chromium.org/113043/diff/20/1009#newcode297
Line 297: /* Why does this not compile??
My c++ is not awesome, but shouldn't you specialize the whole struct?

template <>
struct RunnableMethodTraits<SynchConfigGetter> {
  static void RetainCallee(T* obj) { }
  static void ReleaseCallee(T* obj) { }
};

[sdoyon, 2009-05-07 15:34:47]

On Thu, 7 May 2009, deanm@chromium.org wrote:

> http://codereview.chromium.org/113043/diff/20/1010
> File chrome/app/chrome_dll_main.cc (right):
>
> http://codereview.chromium.org/113043/diff/20/1010#newcode447
> Line 447: g_type_init();
> Wouldn't this happen within gtk_init?  Not suggesting we shouldn't do it
> explicitly, just trying to understand.

Most probably it would (although the code path in gtk seems
convoluted). But then as it was pointed out to me, gconf depends on
glib only, not on gdk or gtk, so I figured the explicit init made
sense, even if a bit pedantic. (Say someone greps for gconf to move it
to a seperate process for example.) It's probably worth adding a note
about this to the comment, so I'll do that.

> http://codereview.chromium.org/113043/diff/20/1011
> File chrome/browser/net/chrome_url_request_context.cc (right):
>
> http://codereview.chromium.org/113043/diff/20/1011#newcode58
> Line 58: g_browser_process->io_thread()->message_loop()) :
> This runs on the UI thread? (I would guess so, it is only safe to access
> g_browser_process on the UI thread).

Yes.
-eroman@ said it does ;-)
-CreateProxyService() is called from CreateOriginal() which also
    references g_browser_process.
-In CreateUsingV8Resolver() I call MessageLoopForUI::current(), which does
    DCHECK_EQ(MessageLoop::TYPE_UI, loop->type());

Note that destruction of ProxyConfigServiceLinux will also need to
happen from the UI thread. This seems to be the case, although I'm
slightly less sure. That is also DCHECK'ed.

Another detail is that when running net_unittests, the
glib_default_loop parameter to
ProxyConfigServiceLinux::SetupAndFetchInitialConfig() is not in fact a
TYPE_UI message loop, but that shouldn't matter.

> http://codereview.chromium.org/113043/diff/20/1007
> File net/proxy/proxy_config_service_linux.cc (right):
>
> http://codereview.chromium.org/113043/diff/20/1007#newcode282
> Line 282: if ((client_ = gconf_client_get_default())) {
> extra sent of parens here.  I guess you are trying to emphasize that you
> are assigning to client_.

In fact, the compiler warns without them.

> I would probably just move the client_ = line up before the if, and do
> if (client_).

Right OK done. I guess I should have taken the hint when the compiler
warned :-).

Hmm actually, the "flow" of that function can be clarified by handling
the failure case and returning right away. Done.

> http://codereview.chromium.org/113043/diff/20/1007#newcode458
> Line 458: if (mode.compare("none") == 0) {
> mode == "none" ?

Oh...! Why was I using compare... Fixed.

> http://codereview.chromium.org/113043/diff/20/1007#newcode588
> Line 588: && desktop_session.compare("gnome") == 0);
> desktop_session == "gnome" ?

Waw... and there even was a bug fix to that line just a couple days
ago. OK fixed.

A few more occurrences in that file... Fixed.

Or should this be in a seperate CL...
Similarly, a small bug in my unittests was exposed by this CL,
the one-line fix could be split out to a seperate patch, which would
need to land first.
-    GConfValues zero_values;
+    GConfValues zero_values = { 0 };
Plus a style change of reset() => Reset().
A CL ought to contain a single logical change, but then this seems
rather trivial.

> http://codereview.chromium.org/113043/diff/20/1007#newcode623
> Line 623: // whereever we can find one.
> spelling?

Done.

> http://codereview.chromium.org/113043/diff/20/1008
> File net/proxy/proxy_config_service_linux.h (right):
>
> http://codereview.chromium.org/113043/diff/20/1008#newcode156
> Line 156: MessageLoop* glib_default_loop_;
> default_loop meaning the UI thread right.  Maybe just add a comment:  //
> UI thread's message loop.

Not exactly: I mean the main loop that glib defaults to, such as when
calling g_timeout_add, g_idle_add, or the one associated with the
context returned by g_main_context_default. That is indeed the UI
thread. This needs a comment. OK done.

> http://codereview.chromium.org/113043/diff/20/1009
> File net/proxy/proxy_config_service_linux_unittest.cc (right):
>
> http://codereview.chromium.org/113043/diff/20/1009#newcode297
> Line 297: /* Why does this not compile??
> My c++ is not awesome,

Nor is mine :-)

> but shouldn't you specialize the whole struct?
>
> template <>
> struct RunnableMethodTraits<SynchConfigGetter> {
> static void RetainCallee(T* obj) { }
> static void ReleaseCallee(T* obj) { }
> };

Err... Well I followed exactly the same pattern as eroman@ used in
proxy_script_fetcher_unittest.cc and proxy_service_unittest.cc. I'm
clearly missing something :-). Your suggested syntax does not work
either.

Many thanks

[sdoyon, 2009-05-07 15:51:19]

Oops, ui_tests break: the assert in the destructor
~ProxyConfigServiceLinux(), about running on the UI thread, fires. The
backtrace mentions MessagePumpLibevent. I may need advice with this.

[eroman, 2009-05-07 22:44:54]

I haven't gone through all the details yet, but sending some initial comments.

I will be looking at the tear-down sequence next, that looks tricky.

http://codereview.chromium.org/113043/diff/25/28
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/113043/diff/25/28#newcode257
Line 257: // IWBN to debounce multiple callbacks in quick succession, since I
What does "IWBN" mean?

http://codereview.chromium.org/113043/diff/25/28#newcode578
Line 578: DCHECK(glib_default_loop_ == NULL ||
I don't know about this, I would expect ProxyConfigService to be deleted from IO
loop. Have you found this to be always true?

If this gets deleted from IO thread I guess we will need to post a cleanup task
to run on the UI thread.

http://codereview.chromium.org/113043/diff/25/29
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/113043/diff/25/29#newcode145
Line 145: bool config_is_valid;
You might be able to make use of the id() field here.

In particular, the default constructor for ProxyConfig will set |config.id_| to
ProxyConfig::INVALID_ID, so you could use this field in your invalid test.

And once it is written into, you can increase the ID to 1 to indicate it is no
longer invalid.

http://codereview.chromium.org/113043/diff/25/29#newcode154
Line 154: bool reference_config_is_valid_;
Same suggestion as above -- this information might fit cleanly into the id
property, and not require an additional boolean.

http://codereview.chromium.org/113043/diff/25/30
File net/proxy/proxy_config_service_linux_unittest.cc (right):

http://codereview.chromium.org/113043/diff/25/30#newcode257
Line 257: // workaround until someone can tell me what stupid mistake it is
The reason you need AddRef / Release is because of the earlier use of:
   PostTask(FROM_HERE, NewRunnableMethod(this, ...).

Specifically when you use NewRunnableMethod() to create a Task* that wraps the
"this" pointer, the task will hold a reference to "this" to prevent the object
from being deleted before the task has fired. It does this by using the
refcounted interface (AddRef / Release).

Typically one inherits from refcountedthreadsafe to avoid this compile error.

In your case it is valid to provide empty implementations, since it is
guaranteed that tasks cannot outlive the object (Since in ~SyncConfigGetter you
are waiting for the task to complete).

[sdoyon, 2009-05-08 06:31:02]

On Thu, 7 May 2009, eroman@chromium.org wrote:

> I will be looking at the tear-down sequence next, that looks tricky.

Yes ;-).
OK I've made an attempt, see below.

>
> http://codereview.chromium.org/113043/diff/25/28
> File net/proxy/proxy_config_service_linux.cc (right):
>
> http://codereview.chromium.org/113043/diff/25/28#newcode257
> Line 257: // IWBN to debounce multiple callbacks in quick succession,
> since I
> What does "IWBN" mean?

:-) it means "it would be nice".

> http://codereview.chromium.org/113043/diff/25/28#newcode578
> Line 578: DCHECK(glib_default_loop_ == NULL ||
> I don't know about this, I would expect ProxyConfigService to be deleted
> from IO loop.

Can you explain the life cycle please

> Have you found this to be always true?

I've not noticed it fail with net_unittests or chrome, but found out
later that ui_tests triggers it.

> If this gets deleted from IO thread I guess we will need to post a
> cleanup task to run on the UI thread.

Yes. It's not clear to me how to do this. The gconf notification
callback has to have something to latch onto until notifications are
turned off.

We could perhaps have GConfSettingGetter survive deletion of
ProxyConfigServiceLinux until gconf can be released, but then that'll
move the race to the gconf notification handler.

It would seem easier to let the whole ProxyConfigServiceLinux survive
until it can be deleted on the UI thread. I even see the
DeleteSoon()#ReleaseSoon() methods on MessageLoop to do this. Only
thing is I don't think this can happen from the destructor.

Here's an attempt: in ProxyService.h,
  scoped_ptr<ProxyConfigService> config_service_;
is changed to an ordinary pointer. A Destroy() method is added to
ProxyConfigService, which the Linux variant uses to post it's own
release onto the UI thread. Destroy() is called from ProxyService()'s
destructor.

This hasn't been fully tested yet.

I don't like affecting other classes and having a specialpurpose
Destroy method, so if you can think of a cleaner way...

I also considered overriding RefCountedThreadSafe's Release() method
(to do ReleaseSoon() when on the wrong thread), but it's not virtual.

I've also added more assertions in GConfSettingGetter to check we are
always on the same thread and it's of TYPE_UI.

> http://codereview.chromium.org/113043/diff/25/29
> File net/proxy/proxy_config_service_linux.h (right):
>
> http://codereview.chromium.org/113043/diff/25/29#newcode145
> Line 145: bool config_is_valid;
> You might be able to make use of the id() field here.
>
> In particular, the default constructor for ProxyConfig will set
> |config.id_| to ProxyConfig::INVALID_ID, so you could use this field in
> your invalid test.
>
> And once it is written into, you can increase the ID to 1 to indicate it
> is no longer invalid.

OK, if you think that's not overloading the meaning of the id field
too much... I've done the change.

I've added is_valid() in proxy_config.h.

> http://codereview.chromium.org/113043/diff/25/29#newcode154
> Line 154: bool reference_config_is_valid_;
> Same suggestion as above -- this information might fit cleanly into the
> id property, and not require an additional boolean.

Done.

> http://codereview.chromium.org/113043/diff/25/30
> File net/proxy/proxy_config_service_linux_unittest.cc (right):
>
> http://codereview.chromium.org/113043/diff/25/30#newcode257
> Line 257: // workaround until someone can tell me what stupid mistake it
> is
> The reason you need AddRef / Release is because of the earlier use of:
>  PostTask(FROM_HERE, NewRunnableMethod(this, ...).
>
> Specifically when you use NewRunnableMethod() to create a Task* that
> wraps the "this" pointer, the task will hold a reference to "this" to
> prevent the object from being deleted before the task has fired. It does
> this by using the refcounted interface (AddRef / Release).

Yes, I've understood that after some fun reading under base/ :-). My
comment was perhaps unclear: I understand why I need AddRef and
Release, but I don't understand why the empty implementations
(template specializations commented out below) don't compile. I think
I've followed exactly the pattern you used in your unittests...

> In your case it is valid to provide empty implementations, since it is
> guaranteed that tasks cannot outlive the object (Since in
> ~SyncConfigGetter you are waiting for the task to complete).

Good, glad I got that right :-).

[sdoyon, 2009-05-08 13:12:07]

OK, the try server says ui_tests succeeded!

And I figured out the deal with the RunnableMethodTraits template
specialization: there was a second level of enclosing namespace that I
hadn't noticed. So that's fixed.

[sdoyon, 2009-05-11 16:49:21]

> I don't like affecting other classes and having a specialpurpose
> Destroy method, so if you can think of a cleaner way...

Looking at this again, it seems entirely possible to move most of
ProxyConfigServiceLinux's innards into a ref counted wrapped
class. Have ProxyConfigServiceLinux as a thin shell around it that
delegates everything to the wrapped class, and in the destructor has
the wrapped class posted to the UI thread to be released. The gconf
notifications would only be involved with the wrapped class. Working
on that now.

[eroman, 2009-05-11 18:58:05]

Yeah that approach sounds good, I was going to suggest something similar :)

I apologize for the lag, please bug me with pings if I am becoming delinquent on
this review.

[sdoyon, 2009-05-11 19:04:53]

Try success! It's ready for review.

Thanks

[eroman, 2009-05-12 07:20:48]

I liked your change of moving into internal refcounted.

My main comment is that having a refcounted class for CachedConfig now seems
unecessary, since it can just be part of Delegate proper.

Other wise LG.

Sorry for letting this linger for a few days! It is complicated stuff, and took
me a while to digest it all.

http://codereview.chromium.org/113043/diff/1048/58
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/113043/diff/1048/58#newcode78
Line 78: virtual ~Delegate();
This doesn't need to be virtual.

http://codereview.chromium.org/113043/diff/1048/58#newcode101
Line 101: // ProxyConfigService methods:
Please remove this comment, as this class isn't extending ProxyConfigService.

http://codereview.chromium.org/113043/diff/1048/58#newcode103
Line 103: virtual int GetProxyConfig(ProxyConfig* config);
This doesn't need to be virtual.

http://codereview.chromium.org/113043/diff/1048/58#newcode119
Line 119: void ParseNoProxyList(const std::string& no_proxy, ProxyConfig*
config);
Be aware that this method is being moved in
<http://codereview.chromium.org/115029> so it can be used on windows/mac. If
that gets committed before you change, you will have to handle the merge
conflict.

http://codereview.chromium.org/113043/diff/1048/58#newcode143
Line 143: // outlive this ProxyConfigServiceLinux slightly should it be
I don't think this comment about it outliving the ProxyConfigServiceLinux is
that useful anymore anymore (since the whole Delegate can now outlive
ProxyConfigServiceLinux).

http://codereview.chromium.org/113043/diff/1048/58#newcode145
Line 145: struct CachedConfig : public base::RefCountedThreadSafe<CachedConfig>
{
I don't think there is the need for this inner class anymore -- since the whole
"Delegate" class is now RefCountedThreadSafe, it seems like overkill to have a
second refcounted inner class.

I would say just put these fields and methods directly into Delegate.

http://codereview.chromium.org/113043/diff/1048/58#newcode150
Line 150: MessageLoop* io_loop;  // for assertions
If this is inlined into Delegate then this field is unecessary, since it
duplicates |io_loop_|.

[sdoyon, 2009-05-12 13:17:25]

On Tue, 12 May 2009, eroman@chromium.org wrote:

> I liked your change of moving into internal refcounted.
>
> My main comment is that having a refcounted class for CachedConfig now
> seems unecessary, since it can just be part of Delegate proper.

That occurred to me, but thinking about it some more, I saw a
problematic scenario. Suppose we move the CachedConfig fields to
ProxyConfigServiceLinux::Delegate and use the ref counting both for
posting SetNewProxyConfig and ReleaseSoon():

The problem is that we want the deletion of Delegate to always happen
on the UI thread (to safely stop gconf notifications). If the ref
counting is also used for posting SetNewProxyConfig, then the final
Release() may happen on the IO thread.

Scenario:

-Say a gconf notification arrives and is queued up for dispatch on the
 UI thread.

-Say the ProxyConfigServiceLinux is deleted on the IO thread, posting
 ReleaseSoon() for delegate_ on the UI thread. Ref count on delegate_
 is now 1.

-The UI thread handles the notification first, posting
 SetNewProxyConfig() to the IO thread. Ref count on delegate_ is now 2.

-Say the UI thread then handles ReleaseTask (from ReleaseSoon()) and
 releases delegate_. Ref count is now 1 and delegate is not deleted.

-The IO thread runs the SetNewProxyConfig task, then releases
 delegate_, which then gets deleted. gconf notifications are stopped
 as a side-effect of deleting delegate_, but the g_object_unref() is
 done on the wrong thread.

That's why I kept CachedConfig. Do you see anything wrong with my
scenario, or a better way to handle this mess?

> Sorry for letting this linger for a few days! It is complicated stuff,
> and took me a while to digest it all.

Well it did take me several iterations to get this straight. Thanks
for your help.

> http://codereview.chromium.org/113043/diff/1048/58
> File net/proxy/proxy_config_service_linux.h (right):
>
> http://codereview.chromium.org/113043/diff/1048/58#newcode78
> Line 78: virtual ~Delegate();
> This doesn't need to be virtual.

Done.

> http://codereview.chromium.org/113043/diff/1048/58#newcode101
> Line 101: // ProxyConfigService methods:
> Please remove this comment, as this class isn't extending
> ProxyConfigService.

Done.

> http://codereview.chromium.org/113043/diff/1048/58#newcode103
> Line 103: virtual int GetProxyConfig(ProxyConfig* config);
> This doesn't need to be virtual.

Done.

> http://codereview.chromium.org/113043/diff/1048/58#newcode119
> Line 119: void ParseNoProxyList(const std::string& no_proxy,
> ProxyConfig* config);
> Be aware that this method is being moved in
> <http://codereview.chromium.org/115029> so it can be used on
> windows/mac. If that gets committed before you change, you will have to
> handle the merge conflict.

Yes, thanks for the warning. Robert actually came to see me about that
one. The merge should be simple AFAICT.

> http://codereview.chromium.org/113043/diff/1048/58#newcode143
> Line 143: // outlive this ProxyConfigServiceLinux slightly should it be
> I don't think this comment about it outliving the
> ProxyConfigServiceLinux is that useful anymore anymore (since the whole
> Delegate can now outlive ProxyConfigServiceLinux).

Woops: I really meant that it outlives Delegate. Fixed.

Added more explanations to CachedConfig and Delegate to explain the
life time and clarify why we need two ref counters.

> http://codereview.chromium.org/113043/diff/1048/58#newcode145
> Line 145: struct CachedConfig : public
> base::RefCountedThreadSafe<CachedConfig> {
> I don't think there is the need for this inner class anymore -- since
> the whole "Delegate" class is now RefCountedThreadSafe, it seems like
> overkill to have a second refcounted inner class.

Discussed above. I'd appreciate if you could confirm my logic for
keeping CachedConfig.

Thanks

[eroman, 2009-05-12 20:28:52]

That teardown sequence is still pretty tricky. I feel it could be simplified by
removing CachedConfig and using "gconf_client_notify_remove()".

Here is how the sequence might work:

0. When registering the notification with "gconf_client_notify_add()", we save
the connection id which it returned.

1. In ProxyConfigServiceLinux's destructor, post an "OnDestroy" task to the
glib_loop.

2. The "OnDestroy" starts by canceling the gconf notifications. It does this by
calling "gconf_client_notify_remove" with the connection id from earlier. This
should prevent "OnGConfChangeNotification" from being called again, and hence we
no longer need to keep Delegate alive for use from the glib_loop. Next, we
destroy all the members of Delegate which lived on the glib thread (i.e. destroy
gconf_getter). Lastly, we release a reference to "this", Delegate.

This may be the final reference. Or it may not, if there were say an outstanding
task posted to the IO loop still holding a refcount. Either situation is fine
though, since the stuff needing to be deleted on glib_loop has already been
deleted (just need to remove the DCHECKs that destructor is called from glib
loop).


Anyhow, let me know what you think. If you feel that the current approach is
better that is fine too.

[sdoyon, 2009-05-13 18:08:00]

On Tue, 12 May 2009, eroman@chromium.org wrote:

> That teardown sequence is still pretty tricky. I feel it could be
> simplified by removing CachedConfig and using
> "gconf_client_notify_remove()".

OK. That should work, and you've convinced me that it should be a bit
clearer. Some caveats about gconf_client_notify_remove() though.

> 1. In ProxyConfigServiceLinux's destructor, post an "OnDestroy" task to
> the glib_loop.
>
> 2. The "OnDestroy" starts by canceling the gconf notifications. It does
> this by calling "gconf_client_notify_remove" with the connection id from
> earlier. This should prevent "OnGConfChangeNotification" from being
> called again,

I'd rather just unref the whole client. gconf_client_notify_add() is a
layer on top of gconf_client_add_dir(). gconf_client_add_dir() alone
still causes the "value_changed" signal to fire. As it is I don't
think it calls back into our code, but I'd rather just stop the whole
machine. And in any case, the gconf client still needs to be
g_object_unref()'ed somewhere and that ought to happen on the UI thread.

> and hence we no longer need to keep Delegate alive for use
> from the glib_loop. Next, we destroy all the members of Delegate which
> lived on the glib thread (i.e. destroy gconf_getter).

We could destroy gconf_getter, but then I feel I'd have to add a bunch
of NULL DCHECKS to all the places that use it. gconf_getter already
has "cruft" to check its own state, because Init() can fail. (We also
release the client on error in SetupNotification() and
SetupAndFetchInitialConfig(), and recycle it for unittests in
Reset().) So it seems best to just put a DCHECK in its destructor to
ensure the underlying client_ was already released.

> Lastly, we release
> a reference to "this", Delegate.

It's a bit magical: with NewRunnableMethod() and scoped_refptr, I
don't appear to need to take or release any refs explicitly anywhere.

OK done.

Also resolved conflicts with r15855.

Oh one more concern...: in trying to test this, on shutting down the
browser, OnDestroy() does not appear to actually run. I think it's
posted on the UI thread, and it'd take an extra round of the UI
MessageLoop to run it, except we exit instead. (OnDestroy() does run
in net_unittests.) This is perhaps not necessarily a problem, as it
does not really accomplish anything. But then not finishing proper
cleanup might at least make some leak detectors unhappy.

Only thing I can think to do is to force an extra round of the UI
loop. Not sure where that would be exatly. OK I've added that in
BrowserMain(), and that does indeed let OnDestroy() run along with the
release of Delegate. I would appreciate confirmation whether this is
safe to do.

[eroman, 2009-05-13 19:36:28]

LGTM

> Only thing I can think to do is to force an extra round
> of the UI loop. Not sure where that would be exatly. OK
> I've added that in BrowserMain(), and that does indeed 
> let OnDestroy() run along with the
> release of Delegate. I would appreciate confirmation 
> whether this is safe to do.

That seems OK to me.

I don't know much about the destruction sequence, but you might be able to
alternatively work around it by only posting the task if the destructor isn't
already running on the glib loop. Maybe the following like the following would
avoid need to drain UI loop:

if (MessageLoop::current() == glib_default_loop) {
  // Already running on glib loop.
  OnDestroy();
} else {
  PostDestroyTask();
}

http://codereview.chromium.org/113043/diff/1058/86
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/113043/diff/1058/86#newcode625
Line 625: LOG(INFO) << "PostDestroyTask";
nit: this is a duplicate of log statement below.

http://codereview.chromium.org/113043/diff/1058/87
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/113043/diff/1058/87#newcode15
Line 15: #include "base/task.h"
nit: is this header used?

[eroman, 2009-05-13 19:48:17]

Actually, I just spoke to Darin, and it is intentional to not call
RunAllPending() during destruction.

So please remove that.

[sdoyon, 2009-05-14 13:47:47]

On Wed, 13 May 2009, eroman@chromium.org wrote:

> Actually, I just spoke to Darin, and it is intentional to not call
> RunAllPending() during destruction.
>
> So please remove that.

OK. Somehow I'm not entirely surprised :-).

Does this mean that we are intentionally leaking some objects, or not
completing some cleanup? Or am I the only one with this problem?

Are we not doing it just for speed, or is there some more subtle
complication we're avoiding?

As I said the final cleanup is not essential. The one caveat I can
think of is that it will confuse memory leak detectors. I don't know
what we do in that area, if it's an issue then conceivably we could
RunAllPending conditionally, say only in debug. We'll also be
forcefully dropping our gconf connection, but I don't think that's
much of a concern.

On Wed, 13 May 2009, eroman@chromium.org wrote:

> I don't know much about the destruction sequence, but you might be able
> to alternatively work around it by only posting the task if the
> destructor isn't already running on the glib loop. Maybe the following
> like the following would avoid need to drain UI loop:
>
> if (MessageLoop::current() == glib_default_loop) {
> // Already running on glib loop.
> OnDestroy();
> } else {
> PostDestroyTask();
> }

Sure we can save the task posting if we're already on the right
thread, but I don't see how that is a workaround. AFAICT, on browser
shutdown, ProxyConfigServiceLinux is not destroyed from the UI
thread. Am I missing something?

It also means two subtly different code paths. Unittests will take
the shortcut and not do the posting for example, while the mainstream
browser shutdown sequence would post. Well, perhaps for unittests it's
not such a bad idea, since posting OnDestroy means we deffer cleanup
into the next test. I will make the change.

> http://codereview.chromium.org/113043/diff/1058/86
> File net/proxy/proxy_config_service_linux.cc (right):
>
> http://codereview.chromium.org/113043/diff/1058/86#newcode625
> Line 625: LOG(INFO) << "PostDestroyTask";
> nit: this is a duplicate of log statement below.

Oh I had meant to take those extra tracing logs out before final
submission. Which in no way explains how I came to have two of the
same there :-).

> http://codereview.chromium.org/113043/diff/1058/87
> File net/proxy/proxy_config_service_linux.h (right):
>
> http://codereview.chromium.org/113043/diff/1058/87#newcode15
> Line 15: #include "base/task.h"
> nit: is this header used?

Not anymore, removed. Incredible that you caught that.

> http://codereview.chromium.org/113043

So this latest version has the shortcut in PostDestroy, and does not
call RunAllPending in BrowserMain.

Thanks

[eroman, 2009-05-14 18:36:26]

Sure we can save the task posting if we're already on the 
> right thread, but I don't see how that is a workaround. 
> AFAICT, on browser shutdown, ProxyConfigServiceLinux is not
> destroyed from the UI thread. Am I missing something?

Ah ok, I misunderstood then. Disregard that comment, no point optimizing for the
unit-test case and having a different codepath.

[eroman, 2009-05-14 18:37:32]

Adding Darin who had some suggestions on how to do the teardown using a service.

[sdoyon, 2009-05-15 12:22:45]

OK, a service? Can you elaborate?

[eroman, 2009-05-18 22:22:36]

This looks good to me...
Unless Darin has alternate suggestions, I would say to check this in.

[sdoyon, 2009-05-20 20:37:58]

Reorganized creation of ProxyService to resolve conflict with r16413.

eroman@ -->> please check that part :-).

thx

[eroman, 2009-05-20 21:23:14]

lgtm

http://codereview.chromium.org/113043/diff/1077/119
File net/proxy/proxy_config_service_linux_unittest.cc (right):

http://codereview.chromium.org/113043/diff/1077/119#newcode36
Line 36: // Undo macro pollution from GDK includes (from message_loop.h).
Instead of doing this undef, would it work to remove "message_loop.h" from
"proxy_config_service_linux.h" (that header could just forward declare
MessageLoop)?

http://codereview.chromium.org/113043/diff/1077/115
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/113043/diff/1077/115#newcode212
Line 212: bool usingV8Resolver,
same comment about using underscores instead of camel case.

http://codereview.chromium.org/113043/diff/1077/115#newcode217
Line 217: CHECK(io_loop);
I would suggest writing these on one line, and as a DCHECK:

DCHECK(pc || io_loop);

http://codereview.chromium.org/113043/diff/1077/115#newcode220
Line 220: CHECK(url_request_context);
I would delete this check (see later suggestion).

http://codereview.chromium.org/113043/diff/1077/115#newcode222
Line 222: // Choose the system configuration service appropriate for each
platform.
This comment looks to be duplicated from next line.

http://codereview.chromium.org/113043/diff/1077/115#newcode228
Line 228: ProxyService* proxy_service;
This looks fine.
I would recommend generalizing it a bit further as:

ProxyResolver* proxy_resolver = using_v8_resolver ?
    new ProxyResolverV8() : CreateNonV8ProxyResolver();

ProxyService* proxy_service = new ProxyService(
    proxy_config_service, proxy_resolver);

if (!proxy_resolver->does_fetch()) {
  // Configure PAC script downloads to be issued using |url_request_context|.
  DCHECK(url_request_context);
  proxy_service->SetProxyScriptFetcher(
      ProxyScriptFetcher::Create(url_request_context));
}

return proxy_service;

http://codereview.chromium.org/113043/diff/1077/116
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/113043/diff/1077/116#newcode92
Line 92: // Iff |usingV8Resolver| is true, then the V8 implementation is
please name using underlines instead of camel case for variables, i.e.
"using_v8_resolver". I would also suggest the active form, "use_v8_resolver".

http://codereview.chromium.org/113043/diff/1077/116#newcode94
Line 94: // |url_request_context| is only used when usingV8Resolver is true:
see comment above -- rename using_v8_resolver.

http://codereview.chromium.org/113043/diff/1077/116#newcode97
Line 97: // |io_loop| points to the IO thread's message loop, It is only
nit: lower case "It" --> "it", or change the comma into a period.

http://codereview.chromium.org/113043/diff/1077/116#newcode106
Line 106: bool usingV8Resolver,
using_v8_resolver

http://codereview.chromium.org/113043/diff/1077/116#newcode111
Line 111: // specified fixed settings. |pc| must not be NULL.
How about instead making the argument type:

const ProxyConfig& pc

since NULL isn't a valid value.

[sdoyon, 2009-05-20 21:24:26]

Woops, this doesn't actually work.

webkit/tools/test_shell/test_shell_request_context.cc creates a proxy
service that may have either a fixed config or let the proxy
config service handle it. This used not to be an issue because it does
not use the v8 implementation and we used not to provide a linux proxy
config service in that case.

I'm not familiar with that code. I'll take a look. Hints welcome.

[eroman, 2009-05-20 22:06:51]

> I'm not familiar with that code. I'll take a look.
> Hints welcome.

I guess the problem you have is with needing the specify the IO loop?

I wouldn't worry about that case too much, since it is just test_shell (other
stuff like PAC is borked in linux test_shell too, due to v8 threading, so not a
big loss). Seems fine to just pass NULL the IO thread and let just use fixed
config on linux.

Otherwise if you want to investigate further, you will find that the IO loop for
test_shell is created in simple_resource_loader_bridge.cc (and private to that
file).

[sdoyon, 2009-05-21 15:52:33]

On Wed, 20 May 2009, eroman@chromium.org wrote:

> lgtm
>
>
> http://codereview.chromium.org/113043/diff/1077/119
> File net/proxy/proxy_config_service_linux_unittest.cc (right):
>
> http://codereview.chromium.org/113043/diff/1077/119#newcode36
> Line 36: // Undo macro pollution from GDK includes (from
> message_loop.h).
> Instead of doing this undef, would it work to remove "message_loop.h"
> from "proxy_config_service_linux.h" (that header could just forward
> declare MessageLoop)?

No, both proxy_config_service_linux and
proxy_config_service_linux_unittest use MessageLoop::current() (and
MessageLoopForUI::current()).

Applied your suggestions for all other comments.

Now for the test_shell issue...

On Wed, 20 May 2009, eroman@chromium.org wrote:

> I guess the problem you have is with needing the specify the IO loop?

Yes.

> I wouldn't worry about that case too much, since it is just test_shell
> (other stuff like PAC is borked in linux test_shell too, due to v8
> threading, so not a big loss). Seems fine to just pass NULL the IO
> thread and let just use fixed config on linux.

Trouble is I had a DCHECK in ProxyService::Create() against a NULL
io_loop. It didn't seem like a good idea to make the io_loop != NULL
requirement specific to linux, although that's certainly an option.

I could force test_shell to pass in the no_proxy fixed config so that
the NULL io_loop is acceptable, but I'd have to move the DCHECK down
into linux-specific code, and I'd still need to add an #ifdef LINUX in
TestShellRequestContext.

OK here's a different idea: how about allowing a NULL io_loop to be
passed in, and simply disabling gconf settings change notifications in
that case? (It's surprisingly easy to do.) I wouldn't want this to be
the general case, but it'd be fine for test setups like
test_shell. It'd even allow making the initial gconf settings fetch,
FWIW.

Oh but then there's another hurdle: in
ProxyService::CreateSystemProxyConfigService() I enforce that we are
being called on a UI thread. But
webkit/tools/test_shell/run_all_tests.cc instantiates a default type
loop. OK' I'll just change it to instantiate a UI loop. Hope that's
appropriate. I assume so since other tests don't seem to trigger this
assertion. Kind of messy...

OK! Try success, finally.

So please check these last minute adjustments, and let me know whether
you'd prefer the first suggested approach.

> Otherwise if you want to investigate further, you will find that the IO
> loop for test_shell is created in simple_resource_loader_bridge.cc (and
> private to that file).

OK, we can fix this later. SimpleResourceLoaderBridge would need to
pass in the io_loop pointer to TestShellRequestContext. We could make
SimpleResourceLoaderBridge responsible for creating
TestShellRequestContext, or perhaps better, split out proxy
initialization as a separate step to be done after creation and before
use. It shouldn't be hard, but I think I shouldn't do it in this
patch, it's been held back long enough.

Thanks

[eroman, 2009-05-21 19:38:36]

lgtm.

> OK here's a different idea: how about allowing a NULL io_loop to be
> passed in, and simply disabling gconf settings change notifications in
> that case? 

Sounds good to me.

> webkit/tools/test_shell/run_all_tests.cc instantiates a default type
> loop. OK' I'll just change it to instantiate a UI loop. Hope that's
> appropriate.

Seems OK.
Although could you also solve the DCHECK by changing:
  loop_ = MessageLoopForUI::current();
to:
  loop_ = MessageLoop::current();

> It shouldn't be hard, but I think I shouldn't do it in this
> patch, it's been held back long enough.

Agreed, please don't block on this! (As a reviewer I much prefer smaller
iterative patches... losing steam on this one).

[sdoyon, 2009-05-22 14:06:24]

On Thu, 21 May 2009, eroman@chromium.org wrote:

>> webkit/tools/test_shell/run_all_tests.cc instantiates a default type
>> loop. OK' I'll just change it to instantiate a UI loop. Hope that's
>> appropriate.
>
> Seems OK.
> Although could you also solve the DCHECK by changing:
> loop_ = MessageLoopForUI::current();
> to:
> loop_ = MessageLoop::current();

In GConfSettingGetterImpl::Init()? I don't think that'd be intirely
safe.

In fact the first assertion that we'll hit would be from
ProxyConfigService* ProxyService::CreateSystemProxyConfigService()
doing
  MessageLoop* glib_default_loop = MessageLoopForUI::current();

That'll DCHECK if the current thread's loop is not of TYPE_UI.

Here's my understanding and logic:

I'm having it skip gconf notifications when the io_loop is NULL, but
it still does the initial gconf fetch, which still needs to be on the
thread running the default glib loop. The ForUI part is to try to
check that we're indeed on the thread with the default glib
context. That happens in message_pump_glib.cc:

MessagePumpForUI::MessagePumpForUI()
    : state_(NULL),
      context_(g_main_context_default()) {

If I understand correctly, the TYPE_DEFAULT MessageLoop won't refer
to glib at all. Now if no thread runs the glib context at all, then
our gconf usage should be safe, (only idles and timeouts might not run
but that probably doesn't matter in a simple test setup). But it seems
safer to ensure that we're on the thread running the default glib main
loop, so as not to make the assumption that no other thread will be
running the glib loop.

> Agreed, please don't block on this! (As a reviewer I much prefer smaller
> iterative patches... losing steam on this one).

I can imagine. Same here. Thanks for your help and patience.