1830884 - Use and update navigable seen node map when interacting with nodes of type Element and ShadowRoot

Status: RESOLVED FIXED

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

Description

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

As per bug 1822466 I'm going to add a navigable seen nodes map to the parent process of Marionette. To allow inter-op tests between both protocols (eg. exchanging Element and ShadowRoot references) the same checks have to be done for WebDriver BiDi as well.

The feature of sending messages between the child and parent actors of the MessageHandler framework will be added on bug 1830404.

We should add it to M7.

Comment 1

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

Attachment: Bug 1830884 - [remote] Make element.sys.mjs a shared module for remote and separate out Marionette specific code.

Depends on D177492

Comment 2

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

Attachment: Bug 1830884 - [webdriver-bidi] Added WebDriver BiDi base module for root and window global in root destination.

Depends on D177493

Comment 3

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

Attachment: Bug 1830884 - [webdriver-bidi] Add serialization / deserialization helpers to WindowGlobalBiDiModule.

Depends on D177494

Comment 4

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

Attachment: Bug 1830884 - [webdriver-bidi] Update Navigable's seen nodes map for known nodes.

Depends on D177495

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31607d74ee69
[remote] Make element.sys.mjs a shared module and separate out WebReference for Marionette only.. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/77f0334c7976
[webdriver-bidi] Added WebDriver BiDi base module for root destination. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/67d5d6a5f321
[webdriver-bidi] Add serialization / deserialization helpers to WindowGlobalBiDiModule. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/7f4052a38bc6
[webdriver-bidi] Update Navigable's seen nodes map for known nodes. r=webdriver-reviewers,jdescottes

Comment 6

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

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

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/31607d74ee69
https://hg.mozilla.org/mozilla-central/rev/77f0334c7976
https://hg.mozilla.org/mozilla-central/rev/67d5d6a5f321
https://hg.mozilla.org/mozilla-central/rev/7f4052a38bc6

Comment 8

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

Upstream PR merged by moz-wptsync-bot

Comment 9

[Natalia Csoregi [:nataliaCs]]

Backed out for causing regressions in the upstream wpt tests.

• backout: https://hg.mozilla.org/mozilla-central/rev/42a42f83eb3b152534541af5495f2701663d03c4

Comment 10

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

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

Comment 11

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

Upstream PR merged by moz-wptsync-bot

Comment 12

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

After the backout we discussed possible solutions for BiDi which will prevent us from having to serialize and deserialize by async code. I've rewritten the code for two possible scenarios and created performance profiles (including this baseline profile) for each of them. None of them actually show a difference in performance which is good!

Try build:
https://treeherder.mozilla.org/jobs?repo=try&revision=0ae8d63a789739e75f99f5191b0d7939e3462100

1. Passing the list of seenNodes through the full stack (profile) [2nd commit at the top]

While this would keep the serialize helper in the base module class synchronous we will have to pass the list of seen nodes through a lot of function calls. This is quite complex and error prone. Also it's unclear if this would work for the case of serializing window handles.

2. Using a single IPC call after serialization to sync with the parent (profile) (top-most commit)

This alternative makes the shared serialize() helper async but only for the required IPC call. The general serialization of the underlying object is still sync and will block until it's completely done. So we should not face issues with modifications of the object while it's getting serialized.

Personally I'm actually in favor of the 2nd approach, which should also make it easier for us to handle windows in the future. But I would like to get some feedback first before continuing. Thanks.

Comment 13

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

Attachment: Bug 1830884 - [webdriver-bidi] WIP: Rewrite to use a single IPC call to sync seen node ids.

Depends on D177496

Comment 14

[Julian Descottes [:jdescottes]]

Thanks for the patches!

Before answering in details, I am having an issue with our current approach for the seenNodes map. We made this a parent process map per navigable, but I am no longer sure this is needed.

My assumptions:

• the seenNodes map in the parent process is only useful to check if an element is stale or not
• "stale" means either that the element is not connected or that it's document is not the "current" document

If that is correct, then for the parent process it doesn't matter if a given node id was seen for the navigable where we tried to find it or not. As long as it was seen "anywhere" it should result in a stale exception. Which means we should be able to make the seenNodes information in the parent process a simple set of ids?

I am probably overlooking something, so if someone can clarify the scenario or use cases where this doesn't hold, it would be helpful.

Edit: Henrik clarified that having it per navigable was important for cleaning up the map when navigables are destroyed, which makes sense as we don't want to indefinitely grow this map.

We can probably have another discussion at this point.

Comment 15

[Julian Descottes [:jdescottes]]

So considering that the browsing context id is needed to cleanup the list of seenNodes, I don't have a strong preference between the two approaches. The extra IPC call feels more complicated to me, but code wise and performance wise the two solutions seem relatively close, so if you think doing the extra IPC call will be helpful to support windows, it sounds fine.

(We also started discussing whether the browsing context id was actually needed when raising "no such element" exceptions even for BiDi. This depends whether the "seen" check can be global or if it should only be "was it seen in this navigable". And this is not completely clear to me. For classic in theory we throw a stale exception if the node's document is not the current document, so it feels like it should be global. But at the same time, since we clear the seenNodes map when a given navigable is destroyed, the "seen" check is never going to be really "global".)

Comment 16

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

With the recent work on that stack of patches to include the browsing context I noticed that we actually use the wrong browsing context to store the generated shared id in the seenNodes list. In case of nodes to be returned from a different browsing context the id would get added to the wrong navigable, and as result trying to access the node (element, shadow root) will cause a no such element or no such shadow root failure.

I created https://github.com/w3c/webdriver-bidi/pull/445 to get this fixed.

Comment 17

[Alexandra Borovova [:Sasha]]

I have basically the same opinion as Julian in terms of choosing the solution. The IPC call feels more complex, but I'm generally fine with it as well, especially if it should help to support windows.

Comment 18

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

Also based on James' reply on the WIP directly I have abandoned it.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/183aa8c0f12e
[remote] Make element.sys.mjs a shared module for remote and separate out Marionette specific code. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/3d43aa441eb2
[webdriver-bidi] Add serialization / deserialization helpers to WindowGlobalBiDiModule. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/6c51face7335
[webdriver-bidi] Update Navigable's seen nodes map for known nodes. r=webdriver-reviewers,jdescottes

Comment 20

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/183aa8c0f12e
https://hg.mozilla.org/mozilla-central/rev/3d43aa441eb2
https://hg.mozilla.org/mozilla-central/rev/6c51face7335