New issue
Advanced search Search tips

Issue 595895 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 652513

Blocking:
issue 606594



Sign in to add a comment

Transition Layout Tests away from RVH/RV messages

Project Member Reported by lukasza@chromium.org, Mar 17 2016

Issue description

It is desirable to make RenderView a non-routable object:
1. This will make Site Isolation architecture better (or so I hear)
2. This will make Layout Tests work in --site-per-process mode without needing swapped-out (or equivalent) exceptions for layout test messages.

At this point it is not entirely clear whether the existing messages should become 1) process/control messages or 2) frame messages or 3) some mixture of both.  And in the long-term everything is supposed to be mojo-ified.

In any case - I am opening this bug to track CLs and discussions related to tweaking how browser and renderer-side of layout tests communicate.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa

commit 23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa
Author: lukasza <lukasza@chromium.org>
Date: Mon Apr 18 19:05:23 2016

Replicate LayoutTestRuntimeFlags across secondary window renderers.

Before this CL, LayoutTestRuntimeFlags would only be replicated across
renderers with frames belonging to the main test window.  After this CL,
LayoutTestRuntimeFlags are replicated across all renderers (including
renderers for secondary window frames).

This CL also switches to use control/process IPC messages (rather than
RenderView messages) for propagating changes to LayoutTestRuntimeFlags.
This is desirable, because LayoutTestRuntimeFlags are a global object,
not really associated with a specific RenderView and/or BlinkTestRunner.
This is also nice from code clean-up perspective - this approach doesn't
need a swapped-out exception in LayoutTestContentClient and other
baroque pieces of code in BlinkTestRunner and BlinkTestController.
Regrettably, even after this CL, TestRunner continues to notify
the world about LayoutTestRuntimeFlags changes via an arbitrarily
picked BlinkTestRunner / WebTestDelegate.

After this CL (and after the earlier https://crrev.com/1875303002)
flags controlling WebContentSettingsClient are replicated to secondary
renderers.  This means that a few extra mixed content layout tests
are passing in --site-per-process mode and allows to remove test
expectations for this mode.

BUG= 587175 ,  582525 , 595895

Review URL: https://codereview.chromium.org/1878863002

Cr-Commit-Position: refs/heads/master@{#387977}

[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/components/test_runner/test_runner.cc
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/browser/layout_test/blink_test_controller.cc
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/browser/layout_test/blink_test_controller.h
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/browser/layout_test/layout_test_message_filter.cc
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/browser/layout_test/layout_test_message_filter.h
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/browser/layout_test/secondary_test_window_observer.cc
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/common/layout_test/layout_test_content_client.cc
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/common/layout_test/layout_test_messages.h
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/common/shell_messages.h
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/renderer/layout_test/blink_test_runner.cc
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/renderer/layout_test/layout_test_render_frame_observer.cc
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/renderer/layout_test/layout_test_render_frame_observer.h
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/renderer/layout_test/layout_test_render_thread_observer.cc
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/content/shell/renderer/layout_test/layout_test_render_thread_observer.h
[modify] https://crrev.com/23c3a85fc9c33eecaec87d9cc85195dcdfb4b3aa/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Project Member

Comment 2 by bugdroid1@chromium.org, Apr 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/042327073131b53a3a3f7d06c7e56d8eadaed9d2

commit 042327073131b53a3a3f7d06c7e56d8eadaed9d2
Author: lukasza <lukasza@chromium.org>
Date: Thu Apr 28 16:01:05 2016

Handle layout tests that finish in OOPIFs.

When a layout test finishes, it calls BlinkTestRunner's TestFinished
method.  If the main test frame resides in another renderer process,
then BlinkTestRunner sends a message to the browser process to let it
know that the test has finished (and to let it forward this notification
to the main test frame).

Before this CL, when a layout test would finish in an OOPIF, then
RenderView associated with BlinkTestRunner would be swapped-out, and
this meant that the notification message would be dropped and the test
would time out.  After this CL, the notification message is sent
as a process/control message - this means that it reaches the browser
regardless of the swapped-out state of RenderViews.  This fixes 25+
timeouts in --site-per-process mode.

This CL depends on many earlier CLs that had to ensure that tests finish
at the right moment - this requires proper tracking of top loading frame
across all renderers (https://crrev.com/1908233002), as well as properly
identifying the main test window (https://crrev.com/1896623002).

This CL also highlights that the main frame receives notification that
the test has finished in another renderer, *not* that another renderer
called testRunner.notifyDone (i.e. BlinkTestRunner::TestFinished can
also be called when test finishes due to the top loading frame finishing
loading).  This means that the main frame doesn't execute
testRunner.notifyDone itself, but directly calls
BlinkTestRunner::TestFinished (calling notifyDone would not have worked
due to earlier resetting of wait_until_done LayoutTestRuntimeFlag).

Finally this CL, tweaks redirect-methods.html test to ensure that it
consistently allows WebFrameClient callbacks to be called (without
--site-per-process the delay would have occured because
top-loading-frame would still be tracked and would have prevented
synchronous finishing of the test when testRunner.notifyDone is called).

BUG=595895

Review-Url: https://codereview.chromium.org/1922653003
Cr-Commit-Position: refs/heads/master@{#390398}

[modify] https://crrev.com/042327073131b53a3a3f7d06c7e56d8eadaed9d2/content/shell/browser/layout_test/blink_test_controller.cc
[modify] https://crrev.com/042327073131b53a3a3f7d06c7e56d8eadaed9d2/content/shell/browser/layout_test/layout_test_message_filter.cc
[modify] https://crrev.com/042327073131b53a3a3f7d06c7e56d8eadaed9d2/content/shell/browser/layout_test/layout_test_message_filter.h
[modify] https://crrev.com/042327073131b53a3a3f7d06c7e56d8eadaed9d2/content/shell/browser/layout_test/secondary_test_window_observer.cc
[modify] https://crrev.com/042327073131b53a3a3f7d06c7e56d8eadaed9d2/content/shell/browser/layout_test/secondary_test_window_observer.h
[modify] https://crrev.com/042327073131b53a3a3f7d06c7e56d8eadaed9d2/content/shell/common/layout_test/layout_test_messages.h
[modify] https://crrev.com/042327073131b53a3a3f7d06c7e56d8eadaed9d2/content/shell/common/shell_messages.h
[modify] https://crrev.com/042327073131b53a3a3f7d06c7e56d8eadaed9d2/content/shell/renderer/layout_test/blink_test_runner.cc
[modify] https://crrev.com/042327073131b53a3a3f7d06c7e56d8eadaed9d2/content/shell/renderer/layout_test/blink_test_runner.h
[modify] https://crrev.com/042327073131b53a3a3f7d06c7e56d8eadaed9d2/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/042327073131b53a3a3f7d06c7e56d8eadaed9d2/third_party/WebKit/LayoutTests/http/tests/loading/redirect-methods.html

Comment 3 by sshru...@google.com, May 18 2016

Labels: Test-Layout

Comment 4 by sshru...@google.com, May 18 2016

Components: -Blink>LayoutTests
Deprecating component:Blink>LayoutTests, to use label Test=Layout instead.
Status: Assigned (was: Started)
Blocking: 606594
Blockedon: 652513
Blocking: -477150
Labels: -Pri-2 Pri-3
This bug is mostly about a refactoring work and doesn't really block issue 477150 (which is about making layout tests work and pass with site-per-process).

Sign in to add a comment