RenderWidgetHostViewChildFrame's called a virtual in its ctor.

RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly constructed object.
Since RenderWidgetHostViewGuest is a subclass of RenderWidgetHostViewChildFrame
its virtual function would not get invoked since the vtable had not
be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added an assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5bf106420af2df80286f4f404c44dbf818469db1
Cr-Commit-Position: refs/heads/master@{#423182}

[dtapuska, 2016-10-03 20:22:21]

Description was changed from

==========
RenderWidgetHostViewGuest's called a virtual in their ctor.

It appears that RenderWidgetHostViewChildFrame invoked a method that
then called back into the object which was a virtual. This would fail
to call the RenderWidgetHostViewGuest's implementation because
it hadn't populated the vtable for the derived class yet.

BUG=652212
==========

to

==========
RenderWidgetHostViewGuest's called a virtual in their ctor.

It appears that RenderWidgetHostViewChildFrame invoked a method that
then called back into the object which was a virtual. This would fail
to call the RenderWidgetHostViewGuest's implementation because
it hadn't populated the vtable for the derived class yet.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dtapuska, 2016-10-03 20:23:38]

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

[commit-bot: I haz the power, 2016-10-03 20:24:27]

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

[dtapuska, 2016-10-03 20:24:59]

dtapuska@chromium.org changed reviewers:
+ clamy@chromium.org, kenrb@chromium.org

[dtapuska, 2016-10-03 20:25:00]

clamy@ PTAL; if you have an alternate pattern to follow I'd be happy to adjust
it to.

kenrb@ this is similar to what I said how we could fix it.

[commit-bot: I haz the power, 2016-10-03 21:11:57]

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

[commit-bot: I haz the power, 2016-10-03 21:11:58]

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

[clamy, 2016-10-04 16:12:21]

@dtapuska: sorry I did not have time to review this today. I will review it
tomorrow.

[dtapuska, 2016-10-04 16:13:57]

On 2016/10/04 16:12:21, clamy wrote:
> @dtapuska: sorry I did not have time to review this today. I will review it
> tomorrow.

thanks for the update. Looks like I got a couple failures on the buildbots I
should fix anyways.

[dtapuska, 2016-10-04 20:29:58]

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

[commit-bot: I haz the power, 2016-10-04 20:30:21]

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-10-04 22:47:45]

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

[commit-bot: I haz the power, 2016-10-04 22:47:45]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-10-05 12:35:54]

Thanks! A few comments below.

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:42:
RenderWidgetHostViewChildFrame* RenderWidgetHostViewChildFrame::Create(
Please add a // static comment above the method.

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:55: protected:
Please move the protected part of the header between the public & private part.

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:79: Init();
nit: For the sake of consistency, I think it'd be nicer to have a create
function that mirrors what is done in RenderWidgetHostViewChildFrame.

https://codereview.chromium.org/2385333002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/2385333002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:535: void
UpdateNeedsBeginFramesInternal();
How is this change (and the similar ones about needs_begin_frames_internal)
related to the issue of the virtual being called in the ctor?

[clamy, 2016-10-05 12:35:54]

Thanks! A few comments below.

[dtapuska, 2016-10-05 13:21:30]

https://codereview.chromium.org/2385333002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/2385333002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:535: void
UpdateNeedsBeginFramesInternal();
On 2016/10/05 12:35:53, clamy wrote:
> How is this change (and the similar ones about needs_begin_frames_internal)
> related to the issue of the virtual being called in the ctor?

So I tried to land a change that did vsync aligned synthetic gestures (see
https://codereview.chromium.org/2372873003/) which had part of this CL. 

And this caused a regression because when SetView is called (in the ctor)
SetNeedsBeginFrame method is called
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...

but its state is completely wrong.

If I don't carry forward some of that original change (as here); it fails an
assert because the frame observer is added twice. And there is a DCHECK that the
frame observer only be added once in the listener table.

[clamy, 2016-10-05 13:47:02]

https://codereview.chromium.org/2385333002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/2385333002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.h:535: void
UpdateNeedsBeginFramesInternal();
On 2016/10/05 13:21:29, dtapuska wrote:
> On 2016/10/05 12:35:53, clamy wrote:
> > How is this change (and the similar ones about needs_begin_frames_internal)
> > related to the issue of the virtual being called in the ctor?
> 
> So I tried to land a change that did vsync aligned synthetic gestures (see
> https://codereview.chromium.org/2372873003/) which had part of this CL. 
> 
> And this caused a regression because when SetView is called (in the ctor)
> SetNeedsBeginFrame method is called
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
> 
> but its state is completely wrong.
> 
> If I don't carry forward some of that original change (as here); it fails an
> assert because the frame observer is added twice. And there is a DCHECK that
the
> frame observer only be added once in the listener table.

Ok so let me see if I get this right (sorry not super familiar with this area of
the code): the virtual method that ends up getting called in the
RenderWidgetHostViewChildFrame ctor is SetNeedsBeginFrame? And because it was
called before RenderWidgetHostViewGuest vtable had been populated it didn't
actually do what it was supposed to do, but now that it does it triggers an
assert, hence the change to UpdateNeedsBeginFramesInternal?

If so, could you please update the CL description by adding the name of the
problematic method & explaining that this also required some modification to the
implementation of the method in some inheriting classes now that the proper
version is being called in the ctor? That wasn't immediately clear when reading
it.

[dtapuska, 2016-10-05 13:50:23]

On 2016/10/05 13:47:02, clamy wrote:
>
https://codereview.chromium.org/2385333002/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_aura.h (right):
> 
>
https://codereview.chromium.org/2385333002/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_aura.h:535: void
> UpdateNeedsBeginFramesInternal();
> On 2016/10/05 13:21:29, dtapuska wrote:
> > On 2016/10/05 12:35:53, clamy wrote:
> > > How is this change (and the similar ones about
needs_begin_frames_internal)
> > > related to the issue of the virtual being called in the ctor?
> > 
> > So I tried to land a change that did vsync aligned synthetic gestures (see
> > https://codereview.chromium.org/2372873003/) which had part of this CL. 
> > 
> > And this caused a regression because when SetView is called (in the ctor)
> > SetNeedsBeginFrame method is called
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
> > 
> > but its state is completely wrong.
> > 
> > If I don't carry forward some of that original change (as here); it fails an
> > assert because the frame observer is added twice. And there is a DCHECK that
> the
> > frame observer only be added once in the listener table.
> 
> Ok so let me see if I get this right (sorry not super familiar with this area
of
> the code): the virtual method that ends up getting called in the
> RenderWidgetHostViewChildFrame ctor is SetNeedsBeginFrame? And because it was
> called before RenderWidgetHostViewGuest vtable had been populated it didn't
> actually do what it was supposed to do, but now that it does it triggers an
> assert, hence the change to UpdateNeedsBeginFramesInternal?
> 
> If so, could you please update the CL description by adding the name of the
> problematic method & explaining that this also required some modification to
the
> implementation of the method in some inheriting classes now that the proper
> version is being called in the ctor? That wasn't immediately clear when
reading
> it.

You got it correct. Will update change description.

[kenrb, 2016-10-05 14:10:40]

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:65:
GetTextInputManager();
Would it make sense to move these other method calls into Init(), to avoid
similar bugs in future?

[dtapuska, 2016-10-05 14:16:11]

Description was changed from

==========
RenderWidgetHostViewGuest's called a virtual in their ctor.

It appears that RenderWidgetHostViewChildFrame invoked a method that
then called back into the object which was a virtual. This would fail
to call the RenderWidgetHostViewGuest's implementation because
it hadn't populated the vtable for the derived class yet.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly
constructed object. Since RenderWidgetHostViewGuest is
a subclass of RenderWidgetHostViewChildFrame its
virtual function would not get invoked since
the vtable had not be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added a double assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dtapuska, 2016-10-05 14:16:38]

Description was changed from

==========
RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly
constructed object. Since RenderWidgetHostViewGuest is
a subclass of RenderWidgetHostViewChildFrame its
virtual function would not get invoked since
the vtable had not be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added a double assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly
constructed object.
Since RenderWidgetHostViewGuest is
a subclass of RenderWidgetHostViewChildFrame its
virtual function would not get invoked since
the vtable had not be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added an assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dtapuska, 2016-10-05 14:16:52]

Description was changed from

==========
RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly
constructed object.
Since RenderWidgetHostViewGuest is
a subclass of RenderWidgetHostViewChildFrame its
virtual function would not get invoked since
the vtable had not be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added an assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly constructed object.
Since RenderWidgetHostViewGuest is
a subclass of RenderWidgetHostViewChildFrame its
virtual function would not get invoked since
the vtable had not be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added an assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dtapuska, 2016-10-05 14:17:37]

Description was changed from

==========
RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly constructed object.
Since RenderWidgetHostViewGuest is
a subclass of RenderWidgetHostViewChildFrame its
virtual function would not get invoked since
the vtable had not be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added an assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly constructed object.
Since RenderWidgetHostViewGuest is a subclass of RenderWidgetHostViewChildFrame
its virtual function would not get invoked since the vtable had not
be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added an assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dtapuska, 2016-10-05 14:24:36]

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

[commit-bot: I haz the power, 2016-10-05 14:24:59]

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

[dtapuska, 2016-10-05 14:25:00]

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:42:
RenderWidgetHostViewChildFrame* RenderWidgetHostViewChildFrame::Create(
On 2016/10/05 12:35:53, clamy wrote:
> Please add a // static comment above the method.

Done.

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:65:
GetTextInputManager();
On 2016/10/05 14:10:40, kenrb wrote:
> Would it make sense to move these other method calls into Init(), to avoid
> similar bugs in future?

done.

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:55: protected:
On 2016/10/05 12:35:53, clamy wrote:
> Please move the protected part of the header between the public & private
part.

Done.

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/2385333002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:79: Init();
On 2016/10/05 12:35:53, clamy wrote:
> nit: For the sake of consistency, I think it'd be nicer to have a create
> function that mirrors what is done in RenderWidgetHostViewChildFrame.

Done.

[kenrb, 2016-10-05 14:34:00]

lgtm

[commit-bot: I haz the power, 2016-10-05 15:18:43]

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

[commit-bot: I haz the power, 2016-10-05 15:18:44]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-10-05 15:29:49]

Thanks! Lgtm with one comment.

https://codereview.chromium.org/2385333002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.h (right):

https://codereview.chromium.org/2385333002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.h:134:
RenderWidgetHostViewGuest(
It seems that no class inherits from RenderWidgetHostViewGuest. If so, can we
make its ctor private instead of protected?

[dtapuska, 2016-10-05 15:37:05]

https://codereview.chromium.org/2385333002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.h (right):

https://codereview.chromium.org/2385333002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.h:134:
RenderWidgetHostViewGuest(
On 2016/10/05 15:29:49, clamy wrote:
> It seems that no class inherits from RenderWidgetHostViewGuest. If so, can we
> make its ctor private instead of protected?

Done.

[dtapuska, 2016-10-05 15:37:11]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-10-05 15:37:11]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2385333002/#ps80001
(title: "Move guest ctor to private")

[commit-bot: I haz the power, 2016-10-05 15:37:39]

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-10-05 16:31:09]

Description was changed from

==========
RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly constructed object.
Since RenderWidgetHostViewGuest is a subclass of RenderWidgetHostViewChildFrame
its virtual function would not get invoked since the vtable had not
be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added an assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly constructed object.
Since RenderWidgetHostViewGuest is a subclass of RenderWidgetHostViewChildFrame
its virtual function would not get invoked since the vtable had not
be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added an assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-05 16:31:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-05 16:33:01]

Description was changed from

==========
RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly constructed object.
Since RenderWidgetHostViewGuest is a subclass of RenderWidgetHostViewChildFrame
its virtual function would not get invoked since the vtable had not
be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added an assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
RenderWidgetHostViewChildFrame's called a virtual in its ctor.

It appears that calling SetView in the ctor can lead to a path where
SetNeedsBeginFrame is invoked on the newly constructed object.
Since RenderWidgetHostViewGuest is a subclass of RenderWidgetHostViewChildFrame
its virtual function would not get invoked since the vtable had not
be constructed yet.

Change SetView to be called after the class is fully
instantiated.

Also fix RenderWidgetHostViewAura since calling this correctly
now leads to an assert that the observer is only added once.
So track whether the observer has been added so that when the
begin frame source is added an assertion doesn't occur.

BUG=652212
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5bf106420af2df80286f4f404c44dbf818469db1
Cr-Commit-Position: refs/heads/master@{#423182}
==========

[commit-bot: I haz the power, 2016-10-05 16:33:03]

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