Issue 1158656 in chromium: Ship MBI Per-RenderProcessHost on Canary ☂️

[dom via monorail]

Status: Untriaged
Owner: d...@chromium.org
CC: kou...@chromium.org, ta...@google.com, har...@chromium.org, blink-iso...@chromium.org
Pri: 3
Type: Task

New issue 1158656 by d...@chromium.org: Ship MBI Per-RenderProcessHost on Canary ☂️
https://bugs.chromium.org/p/chromium/issues/detail?id=1158656

This is an umbrella bug that will track the task of getting MBI in shape to ship for a Canary experiment. So far

--
You received this message because:
1. You were specifically CC'd on the issue

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

Reply to this email to add a comment or make updates.

[dom via monorail]

Updates:
Cc: jste...@chromium.org
Components: Blink>Scheduling

Comment #1 on issue 1158656 by d...@chromium.org: Ship MBI Per-RenderProcessHost on Canary ☂️
https://bugs.chromium.org/p/chromium/issues/detail?id=1158656#c1

...So far many tests are failing on the waterful, so this is a tracking bug to fix them!

[dom via monorail]

Comment #2 on issue 1158656 by d...@chromium.org: Ship MBI Per-RenderProcessHost on Canary ☂️
https://bugs.chromium.org/p/chromium/issues/detail?id=1158656#c2

All failures are tracked in this spreadsheet: https://docs.google.com/spreadsheets/d/1El9BgizGS7AovtoCkcWBG6OcBFUfukCbtLok9bfls60/edit#gid=0

[bugdroid via monorail]

Comment #3 on issue 1158656 by bugdroid: Ship MBI Per-RenderProcessHost on Canary ☂️
https://bugs.chromium.org/p/chromium/issues/detail?id=1158656#c3

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

commit feb841d1b5f250187c0b06e92dee0a67ef49dfa2
Author: Dominic Farolino <d...@chromium.org>
Date: Tue Dec 15 16:00:35 2020

MBI: Fix kLegacy DCHECK in AgentSchedulingGroup for MockASG usages

Before this CL, MockAgentSchedulingGroup would always invoke the
AgentSchedulingGroup base class constructor which is now reserved for
the kLegacy MBI feature mode. This caused the ctor to DCHECK in tests
when MBI mode was enabled.

After this CL, MockAgentSchedulingGroup conditionally invokes the right
AgentSchedulingGroup constructor given the flag. Consequently,
MockRenderThread::GetIOTaskRunner() needs to be able to return a
non-null TaskRunner. This CL gives MockRenderThread the ability to set
a new IOTaskRunner manually so that it can be returned when necessary.

While we're here we also remove
PrintMockRenderThread_set_io_thread_for_testing() since the base class
MockRenderThread now handles that. We also entirely remove
ChromeMockRenderThread.

Bug: 1158656
Change-Id: I1ab1231048ecc1b798549634811706f86a7bafb9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2591929
Commit-Queue: Dominic Farolino <d...@chromium.org>
Reviewed-by: Kinuko Yasuda <kin...@chromium.org>
Reviewed-by: Kouhei Ueno <kou...@chromium.org>
Reviewed-by: Kentaro Hara <har...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837106}

[delete] https://crrev.com/23f02cf0fb4e2b188cd177c1733ae06933ed6909/chrome/renderer/chrome_mock_render_thread.cc
[delete] https://crrev.com/23f02cf0fb4e2b188cd177c1733ae06933ed6909/chrome/renderer/chrome_mock_render_thread.h
[modify] https://crrev.com/feb841d1b5f250187c0b06e92dee0a67ef49dfa2/components/printing/test/print_mock_render_thread.h
[modify] https://crrev.com/feb841d1b5f250187c0b06e92dee0a67ef49dfa2/content/public/test/mock_render_thread.cc
[modify] https://crrev.com/feb841d1b5f250187c0b06e92dee0a67ef49dfa2/chrome/test/BUILD.gn
[modify] https://crrev.com/feb841d1b5f250187c0b06e92dee0a67ef49dfa2/components/printing/test/print_mock_render_thread.cc
[modify] https://crrev.com/feb841d1b5f250187c0b06e92dee0a67ef49dfa2/chrome/renderer/BUILD.gn
[modify] https://crrev.com/feb841d1b5f250187c0b06e92dee0a67ef49dfa2/content/public/test/render_view_test.cc
[modify] https://crrev.com/feb841d1b5f250187c0b06e92dee0a67ef49dfa2/chrome/test/base/chrome_render_view_test.h
[modify] https://crrev.com/feb841d1b5f250187c0b06e92dee0a67ef49dfa2/content/public/test/mock_render_thread.h
[modify] https://crrev.com/feb841d1b5f250187c0b06e92dee0a67ef49dfa2/content/renderer/mock_agent_scheduling_group.cc
[modify] https://crrev.com/feb841d1b5f250187c0b06e92dee0a67ef49dfa2/chrome/test/base/chrome_render_view_test.cc
[modify] https://crrev.com/feb841d1b5f250187c0b06e92dee0a67ef49dfa2/content/renderer/mock_agent_scheduling_group.h

[bugdroid via monorail]

Comment #4 on issue 1158656 by bugdroid: Ship MBI Per-RenderProcessHost on Canary ☂️
https://bugs.chromium.org/p/chromium/issues/detail?id=1158656#c4

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src/+/a61d1187fb03b9d8fd511c32a80036837a286903

commit a61d1187fb03b9d8fd511c32a80036837a286903
Author: Dominic Farolino <d...@chromium.org>
Date: Thu Dec 17 07:06:32 2020

Introduce MockAgentSchedulingGroupHost for some browser unit tests

This CL introduces:
- AgentSchedulingGroupHostFactory
- MockAgentSchedulingGroupHostFactory
- MockAgentSchedulingGroupHost

... and uses them in the appropriate browser unit tests.

For a detailed description of the problem and solution see:
https://docs.google.com/document/d/1ZUNxjEr-TlVaDhJDk7mPtkBF_fK8OWcNQ0eJORSfiUU/edit#heading=h.csweqm2d9mc

Bug: 1158656
Change-Id: I1636cdb47d93a4883aae60d1ffbf0315c4ca8fb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2594317

Commit-Queue: Dominic Farolino <d...@chromium.org>
Reviewed-by: Kinuko Yasuda <kin...@chromium.org>

Reviewed-by: Kentaro Hara <har...@chromium.org>
Reviewed-by: Kouhei Ueno <kou...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837958}

[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/browser/renderer_host/agent_scheduling_group_host.cc
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/browser/renderer_host/agent_scheduling_group_host.h
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/public/test/test_renderer_host.cc
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/public/test/test_renderer_host.h
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/test/BUILD.gn
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/browser/BUILD.gn
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/test/test_render_frame_host.cc
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/browser/media/session/media_session_controller_unittest.cc
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/test/test_render_view_host_factory.h
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/test/test_render_view_host_factory.cc
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/browser/renderer_host/render_frame_host_impl.h
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/browser/renderer_host/render_frame_host_impl.cc
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/test/test_render_frame_host.h
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/public/test/test_content_client_initializer.cc
[modify] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/public/test/test_content_client_initializer.h
[add] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/test/mock_agent_scheduling_group_host.cc
[add] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/test/mock_agent_scheduling_group_host.h
[add] https://crrev.com/a61d1187fb03b9d8fd511c32a80036837a286903/content/browser/renderer_host/agent_scheduling_group_host_factory.h

[bugdroid via monorail]

Comment #5 on issue 1158656 by bugdroid: Ship MBI Per-RenderProcessHost on Canary ☂️
https://bugs.chromium.org/p/chromium/issues/detail?id=1158656#c5

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src/+/ab077b6c1f54959e1df9b49c8e54121867884061

commit ab077b6c1f54959e1df9b49c8e54121867884061
Author: Dominic Farolino <d...@chromium.org>
Date: Tue Feb 02 01:57:14 2021

Remove unused mojom::GuestView methods

Bug: 896679, 1158656

Change-Id: Ibe3bae422a67f678c2558e19c453b6ce428b813e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2650750
Commit-Queue: Dominic Farolino <d...@chromium.org>
Reviewed-by: Reilly Grant <rei...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Lucas Gadani <l...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#849350}

[modify] https://crrev.com/ab077b6c1f54959e1df9b49c8e54121867884061/extensions/browser/guest_view/extensions_guest_view_message_filter.h
[modify] https://crrev.com/ab077b6c1f54959e1df9b49c8e54121867884061/extensions/browser/guest_view/extensions_guest_view_message_filter.cc
[modify] https://crrev.com/ab077b6c1f54959e1df9b49c8e54121867884061/extensions/common/mojom/guest_view.mojom

[bugdroid via monorail]

Comment #6 on issue 1158656 by bugdroid: Ship MBI Per-RenderProcessHost on Canary ☂️
https://bugs.chromium.org/p/chromium/issues/detail?id=1158656#c6

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src/+/0c5c3f49f31a1f438d8515f9cf6967d10e49dea7

commit 0c5c3f49f31a1f438d8515f9cf6967d10e49dea7
Author: Dominic Farolino <d...@chromium.org>
Date: Wed Feb 17 17:40:15 2021

Migrate GinJavaBridgeMessageFilter to be 1:1 with AgentSchedulingGroup

Before this CL, GinJavaBridgeMessageFilter was 1:1 with
RenderProcessHost. GinJavaBridgeMessageFilter was installed on the
process-global IPC channel to handle the GinJavaBridgeHostMsg_*
messages sent from the renderer process.

In MBI mode, this caused a problem because:
- All renderer-to-browser messages bound for the
GinJavaBridgeMessageFilter are sent via the AgentSchedulingGroup IPC
channel that the sender is associated with (via RenderFrameImpl)
- Even if these messages were sent over the process-global IPC channel
and thus received correctly in the browser, the ordering between and
all navigation-associated messages would be broken, which breaks
fundamental guarantees required by the GinJavaBridgeMessage system

After this CL, GinJavaBridgeMessageFilter is 1:1 with
AgentSchedulingGroup(Host), and therefore there are as many filters
associated with a RenderProcessHost as there are
AgentSchedulingGroupHosts. Each GinJavaBridgeMessageFilter is installed
on the appropriate AgentSchedulingGroup's IPC channel, so that it can
correctly receive messages and process messages from the browser. This
fixes 372 failing Android tests in MBI mode. We're currently only
experimenting with the
kEnabledPerRenderProcessHost MBI mode [1], which creates a single
AgentSchedulingGroupHost per RenderProcessHost, and the
AgentSchedulingGroupHost has its own legacy IPC channel over which
messages that don't preserve ordering with the process-global IPC
channel go.

This CL has no effect when MBI mode is disabled, because it makes as
many GinJavaBridgeMessageFilters as there are AgentSchedulingGroupHosts,
and in MBI's kLegacy mode (disabled) there is only one
AgentSchedulingGroupHost per RenderProcessHost, and all communication
through it gets forwarded to the process-global IPC channel, (the
AgentSchedulingGroupHost does not have an IPC channel of its own in this
mode, to preserve the legacy behavior).

The renderer-side GinJavaBridgeObject can outlive the RenderFrame it is
associated with, however it still needs to send a trailing object
deletion message to the browser process on destruction. We can't send
this message over the AgentSchedulingGroup's IPC channel, since the
RenderFrameImpl we would grab the channel from may be gone. This message
must be sent over the process-global IPC channel. So this CL introduces
a new GinJavaBridgeObjectDeletionMessageFilter that only handles the
trailing object deletion message filter sent over the process-global IPC
channel. It is only ever used in MBI mode.

As a part of the Java bridge mojofication work that will be finished in
the future, we can connect these message filters to their Blink-side
mojo receiver objects, completing the mojofication of the Java bridge.
But until then, in order to get MBI mode functioning, the message
granularity changes introduced in this CL are necessary.

[1]: https://source.chromium.org/chromium/chromium/src/+/master:content/public/common/content_features.h;l=99;drc=2ac64302ae161cd6b5e4b1254497bdf5fd6d3415

NOPRESUBMIT=true

Bug: 1158656
Change-Id: I0c8e0f1d64f3602e9fe08e90725c4fb8810ec25c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2681232
Reviewed-by: Richard Coles <to...@chromium.org>
Reviewed-by: Nasko Oskov <na...@chromium.org>
Reviewed-by: Kouhei Ueno <kou...@chromium.org>
Reviewed-by: Tim Volodine <timvo...@chromium.org>
Commit-Queue: Dominic Farolino <d...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#854833}

[modify] https://crrev.com/0c5c3f49f31a1f438d8515f9cf6967d10e49dea7/content/browser/renderer_host/agent_scheduling_group_host.cc
[modify] https://crrev.com/0c5c3f49f31a1f438d8515f9cf6967d10e49dea7/content/browser/renderer_host/agent_scheduling_group_host.h
[modify] https://crrev.com/0c5c3f49f31a1f438d8515f9cf6967d10e49dea7/content/browser/android/java/gin_java_bridge_message_filter.cc
[modify] https://crrev.com/0c5c3f49f31a1f438d8515f9cf6967d10e49dea7/content/browser/android/java/gin_java_bridge_dispatcher_host.cc
[add] https://crrev.com/0c5c3f49f31a1f438d8515f9cf6967d10e49dea7/content/browser/android/java/gin_java_bridge_object_deletion_message_filter.h
[modify] https://crrev.com/0c5c3f49f31a1f438d8515f9cf6967d10e49dea7/content/browser/BUILD.gn
[modify] https://crrev.com/0c5c3f49f31a1f438d8515f9cf6967d10e49dea7/content/public/browser/browser_message_filter.h
[add] https://crrev.com/0c5c3f49f31a1f438d8515f9cf6967d10e49dea7/content/browser/android/java/gin_java_bridge_object_deletion_message_filter.cc
[modify] https://crrev.com/0c5c3f49f31a1f438d8515f9cf6967d10e49dea7/content/browser/android/java/gin_java_bridge_message_filter.h

[bugdroid via monorail]

Comment #7 on issue 1158656 by bugdroid: Ship MBI Per-RenderProcessHost on Canary ☂️
https://bugs.chromium.org/p/chromium/issues/detail?id=1158656#c7

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src/+/9a5323b5fe959e6fea68844b123680a9d1f1e04d

commit 9a5323b5fe959e6fea68844b123680a9d1f1e04d
Author: Dominic Farolino <d...@chromium.org>
Date: Wed Feb 17 17:45:47 2021

Add Multiple Blink Isolates fieldtrial testing entry

NOPRESUBMIT=true

Bug: 1158656
Change-Id: I9c979da2dd47cc8c26dc5cf7ae1284dcdbd98950
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2688841
Commit-Queue: Dominic Farolino <d...@chromium.org>
Reviewed-by: Kouhei Ueno <kou...@chromium.org>
Reviewed-by: Kentaro Hara <har...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#854835}

[modify] https://crrev.com/9a5323b5fe959e6fea68844b123680a9d1f1e04d/testing/variations/fieldtrial_testing_config.json

[shaseley via monorail]

Updates:
Status: Assigned

Comment #8 on issue 1158656 by shas...@chromium.org: Ship MBI Per-RenderProcessHost on Canary ☂️
https://bugs.chromium.org/p/chromium/issues/detail?id=1158656#c8

(No comment was entered for this change.)