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

Status: NEW

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

Description

[Julian Descottes [:jdescottes]]

Follow up to Bug 1694143, we should emit the realm in the payload for the log.entryAdded event.

Comment 1

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

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

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

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

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

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

Comment 4

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

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.

Comment 5

[Julian Descottes [:jdescottes]]

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

Comment 6

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

(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

[Alexandra Borovova [:Sasha]]

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.

Comment 8

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

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

Comment 9

[Julian Descottes [:jdescottes]]

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

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

James was advocating for P2 so lets get his feedback.

Comment 11

[James Graham [:jgraham]]

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.

Comment 12

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

We dropped on this topic for quite a bit but with the work we did over the past half year it seems to be easier to accomplish now.

Nowadays we have the list of realms available via the window global message manager instance. Given that each realm should be tight to a specific inner window, I assume that we should let the console (API) listeners retrieve such a window via the inner window id, which is available for both APIs, and let it forward via their internal event's payload. Then in the WebDriver BiDi's content log module we can use that window reference to filter for the related Realm to be returned via the log.entryAdded event.

I would propose that we get this fixed before clients might to heavily rely on the default realm that we currently return. Also I don't think that this will be 8 points anymore but more 3 (or max. 5) points.

Comment 13

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

Actually as what Sasha already mentioned in comment 7 the innerWindowId is not enough here and we need a unique id that identifies the actual sandbox the event is coming from. The souceId that is emitted eg as part of the nsIScriptError might actually be such a candidate, but it's not clear for us how to actually correlate it to a created sandbox via Cu.Sandbox().

Olli, would sourceId the right choice here and if yes, how could we retrieve it from a sandbox instance? Thanks.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I don't know the answer. It depends on how does something somewhere map sourceId to something else.
I'm not sure how strict/consistent the callers of InitSourceId are.

Comment 15

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

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #14)

I don't know the answer. It depends on how does something somewhere map sourceId to something else.
I'm not sure how strict/consistent the callers of InitSourceId are.

Most importantly for now would be those callers that use console API calls and raise script errors based on web content. So I assume that all these are part of the DOM code somewhere? The main question would still be if sourceId is what we are looking for or if there is something else that should actually be used to identify a sandbox / realm.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

I still don't know the answer. I don't know what you'd do with sourceId, and sourceId is just some random integer.
Given bug 1447244, jimb might remember better what that integer represents.