1756595 - Implement "browsingContext.navigationStarted" event

Status: NEW

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

Description

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

We should be able to fully implement the event. But if it turns out that the navigation id is causing us problems we could move it to a follow-up bug.

Note that when using the WebProgress listener we have a reference to the nsIChannel via the request argument. This could be used to retrieve the channel id for the network request.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1756595 - [bidi] WIP Implement "browsingContext.navigationStarted" event

Comment 2

[Julian Descottes [:jdescottes]]

Won't be working on this for now.

Comment 3

[Julian Descottes [:jdescottes]]

Hi Olli,

The browsingContext.navigationStarted event is supposed to be triggered at step 12 of https://html.spec.whatwg.org/#beginning-navigation
Do you know what corresponds to this in our implementation and if this is observable from chrome JS? For now I'm using a WebProgressListener and STATE_START, but not sure if that's a good match? If it is do you know if I should rather monitor this in the content process or in the parent process?

thanks!

Comment 4

[Julian Descottes [:jdescottes]]

do you know if I should rather monitor this in the content process or in the parent process?

For the record I'm prototyping both, but I have issues with both...
For the parent process approach, I observe browsing-context-attached, and add a webprogress listener for each new browsing context. This seems to work fine in most cases, but there are some edge cases where I miss the STATE_START, for instance with nested iframes using data: uri, such as:

data:text/html,<meta charset=utf8>%0A%20%20%20%20%3Ch1%3ETop%20level%20header%3C%2Fh1%3E%0A%20%20%20%20%3Cp%3EThis%20is%20a%20paragraph.%3C%2Fp%3E%0A%20%20%20%20%3Ciframe%20src%3D%22data%3Atext%2Fhtml%2C%3Cmeta%20charset%3Dutf8%3E%0A%20%20%20%20%20%20%20%20%3Ch2%3EIframe%20header%3C%2Fh2%3E%0A%20%20%20%20%20%20%20%20%3Ciframe%20src%3D'data%3Atext%2Fhtml%2C%3Cmeta%20charset%3Dutf8%3E%3Ch2%3ENested%20Iframe%20header%3C%2Fh2%3E%0A%20%20%20%20%20%20%20%20'%3E%3C%2Fiframe%3E%0A%20%20%20%20%22%3E%3C%2Fiframe%3E

I can never observe STATE_START for the iframes.

If I switch to the content process, and create webprogress listeners there, I can observe all of them. The simplified approach is to have a JSWindowActor instantiated for each and every window, and start a webprogress listener as soon as possible. But in that case I get 2 STATE_START for each navigation. One very early, before the new JSWindowActor is instantiated, and another one much later. Both for the same target URI, but a key difference is that the request for the second one is linked to a valid channel (I can get its channel id etc...).

It feels like monitoring things from the content process is more reliable (at least I haven't found a scenario where I miss navigations yet), but I'm not sure how to deal with those duplicate notifications. I was suspecting that maybe calling addProgressListener once the JSWindowActor spawns forced the webprogress to emit a fake STATE_START? So I was updated my patch to only add a listener once for a given WebProgress, but I still have 2 different notifications for the same navigation. And beyond checking the request's channel, I could not find a good way to differentiate those 2 notifications.

Comment 5

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

Julian, do you have some output with MOZ_LOG="BCWebProgress:5" that you could attach? I might help here.

Comment 6

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

hmm, isn't STATE_START a bit too late. Aren't beforeunload listener called already at that point. And in the spec 16.1 does beforeunload dispatching.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1756595 - [bidi] Implement "browsingContext.navigationStarted" event

WIP to complement the tests

Comment 8

[Julian Descottes [:jdescottes]]

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

hmm, isn't STATE_START a bit too late. Aren't beforeunload listener called already at that point. And in the spec 16.1 does beforeunload dispatching.

We discussed this a bit more on Elements, and indeed beforeunload is dispatched before we get a STATE_START notification.
There is no earlier webprogress event we could use, so we are missing notifications to really monitor the early stages of a navigation.
We can still implement some downgraded version of the event if we want, but it will not be emitted at the right time and will not work for pages using a beforeunload listener.

For the record, also thought about actually listening to beforeunload as a temporary workaround, but that doesn't seem like a good fit, because we need to emit the URL targeted by the navigation and this information is not accessible from the beforeunload event.

From here, we need to list all BiDi events which require a navigation id, and check with DOM peers what we need to change on the platform side to observe those.

We still need to figure out if this would be better observed in the parent process or in the content process, but we can come back to that question later.

Unassigning and moving back to backlog for now, this is more complex than expected. I added my current patches for reference (tests + parent process approach)

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1756595 - [wdspec] Add wdspec tests for browsingContext.navigationStarted

Depends on D163993