Remove 2-stage RenderWidget initialization

Remove 2-stage RenderWidget initialization

We don't need to wait for HWND (etc.) creation before creating the
context any more. Therefore, we don't need to delay StartCompositor.
After that, CompleteInit doesn't do anything beyond sending
ViewHostMsg_RenderViewReady. The ViewHostMsg_RenderViewReady handler
only does a few things that can be done immediately after sending the
message that would have caused CompleteInit (ViewMsg_New /
ViewMsg_CreatingNew_ACK) - IPC ordering ensuring renderer-side order of
operations. Once CompleteInit is empty, ViewMsg_CreatingNew_ACK is not
useful any more either. The only thing that needs to be handled carefully
is calling RenderViewHostDelegate::RenderViewReady, because downstream
code expects it to be only called once a couple of conditions are true
(which were enforced by the renderer round-trip).

This saves an IPC round-trip in all RenderWidget creations.

BUG=535339
Committed: https://crrev.com/c4af307b97653099dda5e5fab24880065dbe0239
Cr-Commit-Position: refs/heads/master@{#351951}

[piman, 2015-09-29 00:11:55]

Patchset #3 (id:40001) has been deleted

[piman, 2015-09-29 00:12:01]

Patchset #2 (id:20001) has been deleted

[piman, 2015-09-29 00:12:07]

Patchset #1 (id:1) has been deleted

[piman, 2015-09-29 00:16:56]

piman@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2015-09-29 03:56:16]

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

[dcheng, 2015-09-29 03:56:17]

The RenderWidget changes seem fine to me, but I think nick@ is probably a better
reviewer for this change.

[ncarter (slow), 2015-09-29 18:28:41]

In the words of CongratBot: This is pure great.

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1486: bool
RenderProcessHostImpl::IsReady() const {
It might be worthwhile to add a comment here to point out that the
channel-connection (which initializes GetPeerPID) and the
result-of-process-launch (GetHandle()) can happen in either ordering.

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2423:
RenderProcessReady(this));
The intent here, right, is to call RenderProcessReady from exactly one of
OnProcessLaunched or OnChannelConnected, whichever occurs last?

We were actually just working through the ChildProcessHost variant of this
pattern, http://crbug.com/484937. In my experiments on that bug, it seemed like
OnChannelConnected usually happened first -- do you know if we have adequate
test coverage of the other ordering?

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2424: }
I like how this operates, but I'm wondering if we should be worried about it
evolving so that RenderProcessReady comes to be called twice?

We could protect against that by adding an explicit, redundant boolean member
(process_ready_) to protect against double notification, or else to make sure
that there's a DCHECK in one of the observers_ (maybe a test-only,
ContentBrowserSanityChecker style DCHECK?).

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2425: 
FYI: an alternative approach discussed in bug 484937, is to just initialize
Handle() from |peer_pid| at OnChannelConnected time, and do the notification
exclusively from OnChannelConnected. This has the advantage of removing the
latency impact of waiting for the process-launch result, but it puts a little
more trust in |peer_pid| (though I'd hope that's okay since there's the
authentication secret).

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1434:
BrowserThread::PostTask(
How confident are you that this posted task won't be in a race with later IPCs
from the renderer?

The worry I have is that there might be some code somewhere that isn't expecting
any IPCs prior to
RenderViewReady/WCO::RenderViewReady()/NOTIFICATION_EXTENSION_HOST_CREATED/NOTIFICATION_WEB_CONTENTS_CONNECTED
etc.

I suppose we could test this by forcing the race outcome: e.g. adding a
sleep(1000) before the PostTask here. If browser_tests and content_browsertests
pass with that sleep() added, then I'm probably okay with calling this safe.

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1438:
weak_factory_.GetWeakPtr()));
Also, what breaks if you just call RenderViewReady directly?

https://codereview.chromium.org/1369603003/diff/80001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/1369603003/diff/80001/content/public/browser/...
content/public/browser/render_process_host.h:120: // RenderProcessExited may
never be called.
Is there a DCHECK that catches this case? (If not, this might be another reason
to consider adding an explicit render_process_ready_ bool to
RenderProcessHostImpl).

https://codereview.chromium.org/1369603003/diff/80001/content/public/browser/...
content/public/browser/render_process_host.h:143: // launched and the channel is
connected. At that point, GetHandle() is valid,
I'm on the fence here, as to whether it's better to keep things simple and say
"Process is launched", or be more explicit, like, "the process launcher reported
a successful launch" or "the process launcher reported back a valid process
handle". With the simple wording it's not clear that the two conditions can
happen in either order. I'll leave it up to you.

https://codereview.chromium.org/1369603003/diff/80001/content/public/test/moc...
File content/public/test/mock_render_process_host.cc (right):

https://codereview.chromium.org/1369603003/diff/80001/content/public/test/moc...
content/public/test/mock_render_process_host.cc:175: bool
MockRenderProcessHost::IsReady() const {
I suppose we can add mock support for this if/when it's needed.

[piman, 2015-09-29 23:52:17]

Patchset #3 (id:100001) has been deleted

[piman, 2015-09-29 23:54:28]

PS3 has updates, PS4 is just a rebase.

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1486: bool
RenderProcessHostImpl::IsReady() const {
On 2015/09/29 18:28:40, ncarter wrote:
> It might be worthwhile to add a comment here to point out that the
> channel-connection (which initializes GetPeerPID) and the
> result-of-process-launch (GetHandle()) can happen in either ordering.

Done.

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2423:
RenderProcessReady(this));
On 2015/09/29 18:28:40, ncarter wrote:
> The intent here, right, is to call RenderProcessReady from exactly one of
> OnProcessLaunched or OnChannelConnected, whichever occurs last?

Right, the contract is, RenderProcessReady is only sent once both of:
- GetHandle() returns a valid handle
- the channel is connected (channel_->GetPeerPid() is valid)
are true.
(this corresponds to IsReady() is true).

GetHandle() is valid once OnProcessLaunched() is called (as a posted task from
the process launcher thread)
The channel is known to be connected once OnChannelConnected() is called (as a
posted task from the IO thread, after the newly running process sends the hello
message).

The 2 posted tasks race with each other, but since they are separate tasks, only
one of the callback will observe the IsReady() transition from false to true.


> We were actually just working through the ChildProcessHost variant of this
> pattern, http://crbug.com/484937. In my experiments on that bug, it seemed
like
> OnChannelConnected usually happened first -- do you know if we have adequate
> test coverage of the other ordering?

Actually, here, I found that it was usually the opposite, GetHandle() being
valid but the channel was not yet connected (I had a patch originally that only
waited for the process to be launched, but one of the tests broke on Windows,
namely
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...
, see ExtensionManagementApiEscalationTest /
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
as to why).

In a way it makes sense, because the LAUNCHER->UI hop is generally shorter than
the LAUNCHER->new process main->IO->UI multiple hop.

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2424: }
On 2015/09/29 18:28:40, ncarter wrote:
> I like how this operates, but I'm wondering if we should be worried about it
> evolving so that RenderProcessReady comes to be called twice?
> 
> We could protect against that by adding an explicit, redundant boolean member
> (process_ready_) to protect against double notification, or else to make sure
> that there's a DCHECK in one of the observers_ (maybe a test-only,
> ContentBrowserSanityChecker style DCHECK?).

I added a DCHECK. I'm fairly confident that we call only once (see above why).

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2425: 
On 2015/09/29 18:28:40, ncarter wrote:
> FYI: an alternative approach discussed in bug 484937, is to just initialize
> Handle() from |peer_pid| at OnChannelConnected time, and do the notification
> exclusively from OnChannelConnected. This has the advantage of removing the
> latency impact of waiting for the process-launch result, but it puts a little
> more trust in |peer_pid| (though I'd hope that's okay since there's the
> authentication secret).

I believe GetHandle and channel_->GetPeerPid return different things (they're a
different type on Windows). As much as it looks like we can extract the process
id from the process handle, I don't know that the opposite is true (not a
Windows expert here....)
I also think there may be places where we need the process handle before the
channel is connected. E.g. the task manager uses the process handle to do
things.

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1434:
BrowserThread::PostTask(
On 2015/09/29 18:28:40, ncarter wrote:
> How confident are you that this posted task won't be in a race with later IPCs
> from the renderer?
> 
> The worry I have is that there might be some code somewhere that isn't
expecting
> any IPCs prior to
>
RenderViewReady/WCO::RenderViewReady()/NOTIFICATION_EXTENSION_HOST_CREATED/NOTIFICATION_WEB_CONTENTS_CONNECTED
> etc.
> 
> I suppose we could test this by forcing the race outcome: e.g. adding a
> sleep(1000) before the PostTask here. If browser_tests and
content_browsertests
> pass with that sleep() added, then I'm probably okay with calling this safe.

So, it is already the case today that IPCs can be sent before
ViewHostMsg_RenderViewReady (e.g. before this patch, since we initialize the
compositor before sending ViewHostMsg_RenderViewReady, we need to do some IPCs
to the browser, e.g. to initialize the context).

Regardless, this patch only makes it better in that respect - instead of
UI->IO->Renderer IO->Renderer Main->Renderer IO->IO->UI->RenderViewReady, we do
UI->UI->RenderViewReady, i.e. RenderViewReady should happen no later in terms of
task ordering on the UI thread (This posts the task from the same function that
sends the message that would have triggered the ViewHostMsg_RenderViewReady on
the renderer side).
Now, of course, it is possible that this patch exposes existing races. I sent
https://codereview.chromium.org/1380573003/ to the trybots, which is the same
patch but changing this into a PostDelayedTask(..., 1000ms). It looks good so
far (modulo bots unable to check out the code), but will wait for completion
before landing.

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1438:
weak_factory_.GetWeakPtr()));
On 2015/09/29 18:28:40, ncarter wrote:
> Also, what breaks if you just call RenderViewReady directly?

There's at least a couple of places that I found, that basically don't expect
RenderViewReady to be called reentrantly.

In BrowserPluginGuest::OnWillAttachComplete, it will call RVHI::Init() (which
signals RenderViewReady), but in the callback expects to find the view on the
WebContentsImpl, whereas it's only set up later.

In the task manager, the TabContentsInformation expects in GetOwnership (which
is called indirectly from RenderViewReady) to find the WebContents in the list
of tabs in the browser. This ends up depending on the implementation of
chrome::Navigate, which does both triggering the navigation (indirectly calling
RenderViewHostImpl::CreateRenderView) and attaching the WebContents into the
list of tabs, but in various orders depending on the case.


It may be possible to fix those, but both of these are security sensitive and
way out of my area of expertise, so I prefer to keep this patch scoped, and not
introduce unexpected reentrancy, since in the end it feels like an extra
optimization.

https://codereview.chromium.org/1369603003/diff/80001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/1369603003/diff/80001/content/public/browser/...
content/public/browser/render_process_host.h:120: // RenderProcessExited may
never be called.
On 2015/09/29 18:28:40, ncarter wrote:
> Is there a DCHECK that catches this case? (If not, this might be another
reason
> to consider adding an explicit render_process_ready_ bool to
> RenderProcessHostImpl).

There's not, and I considered it, but I did not add one. Calling Shutdown is
still valid even if the channel is not connected, the only thing is that we rely
on the channel error to report RenderProcessExited, and on Windows we only see a
channel error if the channel was connected. But Shutdown will still kill the
process (assuming it was started), and there is no difference if there is no
observer.

Note that I'm not changing behavior here (just documenting existing logic).

https://codereview.chromium.org/1369603003/diff/80001/content/public/browser/...
content/public/browser/render_process_host.h:143: // launched and the channel is
connected. At that point, GetHandle() is valid,
On 2015/09/29 18:28:40, ncarter wrote:
> I'm on the fence here, as to whether it's better to keep things simple and say
> "Process is launched", or be more explicit, like, "the process launcher
reported
> a successful launch" or "the process launcher reported back a valid process
> handle". With the simple wording it's not clear that the two conditions can
> happen in either order. I'll leave it up to you.

Ok, I clarified a bit in the latest PS.

https://codereview.chromium.org/1369603003/diff/80001/content/public/test/moc...
File content/public/test/mock_render_process_host.cc (right):

https://codereview.chromium.org/1369603003/diff/80001/content/public/test/moc...
content/public/test/mock_render_process_host.cc:175: bool
MockRenderProcessHost::IsReady() const {
On 2015/09/29 18:28:40, ncarter wrote:
> I suppose we can add mock support for this if/when it's needed.

Right. At the moment, GetChannel() will always return NULL, so IsReady would
always return false.

[piman, 2015-09-30 00:33:18]

Oh, hold on one sec, GetPeerPID() isn't safe on the channel unless it is
known to be connected. We need to keep state around.

On Tue, Sep 29, 2015 at 4:54 PM, <piman@chromium.org> wrote:

> PS3 has updates, PS4 is just a rebase.
>
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:1486: bool
> RenderProcessHostImpl::IsReady() const {
> On 2015/09/29 18:28:40, ncarter wrote:
>
>> It might be worthwhile to add a comment here to point out that the
>> channel-connection (which initializes GetPeerPID) and the
>> result-of-process-launch (GetHandle()) can happen in either ordering.
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:2423:
> RenderProcessReady(this));
> On 2015/09/29 18:28:40, ncarter wrote:
>
>> The intent here, right, is to call RenderProcessReady from exactly one
>>
> of
>
>> OnProcessLaunched or OnChannelConnected, whichever occurs last?
>>
>
> Right, the contract is, RenderProcessReady is only sent once both of:
> - GetHandle() returns a valid handle
> - the channel is connected (channel_->GetPeerPid() is valid)
> are true.
> (this corresponds to IsReady() is true).
>
> GetHandle() is valid once OnProcessLaunched() is called (as a posted
> task from the process launcher thread)
> The channel is known to be connected once OnChannelConnected() is called
> (as a posted task from the IO thread, after the newly running process
> sends the hello message).
>
> The 2 posted tasks race with each other, but since they are separate
> tasks, only one of the callback will observe the IsReady() transition
> from false to true.
>
>
> We were actually just working through the ChildProcessHost variant of
>>
> this
>
>> pattern, http://crbug.com/484937. In my experiments on that bug, it
>>
> seemed like
>
>> OnChannelConnected usually happened first -- do you know if we have
>>
> adequate
>
>> test coverage of the other ordering?
>>
>
> Actually, here, I found that it was usually the opposite, GetHandle()
> being valid but the channel was not yet connected (I had a patch
> originally that only waited for the process to be launched, but one of
> the tests broke on Windows, namely
>
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...
> , see ExtensionManagementApiEscalationTest /
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> as to why).
>
> In a way it makes sense, because the LAUNCHER->UI hop is generally
> shorter than the LAUNCHER->new process main->IO->UI multiple hop.
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:2424: }
> On 2015/09/29 18:28:40, ncarter wrote:
>
>> I like how this operates, but I'm wondering if we should be worried
>>
> about it
>
>> evolving so that RenderProcessReady comes to be called twice?
>>
>
> We could protect against that by adding an explicit, redundant boolean
>>
> member
>
>> (process_ready_) to protect against double notification, or else to
>>
> make sure
>
>> that there's a DCHECK in one of the observers_ (maybe a test-only,
>> ContentBrowserSanityChecker style DCHECK?).
>>
>
> I added a DCHECK. I'm fairly confident that we call only once (see above
> why).
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:2425:
> On 2015/09/29 18:28:40, ncarter wrote:
>
>> FYI: an alternative approach discussed in bug 484937, is to just
>>
> initialize
>
>> Handle() from |peer_pid| at OnChannelConnected time, and do the
>>
> notification
>
>> exclusively from OnChannelConnected. This has the advantage of
>>
> removing the
>
>> latency impact of waiting for the process-launch result, but it puts a
>>
> little
>
>> more trust in |peer_pid| (though I'd hope that's okay since there's
>>
> the
>
>> authentication secret).
>>
>
> I believe GetHandle and channel_->GetPeerPid return different things
> (they're a different type on Windows). As much as it looks like we can
> extract the process id from the process handle, I don't know that the
> opposite is true (not a Windows expert here....)
> I also think there may be places where we need the process handle before
> the channel is connected. E.g. the task manager uses the process handle
> to do things.
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
> content/browser/renderer_host/render_view_host_impl.cc:1434:
> BrowserThread::PostTask(
> On 2015/09/29 18:28:40, ncarter wrote:
>
>> How confident are you that this posted task won't be in a race with
>>
> later IPCs
>
>> from the renderer?
>>
>
> The worry I have is that there might be some code somewhere that isn't
>>
> expecting
>
>> any IPCs prior to
>>
>
>
>
RenderViewReady/WCO::RenderViewReady()/NOTIFICATION_EXTENSION_HOST_CREATED/NOTIFICATION_WEB_CONTENTS_CONNECTED
>
>> etc.
>>
>
> I suppose we could test this by forcing the race outcome: e.g. adding
>>
> a
>
>> sleep(1000) before the PostTask here. If browser_tests and
>>
> content_browsertests
>
>> pass with that sleep() added, then I'm probably okay with calling this
>>
> safe.
>
> So, it is already the case today that IPCs can be sent before
> ViewHostMsg_RenderViewReady (e.g. before this patch, since we initialize
> the compositor before sending ViewHostMsg_RenderViewReady, we need to do
> some IPCs to the browser, e.g. to initialize the context).
>
> Regardless, this patch only makes it better in that respect - instead of
> UI->IO->Renderer IO->Renderer Main->Renderer
> IO->IO->UI->RenderViewReady, we do UI->UI->RenderViewReady, i.e.
> RenderViewReady should happen no later in terms of task ordering on the
> UI thread (This posts the task from the same function that sends the
> message that would have triggered the ViewHostMsg_RenderViewReady on the
> renderer side).
> Now, of course, it is possible that this patch exposes existing races. I
> sent https://codereview.chromium.org/1380573003/ to the trybots, which
> is the same patch but changing this into a PostDelayedTask(..., 1000ms).
> It looks good so far (modulo bots unable to check out the code), but
> will wait for completion before landing.
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
> content/browser/renderer_host/render_view_host_impl.cc:1438:
> weak_factory_.GetWeakPtr()));
> On 2015/09/29 18:28:40, ncarter wrote:
>
>> Also, what breaks if you just call RenderViewReady directly?
>>
>
> There's at least a couple of places that I found, that basically don't
> expect RenderViewReady to be called reentrantly.
>
> In BrowserPluginGuest::OnWillAttachComplete, it will call RVHI::Init()
> (which signals RenderViewReady), but in the callback expects to find the
> view on the WebContentsImpl, whereas it's only set up later.
>
> In the task manager, the TabContentsInformation expects in GetOwnership
> (which is called indirectly from RenderViewReady) to find the
> WebContents in the list of tabs in the browser. This ends up depending
> on the implementation of chrome::Navigate, which does both triggering
> the navigation (indirectly calling RenderViewHostImpl::CreateRenderView)
> and attaching the WebContents into the list of tabs, but in various
> orders depending on the case.
>
>
> It may be possible to fix those, but both of these are security
> sensitive and way out of my area of expertise, so I prefer to keep this
> patch scoped, and not introduce unexpected reentrancy, since in the end
> it feels like an extra optimization.
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/public/browser/...
> File content/public/browser/render_process_host.h (right):
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/public/browser/...
> content/public/browser/render_process_host.h:120: // RenderProcessExited
> may never be called.
> On 2015/09/29 18:28:40, ncarter wrote:
>
>> Is there a DCHECK that catches this case? (If not, this might be
>>
> another reason
>
>> to consider adding an explicit render_process_ready_ bool to
>> RenderProcessHostImpl).
>>
>
> There's not, and I considered it, but I did not add one. Calling
> Shutdown is still valid even if the channel is not connected, the only
> thing is that we rely on the channel error to report
> RenderProcessExited, and on Windows we only see a channel error if the
> channel was connected. But Shutdown will still kill the process
> (assuming it was started), and there is no difference if there is no
> observer.
>
> Note that I'm not changing behavior here (just documenting existing
> logic).
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/public/browser/...
> content/public/browser/render_process_host.h:143: // launched and the
> channel is connected. At that point, GetHandle() is valid,
> On 2015/09/29 18:28:40, ncarter wrote:
>
>> I'm on the fence here, as to whether it's better to keep things simple
>>
> and say
>
>> "Process is launched", or be more explicit, like, "the process
>>
> launcher reported
>
>> a successful launch" or "the process launcher reported back a valid
>>
> process
>
>> handle". With the simple wording it's not clear that the two
>>
> conditions can
>
>> happen in either order. I'll leave it up to you.
>>
>
> Ok, I clarified a bit in the latest PS.
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/public/test/moc...
> File content/public/test/mock_render_process_host.cc (right):
>
>
>
https://codereview.chromium.org/1369603003/diff/80001/content/public/test/moc...
> content/public/test/mock_render_process_host.cc:175: bool
> MockRenderProcessHost::IsReady() const {
> On 2015/09/29 18:28:40, ncarter wrote:
>
>> I suppose we can add mock support for this if/when it's needed.
>>
>
> Right. At the moment, GetChannel() will always return NULL, so IsReady
> would always return false.
>
> https://codereview.chromium.org/1369603003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-09-30 00:40:36]

On 2015/09/30 00:33:18, piman (slow to review) wrote:
> Oh, hold on one sec, GetPeerPID() isn't safe on the channel unless it is
> known to be connected. We need to keep state around.

PS5 should have the fix.

[piman, 2015-09-30 04:35:22]

On 2015/09/30 00:40:36, piman (slow to review) wrote:
> On 2015/09/30 00:33:18, piman (slow to review) wrote:
> > Oh, hold on one sec, GetPeerPID() isn't safe on the channel unless it is
> > known to be connected. We need to keep state around.
> 
> PS5 should have the fix.

The try run with a delay in the PostTask (
https://codereview.chromium.org/1380573003/ ) exposed a race in a test. It's
fixed in PS6.

[ncarter (slow), 2015-09-30 20:30:33]

lgtm

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2423:
RenderProcessReady(this));
On 2015/09/29 23:54:27, piman (slow to review) wrote:
> On 2015/09/29 18:28:40, ncarter wrote:
> > The intent here, right, is to call RenderProcessReady from exactly one of
> > OnProcessLaunched or OnChannelConnected, whichever occurs last?
> 
> Right, the contract is, RenderProcessReady is only sent once both of:
> - GetHandle() returns a valid handle
> - the channel is connected (channel_->GetPeerPid() is valid)
> are true.
> (this corresponds to IsReady() is true).
> 
> GetHandle() is valid once OnProcessLaunched() is called (as a posted task from
> the process launcher thread)
> The channel is known to be connected once OnChannelConnected() is called (as a
> posted task from the IO thread, after the newly running process sends the
hello
> message).
> 
> The 2 posted tasks race with each other, but since they are separate tasks,
only
> one of the callback will observe the IsReady() transition from false to true.
> 
> 
> > We were actually just working through the ChildProcessHost variant of this
> > pattern, http://crbug.com/484937. In my experiments on that bug, it seemed
> like
> > OnChannelConnected usually happened first -- do you know if we have adequate
> > test coverage of the other ordering?
> 
> Actually, here, I found that it was usually the opposite, GetHandle() being
> valid but the channel was not yet connected (I had a patch originally that
only
> waited for the process to be launched, but one of the tests broke on Windows,
> namely
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...
> , see ExtensionManagementApiEscalationTest /
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> as to why).
> 
> In a way it makes sense, because the LAUNCHER->UI hop is generally shorter
than
> the LAUNCHER->new process main->IO->UI multiple hop.

I think I had it somewhat backwards, but the behavior probably varies quite a
bit depending on the OS and number of cores. In my own testing, on Windows/z620,
I actually was having to sandbag the child process launcher to force the
channel-connection to win -- so that matches what you've seen. But in the
originally-reported bug (which was reported against nacl launching, on cros),
the channel-connection was winning on average. It may be that there are extra
process hops involved in nacl processes that delay the result of process
creation? Something something zygote process?

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2424: }
On 2015/09/29 23:54:27, piman (slow to review) wrote:
> On 2015/09/29 18:28:40, ncarter wrote:
> > I like how this operates, but I'm wondering if we should be worried about it
> > evolving so that RenderProcessReady comes to be called twice?
> > 
> > We could protect against that by adding an explicit, redundant boolean
member
> > (process_ready_) to protect against double notification, or else to make
sure
> > that there's a DCHECK in one of the observers_ (maybe a test-only,
> > ContentBrowserSanityChecker style DCHECK?).
> 
> I added a DCHECK. I'm fairly confident that we call only once (see above why).

Great. I'm confident that it's correct now, but I worry that it could be a
little fragile as the code evolves.

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2425: 
On 2015/09/29 23:54:27, piman (slow to review) wrote:
> On 2015/09/29 18:28:40, ncarter wrote:
> > FYI: an alternative approach discussed in bug 484937, is to just initialize
> > Handle() from |peer_pid| at OnChannelConnected time, and do the notification
> > exclusively from OnChannelConnected. This has the advantage of removing the
> > latency impact of waiting for the process-launch result, but it puts a
little
> > more trust in |peer_pid| (though I'd hope that's okay since there's the
> > authentication secret).
> 
> I believe GetHandle and channel_->GetPeerPid return different things (they're
a
> different type on Windows). As much as it looks like we can extract the
process
> id from the process handle, I don't know that the opposite is true (not a
> Windows expert here....)
> I also think there may be places where we need the process handle before the
> channel is connected. E.g. the task manager uses the process handle to do
> things.

You definitely can go pid->handle on Windows, but it's a syscall to do so, and
it'll fail if the process has died in the meantime. Usually those failure cases
are a headache to deal with and it's a better practice to keep things as handles
all the way through. However, this here is the earliest possible moment.

In the ChildProcessHost case, we actually already open the handle from the pid,
just before calling OnChannelConnected:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/chi....
It was because we already had a handle right there, in the calling stack frame,
that I started wondering if we should just use that.

Definitely waiting for both results (as your patch does) feels like the
cleanest, safest approach. There are latency savings to be had only if it's
common for the channel to connect before we hear back from the launcher.

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1434:
BrowserThread::PostTask(
On 2015/09/29 23:54:27, piman (slow to review) wrote:
> On 2015/09/29 18:28:40, ncarter wrote:
> > How confident are you that this posted task won't be in a race with later
IPCs
> > from the renderer?
> > 
> > The worry I have is that there might be some code somewhere that isn't
> expecting
> > any IPCs prior to
> >
>
RenderViewReady/WCO::RenderViewReady()/NOTIFICATION_EXTENSION_HOST_CREATED/NOTIFICATION_WEB_CONTENTS_CONNECTED
> > etc.
> > 
> > I suppose we could test this by forcing the race outcome: e.g. adding a
> > sleep(1000) before the PostTask here. If browser_tests and
> content_browsertests
> > pass with that sleep() added, then I'm probably okay with calling this safe.
> 
> So, it is already the case today that IPCs can be sent before
> ViewHostMsg_RenderViewReady (e.g. before this patch, since we initialize the
> compositor before sending ViewHostMsg_RenderViewReady, we need to do some IPCs
> to the browser, e.g. to initialize the context).
> 
> Regardless, this patch only makes it better in that respect - instead of
> UI->IO->Renderer IO->Renderer Main->Renderer IO->IO->UI->RenderViewReady, we
do
> UI->UI->RenderViewReady, i.e. RenderViewReady should happen no later in terms
of
> task ordering on the UI thread (This posts the task from the same function
that
> sends the message that would have triggered the ViewHostMsg_RenderViewReady on
> the renderer side).
> Now, of course, it is possible that this patch exposes existing races. I sent
> https://codereview.chromium.org/1380573003/ to the trybots, which is the same
> patch but changing this into a PostDelayedTask(..., 1000ms). It looks good so
> far (modulo bots unable to check out the code), but will wait for completion
> before landing.

Ah, okay. I had assumed that RenderViewReady was happening at some deterministic
sequence point in the IPC stream from a new renderer process, and I was worried
that we'd be introducing nondeterminism. If it was already nondeterministic,
then I'm much less worried about regression.

https://codereview.chromium.org/1369603003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1438:
weak_factory_.GetWeakPtr()));
On 2015/09/29 23:54:27, piman (slow to review) wrote:
> On 2015/09/29 18:28:40, ncarter wrote:
> > Also, what breaks if you just call RenderViewReady directly?
> 
> There's at least a couple of places that I found, that basically don't expect
> RenderViewReady to be called reentrantly.
> 
> In BrowserPluginGuest::OnWillAttachComplete, it will call RVHI::Init() (which
> signals RenderViewReady), but in the callback expects to find the view on the
> WebContentsImpl, whereas it's only set up later.
> 
> In the task manager, the TabContentsInformation expects in GetOwnership (which
> is called indirectly from RenderViewReady) to find the WebContents in the list
> of tabs in the browser. This ends up depending on the implementation of
> chrome::Navigate, which does both triggering the navigation (indirectly
calling
> RenderViewHostImpl::CreateRenderView) and attaching the WebContents into the
> list of tabs, but in various orders depending on the case.
> 
> 
> It may be possible to fix those, but both of these are security sensitive and
> way out of my area of expertise, so I prefer to keep this patch scoped, and
not
> introduce unexpected reentrancy, since in the end it feels like an extra
> optimization.

This makes sense.

[piman, 2015-09-30 20:42:11]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2015-09-30 21:02:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1369603003/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1369603003/170001

[commit-bot: I haz the power, 2015-09-30 21:22:08]

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

[commit-bot: I haz the power, 2015-09-30 21:22:08]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[piman, 2015-09-30 22:18:39]

piman@chromium.org changed reviewers:
+ fsamuel@chromium.org

[piman, 2015-09-30 22:18:40]

dcheng: ping for swapped_out_messages.cc and view_messages.g
fsamuel: please take a look at
chrome/browser/apps/guest_view/web_view_browsertest.cc  (the test makes invalid
assumptions about order)

[Fady Samuel, 2015-09-30 22:21:04]

https://chromiumcodereview.appspot.com/1369603003/diff/170001/chrome/browser/...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/1369603003/diff/170001/chrome/browser/...
chrome/browser/apps/guest_view/web_view_browsertest.cc:2944: bool found = false;
Could you please move this to a helper method?
HasGuestTask(task_manager.tasks()) say..

[piman, 2015-09-30 22:36:17]

https://chromiumcodereview.appspot.com/1369603003/diff/170001/chrome/browser/...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/1369603003/diff/170001/chrome/browser/...
chrome/browser/apps/guest_view/web_view_browsertest.cc:2944: bool found = false;
On 2015/09/30 22:21:04, Fady Samuel wrote:
> Could you please move this to a helper method?
> HasGuestTask(task_manager.tasks()) say..

Done.

[Fady Samuel, 2015-09-30 22:39:06]

guest_view lgtm

[piman, 2015-10-02 02:28:51]

dcheng: ping. This just removes a couple of IPCs, should be non-controversial.

[piman, 2015-10-02 02:29:03]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-02 02:29:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1369603003/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1369603003/190001

[dcheng, 2015-10-02 02:54:03]

lgtm

[piman, 2015-10-02 03:25:23]

The CQ bit was unchecked by piman@chromium.org

[piman, 2015-10-02 03:25:24]

The CQ bit was checked by piman@chromium.org

[piman, 2015-10-02 03:25:24]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1369603003/#ps190001
(title: "review comments")

[commit-bot: I haz the power, 2015-10-02 03:25:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1369603003/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1369603003/190001

[commit-bot: I haz the power, 2015-10-02 03:46:07]

Committed patchset #7 (id:190001)

[commit-bot: I haz the power, 2015-10-02 03:47:07]

Patchset 7 (id:??) landed as
https://crrev.com/c4af307b97653099dda5e5fab24880065dbe0239
Cr-Commit-Position: refs/heads/master@{#351951}