Closed Bug 1731589 Opened 3 years ago Closed 1 year ago

Add WebDriver tests for common complex data types as used by `log.entryAdded`

Categories

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

Product:
Remote Protocol
Component:
WebDriver BiDi
Type:
task
Points:
2

Tracking

(firefox114 fixed)

Status:
RESOLVED FIXED
Milestone:
114 Branch
Tracking Flags:
Tracking Status
firefox114 --- fixed

People

(Reporter: whimboo, Assigned: Sasha)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webdriver:m7], [wptsync upstream])

Attachments

(3 files)

Bug 1731589 - [bidi] Use default realm from message handler in log module.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1731589 - [wdspec] Add WebDriver tests for common complex data types as used by `log.entryAdded`.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1731589 - [wdspec] Rename create_console_api_message_for_primitive_value helper to create_console_api_message_from_string.
48 bytes, text/x-phabricator-request
Details | Review

The amount of work here is not that clear yet, but we should have at least support for most-often used data types like

Array
Object
Map
Set
Error

And maybe some others if needed. Otherwise follow-up bugs will be filed.

Points: --- → 8
Priority: -- → P2
Summary: Add serialization and deserialization support common data types as used by `log.entryAdded` → Add serialization and deserialization support for common data types as used by `log.entryAdded`

The deserialization support addition has been split-up into bug 1739976.

Depends on: 1739976
Summary: Add serialization and deserialization support for common data types as used by `log.entryAdded` → Add serialization (and necessary deserialization) support for common data types as used by `log.entryAdded`

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

The amount of work here is not that clear yet, but we should have at least support for most-often used data types like

Lets discuss what we actually want to define as common complex data types. There might be more on top of these listed in comment 0.

Here a list:
https://w3c.github.io/webdriver-bidi/#serialize-as-a-remote-value

Summary: Add serialization (and necessary deserialization) support for common data types as used by `log.entryAdded` → Add serialization support for common complex data types as used by `log.entryAdded`
Whiteboard: [bidi-m2-mvp] → [bidi-m2-mvp][webdriver:triage]

Beside the above mentioned types we should also include support for Promises, which means that we would cover most of the scenarios. Returning platform objects like window might be more complicated, and returning the objectId would be ok for now if the value is too hard to be determined.

Whiteboard: [bidi-m2-mvp][webdriver:triage] → [bidi-m2-mvp]
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
No longer depends on: 1739976

So this is more complex as initially thought given that multiple protocols have to be supported with the underlying object cache implementation, whereby each has its own framework. Work that I have to do now should as best work in all of them, and not cause any major refactoring in a quarter time span. So lets see what needs to be done...

The object cache is a storage for complex objects that cannot be serialized (JSON) as value and as such not directly transmitted via any of the protocols (Marionette, BiDi, CDP) to the client. As such a strong reference to the object is put into a Map that is keyed by a uuid. This uuid can later be used to uniquely identify the object again and use it for further operations. Given that these objects cannot even be transmitted between different Firefox processes each process requires it's own cache. That also implies that once a content process is gone, all the cached data is gone too.

The objects as cached should hereby only be accessible with a specific session id, which is set by the individual protocol that creates the session. Marionette and WebDriver BiDi share the same session, whereby CDP uses it's own mechanism and has different sessions, each for a given tab. To make the cache shareable our CDP implementation would require this session id. We could consider adding a custom CDP command which can be used by a client to set the BiDi session id, and then share it across CDP sessions (tabs) and modules.

Note: Even with Marionette not requiring such an object cache it's needed because element references also have to be stored in the same cache. This is because in CDP evalute is used to retrieve DOM nodes, which are then stored inside the cache.

Beside the session id the object cache also has to be tight to a specific window global (browsing context). Trying to access objects as created for other window globals has to be restricted. In Marionette the ContentDOMReference module is used for that. It uses a global singleton to store the DOM node (only) references with an elements uuid and browsing context as key. But currently Marionette has its element store located in the parent process which makes it harder to share with BiDi and CDP. :(

Given the complexity of these three protocols and their completely different implementations (whereby Marionette is way close given that it uses JSWindowActors) makes it hard to properly forecast what's best to use. But here a proposal:

  1. We create a fork of ContentDOMReference that is able to not only handle DOM nodes but also any kind of object. It still uses a uuid, the browsing context id, and the WebDriver session id as key to reference an object in that cache. All these parts of the key will be known when a command is run in any of the protocols. Further each object cache singleton is bound to a given Firefox (content) process and will be wiped away once the process is gone. No additional clean-up would be necessary.

  2. With BiDi enabled the WindowGlobalMessageHandler class is responsible to load that ContentObjectReference module which will automatically create a global singleton when loaded the very first time for a process. The global window message handler class will get a shortcut method to easily access data via the current session id and browsing context.

2.a) Marionette should also use this forked ContentObjectReference module so that the object cache can be moved back into the appropriate process. This will also work if BiDi is not enabled. Work here is not immediately necessary and can be post-poned until we have the need to handle element references in BiDi in combination with Marionette.

2.b) CDP would have to be updated so it knows about the WebDriver session id. The amount of work here is hard to estimate and should be post-poned for now until we get feedback from the Puppeteer team how their transition strategy looks like.

  1. Connect the object cache with the Serialize to Remove Value code in BiDi so that serialization steps can check for already known objects and return their uuid instead of always creating a new one. For now session id and browsing context id have to be passed in.

  2. (LATER) Once CDP and Marionette are no longer existent remove the global object cache singleton and allow the window global message handler to hold an instance of the object cache. At this point the key would just become the single uuid.

I hope that I covered everything and was able to frame it correctly so it's understandable. At least with Julian I already discussed that but I hope to also get some further feedback from both Julian and James before I'll start the implementation.

Thanks!

Flags: needinfo?(jdescottes)
Flags: needinfo?(james)

Thanks I think the proposal makes sense, and it's good to have the requirements listed here.
I have a few comments & questions, but I think you can go ahead and start with what you have in mind.
It would actually help to see some API / interfaces, but maybe we can jump straight to the review now.

Beside the session id the object cache also has to be tight to a specific window global (browsing context).
Trying to access objects as created for other window globals has to be restricted.

Is this restriction a real requirement? Technically the object cache maps a uuid to an object, and I don't see why it would have to enforce this. Are we worried about this being abused to call methods from 1 window global with arguments from another window global?

However we need some knowledge about the browsingcontext/windowglobal in order to avoid keeping references to objects after the page they were created in was destroyed. I imagine the ObjectReference will use a similar mechanism as the ContentDOMReference (WeakMap keyed by browsing context?) to ensure the cache is automatically cleaned up, without having to wait for the process to be destroyed.

We create a fork of ContentDOMReference that is able to not only handle DOM nodes but also any kind of object

Yes we need a similar object map/cache mechanism. Not sure if presenting it as a fork of ContentDOMReference helps, but no strong opinion. I would not mention Content in the classname/filename however. In the long run, this should also be able to work in the parent process, in worker threads etc... So ObjectCache/ObjectReference/...?

With BiDi enabled the WindowGlobalMessageHandler class is responsible to load that ContentObjectReference
module which will automatically create a global singleton when loaded the very first time for a process

Does it matter who loads the module first?

Connect the object cache with the Serialize to Remove Value code in BiDi so that serialization steps
can check for already known objects and return their uuid instead of always creating a new one

Do you think the serialization step will have to explicitly check for known objects? Can't this check be fully delegated to the ObjectReference, and the serialization step simply asks to get the uuid?

Flags: needinfo?(jdescottes)

If objects are from globals in the same browsing context group they are accessible to each other e.g. consider callFunction with expression return window.opener.document.body. That should produce an object id that's usable both in the context of the context in which the script ran, and also in the opener context. I think, but I'm not sure, that we han have a per-process cache, and rely on the normal DOM security checks. But I also agree that the cache shouldn't extend the lifetime of the objects beyond the lifetime the global would have in the absence of the cache. So I think we likely want all process-global references to be weak, and keep any strong references in the WindowGlobalMessageHandler, or some other object that is already designed to be destroyed when the context is discarded.

Flags: needinfo?(james)

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

It would actually help to see some API / interfaces, but maybe we can jump straight to the review now.

Yes, I can implement some basics to actually demonstrate the wanted behavior. Then we can decide if it makes sense and fully implement the proposal.

Beside the session id the object cache also has to be tight to a specific window global (browsing context).
Trying to access objects as created for other window globals has to be restricted.

Is this restriction a real requirement? Technically the object cache maps a uuid to an object, and I don't see why it would have to enforce this. Are we worried about this being abused to call methods from 1 window global with arguments from another window global?

Each window global (or even Realm?) needs its own object cache where evaluated or retrieved objects are stored. Lets say you are trying to find some element in a window global and return it. A reference of it is stored together with the browsing context id in the cache. If you try to use the element from a different window global, which has a different browsing context id, we have to throw a no such element error. And if a window global has been navigated and the new page doesn't contain the element anymore (or it has been removed via JS), any usage via its browsing context id has to fail with a stale element reference error because it's no longer resolvable to a real DOM node. Similar examples we can have for script.evaluate and script.callFunction.

However we need some knowledge about the browsingcontext/windowglobal in order to avoid keeping references to objects after the page they were created in was destroyed. I imagine the ObjectReference will use a similar mechanism as the ContentDOMReference (WeakMap keyed by browsing context?) to ensure the cache is automatically cleaned up, without having to wait for the process to be destroyed.

I'm not sure if a WeakMap would work here when we keep a possible usage of DevTools in mind? Would it be ok when elements you are inspecting go away? Maybe it works for elements, but we clearly would need a strong reference for arbitrary objects via Debugger.Object. But I know less about the debugger, so it's something that needs to be answered by someone more familiar with.

Yes we need a similar object map/cache mechanism. Not sure if presenting it as a fork of ContentDOMReference helps, but no strong opinion. I would not mention Content in the classname/filename however. In the long run, this should also be able to work in the parent process, in worker threads etc... So ObjectCache/ObjectReference/...?

Sure the name was just an example.

With BiDi enabled the WindowGlobalMessageHandler class is responsible to load that ContentObjectReference
module which will automatically create a global singleton when loaded the very first time for a process

Does it matter who loads the module first?

No, I don't think so. The first load will just trigger the instantiation.

Do you think the serialization step will have to explicitly check for known objects? Can't this check be fully delegated to the ObjectReference, and the serialization step simply asks to get the uuid?

Yes, that is part of the serialization / deserialization steps. Any code that has to check for object id for an object has to do it.

A reference of it is stored together with the browsing context id in the cache. If you try to use the element from a different window global, which has a different browsing context id, we have to throw a no such element error.

That might be true in WebDriver classic since it has a check that the Element is connected to the DOM tree of the current window. But it's not clear to me that it ought to be true for WebDriver-BiDi or CDP; it seems reasonable to pass an object between contexts in any scenario where JS code could do so (e.g. the window.opener.document.body) example from above.

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

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

Do you think the serialization step will have to explicitly check for known objects? Can't this check be fully delegated to the ObjectReference, and the serialization step simply asks to get the uuid?

Yes, that is part of the serialization / deserialization steps. Any code that has to check for object id for an object has to do it.

I guess I was rather asking about where this should be implemented. I imagined that the default behavior of the ObjectReference/ObjectCache to get a uuid would be a "get or create". Therefore consumers which perform the serialization don't have to implement this logic. They can implicitly rely on the ObjectReference/ObjectCache. But that's already going into implementation detail at that stage, so this can wait until the review.

Priority: P2 → P3
Depends on: 1739976
No longer depends on: 1739976
Priority: P3 → P2
Whiteboard: [bidi-m2-mvp] → [bidi-m3-mvp]
Points: 8 → 13
No longer blocks: 1724669
Blocks: 1719287

Currently I'm actually not working on this bug.

Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3

This bug is blocked on the implementation of serializing complex objects (see bug 1770752 and bug 1770754).

When starting the implementation we should not add a complete set of tests for this event to wdspec (given that we will already have that for script evaluation) but only such a subset that gives us confidence that we correctly serialize different types.

I'm going to remove the points for now given that it won't be a 13 point bug when we get started.

Points: 13 → ---
Depends on: 1770752, 1770754
Whiteboard: [bidi-m3-mvp] → [webdriver:backlog]
Depends on: 1792524

Our WebDriver BiDi serialization support has been done and as such it seems that only WebDriver BiDi tests have to be added to cover additional scenarios. Note that we don't have to add a complete set of tests similar to script evaluation but just a basic set. Lets discuss how we want to schedule that given that Selenium users might want to use this feature soon more and more often. Maybe we should move the bug to P2?

Summary: Add serialization support for common complex data types as used by `log.entryAdded` → Add WebDriver tests for common complex data types as used by `log.entryAdded`
Whiteboard: [webdriver:backlog] → [webdriver:backlog][webdriver:triage]
Points: --- → 2
Priority: P3 → P2
Whiteboard: [webdriver:backlog][webdriver:triage] → [webdriver:backlog]
Depends on: 1770733

Julian, would you already have a good idea for sharing the Realm information between the script and the log module? How could that information be accessed for the log.entryAdded event when we have to serialize Nodes? If not it would be good if we could discuss it in a meeting before the next milestone starts.

Flags: needinfo?(jdescottes)

My initial idea was that Realms should be provided by the MessageHandler class, and not by a specific module.

Most of the Realm logic which is at the moment in windowglobal/script could move to WindowGlobalMessageHandler, which would then expose getters such as getRealm and getRealms. We can have a dedicated file/class to handle this, so that WindowGlobalMessageHandler doesn't become huge. Realms are under webdriver-bidi at the moment, but there is nothing bidi-specific about them so I think it should be fine.

The other option would be to keep this in the Script module, but expose #getRealm (etc...) as internal commands so that other modules can use it. But my overall feeling is that commands are quite verbose, and I think it's best to avoid them for communication which necessarily happens within a single MessageHandler "node".

So my suggestion would be: move Realms handling to a dedicated class owned by MessageHandler. We can have a meeting to discuss or we can try to fit that during a regular triage meeting. I'll flag the bug for triage, we can decide if it needs its own meeting or not.

(for reference there was some discussion about this in https://bugzilla.mozilla.org/show_bug.cgi?id=1742589 as well)

Flags: needinfo?(jdescottes)
Whiteboard: [webdriver:backlog] → [webdriver:backlog][webdriver:triage]
Depends on: 1788894

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

So my suggestion would be: move Realms handling to a dedicated class owned by MessageHandler. We can have a meeting to discuss or we can try to fit that during a regular triage meeting. I'll flag the bug for triage, we can decide if it needs its own meeting or not.

(for reference there was some discussion about this in https://bugzilla.mozilla.org/show_bug.cgi?id=1742589 as well)

I've update bug 1788894 to cover this particular work. Lets move the triage flag over to the other bug.

status-firefox111: --- → fixed
status-firefox112: --- → fixed
status-firefox113: --- → fixed
status-firefox-esr102: --- → fixed
Whiteboard: [webdriver:backlog][webdriver:triage] → [webdriver:backlog]
Priority: P2 → P1
Whiteboard: [webdriver:backlog] → [webdriver:m7]
Whiteboard: [webdriver:m7] → [webdriver:m7:blocked]
Priority: P1 → P2
Assignee: nobody → aborovova
Status: NEW → ASSIGNED
Pushed by aborovova@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2159611e1059
[bidi] Use default realm from message handler in log module. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/510d90239468
[wdspec] Rename create_console_api_message_for_primitive_value helper to create_console_api_message_from_string. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/0d4a9640bffd
[wdspec] Add WebDriver tests for common complex data types as used by `log.entryAdded`. r=webdriver-reviewers,jdescottes
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39841 for changes under testing/web-platform/tests
Whiteboard: [webdriver:m7:blocked] → [webdriver:m7:blocked], [wptsync upstream]
Whiteboard: [webdriver:m7:blocked], [wptsync upstream] → [webdriver:m7], [wptsync upstream]

https://hg.mozilla.org/mozilla-central/rev/2159611e1059
https://hg.mozilla.org/mozilla-central/rev/510d90239468
https://hg.mozilla.org/mozilla-central/rev/0d4a9640bffd

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox114: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 114 Branch
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: