Fix wrong initialization of renderer_initiated_creation parameter.

Fix wrong initialization of renderer_initiated_creation
parameter.

Initialize the renderer_initiated_creation parameter as
true only when a RenderFrame was created in the renderer
process. This fixes an issue where the
Render{Frame,View}Created observer from
WebContentsObserver can be called before the process
is alive (and therefore, the RenderFrame can't possibly
have been created).

BUG=492830
Committed: https://crrev.com/269b702fb4c4d30aa2d4d103a3710095a7002b7d
Cr-Commit-Position: refs/heads/master@{#333316}

[lfg, 2015-06-03 23:58:12]

lfg@chromium.org changed reviewers:
+ nasko@chromium.org

[lfg, 2015-06-03 23:58:13]

PTAL, thanks!

[nasko, 2015-06-04 16:56:37]

nasko@chromium.org changed reviewers:
+ nick@chromium.org

[nasko, 2015-06-04 16:56:38]

Thanks for fixing this!

It looks good, but I know Nick and Avi had a discussion in the past about how
tracking newly created WebContents should work and whether it should even be
possible. I'm adding Nick to confirm the changes are in the right spirit.

https://codereview.chromium.org/1159143008/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/1159143008/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1826: class
InstallRFCObserver : public WebContentsAddedObserver {
I'm adding Nick to look over this, as I know there have been some discussions
about tracking newly created WebContents.

https://codereview.chromium.org/1159143008/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1870:
EXPECT_EQ("/files/title2.html", new_web_contents->GetVisibleURL().path());
nit: I'd add also a IsRenderFrameLive check, as this is what was failing. We
want to ensure it gives us the expected answer.

[lfg, 2015-06-04 17:34:13]

https://codereview.chromium.org/1159143008/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/1159143008/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1826: class
InstallRFCObserver : public WebContentsAddedObserver {
On 2015/06/04 16:56:38, nasko wrote:
> I'm adding Nick to look over this, as I know there have been some discussions
> about tracking newly created WebContents.

Ok. Just one note that this is a test-only class.

https://codereview.chromium.org/1159143008/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1870:
EXPECT_EQ("/files/title2.html", new_web_contents->GetVisibleURL().path());
On 2015/06/04 16:56:37, nasko wrote:
> nit: I'd add also a IsRenderFrameLive check, as this is what was failing. We
> want to ensure it gives us the expected answer.

Done. I had to add this to the Observer, since this check can only fail after
the RenderFrameHost is created, but before it navigates (since navigation will
initialize the RenderFrame if it's not yet initialized).

[ncarter (slow), 2015-06-04 18:20:46]

https://codereview.chromium.org/1159143008/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/1159143008/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1822:
->IsRenderFrameLive());
Rather than opening up WebContentsObserver so that we can write this assert,
would it be possible to move these asserts into
https://code.google.com/p/chromium/codesearch#chromium/src/content/test/web_c...
?

That would have the effect of adding them to all tests everywhere, including
unit tests. The downside is that you may discover some failures that need to be
patched up (though hopefully not too many; I think nasko has been treading this
water already).

[lfg, 2015-06-05 20:03:07]

Patchset #12 (id:220001) has been deleted

[lfg, 2015-06-05 20:52:00]

On 2015/06/04 18:20:46, ncarter wrote:
>
https://codereview.chromium.org/1159143008/diff/80001/content/browser/frame_h...
> File content/browser/frame_host/render_frame_host_manager_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/1159143008/diff/80001/content/browser/frame_h...
> content/browser/frame_host/render_frame_host_manager_browsertest.cc:1822:
> ->IsRenderFrameLive());
> Rather than opening up WebContentsObserver so that we can write this assert,
> would it be possible to move these asserts into
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/test/web_c...
> ?
> 
> That would have the effect of adding them to all tests everywhere, including
> unit tests. The downside is that you may discover some failures that need to
be
> patched up (though hopefully not too many; I think nasko has been treading
this
> water already).

Ok, I went for it, had to fix some tests to go along with it as well as setting
the renderer_initialized bit in WebContentsImpl.

Also, I removed the test that I added earlier -- there are already tests that
exercise the noreferrer/target blank and they get this check enabled now that's
in the sanity checker.

nick@, nasko@: please, take a look.

[nasko, 2015-06-05 21:23:43]

Looks good, though one universal comment. Replace the CreateRenderView calls
with InitializeRenderFrameIfNeeded. It does a liveness check first and only
calls the CreateRenderView if needed. This can save us some headaches in the
future if unit test code changes such that it initializes the RF/RV earlier.

https://codereview.chromium.org/1159143008/diff/260001/content/browser/frame_...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1159143008/diff/260001/content/browser/frame_...
content/browser/frame_host/frame_tree_unittest.cc:236:
test_rvh()->CreateRenderView(base::string16(), MSG_ROUTING_NONE,
nit: main_test_rfh()->InitializeRenderFrameIfNeeded()

[lfg, 2015-06-05 21:35:22]

On 2015/06/05 21:23:43, nasko wrote:
> Looks good, though one universal comment. Replace the CreateRenderView calls
> with InitializeRenderFrameIfNeeded. It does a liveness check first and only
> calls the CreateRenderView if needed. This can save us some headaches in the
> future if unit test code changes such that it initializes the RF/RV earlier.
> 
>
https://codereview.chromium.org/1159143008/diff/260001/content/browser/frame_...
> File content/browser/frame_host/frame_tree_unittest.cc (right):
> 
>
https://codereview.chromium.org/1159143008/diff/260001/content/browser/frame_...
> content/browser/frame_host/frame_tree_unittest.cc:236:
> test_rvh()->CreateRenderView(base::string16(), MSG_ROUTING_NONE,
> nit: main_test_rfh()->InitializeRenderFrameIfNeeded()

Done.

[nasko, 2015-06-05 21:39:10]

LGTM

[ncarter (slow), 2015-06-05 21:47:50]

https://codereview.chromium.org/1159143008/diff/260001/chrome/browser/backgro...
File chrome/browser/background/background_contents.cc (right):

https://codereview.chromium.org/1159143008/diff/260001/chrome/browser/backgro...
chrome/browser/background/background_contents.cc:47:
create_params.renderer_initiated_creation = routing_id != MSG_ROUTING_NONE;
What are the (non-test) implications of this fix?

The new code does seem to be the right thing to do, but I am curious -- how did
things behave without the fix? Was there a bogus WebContentsObserver call if
routing_id was MSG_ROUTING_NONE, or was it worse, like no RenderView was ever
actually created?

https://codereview.chromium.org/1159143008/diff/260001/content/test/web_conte...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1159143008/diff/260001/content/test/web_conte...
content/test/web_contents_observer_sanity_checker.cc:47:
CHECK(render_frame_host->GetProcess()->HasConnection());
<< "RenderFrameCreated was called for a RenderFrameHost whose render process is
not currently live, so there's no way for the RenderFrame to have been created."

Or something like that. These CHECKS are lurking pretty far from the code that
triggers them, so we should do our best to orient the unlucky developer who runs
afoul of them.

https://codereview.chromium.org/1159143008/diff/260001/content/test/web_conte...
content/test/web_contents_observer_sanity_checker.cc:49: ->IsRenderFrameLive());
<< "RenderFrameCreated called on for a RenderFrameHost that thinks it is not
alive. RenderFrames should always be alive at the moment of creation."

... or something similar.

[lfg, 2015-06-05 22:07:07]

https://codereview.chromium.org/1159143008/diff/260001/chrome/browser/backgro...
File chrome/browser/background/background_contents.cc (right):

https://codereview.chromium.org/1159143008/diff/260001/chrome/browser/backgro...
chrome/browser/background/background_contents.cc:47:
create_params.renderer_initiated_creation = routing_id != MSG_ROUTING_NONE;
On 2015/06/05 21:47:50, ncarter wrote:
> What are the (non-test) implications of this fix?
> 
> The new code does seem to be the right thing to do, but I am curious -- how
did
> things behave without the fix? Was there a bogus WebContentsObserver call if
> routing_id was MSG_ROUTING_NONE, or was it worse, like no RenderView was ever
> actually created? 

In practice, the only issue is that we call the Render{Frame,View}Created
observer callbacks before the process exists. The reason this doesn't cause
other problems is because it's not possible to listen to the very first
RenderView/Frame created message because it's sent too early during the creation
of the WebContents, and we can't attach the observer that early in the process.

What ends up happening is that right after the creation, during the first
navigation, we create/initialize the RenderFrame/View here (since the process
doesn't exist, IsRenderFrameLive returns false):
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

https://codereview.chromium.org/1159143008/diff/260001/content/test/web_conte...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1159143008/diff/260001/content/test/web_conte...
content/test/web_contents_observer_sanity_checker.cc:47:
CHECK(render_frame_host->GetProcess()->HasConnection());
On 2015/06/05 21:47:50, ncarter wrote:
> << "RenderFrameCreated was called for a RenderFrameHost whose render process
is
> not currently live, so there's no way for the RenderFrame to have been
created."
> 
> Or something like that. These CHECKS are lurking pretty far from the code that
> triggers them, so we should do our best to orient the unlucky developer who
runs
> afoul of them.

Done.

https://codereview.chromium.org/1159143008/diff/260001/content/test/web_conte...
content/test/web_contents_observer_sanity_checker.cc:49: ->IsRenderFrameLive());
On 2015/06/05 21:47:50, ncarter wrote:
> << "RenderFrameCreated called on for a RenderFrameHost that thinks it is not
> alive. RenderFrames should always be alive at the moment of creation."
> 
> ... or something similar.

Done.

[ncarter (slow), 2015-06-05 22:12:09]

lgtm

https://codereview.chromium.org/1159143008/diff/260001/chrome/browser/backgro...
File chrome/browser/background/background_contents.cc (right):

https://codereview.chromium.org/1159143008/diff/260001/chrome/browser/backgro...
chrome/browser/background/background_contents.cc:47:
create_params.renderer_initiated_creation = routing_id != MSG_ROUTING_NONE;
On 2015/06/05 22:07:06, lfg wrote:
> On 2015/06/05 21:47:50, ncarter wrote:
> > What are the (non-test) implications of this fix?
> > 
> > The new code does seem to be the right thing to do, but I am curious -- how
> did
> > things behave without the fix? Was there a bogus WebContentsObserver call if
> > routing_id was MSG_ROUTING_NONE, or was it worse, like no RenderView was
ever
> > actually created? 
> 
> In practice, the only issue is that we call the Render{Frame,View}Created
> observer callbacks before the process exists. The reason this doesn't cause
> other problems is because it's not possible to listen to the very first
> RenderView/Frame created message because it's sent too early during the
creation
> of the WebContents, and we can't attach the observer that early in the
process.
> 
> What ends up happening is that right after the creation, during the first
> navigation, we create/initialize the RenderFrame/View here (since the process
> doesn't exist, IsRenderFrameLive returns false):
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

Thanks for clarifying. Please update the commit message with a summary of this
situation.

[lfg, 2015-06-08 15:51:56]

lfg@chromium.org changed reviewers:
+ atwilson@chromium.org, mmenke@chromium.org

[lfg, 2015-06-08 15:51:57]

Adding reviewers:

+atwilson@: for chrome/browser/background

+mmenke@: for chrome/browser/captive_portal and chrome/browser/net


Please take a look. Thanks!

[mmenke, 2015-06-08 16:09:50]

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:82:
->InitializeRenderFrameIfNeeded();
Why are these needed?  What do they do?  I assume they aren't needed for all
TestWebContents, or this could just be part of CreateTestWebContents().

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:82:
->InitializeRenderFrameIfNeeded();
Need to include the header for RenderFrameHostTester.

[Andrew T Wilson (Slow), 2015-06-08 16:32:17]

lgtm

[lfg, 2015-06-08 17:04:02]

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:82:
->InitializeRenderFrameIfNeeded();
On 2015/06/08 16:09:49, mmenke wrote:
> Need to include the header for RenderFrameHostTester.

The header is already included because of the inheritance (it is defined in
test_renderer_host.h, which defines RenderViewHostTestHarness, which is the
superclass of ChromeRenderViewHostTestHarness).

I can include it explicitly if you prefer.

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:82:
->InitializeRenderFrameIfNeeded();
On 2015/06/08 16:09:50, mmenke wrote:
> Why are these needed?  What do they do?  I assume they aren't needed for all
> TestWebContents, or this could just be part of CreateTestWebContents().

There are a few reasons for this, the main one is that we're adding extra
asserts to make the code more robust and initializing the RenderFrame is
something that needs to be done before we can use it. In a real-world scenario,
creating a WebContents doesn't initialize the RenderFrame. We can't expect to
receive IPCs without initializing it first (i.e. DidStartProvisionalLoadForFrame
comes from an IPC from a renderer).

[mmenke, 2015-06-08 17:15:06]

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:82:
->InitializeRenderFrameIfNeeded();
On 2015/06/08 17:04:01, lfg wrote:
> On 2015/06/08 16:09:49, mmenke wrote:
> > Need to include the header for RenderFrameHostTester.
> 
> The header is already included because of the inheritance (it is defined in
> test_renderer_host.h, which defines RenderViewHostTestHarness, which is the
> superclass of ChromeRenderViewHostTestHarness).
> 
> I can include it explicitly if you prefer.

I'd prefer it be included explicitly - same goes for the browser/net file.

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:82:
->InitializeRenderFrameIfNeeded();
On 2015/06/08 17:04:01, lfg wrote:
> On 2015/06/08 16:09:50, mmenke wrote:
> > Why are these needed?  What do they do?  I assume they aren't needed for all
> > TestWebContents, or this could just be part of CreateTestWebContents().
> 
> There are a few reasons for this, the main one is that we're adding extra
> asserts to make the code more robust and initializing the RenderFrame is
> something that needs to be done before we can use it. In a real-world
scenario,
> creating a WebContents doesn't initialize the RenderFrame. We can't expect to
> receive IPCs without initializing it first (i.e.
DidStartProvisionalLoadForFrame
> comes from an IPC from a renderer).

Could you add some sort of comment here?  It's neither obvious from this method
or the RenderFrameHostTester header what's going on.  Just something like
"WebContents create no main frame on cosntruction, and these tests expect one to
exist."

Same goes for the browser/net file.  Alternatively, could mention something
about it in web_contents_tester.h

[lfg, 2015-06-08 18:25:51]

Please, take another look. Thanks!

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:82:
->InitializeRenderFrameIfNeeded();
On 2015/06/08 17:15:06, mmenke wrote:
> On 2015/06/08 17:04:01, lfg wrote:
> > On 2015/06/08 16:09:49, mmenke wrote:
> > > Need to include the header for RenderFrameHostTester.
> > 
> > The header is already included because of the inheritance (it is defined in
> > test_renderer_host.h, which defines RenderViewHostTestHarness, which is the
> > superclass of ChromeRenderViewHostTestHarness).
> > 
> > I can include it explicitly if you prefer.
> 
> I'd prefer it be included explicitly - same goes for the browser/net file.

Done.

https://codereview.chromium.org/1159143008/diff/300001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:82:
->InitializeRenderFrameIfNeeded();
On 2015/06/08 17:15:06, mmenke wrote:
> On 2015/06/08 17:04:01, lfg wrote:
> > On 2015/06/08 16:09:50, mmenke wrote:
> > > Why are these needed?  What do they do?  I assume they aren't needed for
all
> > > TestWebContents, or this could just be part of CreateTestWebContents().
> > 
> > There are a few reasons for this, the main one is that we're adding extra
> > asserts to make the code more robust and initializing the RenderFrame is
> > something that needs to be done before we can use it. In a real-world
> scenario,
> > creating a WebContents doesn't initialize the RenderFrame. We can't expect
to
> > receive IPCs without initializing it first (i.e.
> DidStartProvisionalLoadForFrame
> > comes from an IPC from a renderer).
> 
> Could you add some sort of comment here?  It's neither obvious from this
method
> or the RenderFrameHostTester header what's going on.  Just something like
> "WebContents create no main frame on cosntruction, and these tests expect one
to
> exist."
> 
> Same goes for the browser/net file.  Alternatively, could mention something
> about it in web_contents_tester.h

Done.

[mmenke, 2015-06-08 18:32:44]

Thanks!  LGTM

[lfg, 2015-06-08 19:23:22]

The CQ bit was checked by lfg@chromium.org

[lfg, 2015-06-08 19:23:22]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
nick@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1159143008/#ps320001
(title: "addressing mmenke@'s comments")

[commit-bot: I haz the power, 2015-06-08 19:23:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159143008/320001

[commit-bot: I haz the power, 2015-06-08 19:28:31]

Committed patchset #16 (id:320001)

[commit-bot: I haz the power, 2015-06-08 19:29:29]

Patchset 16 (id:??) landed as
https://crrev.com/269b702fb4c4d30aa2d4d103a3710095a7002b7d
Cr-Commit-Position: refs/heads/master@{#333316}