Use the sandbox realm for "log.entryAdded" events when entries aren't coming from the default window
Categories
(Remote Protocol :: WebDriver BiDi, task, P2)
Tracking
(Not tracked)
People
(Reporter: jdescottes, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [webdriver:backlog])
Follow up to Bug 1694143, we should emit the realm in the payload for the log.entryAdded event.
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Updated•3 years ago
|
Comment 1•2 years ago
|
||
Realm support is not something we are going to implement soon. But there is bug 1767260 which will add support for the source
field including the browsing context specifier.
Comment 2•2 years ago
|
||
With bug 1767260 we also got the realm
set to the inner window id. But once log entries can be sent out from a sandbox we most likely have to do some updates here.
Would be good to further discuss and better align with the work for script evaluation.
Comment 3•2 years ago
|
||
We should wait until we have proper support for sandbox usage for script evaluation before continuing with this bug.
Updated•2 years ago
|
Comment 4•2 years ago
|
||
All dependencies seem to have been fixed. To get the realm
id added we will have to use the following:
- For the console listener: the
innerWindowID
of the message - For the console API: the
innerID
(basically theinnerWindowID
) of the message can be used
With that ID we would have to retrieve the related Realm
from the available realms in the script module.
Reporter | ||
Comment 5•2 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #4)
All dependencies seem to have been fixed. To get the
realm
id added we will have to use the following:
- For the console listener: the
innerWindowID
of the message- For the console API: the
innerID
(basically theinnerWindowID
) of the message can be usedWith that ID we would have to retrieve the related
Realm
from the available realms in the script module.
We already somehow implemented this in Bug 1767260. It's not exactly done how you suggested, but the end result is the same. The log module adds the innerWindowID to all messages here https://searchfox.org/mozilla-central/rev/abcee8d2c97a5c8a1fbeaf84607ea427be72497a/remote/webdriver-bidi/modules/windowglobal/log.sys.mjs#48.
And the events created for a given module are filtered on innerWindowID for console listener and on innerID for console api listener.
Although we are not going through a realm object, nor are we using the script module. But the id should already be correct.
Comment 6•2 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #5)
We already somehow implemented this in Bug 1767260. It's not exactly done how you suggested, but the end result is the same. The log module adds the innerWindowID to all messages here https://searchfox.org/mozilla-central/rev/abcee8d2c97a5c8a1fbeaf84607ea427be72497a/remote/webdriver-bidi/modules/windowglobal/log.sys.mjs#48.
Ah, right. So the windowglobal log module should call into the script module to actually retrieve the correct realm for the inner id. What we have right now was just a workaround because at the time of implementation we haven't had realm support yet.
Comment 7•2 years ago
|
||
In the scope of bug 1731589, we started using the default realm for serialization of arguments and in the payload for the log.entryAdded
event. But we should actually use the realm from which an event came from to cover the case, when the event is emitted in a sandbox.
We would need, most likely, to use sourceId
from the event to find the right realm. One technical constrain could also be that BiDi might not know about the realm which was created to emit the event.
Updated•2 years ago
|
Comment 8•1 years ago
|
||
As decided in today's meeting we should have a focus on this particular bug for the current or next milestone.
Reporter | ||
Comment 9•1 years ago
|
||
During the meeting we also mentioned that having the correct realm will be crucial for workers which is partly why we bumped the priority.
But since we don't implement workers at the moment, it will be hard to work on that part. Which leaves only the "messages coming from sandboxes" scenario. And most likely this will need a different technical solution than for worker messages. Worker messages should be captured in the worker thread, so we should easily have a way to identify them when we create the event. If we make this bug only about sandbox window realms, does this impact the priority?
Comment 10•1 years ago
|
||
James was advocating for P2 so lets get his feedback.
Comment 11•1 years ago
|
||
My opinion is that continuing to send events with wrong information is bad for the ecosystem as people learn that they can't rely on getting consistent realm ids to identify the source of script things and so have to spend a bunch of time on workarounds. So my argument is that we should probably invest the time in fixing this for sandboxes upfront, allowing clients to depend on it, and then making the introduction of workers easier.
Description
•