WIP CL @ https://crrev.com/c/1008922

I didn't make much progress on this yesterday (had trouble understanding what is wrong / what leads to UaF when running PermissionManagerTest.InsecureOrigin after https://crrev.com/c/1008922).  But... I've started wondering today if maybe ScopedMockRenderProcessHostFactory should be automatically created by TestBrowserThreadBundle rather than by TestingProfile.  One advantage would be that (AFAIR) there can only be one TestBrowserThreadBundle + it is usually created right at the beginning of a test (or during test setup) - this means that we wouldn't have to worry as much about handling cases where ScopedMockRenderProcessHostFactory is created more than once.

PS. Putting ScopedMockRenderProcessHostFactory into either TestingProfile or into TestBrowserThreadBundle feels a little bit icky (dealing with RenderProcessHost creation seems unrelated to either TestingProfile or TestBrowserThreadBundle.  But I guess maybe usability trumps such considerations.  Let's continue thinking about it.

Or maybe it's possible to move it to WebContentsTester. Note that it receives a BrowserContext instance that is likely to be a TestingProfile. Therefore, TestingProfile seems like a proper candidate. TestBrowserThreadBundle doesn't look related to the RenderProcessHost stuff.

Related to RenderProcessHost stuff?:
- ScopedMockRenderProcessHostFactory: yes
- TestBrowserThreadBundle: no
- WebContentsTester: no
- BrowserContext/TestingProfile: no

Only a single instance created at the very start of a test?:
- ScopedMockRenderProcessHostFactory: yes
- TestBrowserThreadBundle: yes
- WebContentsTester: no
- BrowserContext/TestingProfile: no

Test author has to include it separately from other test helpers?:
- TestBrowserThreadBundle: yes
- ScopedMockRenderProcessHostFactory: currently yes

In the end, I am not sure I understand why it is okay to say: 1) if a unit test requires proper browser threads then it has to create TestBrowserThreadBundle, but possibly not okay to say 2) if a unit test triggers creation of new RenderProcessHosts then it has to create ScopedMockRenderProcessHostFactory.  Note that 1) dependency on browser threads and 2) creation of new RenderProcessHosts can be accidental (i.e. both can be caused by a feature only indirectly/remotely triggered by the code under test).

Maybe both content::TestBrowserThreadBundle and content::ScopedMockRenderProcessHostFactory can/should be combined into a new content::TestBrowserGlobalEnvironment?  OTOH, I am not sure if adding yet another class will help discoverability here... :-/

I wouldn't assume anything about our existing situation is "OK".  Individuals have incrementally improved a lot over time, but we're a long way from the ideal situation for test authors, which would probably be that you never have to manually create any "environment" classes, because we magically autodetect that you need them and create the right ones at the right times.  That's technically challenging (maybe infeasible), so any move that reduces the number of manual instantiations is good, regardless of whether we're consistent about it with all test classes.

I challenge the statement "if a unit test triggers creation of new RenderProcessHosts then it has to create ScopedMockRenderProcessHostFactory". The password tests from the CL create a WebContentsTester instance and TestingProfile. Those testing objects create RenderProcessHost somehow.
If a test explicitly uses TestWebContents::Create(), it's obvious that it needs a testing RFH and everything behind it. Why wouldn't we instantiate MockRenderProcessHost automatically?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a488f7b5ce8d296c5c04e9cabc8e592c347ec793

commit a488f7b5ce8d296c5c04e9cabc8e592c347ec793
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Apr 18 22:32:06 2018

Implicitly create MockRenderProcessHostFactory from TestWebContents.

Implicitly creating MockRenderProcessHostFactory from the constructor of
TestWebContents avoids the need to do this explicitly in individual test
suites.

Tests that need to use their own, custom RenderProcessHostFactory may
still do so, by ensuring their factory is hooked up before the first
TestWebContents is created (in this case TestWebContents will not create
and use MockRenderProcessHostFactory).

This CL also makes a few opportunistic changes (adding a for_testing
suffix to methods used only from tests, fixing some |git cl lint|
warnings).

Bug:  832100 
Change-Id: Ie85ef3f8a433e94dbebe2ea0600a447d5c93a8a7
Tbr: lazyboy@chromium.org
Tbr: pavely@chromium.org
Tbr: sadrul@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1008922
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551858}
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/chrome/browser/extensions/test_extension_environment.h
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/chrome/browser/sync/sessions/sync_sessions_web_contents_router_unittest.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer_unittest.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/chrome/browser/ui/passwords/password_manager_porter_unittest.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/chrome/browser/ui/webui/browsing_history_handler_unittest.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/content/browser/frame_host/render_widget_host_view_guest_unittest.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/content/browser/media/session/audio_focus_manager_unittest.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/content/browser/service_worker/service_worker_process_manager_unittest.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/content/browser/shared_worker/shared_worker_service_impl_unittest.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/content/browser/site_instance_impl_unittest.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/content/test/test_render_view_host_factory.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/content/test/test_web_contents.cc
[modify] https://crrev.com/a488f7b5ce8d296c5c04e9cabc8e592c347ec793/ui/views/controls/webview/webview_unittest.cc