-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for messaging between different browsing contexts #29803
Conversation
c31ae78
to
e2ffed0
Compare
// This ensures that no subsequent commands will be buffered in the underlying | ||
// websocket before the navigation starts. This has to be done in the caller | ||
// to avoid a race condition between the caller sending a new command and the | ||
// socket being closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #28950,
- I have something similar to
closeAllChannelSockets()
that suspends polling network requests on the remote context side, - but no
pause()
-ish things, relying on that the caller doesn't send any new commands until the page becomes inactive
so I'm curious how pause()
works.
Particularly, are both of pause()
and closeAllChannelSockets()
needed?
Is there a race condition between these two?
}, | ||
{once: true}); | ||
|
||
async function navigateTo(url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As there are ways to trigger navigation other than location.href
(e.g. form submission), I'd keep prepateNavigation()
and execute_script()
for navigation purpose.
e2ffed0
to
53032f1
Compare
53032f1
to
8cf5ab4
Compare
8cf5ab4
to
9354388
Compare
9354388
to
742bde1
Compare
742bde1
to
6084028
Compare
6084028
to
64bb7c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot! I've reviewed up to infrastructure/channels/serialize_child.html...
html/browsers/browsing-the-web/back-forward-cache/resources/executor.html
Outdated
Show resolved
Hide resolved
html/cross-origin-opener-policy/navigate-to-aboutblank.https.html
Outdated
Show resolved
Hide resolved
html/cross-origin-opener-policy/navigate-to-aboutblank.https.html
Outdated
Show resolved
Hide resolved
html/cross-origin-opener-policy/navigate-to-aboutblank.https.html
Outdated
Show resolved
Hide resolved
html/cross-origin-opener-policy/navigate-to-aboutblank.https.html
Outdated
Show resolved
Hide resolved
44fd479
to
bfaf63c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed infrastructure/ and resources/, only took me 2+ hours.
html/cross-origin-opener-policy/resources/executor-channel.html
Outdated
Show resolved
Hide resolved
|
||
let lastData; | ||
|
||
// Hack: these will be recreated as assert failures in the caller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Can you elaborate a bit on this comment so that it's easier to understand why this has the effect it has, and what the vision for the future is?
typeof self[item.value.type] === "function") { | ||
result = new self[item.value.type](item.value.message); | ||
} else { | ||
result = new Error(item.value.message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If item.value.type
is "Error", the above case should be taken. Can you add a comment that this isn't about deserializing Error
objects, but non-Error
values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've reviewed the Python parts (tools/ and websockets/) too now.
bfaf63c
to
786d1ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All that is left is some minor things.
Thanks for getting through this, it must have taken much longer to write than to review :D
This allows stash-owned queues that can then be accessed across multiple servers etc. In particular it can be used as the backend for a shared message queue in which multiple browsing contexts can send messages using the stash.
This allows creating message channels that can cross multiple browsing contexts or script execution environments. A simple channel pair can be created using the channel function e.g. let [recvChan, sendChan] = await channel(); This generates a channel pair such that messages sent on sendChan will arrive on recvChan. Channels are multiple producer, single consumer, so there can only be a single ReadChannel for each network, but multiple SendChannels. On top of this underlying abstraction there's a higher level API which provides known message structures. There is currently a command message type where each command has an id, a method, a set of params, and a respChannel which is a channel to use to reply to the command. Currently two commands are provided: call, which runs a function in a remote context, and postMessage, which sends a message to a remote context. Messages and script parameters are serialized using a WebDriver BiDi like remote value serialization algorithm which allows passing complex objects over the wire. To work with multiple contexts, one takes the following actions: * In the context that will recieve messages, use: let recvChannel = await start_global_channel(); This starts listening for messages on a channel defined by the testdriver context id (typically in the URL; maybe this should just be always in the URL which could also remove the testdriver.js dependency). If only executeScript is being used nothing more needs to be done. For postMessage, one can register message handlers using `recvChannel.addMessageHandler("message", fn)`. * In the context that will send messages, use the uuid of the remote context to create a RemoteGlobal object. This will allow sending messages to the remote. let remote = RemoteGlobal(uuid); let result = remote.call(selector => document.querySelector(selector).textContent, "body") remote.postMessage("done"); Message channels are backed with websockets, which use the stash as the data storage layer. For use cases where it's important to close all websockets (e.g. testing bfcache), the closeAllChannelSockets helper is provided.
This makes them easier to diagnose.
786d1ff
to
6359481
Compare
These aren't tests, so it can be assumed we're using it carefully.
The current setup assumed that we had one Stash instance per process and so if the lock property on the class was initialised then we must also have initialised the manager. The way that websockets handlers are loaded breaks this assumption, so we need to ensure that the lifecycle of the lock and of the manager object are identical. Sharing the manager state between instances makes sense since we're already sharing the proxy we get from the stash.
6359481
to
8b194c9
Compare
Thanks for all the reviews @foolip! |
This will be split up to land, and will have one or more associated RFCs.
The features added are:
uuid
parameter in the URL.