Use TestingProfile::Builder::AddTestingFactory at ChromeRenderViewHostTestHarness

Use TestingProfile::Builder::AddTestingFactory at ChromeRenderViewHostTestHarness

This test harness is overriding the SigninManagerFactory for its TestingProfiles too late, after the Profile has already been created. Any services that depend on the SigninManager and get created with the Profile are thus left with a dead pointer (e.g. UserPolicySigninService).

This change sets the fake factory before the TestingProfile gets created. It also makes ChromeRenderViewHostTestHarness more consistent by using the fake factory for every TestingProfile created, not just the first one.

BUG=345617
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256379

[Joao da Silva, 2014-03-07 20:27:30]

Hi Pawel, can you have a look? This fixes the crashes found by
https://codereview.chromium.org/187693002.

The more fundamental issue is that SetTestingFactory() may be called for a
service that has already been created. In this case, UPSS had a reference to
SM-1, then the test setup sets the testing factory, SM-1 gets destroyed and
replaced by SM-2, but the UPSS is still referencing SM-1. When it calls
RemoveObserver() at shutdown it leads to the crash.

This CL just fixes the immediate crash; do you have a better suggestion on how
to force a service to be built when its factory says that
ServiceIsNULLWhileTesting?

Colin: FYI

Thanks!

[Paweł Hajdan Jr., 2014-03-07 22:08:12]

https://codereview.chromium.org/189443008/diff/20001/components/browser_conte...
File
components/browser_context_keyed_service/browser_context_keyed_service_factory.h
(right):

https://codereview.chromium.org/189443008/diff/20001/components/browser_conte...
components/browser_context_keyed_service/browser_context_keyed_service_factory.h:53:
void RemoveTestingFactory(content::BrowserContext* context);
This can easily make the code harder to follow. Can we avoid setting the testing
factory to something unwanted in the first place?

[Joao da Silva, 2014-03-07 23:29:20]

https://codereview.chromium.org/189443008/diff/20001/components/browser_conte...
File
components/browser_context_keyed_service/browser_context_keyed_service_factory.h
(right):

https://codereview.chromium.org/189443008/diff/20001/components/browser_conte...
components/browser_context_keyed_service/browser_context_keyed_service_factory.h:53:
void RemoveTestingFactory(content::BrowserContext* context);
On 2014/03/07 22:08:12, Paweł Hajdan Jr. wrote:
> This can easily make the code harder to follow. Can we avoid setting the
testing
> factory to something unwanted in the first place?

That would be great, can you suggest a way to do it?

Note that the factory isn't being set by a test directly;
BrowserContextDependencyManager::DoCreateBrowserContextServices() calls
SetEmptyTestingFactory() when ServiceIsNULLWhileTesting is true, which sets a
NULL factory; that's what's getting cleared here.

[Andrew T Wilson (Slow), 2014-03-10 12:24:09]

I still need to look at the code, but it looks like you're changing
ChromeRenderViewHostTestHarness to properly initialize a SigninManager testing
factory. Given that, why do we need to prevent UPSS from being created in tests?
I want it to show up in tests, to make tests as "real" as possible
(specifically, I want SigninManager to get created during profile creation).

What you're doing is setting up a situation where SigninManager is not pulled in
early in tests, but *is* pulled in early in a real Chrome executable, which
seems really fragile. I spent enough time fixing dependency issues with tests
relying on SigninManager not being created early, and I don't want that code to
bitrot.

[Andrew T Wilson (Slow), 2014-03-10 12:30:45]

I think that you can fix the issue you're concerned about just my modifying
ChromeRenderViewHostTestHarness to use
TestingProfile::Builder::AddTestingFactory() without any other changes.

https://codereview.chromium.org/189443008/diff/20001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service_factory.h (right):

https://codereview.chromium.org/189443008/diff/20001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service_factory.h:49: virtual
bool ServiceIsNULLWhileTesting() const OVERRIDE;
I'm fundamentally opposed to this change - we should load UPSS with tests to
make them as close to a real runtime environment as possible.

https://codereview.chromium.org/189443008/diff/20001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc (right):

https://codereview.chromium.org/189443008/diff/20001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:185:
UserPolicySigninServiceFactory::GetInstance()->RemoveTestingFactory(
Having RemoveTestingFactory() override ServiceIsNULLForTests() seems like a
really weird side-effect.

https://codereview.chromium.org/189443008/diff/20001/chrome/test/base/chrome_...
File chrome/test/base/chrome_render_view_host_test_harness.cc (right):

https://codereview.chromium.org/189443008/diff/20001/chrome/test/base/chrome_...
chrome/test/base/chrome_render_view_host_test_harness.cc:64: TestingProfile*
profile = new TestingProfile();
Use TestingProfile::Builder and set the testing factory in the builder. Then get
rid of all the other changes in this CL.

https://codereview.chromium.org/189443008/diff/20001/components/browser_conte...
File
components/browser_context_keyed_service/browser_context_keyed_service_factory.cc
(right):

https://codereview.chromium.org/189443008/diff/20001/components/browser_conte...
components/browser_context_keyed_service/browser_context_keyed_service_factory.cc:54:
testing_factories_.erase(context);
I think we should get rid of this, but if we keep it, it should be robust by
rebuilding the context, just like SetTestingFactory() does.

[blundell, 2014-03-11 12:02:46]

https://codereview.chromium.org/189443008/diff/20001/chrome/test/base/chrome_...
File chrome/test/base/chrome_render_view_host_test_harness.cc (right):

https://codereview.chromium.org/189443008/diff/20001/chrome/test/base/chrome_...
chrome/test/base/chrome_render_view_host_test_harness.cc:64: TestingProfile*
profile = new TestingProfile();
+1, great catch Drew.

On 2014/03/10 12:30:45, Andrew T Wilson wrote:
> Use TestingProfile::Builder and set the testing factory in the builder. Then
get
> rid of all the other changes in this CL.

[Joao da Silva, 2014-03-11 17:10:02]

Thanks for the hint Drew! TestingProfile::Builder::AddTestingFactory is exactly
what we needed. Now I'm embarrassed I even proposed the the initial CL :-)

Colin, I've verified locally that this fixes the crash at your CL.

@Paweł: please have another look at this

Thanks!

[Paweł Hajdan Jr., 2014-03-11 17:57:41]

LGTM

[Joao da Silva, 2014-03-11 18:16:41]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-03-11 18:30:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/189443008/40001

[commit-bot: I haz the power, 2014-03-12 01:27:58]

Change committed as 256379