Linux: fix a data race in a proxy config service unit test.

Linux: fix a data race in a proxy config service unit test.
BUG=21474
TEST=none


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

[dank, 2009-09-21 18:58:02]

LGTM but maybe run it by sdoyon?

[sdoyon, 2009-09-22 15:03:45]

Hi,

Still woozy from my cold so I'm kind of slow :-). Hope I'm figuring
this out correctly.

So what's the original problem? Ah OK I see that Reset() deliberately
calls Shutdown() on the UI thread, which is inappropriate for the KDE
getter.

So Reset() did two things: clear the cached_config_ state, and reset
the getter. I believe the latter was mostly to avoid an assertion
triggering when Init'ing the getter twice. The code has evolved since
then though: the mock gconf getter now has a harmless noop Init() and
Shutdown(), so looks like there are no such assertions to trip
anymore.

So your solution recreates the ProxyConfigServiceLinux for each
test. In the gconf case, the same wrapped getter is passed every
time. In the KDE test, the config service creates a new KDE getter for
each test, and the full destruction strategy with PostDestroy()
manages cleaning up each KDE getter.

I find the whole thing somewhat confusing. We have
PatsyGConfSettingGetter that doesn't appear to override anything, it
only circumvents the normal life cycle (hairy to begin with) and
prevents deletion of the gconf getter when the config service is
deleted, but that only applies to the case where gconf is already
overridden by a mock with harmless Init() and Shutdown(). All that to
allow reusing the gconf getter across tests, but we actually
reinitialize the state for each test anyway :-). Did I get all that
right?

Recreating the config service for every test is a pretty good idea: we
already have the complicated destruction strategy that handles
threading, might as well have the test run that code. But I don't
think there's any benefit in wrapping that inside the sync
getter. Instead, I would suggest just moving the config service and
sync getter instantiation inside the tests loop. Just don't reuse the
config service, then you can take out the Reset() method, and that's
all.

To clarify:
-take out Reset(),
-move SynchConfigGetter sync_config_getter(new ProxyConfigServiceLinux(...)
 inside the for (...) loop in each test function.

Now SetupAndInitialFetch() is called only once on each config service
object (so each getter is Init'ed only once, and not reused after
shutdown). And no need for the patsy wrappers, AFAICT.

[Mike Mammarella, 2009-09-22 17:00:20]

Indeed this is simpler.

[sdoyon, 2009-09-22 17:19:05]

LGTM