1750065 - Improve performance of getBrowserById with a map of UUID to browser element

Status: NEW

(Remote Protocol :: Agent, defect, P2)

Description

[Julian Descottes [:jdescottes]]

In Bug 1747222 we are introducing a getBrowserById method on remote/shared/TabManager.js which loops on all windows & tabs to see if any browser element matches the provided id.

Instead we could maintain a map from UUID to browser elements, but we therefore need to cleanup this map whenever a browser element is removed.

We will most likely have to keep track of all existing browser elements, similar to what is done in TargetObserver https://searchfox.org/mozilla-central/rev/ad732108b073742d7324f998c085f459674a6846/remote/cdp/observers/TargetObserver.sys.mjs, and everytime a browser element is removed, cleanup the appropriate entry in the map. Note that our UUIDs are bound to the permanentKey of <browser> elements. We will have to make sure there are no scenarios where the same permanent key might be reused for several browser elements, or we'll need to take this into account in our cleanup logic.

Comment 1

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

We might have to fix bug 1691494 first to get parity between CDP and WebDriver even when we have to accept a performance regression.

Comment 2

[Julian Descottes [:jdescottes]]

This can be a mentored bug, but not setting as good-first-bug as this will require to understand a bit how the browser handles browsing contexts. Hopefully we can rely on the current implementation in https://searchfox.org/mozilla-central/rev/ad732108b073742d7324f998c085f459674a6846/remote/cdp/observers/TargetObserver.sys.mjs as a starting point.

Comment 3

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

There is a BrowsingContext.getCurrentTopByBrowserId() helper available which actually let us find the current top-level browsing context based on the browser id. If we really have to use the browser element then we could get it via .embedderElement.

Comment 4

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

I'm not sure yet but with the work on bug 1274251 we might not necessarily need this anymore. Lets mark as dependency for now.

Comment 5

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

Let me actually fix that issue separately via this bug. I'll have a patch that is using the above method later today.

Comment 6

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

I was totally wrong here. The source id here is actually not the browserId but the unique navigable id. As such the proposed solution will not work.