1837925 - Simplify the generation of "seenNodes" by defining serialization and deserialization helper methods within "serialize" and "deserialize"

Status: NEW

(Remote Protocol :: WebDriver BiDi, enhancement, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

As discussed on: https://phabricator.services.mozilla.com/D177496#inline-1000351

There could be commands and events that return a serialized object containing one or more DOM nodes and where we forgot to process the extra data that is introduced with bug 1830884. That means we won't update the WebDriver session's seenNodes map which can later on cause failures because the node hasn't been seen before.

So the question is if we should better require any command and event that uses WindowGlobalBiDiModule.serialize() to have to pass a seenNodes argument to it.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Alternatively we could have all the serialization and deserialization helpers in RemoteValue living within the serialize and deserialize methods - similar to the code in Marionette (json.sys.mjs). As such we could easily return the list of seen nodes as part of the final result without having it to pass through each function invocation.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Similar to json.clone() in Marionette all the helper methods that are needed for serialization and deserialization in the RemoteValue.sys.mjs module should be moved below the entry point. This would allow us to more easily collect extra data like seenNodeIds that one or both commands might have to return.

By doing that no BiDi command would have to explicitly create a new Map first and pass it to WindowGlobalBiDiModule.serialize(), but can rely on the return value to get the wanted data.

Comment 3

[Richard Cole]

New contributor here - I'm going to take this on as my second patch.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Please note that on bug 1832792 we have ongoing work for re-ordering the arguments for all the deserialize related methods. To not end-up in conflicts it might be better to wait a little bit until you start working on this bug if possible.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Hi Richard, the depending bug has now been fixed. That means if you are still interested please pull the latest changes before starting with the work. Thanks.

Comment 6

[Richard Cole]

Hi Henrik, I'll rebase and run all tests and have this for you soon.

Comment 7

[Richard Cole]

Hi Henrik, I had a chance to re-familiarize myself with the code

My current plan -
Keep the wrapper functions of serialize/deserialize within WindowGlobalBiDiModule.sys.mjs (I had initially thought that these needed to be deleted and replaced. My current understanding is that the definitions from the modules will be moved within the wrapper).
Move the definitions of serialize/deserialize from RemoteValue.sys.mjs inside of the 'wrapper functions'.
Run tests

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

As first action I would suggest that you are refactoring the RemoteValue.sys.mjs file based on comment 1. It means that any special serialization / deserialization function should be moved below serialize or deserialize. Such a change should not cause any regressions, which you can verify by running the following tests:

• https://searchfox.org/mozilla-central/source/remote/webdriver-bidi/test/browser
• https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/bidi/script/call_function
• https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/bidi/script/evaluate

Once the above is done you can work on part 2, which should be done in a different revision.

Let me know if you have questions.

Comment 9

[Richard Cole]

Attachment: WIP: Bug 1837925 - Simplify the generation of by defining serialization and deserialization helper methods within and

Comment 10

[Richard Cole]

WIP submitted - https://phabricator.services.mozilla.com/D203322

I moved the following helpers inside of deserialize

• deserializeValueList
• deserializeKeyValueList
• deserializeSharedReference

and the following within serialize

• serializeArrayLike
• serializeList
• serializeMapping
• serializeNode

It wasn't clear to me if functions like getHandleForObject or getSharedIdForNode would need to be moved into serialize as well.

The following tests all pass

• https://searchfox.org/mozilla-central/source/remote/webdriver-bidi/test/browser
• https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/bidi/script/call_function
• https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/bidi/script/evaluate

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

(In reply to Richard Cole from comment #10)

It wasn't clear to me if functions like getHandleForObject or getSharedIdForNode would need to be moved into serialize as well.

Yes, it's fine to leave those out. They do not recursively call into any serialize / deserialize method.

The following tests all pass

• https://searchfox.org/mozilla-central/source/remote/webdriver-bidi/test/browser
• https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/bidi/script/call_function
• https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/bidi/script/evaluate

Please make sure to also run the following test:
https://searchfox.org/mozilla-central/source/remote/webdriver-bidi/test/browser/browser_RemoteValue.js

If the test passes as well you can push your patch again by using the --no-wip argument so it will be moved out of the WiP status. Thanks!

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Hello Richard, I wanted to check back with you if my last comment was helpful and to unblock you from the next steps. Please let me know if there are still issues / questions that need to be solved first. Thanks.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Sadly I didn't receive any further response from Richard. So I assume that there is no interest to continue working on this bug. As such I'll unassign and put it back into the pool.

Comment 14

[Richard Cole]

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

Sadly I didn't receive any further response from Richard. So I assume that there is no interest to continue working on this bug. As such I'll unassign and put it back into the pool.

I apologize for my absence. All four tests are passing, your comment was very helpful. I've submitted to moz-phab - https://phabricator.services.mozilla.com/D203322

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Richard, would you mind submitting the patch with --no-wip? That way it will be put into our review queue so that we can take a look. Thanks!

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Hi Richard, I wanted to check back with you if you are still interested to work on this issue even it takes more time. Maybe you could let us know? Thanks.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit BugBot documentation.