Closed Bug 1648149 Opened 7 months ago Closed 5 months ago

Migrate GeckoViewContentChild.js to actor

Categories

(GeckoView :: General, enhancement, P1)

Unspecified
All
enhancement

Tracking

(firefox81 fixed)

RESOLVED FIXED
81 Branch
Tracking Status
firefox81 --- fixed

People

(Reporter: agi, Assigned: agi)

References

(Blocks 1 open bug)

Details

(Whiteboard: [geckoview:m80] [geckoview:m81])

Attachments

(11 files, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

Whiteboard: [geckoview:m80]
Priority: -- → P2
Assignee: nobody → agi

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.

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.

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

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.

Whiteboard: [geckoview:m80] → [geckoview:m80] [geckoview:m81]
Attachment #9164478 - Attachment description: Bug 1648149 - Move GeckoViewCotent to Actor. → Bug 1648149 - Move GeckoViewContent to Actor.

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.

Attachment #9164477 - Attachment is obsolete: true
Pushed by asferro@mozilla.com:
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
Regressions: 1659012
Pushed by asferro@mozilla.com:
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
Flags: needinfo?(agi)

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

Keywords: leave-open

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.

Pushed by asferro@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Regressions: 1667497
You need to log in before you can comment on or make changes to this bug.