1648149 - Migrate GeckoViewContentChild.js to actor

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[[ex-Mozilla] Agi Sferro | :agi]

As part of the Fission work we need to migrate all our framescripts to Actors. This tracks the work needed to move GeckoViewContentChild.js to a child actor.

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - Refactor Extension delegates handlers.

This patch introduced some improvements to the WebExtension-related Delegate
implementation.

The current WebExtension API lets apps register delegates only after the
WebExtension has been installed. A common pattern is the following

webExtensionController.install(extensionUri).accept(extension -> {
    extension.setTabDelegate(...);
    extension.setMessageDelegate(...);
    // etc
});

A similar pattern exists for Ports, that allow bidirectional communication
between the app and an extension.

public void onConnect(WebExtension.Port port) {
    port.setDelegate(...);
}

The problem with the above examples is that some delegate calls happen before a
delegate is registered. E.g. for extensions, any delegate call that happens
during the background page initialization will happen before the extension is
installed, and thus would be missed from the delegate.

To obviate these problems, we currently store some messages in a map and
release them only after a delegate has been attached. This, however, is done
ad-hoc for some delegate calls, causing some code duplication and subtle bugs
in those delegate calls that are not collected in a pending map.

Additionally, delegates can be keyed off of different objects. The
ActionDelegate, TabDelegate are keyed off the WebExtension,
MessageDelegate is keyed off the couple (WebExtension, NativePort),
PortDelegate off of Port and PromptDelegate and DebuggerDelegate are
singletons. Upcoming DownloadDelegate will be keyed off of Download.

There is some logic that’s error prone with regards to checking which delegate
should receive a message, whether the delegate is registered and with releasing
pending messages when a delegate is attached that is duplicated for each
WebExtension delegate but could be consolidated in one, type agnostic place

• Offer an abstraction that allows GeckoView engineers to implement new
  delegates in WebExtension that provides common functionality like storing
  pending messages, setting and retrieving delegates without writing extra
  code.

• The above abstraction should be flexible enough to handle existing and
  upcoming use cases.

We will provide an interface Handler<Key, Delegate> that can be implemented
to handle delegate calls from Gecko. The type Key will determine the key type
which is used to key off delegates, the Delegate class will be the interface
that is implemented by the app to handle the delegated methods.

A concrete class HandlerController will be responsible to receive messages
from Gecko, retrieve the correct delegate from a message optionally store the
message in a queue if the delegate is not present and invoke the Handler when
the delegate is attached.

The existing Listener controller will be responsible for instantiating the
handlers and controllers for all messages.

We will provide convenience base classes for common use cases:

interface Handler<Key, Delegate>
   |
   +---- DelegateMapHandler<Key, Delegate>
   |        |
   |        +---- ExtensionHandler<Delegate>
   |
   +---- SingletonHandler<Delegate>

Where DelegateMapHandler is a common base class for all handlers that store
delegates in a Map<Key, Delegate>, ExtensionHandler<Delegate> is a handler
that is keyed off WebExtension, SingletonHandler<Delegate> is for the
degenerate case where there is only one handler per extension / runtime.

Abstracting code could make it hard to follow, errors and stacktraces could be
harder to debug. We need to listen to all messages at all times because we
maintain a list of pending messages.

Some Apps might be relying on bugs that the current implementation has, our
tests needed to be adjusted for this very reason.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - Split action test extension in browser and page.

Now that messaging is much more reliable, we need to split the tests into
browser and page action tests otherwise the Page action tests will receive
messages from a browser action and vice versa.

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - Don't send delegate attached message.

Since now we queue delegate events for all messages, there's no need to send a
default action when attaching the delegate anymore.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - temp commit - copy GeckoViewContentChild to actor.

Comment 5

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - Move GeckoViewContent to Actor.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - Add named EventDispatcher's.

This patch adds a "named" EventDispatcher that can be accessed on both java and
javascript by calling

EventDispatcher.byName(...)

A named EventDispatcher will receive messages only from it's counterpart with
the same name and can be used to implement bidirectional communication like
WebExtension ports.

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - Use named EventDispatcher for Port.

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - Removed unused background and gecko listeners.

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - Use runnable for queueNative.

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - Split GeckoViewContent in enable and init phases.

This commit splits the GeckoViewContent actor in two parts:

• GeckoViewContent proper, which runs unconditionally and handles code that
  needs to run regardless of whether we have a delegate installed or not.

• ContentDelegate which runs only when a delegate is first installed.

This emulates the previous paradigm of installing some listeners only when the
delegate is installed.

I discussed it briefly with :nika and she thinks that splitting modules in two
should not affect performance in a measurable manner.

Note that actors cannot be registered per-window, so we will get messages from
all windows as long as one content delegate is registered.

Comment 11

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - Ignore debug and warn in eslint.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f74337b160e
Removed unused background and gecko listeners. r=owlish
https://hg.mozilla.org/integration/autoland/rev/700f8e53ed10
Add named EventDispatcher's. r=snorp
https://hg.mozilla.org/integration/autoland/rev/a41e4158e960
Use named EventDispatcher for Port. r=snorp
https://hg.mozilla.org/integration/autoland/rev/938bf3fab3ee
Move GeckoViewContent to Actor. r=snorp
https://hg.mozilla.org/integration/autoland/rev/470b0fa401ab
Split GeckoViewContent in enable and init phases. r=snorp

Comment 13

[Alexandru Michis [:malexandru]]

Backed out 5 changesets (Bug 1648149) for causing android wpt failures in closed-attribute.window.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/4fb5e934d1b9bb7433e046525709bc4538d61c20

Push with failures:

• tier 1: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=android%2C7.0%2Cx86-64%2Cdebug%2Cweb%2Cplatform%2Ctests%2Ctest-android-em-7.0-x86_64%2Fdebug-geckoview-web-platform-tests-e10s%2Cwpt7&tochange=8b917683232d97369e1183ac0386b4118ed25103&fromchange=17df14f0b129dc1135027f1c1113d86436b5cf9c&selectedTaskRun=fWNpG4XfSkukG9fq-s4VAw.0
• tier 2: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=Android%2C7.0%2Cx86-64%2Copt%2CWeb%2Cplatform%2Ctests%2Ctest-android-em-7.0-x86_64%2Fopt-geckoview-web-platform-tests-e10s%2Cwpt7&tochange=470b0fa401ab6a51038dc9131433e4dadc0601a9&fromchange=638c4f49d052a9acb958926021c79783bd692554&selectedTaskRun=PUre4xeaTJOGCwG4ZfJ7PQ.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312981240&repo=autoland&lineNumber=2434

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12adc788cf54
Removed unused background and gecko listeners. r=owlish
https://hg.mozilla.org/integration/autoland/rev/c5b7a7de4406
Add named EventDispatcher's. r=snorp
https://hg.mozilla.org/integration/autoland/rev/f3ce5393bc26
Use named EventDispatcher for Port. r=snorp
https://hg.mozilla.org/integration/autoland/rev/0b05c9e5ae63
Move GeckoViewContent to Actor. r=snorp
https://hg.mozilla.org/integration/autoland/rev/8b7b5ede6ee6
Split GeckoViewContent in enable and init phases. r=snorp,esawin

Comment 15

[[ex-Mozilla] Agi Sferro | :agi]

Thanks.

Comment 16

[[ex-Mozilla] Agi Sferro | :agi]

I have a quick follow-up I want to land.

Comment 17

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1648149 - Check that a module does not define onInit and onEnable.

After moving to actors it will be common to define both a onInit phase and a
onEnable phase for actors. We don't handle having both phases contain a
main-process script so we should check that.

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/12adc788cf54
https://hg.mozilla.org/mozilla-central/rev/c5b7a7de4406
https://hg.mozilla.org/mozilla-central/rev/f3ce5393bc26
https://hg.mozilla.org/mozilla-central/rev/0b05c9e5ae63
https://hg.mozilla.org/mozilla-central/rev/8b7b5ede6ee6

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4a7195dc0a1
Ignore debug and warn in eslint. r=snorp
https://hg.mozilla.org/integration/autoland/rev/003d649a1d0e
Check that a module does not define onInit and onEnable. r=snorp

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e4a7195dc0a1
https://hg.mozilla.org/mozilla-central/rev/003d649a1d0e