Closed Bug 1756595 Opened 2 years ago Closed 5 months ago

Implement "browsingContext.navigationStarted" event

Categories

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

task
Points:
3

Tracking

(firefox119 fixed)

RESOLVED FIXED
119 Branch
Tracking Status
firefox119 --- fixed

People

(Reporter: whimboo, Assigned: jdescottes)

References

(Blocks 1 open bug, )

Details

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

Attachments

(5 files, 2 obsolete files)

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.

See Also: → 1756610
Depends on: 1730642
Points: --- → 8
Priority: -- → P3
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

Won't be working on this for now.

Assignee: jdescottes → nobody
Status: ASSIGNED → NEW
Whiteboard: [bidi-m3-mvp] → [webdriver:backlog]
Priority: P3 → P2
Points: 8 → 3

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!

Flags: needinfo?(smaug)

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.

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [webdriver:backlog] → [webdriver:m5]

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

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.

Flags: needinfo?(smaug)
Attachment #9276889 - Attachment is obsolete: true

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] 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)

Assignee: jdescottes → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
Whiteboard: [webdriver:m5] → [webdriver:backlog]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Assignee: jdescottes → nobody
Status: ASSIGNED → NEW
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Assignee: jdescottes → nobody
Status: ASSIGNED → NEW

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.

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

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.

Attachment #9338288 - Attachment is obsolete: true
Depends on: 1841010

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.

Blocks: 1854622
Pushed by jdescottes@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/42154 for changes under testing/web-platform/tests
Whiteboard: [webdriver:backlog] → [webdriver:backlog], [wptsync upstream]
Upstream PR merged by moz-wptsync-bot
Whiteboard: [webdriver:backlog], [wptsync upstream] → [webdriver:m8],[wptsync upstream]
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.