1756595 - Implement "browsingContext.navigationStarted" event

Status: RESOLVED FIXED

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

Description

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

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+2]]

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

Comment 10

[Julian Descottes [:jdescottes]]

It seems that the issue I had with data-uri iframes has been fixed since I last tested this. All my tests are now passing with the parent process approach, which is great because it is much simpler. I will update the stack here.

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1756595 - Monitor navigations in content using dedicated bidi navigation module

Comment 12

[Phabricator Automation]

Comment on attachment 9338288 [details]
Bug 1756595 - Monitor navigations in content using dedicated bidi navigation module

Revision D180440 was moved to bug 1763122. Setting attachment 9338288 [details] to obsolete.

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: Bug 1756595 - [bidi] Add a NavigationListener wrapping NavigationManager events

Depends on D163994

The NavigationManager interface is inconsistent with the other listeners.
Wrapping it with a regular "Listener" pattern would help keep the modules using it easier to follow.

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: Bug 1756595 - [remote] NavigationManager should not emit navigation-started/stopped for same document navigations

Depends on D163993

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: Bug 1756595 - [remote] Update puppeteer expectations for navigationStarted and realmCreated

Depends on D188405

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9457ca1ce568
[bidi] Implement "browsingContext.navigationStarted" event r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/d37b2dc612db
[remote] NavigationManager should not emit navigation-started/stopped for same document navigations r=webdriver-reviewers,Sasha,whimboo
https://hg.mozilla.org/integration/autoland/rev/6848d4175058
[wdspec] Add wdspec tests for browsingContext.navigationStarted r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/cacc82330f54
[bidi] Add a NavigationListener wrapping NavigationManager events r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/855033897b90
[remote] Update puppeteer expectations for navigationStarted and realmCreated r=webdriver-reviewers,whimboo

Comment 17

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/9457ca1ce568
https://hg.mozilla.org/mozilla-central/rev/d37b2dc612db
https://hg.mozilla.org/mozilla-central/rev/6848d4175058
https://hg.mozilla.org/mozilla-central/rev/cacc82330f54
https://hg.mozilla.org/mozilla-central/rev/855033897b90

Comment 18

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

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

Comment 19

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

Upstream PR merged by moz-wptsync-bot