Closed Bug 1763122 Opened 2 years ago Closed 5 months ago

Return a valid navigation id from the browsingContext.navigate command

Categories

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

task
Points:
3

Tracking

(firefox117 fixed)

RESOLVED FIXED
117 Branch
Tracking Status
firefox117 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webdriver:m8], [wptsync upstream] [webdriver:relnote])

Attachments

(2 files, 2 obsolete files)

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.

Whiteboard: [webdriver:backlog]
Priority: -- → P3

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?

Flags: needinfo?(nika)

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.

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.

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

Flags: needinfo?(nika)
Blocks: 1805405
Flags: needinfo?(jdescottes)
Depends on: 1835704
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

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

Attachment #9338632 - Attachment description: Bug 1763122 - Monitor navigations in content using dedicated bidi navigation module → Bug 1763122 - Monitor navigations in content using separate JSWindowActors
Attachment #9338632 - Attachment description: Bug 1763122 - Monitor navigations in content using separate JSWindowActors → Bug 1763122 - [remote] Add a NavigationManager to monitor navigations using JSWindowActors
Points: --- → 5
Priority: P3 → P2
Whiteboard: [webdriver:backlog] → [webdriver:m8]
Depends on: 1841010
No longer blocks: 1805405
Flags: needinfo?(jdescottes)

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.

Points: 5 → 3
Whiteboard: [webdriver:m8] → [webdriver:m8:blocked]
Whiteboard: [webdriver:m8:blocked] → [webdriver:m8]

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

Whiteboard: [webdriver:m8] → [webdriver:m8:blocked]

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.

Attachment #9338632 - Attachment is obsolete: true

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.

Attachment #9339356 - Attachment is obsolete: true
Pushed by jdescottes@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41123 for changes under testing/web-platform/tests
Whiteboard: [webdriver:m8:blocked] → [webdriver:m8:blocked], [wptsync upstream]
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch
Whiteboard: [webdriver:m8:blocked], [wptsync upstream] → [webdriver:m8], [wptsync upstream]
Upstream PR merged by moz-wptsync-bot
No longer depends on: 1835704
See Also: → 1835704
Whiteboard: [webdriver:m8], [wptsync upstream] → [webdriver:m8], [wptsync upstream] [webdriver:relnote]
You need to log in before you can comment on or make changes to this bug.