RenderWidget: make routing_id be a parameter of the ctor

RenderWidget: make routing_id be a parameter of the ctor

This fixes a longstanding issue where, in layout-test-specific code, a
RenderViewObserver was instantiated before the RenderView was fully
initialized with a routing ID. RenderViewObserver internally caches
the routing id, so that's dangerous.

BUG=627852
Committed: https://crrev.com/8331f8ad9053ee1effd499ad29b5a806d9959ef4
Cr-Commit-Position: refs/heads/master@{#432246}

[ncarter (slow), 2016-11-11 19:37:32]

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

[commit-bot: I haz the power, 2016-11-11 19:38:10]

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-11 19:53:08]

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

[commit-bot: I haz the power, 2016-11-11 19:53:09]

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

[ncarter (slow), 2016-11-11 20:33:48]

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

[commit-bot: I haz the power, 2016-11-11 20:34:44]

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-11 20:38:52]

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

[commit-bot: I haz the power, 2016-11-11 20:38:53]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ncarter (slow), 2016-11-11 21:07:01]

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

[commit-bot: I haz the power, 2016-11-11 21:08: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-11 21:11:14]

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

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

Description was changed from

==========
RenderWidget: make routing_id be a parameter of the ctor

This fixes a longstanding issue where, in layout-test-specific code, a
RenderViewObserver was instantiated before the RenderView was fully
initialized with a routing ID. RenderViewObserver internally caches
the routing id, so that's dangerous.

BUG=
==========

to

==========
RenderWidget: make routing_id be a parameter of the ctor

This fixes a longstanding issue where, in layout-test-specific code, a
RenderViewObserver was instantiated before the RenderView was fully
initialized with a routing ID. RenderViewObserver internally caches
the routing id, so that's dangerous.

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

[ncarter (slow), 2016-11-11 21:36:50]

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

[commit-bot: I haz the power, 2016-11-11 21:37:18]

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-11 23:47:55]

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

[commit-bot: I haz the power, 2016-11-11 23:48:28]

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-12 00:16:17]

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

[commit-bot: I haz the power, 2016-11-12 00:16:18]

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

[ncarter (slow), 2016-11-14 18:46:16]

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

[commit-bot: I haz the power, 2016-11-14 18:47:31]

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-14 19:31:50]

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

[ncarter (slow), 2016-11-14 19:31:51]

Please review

https://codereview.chromium.org/2493293002/diff/90001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2493293002/diff/90001/content/renderer/render...
content/renderer/render_widget.cc:399: input_handler_.reset(new
RenderWidgetInputHandler(
The ordering change here is the primary source of risk I see in this CL.

For non-RenderView RenderWidgets, this is the same. But for a RenderView, the
RenderWidgetInputHandler is now being initialized after the WebWidget is
created, instead of before. I don't know of any interaction between those, but I
had assumed that input_handler_ was initialized before Init for a reason.

If this breaks tests, I can make InitRoutingID() be InitInputHandler, but I was
hoping to drop the complexity of two Init functions.

Initializing input_handler_ in the ctor breaks the
compositor_mus_connection_unittests, which never called Init and which manually
override the input_handler_. We can also fix those via more ctor dependency
injection, or an explicit factory function, or something.

[commit-bot: I haz the power, 2016-11-14 19:34:45]

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

[commit-bot: I haz the power, 2016-11-14 19:34:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ncarter (slow), 2016-11-14 21:08:58]

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

[ncarter (slow), 2016-11-14 21:12:13]

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

[commit-bot: I haz the power, 2016-11-14 21:14:26]

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

[lfg, 2016-11-14 22:09:59]

lgtm with nit.

https://codereview.chromium.org/2493293002/diff/130001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2493293002/diff/130001/content/renderer/rende...
content/renderer/render_view_impl.cc:701: RenderWidget::InitInputHandler();
Why do you need to add this? I assume tests were failing. Can you add a comment
explaining?

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

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

[commit-bot: I haz the power, 2016-11-14 23:09:00]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-11-14 23:25:31]

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

[commit-bot: I haz the power, 2016-11-14 23:27:20]

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-14 23:33:56]

ptal

https://codereview.chromium.org/2493293002/diff/130001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2493293002/diff/130001/content/renderer/rende...
content/renderer/render_view_impl.cc:701: RenderWidget::InitInputHandler();
On 2016/11/14 22:09:59, lfg wrote:
> Why do you need to add this? I assume tests were failing. Can you add a
comment
> explaining?

It wasn't strictly necessary; I figured that so long as I had InitInputHandler()
around, I could preserve the old ordering in case there was any unseen
dependency between WebView::create(this) and the input handler.

However, I went back and figured out the render_widget_unittest situation, which
was a leak due to an unbalanced bare AddRef(). So now, InitInputHandler is gone
again.

https://codereview.chromium.org/2493293002/diff/150001/content/renderer/rende...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/2493293002/diff/150001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:140: // here to ensure |widget_| is
properly freed.
I looked at using MockRenderThread::SendCloseMessage, or doing the Close
directly, but both would require more extensive changes to this test.
SendCloseMessage only works currently with RenderViews (there's no such thing as
RenderWidget::FromRouteID), and even if there were, we'd have to create a
message loop so that the PostTask within OnClose worked properly.

So this seemed like a better option.

[commit-bot: I haz the power, 2016-11-15 02:47:39]

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

[commit-bot: I haz the power, 2016-11-15 02:47:39]

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

[lfg, 2016-11-15 04:48:37]

Thanks for figuring it out!

lgtm

https://codereview.chromium.org/2493293002/diff/150001/content/renderer/rende...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/2493293002/diff/150001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:463: new
PopupRenderWidget(&compositor_deps_));
asan bots is complaining that this widget also needs balancing the refcount.

[ncarter (slow), 2016-11-15 18:03:51]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2016-11-15 18:03:52]

The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org
Link to the patchset: https://codereview.chromium.org/2493293002/#ps170001
(title: "Add a release for asan bots.")

[ncarter (slow), 2016-11-15 18:04:15]

https://codereview.chromium.org/2493293002/diff/150001/content/renderer/rende...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/2493293002/diff/150001/content/renderer/rende...
content/renderer/render_widget_unittest.cc:463: new
PopupRenderWidget(&compositor_deps_));
On 2016/11/15 04:48:37, lfg wrote:
> asan bots is complaining that this widget also needs balancing the refcount.

Thanks for figuring that out!

[commit-bot: I haz the power, 2016-11-15 18:04:41]

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-15 20:43:14]

Description was changed from

==========
RenderWidget: make routing_id be a parameter of the ctor

This fixes a longstanding issue where, in layout-test-specific code, a
RenderViewObserver was instantiated before the RenderView was fully
initialized with a routing ID. RenderViewObserver internally caches
the routing id, so that's dangerous.

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

to

==========
RenderWidget: make routing_id be a parameter of the ctor

This fixes a longstanding issue where, in layout-test-specific code, a
RenderViewObserver was instantiated before the RenderView was fully
initialized with a routing ID. RenderViewObserver internally caches
the routing id, so that's dangerous.

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

[commit-bot: I haz the power, 2016-11-15 20:43:15]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2016-11-15 20:55:41]

Description was changed from

==========
RenderWidget: make routing_id be a parameter of the ctor

This fixes a longstanding issue where, in layout-test-specific code, a
RenderViewObserver was instantiated before the RenderView was fully
initialized with a routing ID. RenderViewObserver internally caches
the routing id, so that's dangerous.

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

to

==========
RenderWidget: make routing_id be a parameter of the ctor

This fixes a longstanding issue where, in layout-test-specific code, a
RenderViewObserver was instantiated before the RenderView was fully
initialized with a routing ID. RenderViewObserver internally caches
the routing id, so that's dangerous.

BUG=627852

Committed: https://crrev.com/8331f8ad9053ee1effd499ad29b5a806d9959ef4
Cr-Commit-Position: refs/heads/master@{#432246}
==========

[commit-bot: I haz the power, 2016-11-15 20:55:42]

Patchset 10 (id:??) landed as
https://crrev.com/8331f8ad9053ee1effd499ad29b5a806d9959ef4
Cr-Commit-Position: refs/heads/master@{#432246}