1713443 - MessageHandler: Support shared global session data that supports BiDi and CDP

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, task, P2)

Description

[Julian Descottes [:jdescottes]]

The CommandsHandler architecture should be able to broadcast and persist session data.
For instance, for a Command such as Session::subscribe, the event which is being subscribed to should be broadcasted to all relevant contexts (eg reusing the broadcasting support from Bug 1713441) and should also be made available to new contexts spawning during the session.

This is what is currently done in DevTools using a mix of broadcasting + sharedData.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please note that we have multiple notions of session in Bidi. First it's the WebDriver Session, which holds session global state, and then it's the session module. The latter should only contain commands that interact with data as stored in the WebDriver Session. As such I think we should make this bug explicit so that it is about sharing the global session data from the WebDriver session.

Comment 2

[Julian Descottes [:jdescottes]]

Right, I agree it's not linked to the Session Module. At the same time, CommandsHandler should not be protocol specific so the notion of Session here should maybe not be named after webdriver.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh, for sure! I missed that part when renaming the summary. So lets update again - hopefully the last time for now.

Comment 4

[Julian Descottes [:jdescottes]]

Uploaded a very simple prototype that adds some support for new contexts: https://phabricator.services.mozilla.com/D117269

It needs to be refined a lot:

• all the logic is for now in a Session module, where it should be outside of a module I think, in a dedicated "SessionDataHandler" (?) created for each CommandsHandler
• the API for subscribing too events exposes too much to the consumer. Need to specify the module implementing the event as well as the name of the command that handles the event. we should simply hide that.
• the stored data is currently assumed to just be a list of events. However in the long run we should support breakpoints, configuration etc...
• the data is only persisted in shared data and applied via a broadcast, there is no local copy of shared data yet. We should decide where this copy should be handled. In theory we only need one per technical layer, but it might just be easier to maintain one per commands handler node
• there is still no "filtering" per tab or browserId, so we are missing all the logic which should decide if a given BrowsingContext actually needs to spawn a CommandsHandler. Right now, as long as there's anything in shareddata, we'll create a CommandsHandler in any new context (but that filtering limitation applies to the whole prototype at the moment)

On the topic of "which data should be stored", as mentioned above in the patch it's only a list of events. I think we should just make it generic like devtools and support any kind of data. It will be the job of the SessionDataHandler to understand how to process each data type (most likely, translate it into a command).

But for a moment I imagined that instead of storing data, we could actually store a list of commands that need to run. When a new context pops up, it would simply get this list of commands to run, and would apply them one by one. I think it would work technically, but it might be hard to reason about that, especially when we start thinking about "updating" some of the data. Eg when you unsubscribe from an event, it would mean removing the corresponding "subscribe" commands from the list. Same issue when updating a list of breakpoints for instance.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Part of our goals for milestone 2.

Comment 6

[Julian Descottes [:jdescottes]]

This bug will be about designing a layer that can be used to store and share MessageHandler data across various processes. This data should be partitioned by session, and each "session data" should hold the configuration of a given session (eg which events are we currently listening to, for which "browsing contexts" etc.). APIs should be provided to set and get the data from any process. The data should be transparently synchronized across all processes, so that all MessageHandler instances can access the same data (this will mostly be handled by relying on sharedData)

Before uploading patches, I want to do a quick summary of how WatchedData is used in DevTools, because it is very similar to the session data we want to implement here.

DevTools WatchedData

In DevTools, we use the concept of WatchedData to store all the information related to a DevTools "session". However the notion of session is not very explicit in DevTools, so we named this data after the "WatcherActor" which is a very important DevTools actor, providing access to DevTools targets, resources etc... and has the same lifecycle as the UI/client (eg a toolbox). The naming will progressively change to use session instead of Watcher (Bug 1731967), because the "watcher" name is not self explanatory unless you know the details of this WatcherActor.

DevTools relies on sharedData, and stores all the WatchedData for all WatcherActors under a single key DevTools:watchedPerWatcher. For this key, we store a map between a WatcherActor id and a "watchedData" object (plain js object) which contains all the data relevant for a specific watcher actor.

Each watchedData object will contain a few mandatory properties:

• watcherTraits: this is a map of flags which describe if certain features are enabled or not for the watcher. Features could be globally disabled or simply be disabled due to the nature of the watcher
• isServerTargetSwitchingEnabled: boolean, similar to a watcherTrait, just exposed differently (probably no good reason for this)
• connectionPrefix: string, this is pretty specific to DevTools, how DevTools actors are created and how RDP messages are routed. It has a similar specificity to the contextId we have on MessageHandler instances.
• browserId: string, this is very important and defines the scope of the devtools "session". If a toolbox is debugging a tab, this will be set to the browserId of this tab. It will then be used to know whether a target should be created when a new window global is created (ie "is there a watchedData with a browserId which is the parent of this window global"). This is going to be one key difference between the DevTools watchedData and MessageHandler's data. With a MessageHandler session, you can simultaneously listen to one event globally and to another event just for a tab. So we won't have a notion of global scope like this.

Then on top of this mandatory data, watchedData will contain "data entries" which contain the configuration of the session. Some examples:

• list of resource types to emit (NETWORK_EVENT, CONSOLE_MESSAGE, ...), similar to MessageHandler events
• list of target types to emit (FRAME, WORKER, ...), also similar to resources (& events) but for legacy reasons Targets receive a very special treatment in DevTools
• list of breakpoints
• list of BrowsingContext configuration flags
• ...

As you can see, all those are "lists". Data entries started as simple arrays and all the APIs were built around adding or removing items from those. This worked just fine when we only had to store resource or target types. Now that we are storing more complex objects (eg breakpoints) we have some logic to generate a unique key for each data-entry we need to store. So in general you can consider each data entry as a custom "set", with unique entries.

In practice, when you look at a watchedData object, it could look like:

{
  "browserId": 300,
  "connectionPrefix": "server0.conn8.",
  "watcherTraits": {
    "frame": true,
    "saveRequestAndResponseBodies": true,
    "network-persist": true,
  },
  "isServerTargetSwitchingEnabled": true,
  "breakpoints": [
    {
      "location": { /* location object */ },
      "options": {}
    },
    {
      "location": { /* location object */ },
      "options": {}
    },
  ],
  "xhr-breakpoints": [],
  "event-breakpoints": [],
  "resources": [
    "document-event",
    "console-message",
    "source"
  ],
  "target-configuration": [
    {
      "key": "cacheDisabled",
      "value": true
    }
  ],
  "targets": [
    "frame",
    "worker"
  ]
}

In terms of APIs, DevTools has APIs operating at different levels. There is a first set of APIs to get or create the watchedData or a given watcher actor id. This is typically used only by the framework to make sure we dynamically create the watchedData entry for a given watcher actor if it doesn't exist.

Then there is a second class of APIs available on the WatcherActor in order to update data entries in the watchedData for this WatcherActor: addWatcherDataEntry/removeWatcherDataEntry. Those will typically be used by other actors. For instance the actor responsible for breakpoints will use those APIs when the client adds or removes a breakpoint.

It's interesting to note that we only update watchedData from the parent process (I don't know if shared data can actually be updated from other processes?).

A final note about what happens when you call addWatcherDataEntry and how this is actively propagated to existing Targets in all processes. Calling addWatcherDataEntry will first update sharedData with the new information. But this is actually mostly useful when a new target is created, so that it can have access to the latest snapshot of watchedData. For existing targets, it is very important for DevTools to make sure that the data update has been processed by all existing targets. So on top of updating sharedData, we also send the updated data entries to all existing targets, and the Target actors have custom logic to handle new data entries.

(It will be interesting to see how this translates to MessageHandler. Typically when you subscribe for log.entryAdded, how should we both store the information for upcoming MessageHandlers and send the information to existing one. Should we still send a subscribe command internally? Should we use something custom dedicated to applying session data... )

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks for the write-up Julian! It's very informative to hear what happens in DevTools. As it sounds like for BiDi we might want to have a dedicated session / meeting to chat about a proper implementation?

Regarding your last paragraph I think that using the subscribe method here would make sense so that we do not have several different implementations of the same code.

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #7)

Thanks for the write-up Julian! It's very informative to hear what happens in DevTools. As it sounds like for BiDi we might want to have a dedicated session / meeting to chat about a proper implementation?

Will schedule something once I have a first WIP

Regarding your last paragraph I think that using the subscribe method here would make sense so that we do not have several different implementations of the same code.

I think I agree, we'll see how the implementation goes.

Another interesting thing to note about the fact that DevTools "only updates watchedData from the parent process". As a consequence the wrapper WatcherRegistry.jsm is only used from the parent process, and other spots which need to access the watched data in content processes duplicate some of the logic. Of course in content processes you reach the shared data differently (Services.cpmm vs Services.ppmm) but still it would be great to have a single entry point, or at least share some logic between parent & content process implementations.

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1713443 - [remote] Support shared global session data that supports BiDi and CDP

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1713443 - [remote] Add xpcshell test for basic SessionData APIs

Depends on D127698

We cannot test the cross process data sharing with xpcshell tests, but we can use it to check the basic API

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e2ff229163c
[remote] Support shared global session data that supports BiDi and CDP r=webdriver-reviewers,whimboo,ochameau

Comment 12

[Cristian Tuns]

Backed out for causing mochitest(SessionDataReader.jsm) and remote(browser_session_data.js) failures

• Backout link
• Push with failures
• Mochitest failure Log
• Remote failure Log

Comment 13

[Julian Descottes [:jdescottes]]

Looks like the rebase messed up the part of the code that should be in RootMessageHandler.jsm. The whole stack works, but first patch has an issue. Will fix.

Comment 14

[Julian Descottes [:jdescottes]]

Hmmpf it's worst than that. The RootMessageHandler part of the patch has always been in the second changeset. I will move only the necessary bits for the first patch to pass the test (ie create & destroy the sessiondata)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/162323da0ea5
[remote] Support shared global session data that supports BiDi and CDP r=webdriver-reviewers,whimboo,ochameau

Comment 16

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/162323da0ea5