Fix WebContentsDelegate::ShouldCreateWebContents implementations.

Fix WebContentsDelegate::ShouldCreateWebContents implementations.

The problem was that |route_id| is scoped to a particular child process,
but the process was not being plumbed in correctly, resulting in a
situation where we would create (1) a RenderFrameHost with a routing ID
that might later be reused, and also, (2) a RenderFrameHost with a
plumbed-in routing ID, but no corresponding extant RenderFrame in the
child process.

The solution involves plumbing in the actual SiteInstance, and only
using |route_id| if we don't change to a different SiteInstance.

BUG=627852
TBR=brettw@chromium.org

Committed: https://crrev.com/f5e618c7175237cd27bfb6e94fd4fa066367e36a
Cr-Commit-Position: refs/heads/master@{#434743}

[ncarter (slow), 2016-11-21 23:23:41]

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

[commit-bot: I haz the power, 2016-11-21 23:24:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ncarter (slow), 2016-11-21 23:30:53]

Description was changed from

==========
Fix WebContentsDelegate::ShouldCreateWebContents implementations.

The problem was that |route_id| is scoped to a particular child process,
but the process was not being plumbed in correctly, resulting in a
situation where we would create (1) a RenderFrameHost with a routing ID
that might later be reused, and also, (2) a RenderFrameHost with a
plumbed-in routing ID, but no corresponding extant RenderFrame in the
child process.

This is a speculative fix for bug 658603.

BUG=658603
==========

to

==========
Fix WebContentsDelegate::ShouldCreateWebContents implementations.

The problem was that |route_id| is scoped to a particular child process,
but the process was not being plumbed in correctly, resulting in a
situation where we would create (1) a RenderFrameHost with a routing ID
that might later be reused, and also, (2) a RenderFrameHost with a
plumbed-in routing ID, but no corresponding extant RenderFrame in the
child process.

The solution involves plumbing in the actual SiteInstance, and only
using |route_id| if we don't change to a different SiteInstance.

This is a speculative fix for bug 658603.

BUG=658603
==========

[ncarter (slow), 2016-11-21 23:31:35]

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

[commit-bot: I haz the power, 2016-11-21 23:32:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ncarter (slow), 2016-11-21 23:33:03]

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

[commit-bot: I haz the power, 2016-11-22 00:03:17]

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

[commit-bot: I haz the power, 2016-11-22 00:03:18]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ncarter (slow), 2016-11-22 00:23:03]

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

[commit-bot: I haz the power, 2016-11-22 00:23:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 00:50:30]

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

[commit-bot: I haz the power, 2016-11-22 00:50:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ncarter (slow), 2016-11-22 21:32:38]

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

[commit-bot: I haz the power, 2016-11-22 21:33:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 22:40:29]

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

[commit-bot: I haz the power, 2016-11-22 22:40:30]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-11-22 23:34:34]

Charlie, please take a look.

[Charlie Reis, 2016-11-23 06:59:47]

This makes a lot of sense-- thanks for finding these cases.  LGTM.

(For the speculative Android fix, is the guess that
Browser::ShouldCreateWebContents was the culprit?  Does that run on Android?  I
noticed that there were no functional changes to Android's
DocumentWebContentsDelegate or WebContentsDelegateAndroid.)

https://codereview.chromium.org/2521793003/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (left):

https://codereview.chromium.org/2521793003/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:2530: GURL opener_url =
opener_web_contents->GetURL();
Yuck!  Thanks for fixing this.

https://codereview.chromium.org/2521793003/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:2557: SiteInstance* opener_site_instance =
opener_web_contents->GetSiteInstance();
And this!

https://codereview.chromium.org/2521793003/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/2521793003/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:862: content::SiteInstance* site_instance,
nit: Maybe this should be source_site_instance like the other changes?

https://codereview.chromium.org/2521793003/diff/60001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/2521793003/diff/60001/content/public/browser/...
content/public/browser/web_contents_delegate.h:296: // The embedder has to
synchronously adopt |route_id| or else the view will
Is this comment wrong as well?  We should probably give some guidelines of when
to use the various routing IDs and when not to.

https://codereview.chromium.org/2521793003/diff/60001/content/public/browser/...
content/public/browser/web_contents_delegate.h:300: SiteInstance*
source_site_instance,
Maybe also something in the comment about how web_contents and
source_site_instance are related?  Is |web_contents| the opener?  Is
|source_site_instance| guaranteed to come from a frame inside |web_contents|?

[ncarter (slow), 2016-11-23 23:35:34]

Description was changed from

==========
Fix WebContentsDelegate::ShouldCreateWebContents implementations.

The problem was that |route_id| is scoped to a particular child process,
but the process was not being plumbed in correctly, resulting in a
situation where we would create (1) a RenderFrameHost with a routing ID
that might later be reused, and also, (2) a RenderFrameHost with a
plumbed-in routing ID, but no corresponding extant RenderFrame in the
child process.

The solution involves plumbing in the actual SiteInstance, and only
using |route_id| if we don't change to a different SiteInstance.

This is a speculative fix for bug 658603.

BUG=658603
==========

to

==========
Fix WebContentsDelegate::ShouldCreateWebContents implementations.

The problem was that |route_id| is scoped to a particular child process,
but the process was not being plumbed in correctly, resulting in a
situation where we would create (1) a RenderFrameHost with a routing ID
that might later be reused, and also, (2) a RenderFrameHost with a
plumbed-in routing ID, but no corresponding extant RenderFrame in the
child process.

The solution involves plumbing in the actual SiteInstance, and only
using |route_id| if we don't change to a different SiteInstance.

BUG=627852
==========

[ncarter (slow), 2016-11-23 23:58:27]

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

[commit-bot: I haz the power, 2016-11-23 23:59:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ncarter (slow), 2016-11-24 00:00:05]

ptal

https://codereview.chromium.org/2521793003/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (left):

https://codereview.chromium.org/2521793003/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:2530: GURL opener_url =
opener_web_contents->GetURL();
On 2016/11/23 06:59:48, Charlie Reis (slow) wrote:
> Yuck!  Thanks for fixing this.

Acknowledged.

https://codereview.chromium.org/2521793003/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:2557: SiteInstance* opener_site_instance =
opener_web_contents->GetSiteInstance();
On 2016/11/23 06:59:48, Charlie Reis (slow) wrote:
> And this!

Done.

https://codereview.chromium.org/2521793003/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/2521793003/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:862: content::SiteInstance* site_instance,
On 2016/11/23 06:59:48, Charlie Reis (slow) wrote:
> nit: Maybe this should be source_site_instance like the other changes?

Done.

https://codereview.chromium.org/2521793003/diff/60001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/2521793003/diff/60001/content/public/browser/...
content/public/browser/web_contents_delegate.h:296: // The embedder has to
synchronously adopt |route_id| or else the view will
On 2016/11/23 06:59:48, Charlie Reis (slow) wrote:
> Is this comment wrong as well?  We should probably give some guidelines of
when
> to use the various routing IDs and when not to.

The comment wasn't wrong, just imprecise. In truth, this is an absolutely awful
API, but I've hopefully clarified it with the new comment.

https://codereview.chromium.org/2521793003/diff/60001/content/public/browser/...
content/public/browser/web_contents_delegate.h:300: SiteInstance*
source_site_instance,
On 2016/11/23 06:59:48, Charlie Reis (slow) wrote:
> Maybe also something in the comment about how web_contents and
> source_site_instance are related?  Is |web_contents| the opener?  Is
> |source_site_instance| guaranteed to come from a frame inside |web_contents|?

Done.

[ncarter (slow), 2016-11-24 00:28:40]

Description was changed from

==========
Fix WebContentsDelegate::ShouldCreateWebContents implementations.

The problem was that |route_id| is scoped to a particular child process,
but the process was not being plumbed in correctly, resulting in a
situation where we would create (1) a RenderFrameHost with a routing ID
that might later be reused, and also, (2) a RenderFrameHost with a
plumbed-in routing ID, but no corresponding extant RenderFrame in the
child process.

The solution involves plumbing in the actual SiteInstance, and only
using |route_id| if we don't change to a different SiteInstance.

BUG=627852
==========

to

==========
Fix WebContentsDelegate::ShouldCreateWebContents implementations.

The problem was that |route_id| is scoped to a particular child process,
but the process was not being plumbed in correctly, resulting in a
situation where we would create (1) a RenderFrameHost with a routing ID
that might later be reused, and also, (2) a RenderFrameHost with a
plumbed-in routing ID, but no corresponding extant RenderFrame in the
child process.

The solution involves plumbing in the actual SiteInstance, and only
using |route_id| if we don't change to a different SiteInstance.

BUG=627852
TBR=brettw@chromium.org
==========

[Charlie Reis, 2016-11-24 00:31:04]

LGTM

https://codereview.chromium.org/2521793003/diff/60001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/2521793003/diff/60001/content/public/browser/...
content/public/browser/web_contents_delegate.h:296: // The embedder has to
synchronously adopt |route_id| or else the view will
On 2016/11/24 00:00:05, ncarter (thanksgiving in usa) wrote:
> On 2016/11/23 06:59:48, Charlie Reis (slow) wrote:
> > Is this comment wrong as well?  We should probably give some guidelines of
> when
> > to use the various routing IDs and when not to.
> 
> The comment wasn't wrong, just imprecise. In truth, this is an absolutely
awful
> API, but I've hopefully clarified it with the new comment.

Wow.  Yes, that sounds more accurate, and clearly shows why this API is awful. 
:)  Good candidate for future cleanup.

[commit-bot: I haz the power, 2016-11-24 02:03:51]

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

[commit-bot: I haz the power, 2016-11-24 02:03:52]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[ncarter (slow), 2016-11-28 18:01:57]

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

[ncarter (slow), 2016-11-28 18:02:03]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-11-28 18:03:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 22:38:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480356123498410,
"parent_rev": "7a9acb1e1bd213a015b6f8785e9e3a0a3c46ed68", "commit_rev":
"4354d02787184c30c68bb597f0c8544936d3f9c3"}

[commit-bot: I haz the power, 2016-11-28 22:39:03]

Description was changed from

==========
Fix WebContentsDelegate::ShouldCreateWebContents implementations.

The problem was that |route_id| is scoped to a particular child process,
but the process was not being plumbed in correctly, resulting in a
situation where we would create (1) a RenderFrameHost with a routing ID
that might later be reused, and also, (2) a RenderFrameHost with a
plumbed-in routing ID, but no corresponding extant RenderFrame in the
child process.

The solution involves plumbing in the actual SiteInstance, and only
using |route_id| if we don't change to a different SiteInstance.

BUG=627852
TBR=brettw@chromium.org
==========

to

==========
Fix WebContentsDelegate::ShouldCreateWebContents implementations.

The problem was that |route_id| is scoped to a particular child process,
but the process was not being plumbed in correctly, resulting in a
situation where we would create (1) a RenderFrameHost with a routing ID
that might later be reused, and also, (2) a RenderFrameHost with a
plumbed-in routing ID, but no corresponding extant RenderFrame in the
child process.

The solution involves plumbing in the actual SiteInstance, and only
using |route_id| if we don't change to a different SiteInstance.

BUG=627852
TBR=brettw@chromium.org
==========

[commit-bot: I haz the power, 2016-11-28 22:39:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-28 22:41:15]

Description was changed from

==========
Fix WebContentsDelegate::ShouldCreateWebContents implementations.

The problem was that |route_id| is scoped to a particular child process,
but the process was not being plumbed in correctly, resulting in a
situation where we would create (1) a RenderFrameHost with a routing ID
that might later be reused, and also, (2) a RenderFrameHost with a
plumbed-in routing ID, but no corresponding extant RenderFrame in the
child process.

The solution involves plumbing in the actual SiteInstance, and only
using |route_id| if we don't change to a different SiteInstance.

BUG=627852
TBR=brettw@chromium.org
==========

to

==========
Fix WebContentsDelegate::ShouldCreateWebContents implementations.

The problem was that |route_id| is scoped to a particular child process,
but the process was not being plumbed in correctly, resulting in a
situation where we would create (1) a RenderFrameHost with a routing ID
that might later be reused, and also, (2) a RenderFrameHost with a
plumbed-in routing ID, but no corresponding extant RenderFrame in the
child process.

The solution involves plumbing in the actual SiteInstance, and only
using |route_id| if we don't change to a different SiteInstance.

BUG=627852
TBR=brettw@chromium.org

Committed: https://crrev.com/f5e618c7175237cd27bfb6e94fd4fa066367e36a
Cr-Commit-Position: refs/heads/master@{#434743}
==========

[commit-bot: I haz the power, 2016-11-28 22:41:16]

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