Prevent the browser process from creating duplicate RenderViewHosts.

Prevent the browser process from creating duplicate RenderViewHosts.

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

[nasko, 2013-11-28 01:23:36]

Hey Charlie,
I finally nailed a test for the duplicate RVH. The fix should be ready for
review.
Thanks,
Nasko

[Charlie Reis, 2013-12-02 19:22:49]

Great!  Nice strategy with the test, to use a RVH factory.  A few questions
below, plus some nits.

https://codereview.chromium.org/92873004/diff/40001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/92873004/diff/40001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:385: // should be
created associated with the given route, but it should not be
nit: given |route_id| in process |render_process_id|

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:68: explicit
TrackingRenderViewHostFactory() : has_duplicates_(false) {
nit: explicit is only needed for one-arg constructors.

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:104: typedef
base::hash_map<std::pair<int,int>, RenderViewHostImpl*> DuplicatesMap;
Is this really a map of duplicates?  Looks like we might be replacing the
previous RVH if we see a duplicate, since it's a hash_map.  If we don't need to
retrieve the values at all, maybe it should just be a hash_set of ID pairs,
called something like IDMap?

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:105: DuplicatesMap rvh_map;
style nit: trailing underscore

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:114: // It creates a couple of
windows in process (1), which causes a few routing ids
nit: No parens around 1 or 2.  That makes it feel like a bulleted list to me.

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:138:
shell2->web_contents()->GetRenderViewHost()->GetRoutingID();
Should we have a EXPECT_NE for the routing IDs, since it's mentioned in the
comment?

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:140: // Now, simulate a click to
chrome-extension:// URL coming from the renderer.
chrome-extension:// shouldn't do anything special in content_browsertests, since
there's no ChromeContentBrowserClient.  What's the reason it's needed?

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:168: new
SessionStorageNamespaceImpl(dom_storage_context);
Huh, unfortunate that we have to add this SessionStorageNamespace creation here,
since it could be fragile if the original code changes.  If it's copied from
somewhere, perhaps we could make it a private helper method exposed to the test
instead?

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:176:
EXPECT_FALSE(rvh_factory.has_duplicates());
Do we expect the process to be killed?

https://codereview.chromium.org/92873004/diff/40001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/92873004/diff/40001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1273: // Creating a new window
can only come from the active process for this
nit: A message to create a new window can only

https://codereview.chromium.org/92873004/diff/40001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1274: // WebContentsImpl
instance. If any other process originates the request,
nit: s/originates/sends/

https://codereview.chromium.org/92873004/diff/40001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1275: // it is invalid and
must be terminated.
nit: and the process must be

(it == the request)

https://codereview.chromium.org/92873004/diff/40001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1277:
site_instance->GetProcess()->GetID() != render_process_id) {
This doesn't look right to me, since |site_instance| may be newly created,
rather than the being the "active process for this WebContentsImpl instance."

If we really mean to check it against the current process, we should use
GetRenderProcessHost() here.  Is that the right thing to do, or does that cause
problems in the opener_suppressed (e.g., rel=noreferrer) case?

[nasko, 2013-12-02 20:19:12]

Addressed all comments.

https://codereview.chromium.org/92873004/diff/40001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/92873004/diff/40001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:385: // should be
created associated with the given route, but it should not be
On 2013/12/02 19:22:50, creis wrote:
> nit: given |route_id| in process |render_process_id|

Done.

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:68: explicit
TrackingRenderViewHostFactory() : has_duplicates_(false) {
On 2013/12/02 19:22:50, creis wrote:
> nit: explicit is only needed for one-arg constructors.

Done.

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:104: typedef
base::hash_map<std::pair<int,int>, RenderViewHostImpl*> DuplicatesMap;
On 2013/12/02 19:22:50, creis wrote:
> Is this really a map of duplicates?  Looks like we might be replacing the
> previous RVH if we see a duplicate, since it's a hash_map.  If we don't need
to
> retrieve the values at all, maybe it should just be a hash_set of ID pairs,
> called something like IDMap?

Yes, using a hash_set definitely fits here. Fixed.

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:105: DuplicatesMap rvh_map;
On 2013/12/02 19:22:50, creis wrote:
> style nit: trailing underscore

Done.

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:114: // It creates a couple of
windows in process (1), which causes a few routing ids
On 2013/12/02 19:22:50, creis wrote:
> nit: No parens around 1 or 2.  That makes it feel like a bulleted list to me.

Done.

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:138:
shell2->web_contents()->GetRenderViewHost()->GetRoutingID();
On 2013/12/02 19:22:50, creis wrote:
> Should we have a EXPECT_NE for the routing IDs, since it's mentioned in the
> comment?

Done.

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:140: // Now, simulate a click to
chrome-extension:// URL coming from the renderer.
On 2013/12/02 19:22:50, creis wrote:
> chrome-extension:// shouldn't do anything special in content_browsertests,
since
> there's no ChromeContentBrowserClient.  What's the reason it's needed?

No real reason other than the PoC used it and it stuck. I've changed it to a
different cross-site URL.

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:168: new
SessionStorageNamespaceImpl(dom_storage_context);
On 2013/12/02 19:22:50, creis wrote:
> Huh, unfortunate that we have to add this SessionStorageNamespace creation
here,
> since it could be fragile if the original code changes.  If it's copied from
> somewhere, perhaps we could make it a private helper method exposed to the
test
> instead?

This pattern is used very rarely. I'd rather not complicate this CL with
extracting out a pattern into a helper method.

https://codereview.chromium.org/92873004/diff/40001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:176:
EXPECT_FALSE(rvh_factory.has_duplicates());
On 2013/12/02 19:22:50, creis wrote:
> Do we expect the process to be killed?

In the lifetime of the test, the process doesn't have a chance to be created.
This is the main reason I'm relying on the RVH factory than an actual process
kill.

https://codereview.chromium.org/92873004/diff/40001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/92873004/diff/40001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1273: // Creating a new window
can only come from the active process for this
On 2013/12/02 19:22:50, creis wrote:
> nit: A message to create a new window can only

Done.

https://codereview.chromium.org/92873004/diff/40001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1274: // WebContentsImpl
instance. If any other process originates the request,
On 2013/12/02 19:22:50, creis wrote:
> nit: s/originates/sends/

Done.

https://codereview.chromium.org/92873004/diff/40001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1275: // it is invalid and
must be terminated.
On 2013/12/02 19:22:50, creis wrote:
> nit: and the process must be
> 
> (it == the request)

Done.

https://codereview.chromium.org/92873004/diff/40001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1277:
site_instance->GetProcess()->GetID() != render_process_id) {
On 2013/12/02 19:22:50, creis wrote:
> This doesn't look right to me, since |site_instance| may be newly created,
> rather than the being the "active process for this WebContentsImpl instance."
> 
> If we really mean to check it against the current process, we should use
> GetRenderProcessHost() here.  Is that the right thing to do, or does that
cause
> problems in the opener_suppressed (e.g., rel=noreferrer) case?

I don't think using GetRenderProcessHost() should impact the opener_suppressed
case, since only the current RVH should be sending a CreateNewWindow message.

[Charlie Reis, 2013-12-02 22:25:34]

Great, just a few more nits.

https://codereview.chromium.org/92873004/diff/80001/content/browser/security_...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/92873004/diff/80001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:104: typedef
base::hash_set<std::pair<int,int> > DuplicatesSet;
nit: DuplicatesSet -> IDSet

(It doesn't actually contain duplicates.)

https://codereview.chromium.org/92873004/diff/80001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:105: DuplicatesSet rvh_map_;
nit: rvh_map_ isn't the right name anymore.  rvh_id_set_ might work.

https://codereview.chromium.org/92873004/diff/80001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/92873004/diff/80001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1258: int route_id,
I had trouble following how route_id could be used in the opener_suppressed
case, but that's set to MSG_ROUTING_NONE by RenderWidgetHelper.  Let's add a
DCHECK to help make that explicit and discoverable.

https://codereview.chromium.org/92873004/diff/80001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1276: if
(GetRenderProcessHost() &&
nit: No need to check for null here.  This should always be valid, and we would
have crashed on line 1266 if not anyway.

[nasko, 2013-12-02 22:51:07]

Fixed nits.

https://codereview.chromium.org/92873004/diff/80001/content/browser/security_...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/92873004/diff/80001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:104: typedef
base::hash_set<std::pair<int,int> > DuplicatesSet;
On 2013/12/02 22:25:34, creis wrote:
> nit: DuplicatesSet -> IDSet
> 
> (It doesn't actually contain duplicates.)

Done.

https://codereview.chromium.org/92873004/diff/80001/content/browser/security_...
content/browser/security_exploit_browsertest.cc:105: DuplicatesSet rvh_map_;
On 2013/12/02 22:25:34, creis wrote:
> nit: rvh_map_ isn't the right name anymore.  rvh_id_set_ might work.

Done.

https://codereview.chromium.org/92873004/diff/80001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/92873004/diff/80001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1258: int route_id,
On 2013/12/02 22:25:34, creis wrote:
> I had trouble following how route_id could be used in the opener_suppressed
> case, but that's set to MSG_ROUTING_NONE by RenderWidgetHelper.  Let's add a
> DCHECK to help make that explicit and discoverable.

Done.

https://codereview.chromium.org/92873004/diff/80001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1276: if
(GetRenderProcessHost() &&
On 2013/12/02 22:25:34, creis wrote:
> nit: No need to check for null here.  This should always be valid, and we
would
> have crashed on line 1266 if not anyway.

Done.

[Charlie Reis, 2013-12-02 23:17:51]

Thanks!  LGTM.

[commit-bot: I haz the power, 2013-12-03 01:54:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/92873004/160001

[commit-bot: I haz the power, 2013-12-03 02:16:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/92873004/160001

[commit-bot: I haz the power, 2013-12-03 05:50:27]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[nasko, 2013-12-03 21:54:42]

Charlie,
I've changed the test and added the CHECK we've discussed. PTAL.
Thanks,
Nasko

[Charlie Reis, 2013-12-03 22:15:48]

Much nicer.  Just a bit of cleanup and it should be good to go.

https://codereview.chromium.org/92873004/diff/180001/content/browser/security...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/92873004/diff/180001/content/browser/security...
content/browser/security_exploit_browsertest.cc:65: // This is a helper function
for the tests which attempt to create a
I think helper functions for tests usually go at the top of the file, above the
class definition (e.g., browser_browsertest.cc).

https://codereview.chromium.org/92873004/diff/180001/content/browser/security...
content/browser/security_exploit_browsertest.cc:70: // new process 2 to be
created and have a pending RenderViewHost for it.
Please mention something about how the pending RenderViewHost is returned and
what the out parameter is.

Actually, I think the names here are a bit confusing, since this helper function
doesn't appear to create any duplicates.  Maybe PrepareToDuplicateHosts(shell,
target_routing_id) would be clearer?

https://codereview.chromium.org/92873004/diff/180001/content/browser/security...
content/browser/security_exploit_browsertest.cc:123:
SetupForDuplicateHosts(shell(), &duplicate_routing_id);
Let's check that this is no longer MSG_ROUTING_NONE.

https://codereview.chromium.org/92873004/diff/180001/content/browser/security...
content/browser/security_exploit_browsertest.cc:138: 
nit: No blank line needed.

https://codereview.chromium.org/92873004/diff/180001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/92873004/diff/180001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1272:
(params.opener_suppressed && route_id == MSG_ROUTING_NONE));
nit: No need for the second params.opener_suppressed.  We know it's true if the
first clause failed.

[nasko, 2013-12-03 22:25:38]

Cleanup performed!

https://codereview.chromium.org/92873004/diff/180001/content/browser/security...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/92873004/diff/180001/content/browser/security...
content/browser/security_exploit_browsertest.cc:65: // This is a helper function
for the tests which attempt to create a
On 2013/12/03 22:15:49, creis wrote:
> I think helper functions for tests usually go at the top of the file, above
the
> class definition (e.g., browser_browsertest.cc).

Done.

https://codereview.chromium.org/92873004/diff/180001/content/browser/security...
content/browser/security_exploit_browsertest.cc:70: // new process 2 to be
created and have a pending RenderViewHost for it.
On 2013/12/03 22:15:49, creis wrote:
> Please mention something about how the pending RenderViewHost is returned and
> what the out parameter is.
> 
> Actually, I think the names here are a bit confusing, since this helper
function
> doesn't appear to create any duplicates.  Maybe PrepareToDuplicateHosts(shell,
> target_routing_id) would be clearer?

Done.

https://codereview.chromium.org/92873004/diff/180001/content/browser/security...
content/browser/security_exploit_browsertest.cc:123:
SetupForDuplicateHosts(shell(), &duplicate_routing_id);
On 2013/12/03 22:15:49, creis wrote:
> Let's check that this is no longer MSG_ROUTING_NONE.

Done.

https://codereview.chromium.org/92873004/diff/180001/content/browser/security...
content/browser/security_exploit_browsertest.cc:138: 
On 2013/12/03 22:15:49, creis wrote:
> nit: No blank line needed.

Done.

https://codereview.chromium.org/92873004/diff/180001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/92873004/diff/180001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1272:
(params.opener_suppressed && route_id == MSG_ROUTING_NONE));
On 2013/12/03 22:15:49, creis wrote:
> nit: No need for the second params.opener_suppressed.  We know it's true if
the
> first clause failed.

I like the explicitness of it, but not strongly. Fixed.

[Charlie Reis, 2013-12-03 22:29:04]

LGTM

[commit-bot: I haz the power, 2013-12-04 00:28:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/92873004/200001

[commit-bot: I haz the power, 2013-12-04 04:58:27]

Change committed as 238575