1741834 - MessageHandler: support transactions when using sessionData

Status: RESOLVED FIXED

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

Description

[Julian Descottes [:jdescottes]]

This is a followup to the initial MessageHandler SessionData implementation added in Bug 1713443.

Once we support specific contexts for APIs such as session.subscribe, the SessionData will need to reflect for which contexts a given event is currently subscribed to. Given the specification, this might be difficult to implement efficiently with the current SessionData API.

For session.subscribe, if you:

• subscribe to an event for a specific context
• subscribe to the same event globally
• unsubscribe globally

You should be fully unsubscribed from the event, meaning that the initial "context-specific" subscription was overridden by the global one.

If we use the current SessionData API, this would look as follows:

// !! THIS IS PSEUDOCODE !!

// 1. subscribe to an event for a specific context
addSessionData("log", "events", { type: "top-browsing-context", id: ${idForTopLevelBC} }, ["entryAdded"]);

// 2. subscribe to the same event globally
// 2.a. First need to remove all the specific contexts (assuming we'd have a way to list them)
removeSessionData("log", "events", { type: "top-browsing-context", id: ${idForTopLevelBC} }, ["entryAdded"]);
// 2.b. then add a global subscription
addSessionData("log", "events", { type: "all" }, ["entryAdded"]);

// 3. unsubscribe globally
removeSessionData("log", "events", { type: "all" }, ["entryAdded"]);

The issue with this is that the intermediary step 2.a. will lead to unsubscribe from entryAdded on the specific contexts which already had created a listener. And right after, we would re-subscribe for step 2.b. Meaning we could miss events in that window.

So instead we want step 2 to be atomic. Contexts which didn't previously subscribe to the event should create the listeners, and contexts which already had the listeners should not do anything. But we still want to reflect in SessionData the fact the only the "all" context remains, to be consistent with what the spec expects.

A suggestion to handle this is to allow transactions when interacting with SessionData, where you can call addSessionData/removeSessionData as many times as needed, which would then be committed as a single transaction.

This would avoid pushing more logic to the SessionData layer, because the way we want to handle contexts for session.subscribe might be different from what other Commands will need.

Comment 1

[Julian Descottes [:jdescottes]]

We don't need this to be in milestone 2. We will allow for to filter subscribe calls on specific browsing contexts in milestone 2, but we can limit this to new subscribe calls and say that this doesn't apply to new contexts yet.

Note that supporting transactions would be easier if we would simply broadcast the relevant sessionData to existing contexts instead of broadcasting a "diff". Right now the diff is very easy to compute because we broadcast for each individual add/removeSessionData call. But if we allowed transaction, computing the diff would be harder. If instead we broadcast the whole session data (limited to a given module/category to avoid broadcasting too much), then individual modules would be responsible for computing the diff which is relevant to them.

Comment 2

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

This is related to the full implementation for subscribe and unsubscribe (see bug 1723102).

Comment 3

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1741834 - [messagehandler] Support transactions when using sessionData.

Comment 5

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1741834 - [wdspec] Add test for browsingContext.contextCreated for a specific context.

Depends on D163957

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc541786a015
[messagehandler] Support transactions when using sessionData. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/955a99216cfa
[wdspec] Add test for browsingContext.contextCreated for a specific context. r=webdriver-reviewers,jdescottes

Comment 7

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/37626 for changes under testing/web-platform/tests

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/bc541786a015
https://hg.mozilla.org/mozilla-central/rev/955a99216cfa

Comment 9

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot