1747222 - Use an unique ID of top-level browsing contexts (window handles)

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

With cross browsing group navigations we still get a new browsing context for the top-level browsing context. To have an unique id we should base the id on the browserId and generate an unique id and keep it in a map.

This map should also be used in Marionette and when using window handles.

Comment 1

[Julian Descottes [:jdescottes]]

The current implementation for Marionette's WindowManager uses a Map between browser permanentKey and custom UUIDs. However permanentKey is supposed to be ultimately removed:

  /**
   * The browser's permanent key. This was added temporarily for Session Store,
   * and will be removed in bug 1716788.
   */
  readonly attribute jsval permanentKey;

https://searchfox.org/mozilla-central/rev/6b4e19ad33650fdf9cd8529cd68eeb98bff1b935/dom/interfaces/base/nsIBrowser.idl#54-59

So it's probably better to replace this with something based on browserId.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

That sounds good. Thanks Julian!

Comment 3

[Julian Descottes [:jdescottes]]

There is still the issue that unloaded tabs always have a browserId of 0. This was already known and mentioned at https://bugzilla.mozilla.org/show_bug.cgi?id=1680479#c2 but I forgot about it (thankfully I was reminded by our unit tests).

Maybe a better path here is to ask for permanentKey to be a stable API we can rely on and reuse this BiDi. This means that this bug would be about extracting the permanentKey -> uuid weakmap to a dedicated helper, or modify the existing WindowManager.

In terms of API, I imagine we need to be able to

• get browser by UUID
• get UUID for browser
• get all UUIDs for all the current browsers

Henrik, would this API be sufficient for BiDi? And please let me know if I'm missing something, because as is it seems that WindowManager should already work well enough for BiDi. I imagine we will still need to read and propagate the browserId in the sessionData because it will be useful to check if a new BrowsingContext corresponds to a given tab or not. But as far as generating UUIDs for toplevel browsing contexts, the permanentKey should still be ok.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to Julian Descottes [:jdescottes] from comment #3)

There is still the issue that unloaded tabs always have a browserId of 0. This was already known and mentioned at https://bugzilla.mozilla.org/show_bug.cgi?id=1680479#c2 but I forgot about it (thankfully I was reminded by our unit tests).

Indeed. I also completely forgot about this fact. So it's not usable for us to identify a specific tab.

Maybe a better path here is to ask for permanentKey to be a stable API we can rely on and reuse this BiDi. This means that this bug would be about extracting the permanentKey -> uuid weakmap to a dedicated helper, or modify the existing WindowManager.

Agree. Maybe we just keep using permanentKey and if this is going away we would need a proper replacement.

In terms of API, I imagine we need to be able to

• get browser by UUID
• get UUID for browser
• get all UUIDs for all the current browsers

Sounds like a good starting point. Whereby I wonder if we should have it as part of a TabManager module. Also in regards of different applications we might want to have an additional layer and only package the appropriate manager for that application. That way we can get rid of all the if/else conditions in browser.js too. This should probably be a separate step. What do you think?

I imagine we will still need to read and propagate the browserId in the sessionData because it will be useful to check if a new BrowsingContext corresponds to a given tab or not. But as far as generating UUIDs for toplevel browsing contexts, the permanentKey should still be ok.

The check for new browsing contexts could still be done directly via the permanentKey or? If we go through the browserId we would still be affected by unloaded tabs (which might only occur on startup when restoring the session and keep tabs unloaded)?

Comment 5

[Julian Descottes [:jdescottes]]

thanks for the feedback!

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #4)

(In reply to Julian Descottes [:jdescottes] from comment #3)

I imagine we will still need to read and propagate the browserId in the sessionData because it will be useful to check if a new BrowsingContext corresponds to a given tab or not. But as far as generating UUIDs for toplevel browsing contexts, the permanentKey should still be ok.

The check for new browsing contexts could still be done directly via the permanentKey or? If we go through the browserId we would still be affected by unloaded tabs (which might only occur on startup when restoring the session and keep tabs unloaded)?

Not sure, the use case I am specifically thinking about is if we have some session data which is scoped to a given tab (eg subscribing to log.entryAdded only for a specific top-level browsing context). Imagine a new browsing context is created, a MessageHandlerFrameChild actor will be created and will have to review the session data to know if it needs to spawn a MessageHandler (and start listeners etc...). So at this point we will need to know if the BrowsingContext for this MessageHandlerFrameChild instance matches the tab for which we subscribed to log.entryAdded. That's where I imagine that browserId is useful. I'm not sure if you could use permanentKey in that situation.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to Julian Descottes [:jdescottes] from comment #5)

Not sure, the use case I am specifically thinking about is if we have some session data which is scoped to a given tab (eg subscribing to log.entryAdded only for a specific top-level browsing context). Imagine a new browsing context is created, a MessageHandlerFrameChild actor will be created and will have to review the session data to know if it needs to spawn a MessageHandler (and start listeners etc...). So at this point we will need to know if the BrowsingContext for this MessageHandlerFrameChild instance matches the tab for which we subscribed to log.entryAdded. That's where I imagine that browserId is useful. I'm not sure if you could use permanentKey in that situation.

Personally I would avoid to use browserId given the issues with unloaded tabs and all of the ids being 0. For a new browsing context including a top-level one we should still be able to identify the right browser via a unique identifier, and so far it seems to be permanentKey. But maybe there might be an alternative solution? Or maybe the browserId should not be set to 0 if a page gets unloaded? Nevertheless I think it might make sense to ask someone from the platform team for help.

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #6)

(In reply to Julian Descottes [:jdescottes] from comment #5)

Not sure, the use case I am specifically thinking about is if we have some session data which is scoped to a given tab (eg subscribing to log.entryAdded only for a specific top-level browsing context). Imagine a new browsing context is created, a MessageHandlerFrameChild actor will be created and will have to review the session data to know if it needs to spawn a MessageHandler (and start listeners etc...). So at this point we will need to know if the BrowsingContext for this MessageHandlerFrameChild instance matches the tab for which we subscribed to log.entryAdded. That's where I imagine that browserId is useful. I'm not sure if you could use permanentKey in that situation.

Personally I would avoid to use browserId given the issues with unloaded tabs and all of the ids being 0. For a new browsing context including a top-level one we should still be able to identify the right browser via a unique identifier, and so far it seems to be permanentKey. But maybe there might be an alternative solution? Or maybe the browserId should not be set to 0 if a page gets unloaded? Nevertheless I think it might make sense to ask someone from the platform team for help.

We are talking about the content process here, I don't think we can even get the permanent key object from here.

Comment 8

[Julian Descottes [:jdescottes]]

I imagine that whenever a browsing context gets created in a given tab, it is no longer unloaded. So in theory at this point, some BrowserId should be properly defined for it.

If we consider the following use case:

• Firefox opened with an unloaded tab
• subscribe to log.entryAdded for the unloaded tab
• switch to the unloaded tab

When we subscribe, the browserId for the unloaded tab will be 0. But when we switch to the unloaded tab and the tab content actually loads we should have a valid browserId. But the question is what would make sense to be set in our session data. I really doubt we can check permanentKey in the content process. However we could use the UUID computed for the unloaded tab in our session data, and separately provide via sharedData a map between UUID -> browserId, which we would update whenever a top level browsing context gets created.

The goal is to make sure that we have enough information in the content process so that we can decide whether a MessageHandlerFrameChild should create a MessageHandler. Maybe you were thinking about doing that check in the parent process, but one of our goals is to be able to setup the MessageHandler as early as possible in order to enable features such as early breakpoints.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to Julian Descottes [:jdescottes] from comment #8)

However we could use the UUID computed for the unloaded tab in our session data, and separately provide via sharedData a map between UUID -> browserId, which we would update whenever a top level browsing context gets created.

I think that sounds reasonable and it should actually not happen that often that we have to deal with unloaded tabs because 1) the feature is disabled for WebDriver and 2) we only see unloaded tabs when restoring a previous session via session restore.

The goal is to make sure that we have enough information in the content process so that we can decide whether a MessageHandlerFrameChild should create a MessageHandler. Maybe you were thinking about doing that check in the parent process, but one of our goals is to be able to setup the MessageHandler as early as possible in order to enable features such as early breakpoints.

Yes, that sounds less complex and we won't have to do a sync call to the parent process. The only thing we have to be aware of is that the window global child has an up-to-date sessionData entry (e.g unloaded tab gets loaded for a page with frames) before querying this information.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1747222 - [remote] Migrate TOP BROWSING CONTEXT to use custom UUID instead of browserId

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1747222 - [remote] Move browsing context APIs from browser.js and WindowManager to TabManager

Depends on D135414

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1747222 - [marionette] Move generic marionette/browser.js APIs to TabManager

Depends on D135415

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23d2f0bfe243
[remote] Migrate TOP BROWSING CONTEXT to use custom UUID instead of browserId r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/bec07511dd3c
[remote] Move browsing context  APIs from browser.js and WindowManager to TabManager r=webdriver-reviewers,whimboo

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/23d2f0bfe243
https://hg.mozilla.org/mozilla-central/rev/bec07511dd3c