Track pending WebContents and widgets by (process_id, routing_id) pair.

Track pending WebContents and widgets by (process_id, routing_id) pair.

Currently, WebContentsImpl tracks pending widgets (widgets that are
created but not yet shown) in the pending_widget_views_ map, which is
keyed by the new widget's routing_id only.  Similarly, pending
WebContents are tracked in pending_contents_, also keyed by a single
routing_id.  This is prone to a race when two OOPIFs in different
processes (but part of the same WebContents) create new widgets, and
the two widgets happen to use the same routing ID, colliding in one of
these maps.  This CL changes both of these maps to be keyed by
(process_id, routing_id).

BUG=612276, 593522
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[alexmos, 2016-05-17 00:50:24]

Description was changed from

==========
Track pending WebContents and widgets by (process_id, routing_id) pair.

Currently, WebContentsImpl tracks pending widgets (widgets that are
created but not yet shown) in the pending_widget_views_ map, which is
keyed by the new widget's routing_id only.  Similarly, pending
WebContents are tracked in pending_contents_, also keyed by a single
routing_id.  This is prone to a race when two OOPIFs in different
processes (but part of the same WebContents) create new widgets, and
the two widgets happen to use the same routing ID, colliding in one of
these maps.  This CL changes both of these maps to be keyed by
(process_id, routing_id).

BUG=612276
==========

to

==========
Track pending WebContents and widgets by (process_id, routing_id) pair.

Currently, WebContentsImpl tracks pending widgets (widgets that are
created but not yet shown) in the pending_widget_views_ map, which is
keyed by the new widget's routing_id only.  Similarly, pending
WebContents are tracked in pending_contents_, also keyed by a single
routing_id.  This is prone to a race when two OOPIFs in different
processes (but part of the same WebContents) create new widgets, and
the two widgets happen to use the same routing ID, colliding in one of
these maps.  This CL changes both of these maps to be keyed by
(process_id, routing_id).

BUG=612276
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-05-20 22:19:33]

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

[alexmos, 2016-05-20 22:19:33]

Nick, can you please take a look?

[ncarter (slow), 2016-05-20 23:38:01]

lgtm

yes please

https://codereview.chromium.org/1986643002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1986643002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6956: if (!done_)
FWIW, I don't think you need the |done_| member here.

RunLoop, at least, is totally cool with you calling Quit() before Run(); it
turns Run into a noop. I would hope that MessageLoopRunner doesn't screw that
up.

https://codereview.chromium.org/1986643002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:7050: NEW_FOREGROUND_TAB,
gfx::Rect(), true);
Interesting test technique. I really worry about how we'll possibly do this sort
of thing with Mojo. Hopefully there's a pattern for it.

[alexmos, 2016-05-23 16:46:24]

Thanks for reviewing!

https://codereview.chromium.org/1986643002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1986643002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6956: if (!done_)
On 2016/05/20 23:38:01, ncarter wrote:
> FWIW, I don't think you need the |done_| member here.
> 
> RunLoop, at least, is totally cool with you calling Quit() before Run(); it
> turns Run into a noop. I would hope that MessageLoopRunner doesn't screw that
> up.

Ah yes, good point.  Removed.

[alexmos, 2016-05-23 16:46:36]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-05-23 16:46:37]

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/1986643002/#ps80001
(title: "Remove done_")

[commit-bot: I haz the power, 2016-05-23 16:46:56]

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

[alexmos, 2016-05-23 17:20:28]

Description was changed from

==========
Track pending WebContents and widgets by (process_id, routing_id) pair.

Currently, WebContentsImpl tracks pending widgets (widgets that are
created but not yet shown) in the pending_widget_views_ map, which is
keyed by the new widget's routing_id only.  Similarly, pending
WebContents are tracked in pending_contents_, also keyed by a single
routing_id.  This is prone to a race when two OOPIFs in different
processes (but part of the same WebContents) create new widgets, and
the two widgets happen to use the same routing ID, colliding in one of
these maps.  This CL changes both of these maps to be keyed by
(process_id, routing_id).

BUG=612276
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Track pending WebContents and widgets by (process_id, routing_id) pair.

Currently, WebContentsImpl tracks pending widgets (widgets that are
created but not yet shown) in the pending_widget_views_ map, which is
keyed by the new widget's routing_id only.  Similarly, pending
WebContents are tracked in pending_contents_, also keyed by a single
routing_id.  This is prone to a race when two OOPIFs in different
processes (but part of the same WebContents) create new widgets, and
the two widgets happen to use the same routing ID, colliding in one of
these maps.  This CL changes both of these maps to be keyed by
(process_id, routing_id).

BUG=612276, 593522
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-05-23 17:41:33]

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

[commit-bot: I haz the power, 2016-05-23 17:41:34]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alexmos, 2016-05-23 21:13:19]

Nick - I disabled my popup menu test for Mac and Android, as I've realized that
they don't even use the CreateWidget/ShowWidget flow, so it doesn't make sense
to test that plumbing for them.  Does that sound good?  

(Apparently, they instead rely on FrameHostMsg_ShowPopup to ask the browser to
build and display the actual popup using native controls - see the
blink::WebView::setUseExternalPopupMenus(true) call in
RenderThreadImpl::Init().)

[ncarter (slow), 2016-05-23 22:02:07]

lgtm

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

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-05-23 22:02:39]

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

[commit-bot: I haz the power, 2016-05-23 22:20:03]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-23 22:24:09]

Description was changed from

==========
Track pending WebContents and widgets by (process_id, routing_id) pair.

Currently, WebContentsImpl tracks pending widgets (widgets that are
created but not yet shown) in the pending_widget_views_ map, which is
keyed by the new widget's routing_id only.  Similarly, pending
WebContents are tracked in pending_contents_, also keyed by a single
routing_id.  This is prone to a race when two OOPIFs in different
processes (but part of the same WebContents) create new widgets, and
the two widgets happen to use the same routing ID, colliding in one of
these maps.  This CL changes both of these maps to be keyed by
(process_id, routing_id).

BUG=612276, 593522
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Track pending WebContents and widgets by (process_id, routing_id) pair.

Currently, WebContentsImpl tracks pending widgets (widgets that are
created but not yet shown) in the pending_widget_views_ map, which is
keyed by the new widget's routing_id only.  Similarly, pending
WebContents are tracked in pending_contents_, also keyed by a single
routing_id.  This is prone to a race when two OOPIFs in different
processes (but part of the same WebContents) create new widgets, and
the two widgets happen to use the same routing ID, colliding in one of
these maps.  This CL changes both of these maps to be keyed by
(process_id, routing_id).

BUG=612276, 593522
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-05-23 22:24:10]

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