Open Bug 1742589 Opened 3 years ago Updated 1 years ago

Use the sandbox realm for "log.entryAdded" events when entries aren't coming from the default window

Categories

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

task
Points:
8

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.

Whiteboard: [webdriver:triage] → [bidi-m3-mvp]
Whiteboard: [bidi-m3-mvp]
Priority: -- → P3
Whiteboard: [bidi-m3-mvp]
Points: --- → 8
No longer blocks: 1724669

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.

Depends on: 1767260
Whiteboard: [bidi-m3-mvp] → [webdriver:backlog]

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.

Whiteboard: [webdriver:backlog] → [webdriver:backlog][webdriver:triage]

We should wait until we have proper support for sandbox usage for script evaluation before continuing with this bug.

Depends on: 1770480
Whiteboard: [webdriver:backlog][webdriver:triage] → [webdriver:backlog]

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 the innerWindowID) 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.

(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 the innerWindowID) 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.

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.

(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.

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.

Whiteboard: [webdriver:backlog] → [webdriver:backlog][webdriver:triage]

As decided in today's meeting we should have a focus on this particular bug for the current or next milestone.

Priority: P3 → P2
Summary: Support realms in log.entryAdded → Use the sandbox realm for "log.entryAdded" events when entries aren't coming from the default window
Whiteboard: [webdriver:backlog][webdriver:triage] → [webdriver:backlog]

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?

James was advocating for P2 so lets get his feedback.

Flags: needinfo?(james)

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.

Flags: needinfo?(james)
You need to log in before you can comment on or make changes to this bug.