ABANDONED CL: Fix delivery of TestFinishedInSecondaryWindow message from OOPIFs.

Fix delivery of TestFinishedInSecondaryWindow message from OOPIFs.

Layout Tests run with --site-per-process flag and finishing in an OOPIF
were timing out because:

- BlinkTestController / NotifyDoneForwarder were only handling
  ShellViewHostMsg_TestFinishedInSecondaryWindow coming from secondary
  test windows (in the fixed cases, the message is coming from an OOPIF;
  note that the current CL fixes handling and routing of the message and
  naming is OOPIF-ied ["secondary window" -> "secondary renderer"] in a
  separate, earlier CL - crrev.com/1746393002).

- In case of LayoutTests + OOPIF, the RenderViewObserver can see a
  RenderView before it gets a routing id [1].  Before this CL
  RenderViewObserver would stash the incorrect routing id which
  BlinkTestRunner would attempt to use when sending
  ShellViewHostMsg_TestFinishedInSecondaryWindow.  This CL removes the
  unnecessary RenderViewObserver::routing_id_ field and simply forwards
  the routing id calculations/queries into the currently observed
  RenderView.

- ShellViewHostMsg_TestFinishedInSecondaryWindow would be dropped on
  the floor when the RenderView is swapped out.

This CL fixes the 3 issues described above and updates layout test
expectations for --site-per-process flag accordingly (note that some
tests that used to time out are passing now, but some simply changed the
failure mode from a timeout into a text diff or another failure mode).

BUG=477150
[1] Example callstack when RenderView doesn't yet have a routing id when
    it is passed to the constructor of RenderViewObserver:
      1 0x7f1fd66cfc36 content::RenderViewObserver::RenderViewObserver()
      2 0x0000004758a3 content::BlinkTestRunner::BlinkTestRunner()
      3 0x000000450bb3 content::LayoutTestContentRendererClient::WebTestProxyCreated()
      4 0x00000047c237 content::(anonymous namespace)::CreateWebTestProxy()
      5 0x7f1fd6770294 content::RenderViewImpl::Create()
      6 0x7f1fd676a5f5 content::RenderThreadImpl::OnControlMessageReceived()

[Łukasz Anforowicz, 2016-03-04 01:18:16]

Description was changed from

==========
Handle layout tests finishing in an OOPIF.

Layout Tests run with --site-per-process flag and finishing in an OOPIF
were timing out because:

- BlinkTestController / NotifyDoneForwarder were only handling
  ShellViewHostMsg_TestFinishedInSecondaryWindow coming from secondary
  test windows (in the fixed cases, the message is coming from an OOPIF;
  note that the current CL fixes handling and routing of the message and
  naming (secondary window -> secondary renderer) is OOPIF-ied in a
  separate CL - crrev.com/1746393002).

- In case of LayoutTests + OOPIF, the RenderViewObserver can see a
  RenderView before it gets a routing id [1].  Before this CL
  RenderViewObserver would stash the incorrect routing id which
  BlinkTestRunner would attempt to use when sending
  ShellViewHostMsg_TestFinishedInSecondaryWindow.  This CL removes the
  unnecessary RenderViewObserver::routing_id_ field and simply forwards
  the routing id from the currently observed RenderView.

- ShellViewHostMsg_TestFinishedInSecondaryWindow would be dropped on
  the floor, because when the test finished in an OOPIF, the RenderView
  is swapped out.

This CL fixes the 3 issues described above and updates layout test
expectations for --site-per-process flag accordingly (note that some
tests that used to time out are passing now, but some simply changed
the failure mode from a timeout into a text diff).

BUG=477150

[1] Example callstack when RenderView doesn't yet have a routing id when
    it is passed to the constructor of RenderViewObserver:
      1 0x7f1fd66cfc36 content::RenderViewObserver::RenderViewObserver()
      2 0x0000004758a3 content::BlinkTestRunner::BlinkTestRunner()
      3 0x000000450bb3
content::LayoutTestContentRendererClient::WebTestProxyCreated()
      4 0x00000047c237 content::(anonymous namespace)::CreateWebTestProxy()
      5 0x7f1fd6770294 content::RenderViewImpl::Create()
      6 0x7f1fd676a5f5 content::RenderThreadImpl::OnControlMessageReceived()
==========

to

==========
Handle layout tests finishing in an OOPIF.

Layout Tests run with --site-per-process flag and finishing in an OOPIF
were timing out because:

- BlinkTestController / NotifyDoneForwarder were only handling
  ShellViewHostMsg_TestFinishedInSecondaryWindow coming from secondary
  test windows (in the fixed cases, the message is coming from an OOPIF;
  note that the current CL fixes handling and routing of the message and
  naming is OOPIF-ied ("secondary window" -> "secondary renderer") in a
  separate CL - crrev.com/1746393002).

- In case of LayoutTests + OOPIF, the RenderViewObserver can see a
  RenderView before it gets a routing id [1].  Before this CL
  RenderViewObserver would stash the incorrect routing id which
  BlinkTestRunner would attempt to use when sending
  ShellViewHostMsg_TestFinishedInSecondaryWindow.  This CL removes the
  unnecessary RenderViewObserver::routing_id_ field and simply forwards
  the routing id from the currently observed RenderView.

- ShellViewHostMsg_TestFinishedInSecondaryWindow would be dropped on
  the floor, because when the test finished in an OOPIF, the RenderView
  is swapped out.

This CL fixes the 3 issues described above and updates layout test
expectations for --site-per-process flag accordingly (note that some
tests that used to time out are passing now, but some simply changed
the failure mode from a timeout into a text diff).

BUG=477150

[1] Example callstack when RenderView doesn't yet have a routing id when
    it is passed to the constructor of RenderViewObserver:
      1 0x7f1fd66cfc36 content::RenderViewObserver::RenderViewObserver()
      2 0x0000004758a3 content::BlinkTestRunner::BlinkTestRunner()
      3 0x000000450bb3
content::LayoutTestContentRendererClient::WebTestProxyCreated()
      4 0x00000047c237 content::(anonymous namespace)::CreateWebTestProxy()
      5 0x7f1fd6770294 content::RenderViewImpl::Create()
      6 0x7f1fd676a5f5 content::RenderThreadImpl::OnControlMessageReceived()
==========

[Łukasz Anforowicz, 2016-03-04 02:55:05]

lukasza@chromium.org changed reviewers:
+ jochen@chromium.org

[Łukasz Anforowicz, 2016-03-04 02:55:06]

jochen@, could you please take a look?

This is the smallest of the CLs that I've recently been spamming you with, so
maybe this is the one to start with :-).

Also - I'll probably ask nasko@ to review swapped out and jam@ to review
RenderViewObserver::routing_id_ (so it is okay if you want to focus on changes
under content/shell/browser/layout_test/blink_test_controller.h + on the overall
test finishing flow).

[Łukasz Anforowicz, 2016-03-04 05:20:12]

Hmmm... I guess I need to take a look at the linux_site_isolation failures. 
Weird that I haven't reproed them locally...

On 2016/03/04 02:55:06, Łukasz Anforowicz wrote:
> jochen@, could you please take a look?
> 
> This is the smallest of the CLs that I've recently been spamming you with, so
> maybe this is the one to start with :-).
> 
> Also - I'll probably ask nasko@ to review swapped out and jam@ to review
> RenderViewObserver::routing_id_ (so it is okay if you want to focus on changes
> under content/shell/browser/layout_test/blink_test_controller.h + on the
overall
> test finishing flow).

[Łukasz Anforowicz, 2016-03-07 19:32:32]

Description was changed from

==========
Handle layout tests finishing in an OOPIF.

Layout Tests run with --site-per-process flag and finishing in an OOPIF
were timing out because:

- BlinkTestController / NotifyDoneForwarder were only handling
  ShellViewHostMsg_TestFinishedInSecondaryWindow coming from secondary
  test windows (in the fixed cases, the message is coming from an OOPIF;
  note that the current CL fixes handling and routing of the message and
  naming is OOPIF-ied ("secondary window" -> "secondary renderer") in a
  separate CL - crrev.com/1746393002).

- In case of LayoutTests + OOPIF, the RenderViewObserver can see a
  RenderView before it gets a routing id [1].  Before this CL
  RenderViewObserver would stash the incorrect routing id which
  BlinkTestRunner would attempt to use when sending
  ShellViewHostMsg_TestFinishedInSecondaryWindow.  This CL removes the
  unnecessary RenderViewObserver::routing_id_ field and simply forwards
  the routing id from the currently observed RenderView.

- ShellViewHostMsg_TestFinishedInSecondaryWindow would be dropped on
  the floor, because when the test finished in an OOPIF, the RenderView
  is swapped out.

This CL fixes the 3 issues described above and updates layout test
expectations for --site-per-process flag accordingly (note that some
tests that used to time out are passing now, but some simply changed
the failure mode from a timeout into a text diff).

BUG=477150

[1] Example callstack when RenderView doesn't yet have a routing id when
    it is passed to the constructor of RenderViewObserver:
      1 0x7f1fd66cfc36 content::RenderViewObserver::RenderViewObserver()
      2 0x0000004758a3 content::BlinkTestRunner::BlinkTestRunner()
      3 0x000000450bb3
content::LayoutTestContentRendererClient::WebTestProxyCreated()
      4 0x00000047c237 content::(anonymous namespace)::CreateWebTestProxy()
      5 0x7f1fd6770294 content::RenderViewImpl::Create()
      6 0x7f1fd676a5f5 content::RenderThreadImpl::OnControlMessageReceived()
==========

to

==========
Fix delivery of TestFinishedInSecondaryWindow message from OOPIFs.

Layout Tests run with --site-per-process flag and finishing in an OOPIF
were timing out because:

- BlinkTestController / NotifyDoneForwarder were only handling
  ShellViewHostMsg_TestFinishedInSecondaryWindow coming from secondary
  test windows (in the fixed cases, the message is coming from an OOPIF;
  note that the current CL fixes handling and routing of the message and
  naming is OOPIF-ied ["secondary window" -> "secondary renderer"] in a
  separate, earlier CL - crrev.com/1746393002).

- In case of LayoutTests + OOPIF, the RenderViewObserver can see a
  RenderView before it gets a routing id [1].  Before this CL
  RenderViewObserver would stash the incorrect routing id which
  BlinkTestRunner would attempt to use when sending
  ShellViewHostMsg_TestFinishedInSecondaryWindow.  This CL removes the
  unnecessary RenderViewObserver::routing_id_ field and simply forwards
  the routing id calculations/queries into the currently observed
  RenderView.

- ShellViewHostMsg_TestFinishedInSecondaryWindow would be dropped on
  the floor when the RenderView is swapped out.

This CL fixes the 3 issues described above and updates layout test
expectations for --site-per-process flag accordingly (note that some
tests that used to time out are passing now, but some simply changed the
failure mode from a timeout into a text diff or another failure mode).

BUG=477150

[1] Example callstack when RenderView doesn't yet have a routing id when
    it is passed to the constructor of RenderViewObserver:
      1 0x7f1fd66cfc36 content::RenderViewObserver::RenderViewObserver()
      2 0x0000004758a3 content::BlinkTestRunner::BlinkTestRunner()
      3 0x000000450bb3
content::LayoutTestContentRendererClient::WebTestProxyCreated()
      4 0x00000047c237 content::(anonymous namespace)::CreateWebTestProxy()
      5 0x7f1fd6770294 content::RenderViewImpl::Create()
      6 0x7f1fd676a5f5 content::RenderThreadImpl::OnControlMessageReceived()
==========

[Łukasz Anforowicz, 2016-03-07 19:32:56]

Description was changed from

==========
Fix delivery of TestFinishedInSecondaryWindow message from OOPIFs.

Layout Tests run with --site-per-process flag and finishing in an OOPIF
were timing out because:

- BlinkTestController / NotifyDoneForwarder were only handling
  ShellViewHostMsg_TestFinishedInSecondaryWindow coming from secondary
  test windows (in the fixed cases, the message is coming from an OOPIF;
  note that the current CL fixes handling and routing of the message and
  naming is OOPIF-ied ["secondary window" -> "secondary renderer"] in a
  separate, earlier CL - crrev.com/1746393002).

- In case of LayoutTests + OOPIF, the RenderViewObserver can see a
  RenderView before it gets a routing id [1].  Before this CL
  RenderViewObserver would stash the incorrect routing id which
  BlinkTestRunner would attempt to use when sending
  ShellViewHostMsg_TestFinishedInSecondaryWindow.  This CL removes the
  unnecessary RenderViewObserver::routing_id_ field and simply forwards
  the routing id calculations/queries into the currently observed
  RenderView.

- ShellViewHostMsg_TestFinishedInSecondaryWindow would be dropped on
  the floor when the RenderView is swapped out.

This CL fixes the 3 issues described above and updates layout test
expectations for --site-per-process flag accordingly (note that some
tests that used to time out are passing now, but some simply changed the
failure mode from a timeout into a text diff or another failure mode).

BUG=477150

[1] Example callstack when RenderView doesn't yet have a routing id when
    it is passed to the constructor of RenderViewObserver:
      1 0x7f1fd66cfc36 content::RenderViewObserver::RenderViewObserver()
      2 0x0000004758a3 content::BlinkTestRunner::BlinkTestRunner()
      3 0x000000450bb3
content::LayoutTestContentRendererClient::WebTestProxyCreated()
      4 0x00000047c237 content::(anonymous namespace)::CreateWebTestProxy()
      5 0x7f1fd6770294 content::RenderViewImpl::Create()
      6 0x7f1fd676a5f5 content::RenderThreadImpl::OnControlMessageReceived()
==========

to

==========
Fix delivery of TestFinishedInSecondaryWindow message from OOPIFs.

Layout Tests run with --site-per-process flag and finishing in an OOPIF
were timing out because:

- BlinkTestController / NotifyDoneForwarder were only handling
  ShellViewHostMsg_TestFinishedInSecondaryWindow coming from secondary
  test windows (in the fixed cases, the message is coming from an OOPIF;
  note that the current CL fixes handling and routing of the message and
  naming is OOPIF-ied ["secondary window" -> "secondary renderer"] in a
  separate, earlier CL - crrev.com/1746393002).

- In case of LayoutTests + OOPIF, the RenderViewObserver can see a
  RenderView before it gets a routing id [1].  Before this CL
  RenderViewObserver would stash the incorrect routing id which
  BlinkTestRunner would attempt to use when sending
  ShellViewHostMsg_TestFinishedInSecondaryWindow.  This CL removes the
  unnecessary RenderViewObserver::routing_id_ field and simply forwards
  the routing id calculations/queries into the currently observed
  RenderView.

- ShellViewHostMsg_TestFinishedInSecondaryWindow would be dropped on
  the floor when the RenderView is swapped out.

This CL fixes the 3 issues described above and updates layout test
expectations for --site-per-process flag accordingly (note that some
tests that used to time out are passing now, but some simply changed the
failure mode from a timeout into a text diff or another failure mode).

BUG=477150

[1] Example callstack when RenderView doesn't yet have a routing id when
    it is passed to the constructor of RenderViewObserver:
      1 0x7f1fd66cfc36 content::RenderViewObserver::RenderViewObserver()
      2 0x0000004758a3 content::BlinkTestRunner::BlinkTestRunner()
      3 0x000000450bb3
content::LayoutTestContentRendererClient::WebTestProxyCreated()
      4 0x00000047c237 content::(anonymous namespace)::CreateWebTestProxy()
      5 0x7f1fd6770294 content::RenderViewImpl::Create()
      6 0x7f1fd676a5f5 content::RenderThreadImpl::OnControlMessageReceived()
==========

[jochen (gone - plz use gerrit), 2016-03-08 16:33:18]

https://codereview.chromium.org/1765623003/diff/60001/content/common/swapped_...
File content/common/swapped_out_messages.cc (right):

https://codereview.chromium.org/1765623003/diff/60001/content/common/swapped_...
content/common/swapped_out_messages.cc:12: #include
"content/shell/common/shell_messages.h"
WAT?

can you please introduce a ContentClient callback and override that from content
shell? content musn't depend on shell

[Łukasz Anforowicz, 2016-03-08 17:31:06]

nasko@ and alexmos@ to CC - FYI
(nasko@ for swapped out, alexmos@ for the #include pointed out by jochen@)

Thanks for reviewing jochen@.  Let me land the other CLs first and do some more
investigation between re-reviewing this one and deciding what needs to be done
(and who knows - maybe by that time swapped out will be already ripped out of
the code base? :-).

https://codereview.chromium.org/1765623003/diff/60001/content/common/swapped_...
File content/common/swapped_out_messages.cc (right):

https://codereview.chromium.org/1765623003/diff/60001/content/common/swapped_...
content/common/swapped_out_messages.cc:12: #include
"content/shell/common/shell_messages.h"
On 2016/03/08 16:33:18, jochen wrote:
> WAT?
> 
> can you please introduce a ContentClient callback and override that from
content
> shell? content musn't depend on shell

Hmmm... good point.  How could I miss that... :-/

1. I am trying to tighten DEPS rules in
https://codereview.chromium.org/1777503003/

2. nasko@ says that this extra temporary hole in swapped out processing might be
ok (he plans to yank out swapped out in the next few weeks).

3. before proceeding, I probably should double-check why we end up sending a
message in swapped out state here.  For example - what exactly causes the test
to finish?  If test finishes because of didFinishLoad, then is
TestRunner::topLoadingFrame a main frame?  Is it ok if it is not a main frame? 
Maybe we should communicate with the browser via a randomly-picked local frame
(just as we do in https://codereview.chromium.org/1715573002/#msg9)?

[jochen (gone - plz use gerrit), 2016-03-09 15:06:40]

On 2016/03/08 at 17:31:06, lukasza wrote:
> nasko@ and alexmos@ to CC - FYI
> (nasko@ for swapped out, alexmos@ for the #include pointed out by jochen@)
> 
> Thanks for reviewing jochen@.  Let me land the other CLs first and do some
more investigation between re-reviewing this one and deciding what needs to be
done (and who knows - maybe by that time swapped out will be already ripped out
of the code base? :-).
> 
>
https://codereview.chromium.org/1765623003/diff/60001/content/common/swapped_...
> File content/common/swapped_out_messages.cc (right):
> 
>
https://codereview.chromium.org/1765623003/diff/60001/content/common/swapped_...
> content/common/swapped_out_messages.cc:12: #include
"content/shell/common/shell_messages.h"
> On 2016/03/08 16:33:18, jochen wrote:
> > WAT?
> > 
> > can you please introduce a ContentClient callback and override that from
content
> > shell? content musn't depend on shell
> 
> Hmmm... good point.  How could I miss that... :-/
> 
> 1. I am trying to tighten DEPS rules in
https://codereview.chromium.org/1777503003/
> 
> 2. nasko@ says that this extra temporary hole in swapped out processing might
be ok (he plans to yank out swapped out in the next few weeks).
> 

it's in since almost a year. I disagree on the label "temporary" :)

we should either fix this, or skip the test.

> 3. before proceeding, I probably should double-check why we end up sending a
message in swapped out state here.  For example - what exactly causes the test
to finish?  If test finishes because of didFinishLoad, then is
TestRunner::topLoadingFrame a main frame?  Is it ok if it is not a main frame? 
Maybe we should communicate with the browser via a randomly-picked local frame
(just as we do in https://codereview.chromium.org/1715573002/#msg9)?

that's an excellent question. 

in the process that has the main frame, the topLoadingFrame should always be the
main frame.