1763122 - Return a valid navigation id from the browsingContext.navigate command

Status: RESOLVED FIXED

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

Description

[Julian Descottes [:jdescottes]]

We are landing a basic implementation for browsingContext.navigate in Bug 1730642, but we don't return a navigation id yet.

This navigation id should be a stable id that can represent the complete navigation. Our CDP implementation uses the channelId from network events, but BrowsingContext's CurrentLoadIdentifier might also be used here. CurrentLoadIdentifier is not yet exposed to chrome JS.

Comment 1

[Julian Descottes [:jdescottes]]

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1730642#c6 and https://bugzilla.mozilla.org/show_bug.cgi?id=1730642#c7 I tried to poke around BrowsingContext's currentLoadIdentifier but as far as I can tell it is usually set after we get notified about a STATE_START change in the we progress listener, and I think it gets "unset" quite early as well, so it doesn't seem like I could use this as a navigation id.

For webdriver bidi we need to expose a unique navigation id in various events and commands, so that consumers can know which navigation relates to which events/commands. This navigation id is the one you can find in the specs at https://html.spec.whatwg.org/#navigation-id, but I don't really know how to re-create this in Firefox.

This id is supposed to be available very early in the navigation (for BiDi I think the first observable event which should have it is the one triggered from step 12 of https://html.spec.whatwg.org/#beginning-navigation.

Nika, do you have suggestions here? Is this navigation id something we could implement on top of some other id / object that should be available for any navigation (even those which don't involve the network)? Or would this require more changes on the platform side?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Unfortunately, as far as I know we don't currently have an implementation of the navigation-id concept in the browser. We do, in roundabout ways, track navigations from source to destination, but IIRC it's not handled with a consistent UUID identifier, but rather with a bunch of independent mechanisms.

Off the top of my head there's:

• The nsDocShellLoadState::LoadIdentifier() (the same one which is used for currentLoadIdentifier)
  • This isn't available in WebProgressListener notifications, and isn't consistently available in content processes.
  • As you mentioned, it's currently cleared when redirecting to the final process, rather than when the load is actually finished, as DocumentLoadListener currently fully hands-off control over the load to nsDocLoader/necko once it's been targeted to the final process.
• The nsIIdentChannel::channelId
  • This was added for the devtools netmonitor to track a HttpChannel across processes, and is only implemented for DocumentChannel and HttpChannel, so wouldn't work for any non-standard channels.
  • I don't think this necessarily persists across redirects, but I can't remember for sure.
  • Not currently available in parent-process WebProgressListener notifications, but would perhaps be the easiest to add.
• The SHIP-only LoadingSessionHistoryEntry's LoadId
  • This is only available with SHIP (which should hopefully be the only session history backend soon)
  • I believe is often not set until we're in the parent process
  • Not available in WebProgressListener notifications, and not clearly directly connected to them (as those notifications are generally triggered through the DocLoader)

My intuition suggests that the channelId won't work for your purposes, as it's intended for very different things. In many ways the LoadingSessionHistoryEntry feels like it might be appropriate, as it should often be available on the DocShell during the final stages of the load, but it won't be present for many error cases, or when starting the load, as we often only fill it in in the parent process, and tell content once it's actually going to load something.

I think enhancing LoadIdentifier() until it's actually usable for this purpose is probably the cleanest pathway forward here, though it might be tricky to get everything to line up properly across processes. Other than that, most code needing to track a specific navigation tends to track it manually, using progress listeners to tell when it has completed.

Comment 3

[Julian Descottes [:jdescottes]]

Thanks a lot for the detailed answer Nika!

channelId
I don't think this necessarily persists across redirects, but I can't remember for sure.

I don't think it is. I had to use the WrappedChannel's id to get a consistent id across redirects.
It sounds tempting to expose it somehow the parent process webprogresslistener, but that probably won't work for navigations which don't hit the network.

Since there's nothing which perfectly matches at the moment, I think we will move forward with some manual tracking based on webprogresslisteners. We can file another bug to improve LoadIdentifier(), not sure we will have time to work on it.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

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

Since there's nothing which perfectly matches at the moment, I think we will move forward with some manual tracking based on webprogresslisteners. We can file another bug to improve LoadIdentifier(), not sure we will have time to work on it.

Yeah, whatever mechanism ends up being built even if it uses LoadIdentifier() will probably need to work based on web progress listeners in some way, so that's probably what I'd reach for as well.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1763122 - [remote] Add a NavigationManager to monitor navigations using JSWindowActors

Comment 6

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

Just as an information, this is a feature Puppeteer would need for correctly returning the network response from page.goto().

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1763122 - [remote] Start monitoring navigations on WebDriver session startup

Depends on D180440

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1763122 - [bidi] Add navigation id to browsingContext.navigate result

Depends on D181127

Comment 9

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

Bug 1841010 will cover the retrieval of the navigation id. As such this bug gets simpler and we agreed on to reduce the points appropriately.

Comment 10

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

This is still blocked on bug 1841010 as I wrote in comment 9.

Comment 11

[Phabricator Automation]

Comment on attachment 9338632 [details]
Bug 1763122 - [remote] Add a NavigationManager to monitor navigations using JSWindowActors

Revision D180440 was moved to bug 1841010. Setting attachment 9338632 [details] to obsolete.

Comment 12

[Phabricator Automation]

Comment on attachment 9339356 [details]
Bug 1763122 - [remote] Start monitoring navigations on WebDriver session startup

Revision D181127 was moved to bug 1841010. Setting attachment 9339356 [details] to obsolete.

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: Bug 1763122 - [wdspec] Add tests for navigation id in browsingContext navigate tests

Depends on D181129

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0537362c5280
[bidi] Add navigation id to browsingContext.navigate result r=webdriver-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/6edc9521a415
[wdspec] Add tests for navigation id in browsingContext navigate tests r=webdriver-reviewers,jgraham

Comment 15

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41123 for changes under testing/web-platform/tests

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/0537362c5280
https://hg.mozilla.org/mozilla-central/rev/6edc9521a415

Comment 17

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot