1632008 - Page.navigate should return loaderId for about:*, file: and data: urls

Status: RESOLVED FIXED

(Remote Protocol :: CDP, enhancement, P3)

Description

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Example

 Page.navigate({"url":"about:blank"})
 Page.navigate({"url":"about:blank"}) = {"frameId":"ECDF1F217C0F19BA543330D28204F18B","loaderId":"E5ECB29B91FA2DF71CCF5C44E9637147"}
 Page.frameStartedLoading({"frameId":"ECDF1F217C0F19BA543330D28204F18B"})
      Page.frameNavigated({"frame":{"id":"ECDF1F217C0F19BA543330D28204F18B","loaderId":"E5ECB29B91FA2DF71CCF5C44E9637147","mimeType":"text/html","securityOrigin":"://","url":"about:blank"}})
      Page.loadEventFired({"timestamp":82917.873615})
 Page.frameStoppedLoading({"frameId":"ECDF1F217C0F19BA543330D28204F18B"})
Page.domContentEventFired({"timestamp":82917.874269

Comment 2

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

(In reply to Julian Descottes [:jdescottes] from bug 1770135 comment #0)

When using Page.navigate we don't return a proper loaderId when the target url does not start with http/https: https://searchfox.org/mozilla-central/rev/7f729f601c0b738f870ae0ed49098f9268e250f9/remote/cdp/domains/parent/Page.jsm#107-110

Because of this, the current implementation of page.goto in puppeteer actually dismisses the event. Navigation works by accident because we incorrectly emit navigatedWithinDocument which tricks puppeteer into thinking that the navigation was a "same document navigation".

However we want to stop emitting those incorrect navigatedWithinDocument events and therefore we should fix navigate to always return a proper loaderId.

Note that this will be irrelevant for puppeteer once https://github.com/puppeteer/puppeteer/pull/8369 (re)lands, because loaderId will no longer be checked, but we need to ensure minimal backward compatibility with the current puppeteer so we can't just ignore the loaderId issue.

Comment 3

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

about:* documents will not served via the network layer and as such we won't have a loader id. I wonder if we should just set a random uuid instead, which we would have to store in a map keyed by the browsing context id. Then we could re-use the same id for the various navigation events.

Comment 4

[Julian Descottes [:jdescottes]]

Ah thanks for finding the duplicate, bugzilla didn't suggest it to me when I filed the new one.

Not directly related but I noticed we have a frameIdToLoaderId map already in the content Page domain: https://searchfox.org/mozilla-central/rev/7f729f601c0b738f870ae0ed49098f9268e250f9/remote/cdp/domains/content/Page.jsm#31-36

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1632008 - [cdp] Return a randomly generated loaderId for file/data/about navigations

Per CDP documentation, loaderId should be a string so we generate a random uuid here.
To make sure the loaderId will be consistent with other events, we notify the content Page domain
about this loaderId via "_updateLoaderId" which is already used to map browsing context ids to
loader ids.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9a06f126bb3
[cdp] Return a randomly generated loaderId for file/data/about navigations r=webdriver-reviewers,whimboo

Comment 7

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/d9a06f126bb3