1694389 - Implement browsingContext.contextCreated event

Status: RESOLVED FIXED

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

Description

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

This event is needed for clients to handle open and newly created browsing contexts.

Comment 1

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

To support registering of log events for newly created browsing contexts, we have to get this feature implemented for milestone 2.

Comment 2

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

Actually this event doesn't need any serialization of data.

Comment 3

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

I had a quick peak into trying to get this implemented and there are a couple of questions that came up.

1. Where do we have to emit this event from? Events can be subscribed globally or for a top-level browsing context (not supported yet). As such we might want to have listeners attached only for the top-level one and not for all the browsing contexts in the current tab? If we want to do the latter the subscription state has to be saved on each window global message handler instance.

2. Which event do we have to listen for? Is DOMWindowCreated too late and webnavigation-create better? When using DOMWindowCreated we could actually emit the event when the appropriate window global message handler gets it's shared state set.

3. What about caching? As given by James this will not be necessary and we should emit all the events again after a re-subscription.

I'll discuss these topics with Julian today.

Comment 4

[Julian Descottes [:jdescottes]]

Quick notes:
Looking at the spec, the payload is supposed to contain the URL, so whatever event we use, we need to use one where the URL is ready.

Also we need to return the children info (for a given depth). If we emit this event from the content process, assembling the BrowsingContextInfo seems maybe a bit more complicated. The windowglobal MessageHandler would probably not be able to gather the information about its children, potentially living in other processes? So:

• either the windowglobal MessageHandler needs a way to call commands on root modules. That's currently not allowed, you can only go from ROOT to WINDOWGLOBAL, not the other way around.
• or the windowglobal MessageHandler emits the event without the children payload, and the root MessageHandler will have to populate this information. To do that either we can introduce an internal event, which would be listened to by a root module "internalContextCreated". This event's payload does not contain the children info. When the root module receives this event, it fetches the information about the children and emits the "contextCreated" event. If we don't want to split that in 2 events, we could allow the root module to "intercept" the event when it's bubbling back. Right now there is no way in the framework to intercept a response or an event. The idea was rather to use separate event and method names.

Or we do everything from the parent process?

Comment 5

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

(In reply to Julian Descottes [:jdescottes] from comment #4)

Looking at the spec, the payload is supposed to contain the URL, so whatever event we use, we need to use one where the URL is ready.

Right. And actually it doesn't seem to be set for both the above mentioned events. Maybe we will have to make use of the document-element-inserted observer notification - similar to what I have used when making CDP Fission compatible.

Also we need to return the children info (for a given depth). If we emit this event from the content process, assembling the BrowsingContextInfo seems maybe a bit more complicated. The windowglobal MessageHandler would probably not be able to gather the information about its children, potentially living in other processes? So:

Yes, and we want to emit this event only once. Consider when multiple nested frames are getting added. The event should be sent out only once.

• either the windowglobal MessageHandler needs a way to call commands on root modules. That's currently not allowed, you can only go from ROOT to WINDOWGLOBAL, not the other way around.

I wouldn't consider doing that. If we can pass all the information back to the parent process it should assemble all necessary information. If necessary it can also call itself into the window global commands.

• or the windowglobal MessageHandler emits the event without the children payload, and the root MessageHandler will have to populate this information. To do that either we can introduce an internal event, which would be listened to by a root module "internalContextCreated". This event's payload does not contain the children info. When the root module receives this event, it fetches the information about the children and emits the "contextCreated" event. If we don't want to split that in 2 events, we could allow the root module to "intercept" the event when it's bubbling back. Right now there is no way in the framework to intercept a response or an event. The idea was rather to use separate event and method names.

This way we would still get multiple internal events from each of the created browsing contexts within the current browsing context tree. So not if that would be optimal. We would have to force the event registration to only the top-level browsing context.

Or we do everything from the parent process?

It would help to not have extra registrations for the event / observer, and to only sent it out once. But then we would have to store the subscription information for individual browsing contexts at one place, which we should hopefully have once implemented.

Comment 6

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

Quick wrap-up from yesterday when talking to James:

• Whenever browsing contexts gets created a dedicated event for each of them needs to get sent out. Hereby the maxDepth will be 0 so that no child browsing contexts will be included in the BrowsingContextInfo.

• It would be good to get the event sent out when the URI of the newly created browsing context isn't the initial (temporary) about:blank page. Note initial here... opening a new tab/window which has the about:blank page as target needs to work.

What's the status of the investigation so far:

• We should make use of the browsing-context-attached observer. That one works in the parent and content process, but is emitted quite early while the new browsing context doesn't have an URI assigned at all. As such we would have to delay emitting the event.

• Note that browsing-context-attached is emitted before the DOMWindowCreated event and as such before our JSWindowActor pair gets created. Also the actor options cannot be updated to also listen for this observer to when the actor instances need to be created because only observers with a subject of type window proxy are allowed. As such I think it's not useful to concentrate more on working with this observer in the content process.

• If browsing-context-attached is registered in the window global message handler each new browsing context will cause multiple notifications depending on how many tabs and frames exist. Filtering could only be done by the top-level browsing context but still not by browserId because the message handler isn't initialized yet (see last point).

• To wait for a document being attached the document-element-inserted observer could be used. But the problem here is that it only works for documents from the same process. As such it cannot be simply used in our root message handler.

Possible options for the implementation:

• Use the browsing-context-attached observer in the root message handler if browsingContext.created is subscribed globally or for the browsing context tree in the current tab.
  ** Use maybe a webProgress listener (not tested yet) to further delay the event until the initial document has been replaced by the real document.
  ** Use the document-element-inserted observer in the window global message handler and emit an internal event to the root actor that indicates when the initial document has been replaced. The root actor can then check the cached not yet initialized browsing context references for the event coming from the actor and then send out the event.

Comment 7

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

After some discussion on Matrix around the unclarity if browsing context replacements due to coop/coep navigations should also be counted in, we agreed on to only emit these events for new tabs and iframes. This will keep the behavior close to what CDP does and what the proof of concept from Google includes.

As such I filed bug 1746332 to request a replace flag for the browsing-context-attached observer. It will help to easily filter out these unwanted notifications.

Nevertheless we would still have to delay sending out the event until a proper URI has been set. As such we could let the Window Global Message handler send an internal event to the Root Message Handler when the DOMWindowCreated event has been received and the handler be created. By that time we have a valid active document and we do not ignore a temporary initial about:blank.

Comment 8

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

And what I forgot to say in my last comment is that webProgress is only supported for top-level browsing contexts, and do not send out notifications for sub frames. So this proposed solution from comment 6 won't work.

Comment 9

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

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

And what I forgot to say in my last comment is that webProgress is only supported for top-level browsing contexts, and do not send out notifications for sub frames. So this proposed solution from comment 6 won't work.

This information is actually wrong and the webidl file needs an update. webProgress listeners of sub frames do indeed send out notifications but there are problems with file:// URLs (bug 1746483) as noticed when testing locally.

Nevertheless a cleaner approach for us is to use an internal event from the window global message handler that indicates when it is actually ready to be used. Before that the attached browsing context is basically useless because no document has been assigned yet.

As discussed with Julian here is our proposed workflow:

1. There will be a new context listener module that will inform about attached and detached browsing contexts by sending out appropriate events.
2. A new browsing context module in the root folder will make use of this context listener and subscribe to these events.
3. When a new browsing context is attached the root module will store it in a list of uninitialized browsing contexts.
4. When the window global message handler gets created and the shared data has been synced an internal event is sent to inform the root handler about its creation. This event contains the reference to the browsing context.
5. When the root module detects that event and has that browsing context marked as uninitialized a browsingContext.contextCreated event will be emitted, and the browsing context removed from the list

Features to work on:

1. Setup of the internal event infrastructure
2. Let the window global message handler emit an internal event (window-global-created)
3. Create the context listener module
4. Create the browsingContex.jsm root module

Comment 10

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

Attachment: WIP: Bug 1694389 - [remote] Add support for internal message handler events.

Comment 11

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

Attachment: WIP: Bug 1694389 - [remote] Emit internal event when window global message handler has been created.

Depends on D134264

Comment 12

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

Attachment: WIP: Bug 1694389 - [remote] Don't try to apply session data for modules that don't exist.

Depends on D134265

Comment 13

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

Attachment: Bug 1694389 - [webdriver-bidi] Add support for "browsingContext.contextCreated" event.

Depends on D134266

Comment 14

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

I've uploaded a couple of WIP patches that will allow us to send out this event. Julian, when you find the time maybe you could have a look over these and tell me your opinion? I still struggle with the protocol event handling, and maybe we should indeed set these up as separate events (keeping them apart from the message handler events)? Thanks.

Comment 15

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

Julian directly had a look at the patch. So after having a chat with him here the following todo items:

• In the future session.subscribe will always have to update the session data directly and it doesn't know in which context of the protocol module the event is going to be handled and emitted. As such calling _applySessionData should not throw a failure if the module isn't existent but simply ignore it. But we should still throw if the module exists but does not override _applySessionData.

• To better separate the internal from the protocol events we want to go ahead with two pipelines, one for the internal and the other for protocol events. That way we can make sure that no protocol events inappropriately get emitted as internal events and vice versa.

• Currently browsingContext.contextCreated always emits a payload with about:blank as URL. We should still check if that is what we want. If we really wanna see the target documents URL here we may have to wait for a different event, which might be DOMDocElementInserted instead.

I might continue later this week or in the first week of January.

Comment 16

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

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

• To better separate the internal from the protocol events we want to go ahead with two pipelines, one for the internal and the other for protocol events. That way we can make sure that no protocol events inappropriately get emitted as internal events and vice versa.

This would actually cause a lot of duplicated code. After a further discussion with Julian we decided to leave it as is and only add the isProtocolEvent flag.

Comment 17

[Phabricator Automation]

Comment on attachment 9256119 [details]
WIP: Bug 1694389 - [remote] Add support for internal message handler events.

Revision D134264 was moved to bug 1749507. Setting attachment 9256119 [details] to obsolete.

Comment 18

[Phabricator Automation]

Comment on attachment 9256120 [details]
WIP: Bug 1694389 - [remote] Emit internal event when window global message handler has been created.

Revision D134265 was moved to bug 1749507. Setting attachment 9256120 [details] to obsolete.

Comment 19

[Phabricator Automation]

Comment on attachment 9256121 [details]
WIP: Bug 1694389 - [remote] Don't try to apply session data for modules that don't exist.

Revision D134266 was moved to bug 1749675. Setting attachment 9256121 [details] to obsolete.

Comment 20

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

Attachment: WIP: Bug 1694389 - [remote] Return target URI from waitForInitialNavigationCompleted.

When requested to return early from waitForInitialNavigationCompleted
when the navigation starts, the URIs of the current location and for the
destination will be returned.

Comment 21

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

Attachment: Bug 1694389 - [remote] Add API to get the unique id for a browsing context.

Depends on D137539

Comment 22

[Phabricator Automation]

Comment on attachment 9261703 [details]
WIP: Bug 1694389 - [remote] Return target URI from waitForInitialNavigationCompleted.

Revision D137539 was moved to bug 1753288. Setting attachment 9261703 [details] to obsolete.

Comment 23

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

Attachment: Bug 1694389 - [wdspec] Tests for "browsingContext.contextCreated" event.

Depends on D134267

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c08f0b87cebf
[remote] Add API to get the unique id for a browsing context. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/9baf07f96c75
[webdriver-bidi] Add support for "browsingContext.contextCreated" event. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/28bbf5924c83
[wdspec] Tests for "browsingContext.contextCreated" event. r=webdriver-reviewers,jgraham

Comment 25

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

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

Comment 26

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/c08f0b87cebf
https://hg.mozilla.org/mozilla-central/rev/9baf07f96c75
https://hg.mozilla.org/mozilla-central/rev/28bbf5924c83

Comment 27

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

Upstream PR merged by moz-wptsync-bot

Comment 28

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

Attachment: Bug 1694389 - [wdspec] Disable tests for browsingContext.contextCreated on release channels.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/189762c4619d
[wdspec] Disable tests for browsingContext.contextCreated on release channels. r=webdriver-reviewers,jdescottes

Comment 30

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/189762c4619d