Eliminate potential for flaky crash in BCKSFactory::SetTestingFactory().

Eliminate potential for flaky crash in BCKSFactory::SetTestingFactory().

BCKSFactory::SetTestingFactory() calls BCKSFactory::BrowserContextShutdown(),
which a BCKSFactory subclass may override to perform operations that result in
BrowserContextDependencyManager::AssertBrowserContextWasntDestroyed() being
called (e.g., BCKSFactory::GetServiceForContext()). When used with
TestingProfile::Builder, BCKSFactory::SetTestingFactory() is called *before*
the TestingProfile calls
BrowserContextDependencyManager::CreateBrowserContextServicesForTest(). These
facts set up the potential for a flaky crash: if the BrowserContext instance
being used is at the same address as one that had been used in a previous test,
then the call to AssertBrowserContextWasntDestroyed() will raise an error. This
CL eliminates the potential for that crash by explicitly informing
BrowserContextDependencyManager that the BrowserContext instance being used in
BCKSFactory::SetTestingFactory() is live.

NOTRY=true

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

[blundell, 2014-02-11 16:47:45]

Elliot,

Could you take a look? The other solution I thought of was to create a new
public method in BrowserContextDependencyManager like |BrowserContextCreated()|
that just marks the BrowserContext live and have all Profile creation flows call
that method near the top of their flow. That seems a little like overkill to me.
Note that it doesn't work to move the call to
CreateBrowserContextServicesForTest() to before the calls to set the testing
factories as the calls to set the testing factories are first precisely so that
they can be set before the services are created.

Thanks,

Colin

[blundell, 2014-02-13 14:53:56]

Ping :).

[Elliot Glaysher, 2014-02-13 17:23:53]

lgtm

https://codereview.chromium.org/159763004/diff/1/components/browser_context_k...
File
components/browser_context_keyed_service/browser_context_dependency_manager.cc
(right):

https://codereview.chromium.org/159763004/diff/1/components/browser_context_k...
components/browser_context_keyed_service/browser_context_dependency_manager.cc:77:
// at 0xWhatever.
The only thing I can think of to improve this patch is moving this comment to
the new method and then calling MarkBrowserContextLiveForTesting() here.

[blundell, 2014-02-13 21:45:40]

Thanks!

https://codereview.chromium.org/159763004/diff/1/components/browser_context_k...
File
components/browser_context_keyed_service/browser_context_dependency_manager.cc
(right):

https://codereview.chromium.org/159763004/diff/1/components/browser_context_k...
components/browser_context_keyed_service/browser_context_dependency_manager.cc:77:
// at 0xWhatever.
On 2014/02/13 17:23:53, Elliot Glaysher wrote:
> The only thing I can think of to improve this patch is moving this comment to
> the new method and then calling MarkBrowserContextLiveForTesting() here.

Done.

[blundell, 2014-02-13 21:45:46]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-02-13 21:49:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/159763004/70001

[commit-bot: I haz the power, 2014-02-13 22:21:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 22:21:07]

Retried try job too often on mac_rel for step(s) app_list_unittests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chromedriver_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests,
ipc_tests, jingle_unittests, media_unittests, message_center_unittests,
nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests,
sql_unittests, sync_integration_tests, sync_unit_tests,
telemetry_perf_unittests, telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[blundell, 2014-02-14 09:05:33]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-02-14 09:05:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/159763004/350001

[commit-bot: I haz the power, 2014-02-14 10:28:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 10:28:52]

Try jobs failed on following builders: linux_chromium_rel

[blundell, 2014-02-14 10:51:56]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-02-14 10:52:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/159763004/350001

[commit-bot: I haz the power, 2014-02-14 10:52:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 10:52:30]

Try jobs failed on following builders: linux_chromium_rel

[blundell, 2014-02-14 11:01:50]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-02-14 11:02:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/159763004/350001

[commit-bot: I haz the power, 2014-02-14 15:33:54]

Change committed as 251336