1763124 - Handle navigation to error pages for browsingContext.navigate

Status: RESOLVED FIXED

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

Description

[Julian Descottes [:jdescottes]]

Follow up to handle more specific navigation use cases.
We should support navigation to error pages, such as about:neterror

Comment 1

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

As discussed we will try to have a look next week as long as we are working on navigation in M3.

Comment 2

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

Note that the WebProgress listener supports the LOCATION_CHANGE_ERROR_PAGE flag for onLocationChange notifications:
https://searchfox.org/mozilla-central/rev/da6a85e615827d353e5ca0e05770d8d346b761a9/uriloader/base/nsIWebProgressListener.idl#431-443

Comment 3

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

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #2)

Note that the WebProgress listener supports the LOCATION_CHANGE_ERROR_PAGE flag for onLocationChange notifications:
https://searchfox.org/mozilla-central/rev/da6a85e615827d353e5ca0e05770d8d346b761a9/uriloader/base/nsIWebProgressListener.idl#431-443

This is actually not the right solution. Instead we should still check a possible failure state when the STATE_STOP state change is notified:
https://searchfox.org/mozilla-central/rev/7f729f601c0b738f870ae0ed49098f9268e250f9/uriloader/base/nsIWebProgressListener.idl#375-387

That way we should also be able to return details of the error to the client similar to what Marionette does:

[task 2022-05-19T11:34:04.132Z] 11:34:04 INFO - 1652960044132 Marionette DEBUG 5 <- [1,38,{"error":"unknown error","message":"Reached error page: about:neterror?e=unknownProtocolFound&u=thisprotocoldoesnotexis ... /EventEmitter.jsm:160:20\nreceiveMessage@chrome://remote/content/marionette/actors/MarionetteEventsParent.jsm:44:25\n"},null]

Comment 4

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

In XPCOM we have very details error messages:
https://searchfox.org/mozilla-central/source/js/xpconnect/src/xpc.msg#132

It would be great to use the appropriate one with the error message as returned to the client. Sadly there is only ChromeUtils.getXPCOMErrorName() but nothing for the message. After speaking to Smaug it might make sense to add a new method here that would allow to return the error message instead.

Comment 5

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

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #4)

It would be great to use the appropriate one with the error message as returned to the client. Sadly there is only ChromeUtils.getXPCOMErrorName() but nothing for the message. After speaking to Smaug it might make sense to add a new method here that would allow to return the error message instead.

Nika, do you know if there is already a way to get these messages from JS modules? If not would ChromeUtils.cpp be a good place for that as well? Thanks.

Comment 6

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

It looks like there are cases when a navigation doesn't start at all and as such also no state change notification is sent. Instead I only see a location change notification, so we would still have to use the flag as mentioned in comment 2.

 0:15.47 pid:31003 1652988345090	WebDriver BiDi	DEBUG	89d7cf72-a12d-41a5-aaea-ecfe3a540d38 -> {"id":8,"method":"browsingContext.navigate","params":{"context":"2a7edbee-d43e-4fec-aba2-9ae54bcfbee4","url":"http://localhost:0","wait":"complete"}}
 0:15.47 pid:31003 1652988345090	RemoteAgent	TRACE	Received command browsingContext.navigate for destination ROOT
 0:15.47 pid:31003 1652988345090	RemoteAgent	TRACE	Received command browsingContext._getBaseURL for destination WINDOW_GLOBAL
 0:15.47 pid:31003 1652988345090	RemoteAgent	TRACE	Created MessageHandler WINDOW_GLOBAL for session 89d7cf72-a12d-41a5-aaea-ecfe3a540d38
 0:15.47 pid:31003 1652988345090	RemoteAgent	TRACE	Received command browsingContext._getBaseURL for destination WINDOW_GLOBAL
 0:15.47 pid:31003 1652988345090	RemoteAgent	TRACE	Module windowglobal/browsingContext.jsm found for WINDOW_GLOBAL
 0:15.47 pid:31003 [Parent 31005: Main Thread]: I/BCWebProgress OnLocationChange({isTopLevel:1, isLoadingDocument:0}, {URI:http://localhost:0/, originalURI:http://localhost:0/}, http://localhost:0/, LOCATION_CHANGE_ERROR_PAGE) on {top:1, id:30, url:http://localhost:0/}
 0:15.47 pid:31003 1652988345097	RemoteAgent	TRACE	MessageHandler WINDOW_GLOBAL for session 89d7cf72-a12d-41a5-aaea-ecfe3a540d38 is being destroyed
 0:15.47 pid:31003 1652988345097	RemoteAgent	TRACE	Unregistered MessageHandler WINDOW_GLOBAL for session 89d7cf72-a12d-41a5-aaea-ecfe3a540d38
 0:15.47 pid:31003 [Parent 31005: Main Thread]: I/BCWebProgress OnSecurityChange(<null>, <null>, 4) on {top:1, id:30, url:http://localhost:0/}
 0:43.07 TEST_END: TIMEOUT, expected OK

Comment 7

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

I'm not aware of a convenient way to get these messages right now. For now, the main way they're used is when throwing an XPCOM exception across XPConnect IIRC. That file is also what is used to populate Cr[...] but the message isn't exposed there.

I'm generally fine with adding something to chromeutils if you need this for whatever reason I suppose? If you do take that approach, we should probably fetch it with nsXPCException::NameAndFormatForNSResult (https://searchfox.org/mozilla-central/rev/9b43f4743a4d1ffa8e9da4defe2675daa363a5cb/js/xpconnect/src/xpcprivate.h#1909-1910) rather than building out a new table.

Comment 8

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

Attachment: Bug 1763124 - [remote] Use Deferred instead of a plain Promise for ProgressListener.

Comment 9

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

Attachment: Bug 1763124 - [webdriver-bidi] Support error handling for browsingContext.navigate.

Depends on D146907

Comment 10

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

Attachment: Bug 1763124 - [wdspec] Add tests for erroneous requests for "browsingContext.navigate".

Depends on D146908

Comment 11

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

(In reply to Nika Layzell [:nika] (ni? for response) from comment #7)

I'm not aware of a convenient way to get these messages right now. For now, the main way they're used is when throwing an XPCOM exception across XPConnect IIRC. That file is also what is used to populate Cr[...] but the message isn't exposed there.

I'm generally fine with adding something to chromeutils if you need this for whatever reason I suppose? If you do take that approach, we should probably fetch it with nsXPCException::NameAndFormatForNSResult (https://searchfox.org/mozilla-central/rev/9b43f4743a4d1ffa8e9da4defe2675daa363a5cb/js/xpconnect/src/xpcprivate.h#1909-1910) rather than building out a new table.

Thanks Nika. I think for now we should be fine with just the error names given that these explain the failure quite well. If it turns out that we need even the extended data I would come back to your proposal in how to retrieve the message.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/500c5063b7d3
[remote] Use Deferred instead of a plain Promise for ProgressListener. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/a2dc6a551dcf
[webdriver-bidi] Support error handling for browsingContext.navigate. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/465c1e1af734
[wdspec] Add tests for erroneous requests for "browsingContext.navigate". r=webdriver-reviewers,jdescottes

Comment 13

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

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

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/500c5063b7d3
https://hg.mozilla.org/mozilla-central/rev/a2dc6a551dcf
https://hg.mozilla.org/mozilla-central/rev/465c1e1af734

Comment 15

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

Upstream PR was closed without merging

Comment 16

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

Upstream PR was closed without merging

Comment 17

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

Upstream PR merged by whimboo