1914407 - Improve error message when navigation fails due to a window.location update

Status: ASSIGNED

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

Description

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

Originally filed at: https://github.com/puppeteer/puppeteer/issues/12929

We should check what exactly is causing us to fail the navigation command.

Comment 1

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

Attachment: BCWebProgress:5 log

The problem here seems to be the redirect to https://twitter.com. Not sure how this is done but it looks like it's not a usual redirect, but maybe they manually call window.location as well?

Comment 2

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

Attachment: Raw response for https://x.com

Actually using DevTools network monitor the raw response from loading https://x.com shows indeed that window.location.href is used but there is also a call to replaceState. Not sure which exactly is executed here.

Comment 3

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

This bug seems to rely on the same underlying discussion as bug 1914405.

Comment 4

[Julian Descottes [:jdescottes]]

I tried to investigate a bit why nsIRequest.canceledReason didn't give any extra info here.
When the load is interrupted by a window.location update, as pointed out by :Nika, the docshell will stop the load at https://searchfox.org/mozilla-central/rev/cc01f11adfacca9cd44a75fd140d2fdd8f9a48d4/docshell/base/nsDocShell.cpp#9388-9401

// Stop any current network activity.
// Also stop content if this is a zombie doc. otherwise
// the onload will be delayed by other loads initiated in the
// background by the first document that
// didn't fully load before the next load was initiated.
// If not a zombie, don't stop content until data
// starts arriving from the new URI...

if ((mDocumentViewer && mDocumentViewer->GetPreviousViewer()) ||
    LOAD_TYPE_HAS_FLAGS(aLoadState->LoadType(), LOAD_FLAGS_STOP_CONTENT)) {
  rv = Stop(nsIWebNavigation::STOP_ALL);
} else {
  rv = Stop(nsIWebNavigation::STOP_NETWORK);
}

And interestingly, we actually cancel the requests with a proper reason "nsDocLoader::Stop". While this wouldn't be enough to differentiate between a stop due to a navigation and one due to the user canceling the navigation (eg clicking on the cancel icon or pressing escape), it would still be interesting to have this reason available to our Navigation helper.

However the request available from the WebProgress in the parent process is a RemoteWebProgressRequest, which is not the real request, and this request doesn't have the proper canceledReason set.

Nika, Valentin: do you know if we could get the canceledReason for the real request and set it correctly on the RemoteWebProgressRequest?

[On the other hand, I tried to check if we could reuse our NavigationManager to detect the new navigation, but sadly in the parent process we seem to get the notification about the new navigation after we get NS_ABORTED.]

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Julian Descottes [:jdescottes] from comment #4)

Nika, Valentin: do you know if we could get the canceledReason for the real request and set it correctly on the RemoteWebProgressRequest?

RemoteWebProgressRequest is a tiny type used for web progress listeners in the parent process, so doesn't have any extra details on it beyond the basic mURI, mOriginalURI, and mMatchedList (https://searchfox.org/mozilla-central/rev/90dc3743ea284c258148124dc54dfd123e82586f/dom/ipc/RemoteWebProgressRequest.h#29-32). While the mCanceledReason exists because of being on nsIRequest, it is not included in the RequestData struct sent over IPC (https://searchfox.org/mozilla-central/rev/90dc3743ea284c258148124dc54dfd123e82586f/dom/ipc/PBrowser.ipdl#125-130), and therefore isn't transferred across processes.

If this is something which you think would be valuable to have available on remote web progress notifications, we could add it as an extra piece of information being sent down, but we unfortunately can't easily have a "real" channel available on these progress notifications.

Comment 6

[Julian Descottes [:jdescottes]]

Thanks for the feedback, let's discuss this during triage today. I think that would allow us to at least craft a better error message, and potentially wait for the next navigation (although our spec currently doesn't support this).

Comment 7

[Julian Descottes [:jdescottes]]

After discussing we agree it would be great to have the canceled reason on this request, and based on this we will emit a better error message. We should also add a wdspec test for this.

Also we should probably write a script to check how BiDi behaves when navigating to eg the top 1000 sites.

Comment 8

[Julian Descottes [:jdescottes]]

Interestingly, when writing a wdspec test to reproduce the issue, I also get a failure with chrome:

future     = <Future finished exception=UnknownErrorException(unknown error, navigation canceled, as new navigation is requested by scriptInitiated, Error
    at new UnknownErrorException (<anonymous>:81:4591)
    at <anonymous>:597:6717
    at <anonymous>:1:1010
    at Array.map (<anonymous>)
    at Object.emit (<anonymous>:1:993)
    at MapperCdpClient.emit (<anonymous>:17:387)
    at #onMessage (<anonymous>:823:2977)
    at WindowCdpTransport.window.cdp.onmessage (<anonymous>:888:4763)
    at <anonymous>:1:18)>

Maybe this doesn't occur in puppeteer because of specific feature which gets enabled in Chrome by puppeteer?

Comment 9

[Julian Descottes [:jdescottes]]

And I also get the same when connecting the client from https://juliandescottes.github.io/bidi-web-client/web/ to a regular Chrome instance and doing a navigation to mozilla.org.

So at least the behavior seems consistent in both browsers, although arguably the error message is much more helpful in Chrome.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1914407 - Add canceledReason to RemoteWebProgressRequest

The canceledReason is added to the RequestData used to build the
RemoteWebProgressRequest. This canceledReason is useful for consumers such
as WebDriverBiDi which need to emit different error messages depending on
the reason why a specific navigation request was canceled.

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1914407 - [bidi] Send custom error when navigation is aborted with nsDocLoader::Stop

Depends on D223174

Comment 12

[Phabricator Automation]

Comment on attachment 9426551 [details]
Bug 1914407 - Add canceledReason to RemoteWebProgressRequest

Revision D223174 was moved to bug 1921119. Setting attachment 9426551 [details] to obsolete.

Comment 13

[amunizp]

Cool! so this is an update on firefox that will help me run puppeteer with it as announced here? https://hacks.mozilla.org/2024/08/puppeteer-support-for-firefox/

Comment 14

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

(In reply to amunizp from comment #13)

Cool! so this is an update on firefox that will help me run puppeteer with it as announced here? https://hacks.mozilla.org/2024/08/puppeteer-support-for-firefox/

You are already able to use Puppeteer with Firefox via the Webdriver BiDi protocol. It's just that in some cases websites do quirky stuff like setting window.location to a different URL itself to trigger kinda redirect. That currently fails and bug 1921973 got filed for that. Here we are only updating the error message to make it more clear to the user why the call to the browsingContext.navigate command failed.