1663641 - "WebDriver:GetCurrentURL" should not return "about:error" prefixes for erroneous navigation requests

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

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

Since bug 1658928 has been fixed we return the about:error prefix for URLs when a former navigation wasn't successful. Here an example for an invalid certificate failure:

 0:10.13 pid:63104 1599568819860	Marionette	DEBUG	0 -> [0,4,"WebDriver:Navigate",{"url":"https://web-platform.test:8443/webdriver/tests/support/data/test.html"}]
 0:10.13 pid:63104 1599568819862	Marionette	TRACE	Using browsing context 22
 0:10.13 pid:63104 1599568819866	Marionette	TRACE	[22] Received DOM event beforeunload for about:blank
 0:10.15 pid:63104 1599568819883	Marionette	TRACE	[22] Received DOM event beforeunload for about:blank
 0:10.15 pid:63104 1599568819885	Marionette	TRACE	[22] Received DOM event pagehide for about:blank
 0:10.29 pid:63104 1599568820027	Marionette	TRACE	[22] Received DOM event DOMContentLoaded for about:certerror?e=nssBadCert&u=https%3A//web-platform.test%3A8443/webdriver/tests/support/data/test.html&c=UTF-8&d=%20
 0:10.30 pid:63104 1599568820033	Marionette	DEBUG	0 <- [1,4,{"error":"insecure certificate","message":"","stacktrace":"WebDriverError@chrome://marionette/content/error.js:175:5\nIn ... adyState@chrome://marionette/content/listener.js:295:21\nhandleEvent@chrome://marionette/content/listener.js:266:14\n"},null]
 0:10.32 pid:63104 1599568820045	webdriver::server	DEBUG	<- 400 Bad Request {"value":{"error":"insecure certificate","message":"","stacktrace":"WebDriverError@chrome://marionette/content/error.js:175:5\nInsecureCertificateError@chrome://marionette/content/error.js:296:5\nhandleReadyState@chrome://marionette/content/listener.js:295:21\nhandleEvent@chrome://marionette/content/listener.js:266:14\n"}}
 0:10.32 pid:63104 1599568820047	webdriver::server	DEBUG	-> GET /session/3b0cd829-c514-9440-8681-eade0e5b5686/url
 0:10.32 pid:63104 1599568820052	Marionette	DEBUG	0 -> [0,5,"WebDriver:GetCurrentURL",{}]
 0:10.32 pid:63104 1599568820052	Marionette	TRACE	Using browsing context 22
 0:10.32 pid:63104 1599568820052	Marionette	DEBUG	0 <- [1,5,null,{"value":"about:certerror?e=nssBadCert&u=https%3A//web-platform.test%3A8443/webdriver/tests/support/data/test.html&c=UTF-8&d=%20"}]

Currently we make use of currentWindowGlobal.documentURI that returns this URL. Given that there is no other property on this class, I wonder what's best to return the real URL. Or is that a core bug and needs a fix in Firefox?

Comment 1

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

When handling and logging the DOM events I can use event.target.location.href, but again I don't see a way to directly get the location of the top-browsing context. Maybe we have to use the actor and get it from the child directly? At least as long as the WindowGlobalParent doesn't support it.

Comment 2

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

location.href is currently not reflected on WindowGlobalParent, though it would be possible to do so. Perhaps it could be reflected as GetExposedURI in the parent process?

Comment 3

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

Ok, so based on the conversation on Matrix we would have to use the Marionette actor for now, and retrieve the location via the actor child.

Comment 4

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

The code in question here is:

+  return new Promise(resolve => {
+    const timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+    timer.init(resolve, timeout, TYPE_ONE_SHOT);
+  });

When I add dump statements before calling init() and inside a callback wrapping resolve I can see that the timer never fires! It does't matter which value as timeout is passed in here. It's just randomly hanging. I also tried with initWithCallback but the same problem exists there.

I can reproduce easily when running the wdspec tests with the patch applied on MacOS too:

cd testing/geckodriver
cargo build
cd ../../
mach wpt --webdriver-binary=target/debug/geckodriver --webdriver-arg=-vv testing/web-platform/tests/webdriver/tests/perform_actions/pointer*

Olli, are you aware of any timer issue, which could lead in not firing the callback? What's strange here is that the TimedPromise uses the same technique but never has this problem.

Comment 5

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

Sorry that was the wrong bug.

Comment 6

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

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

Ok, so based on the conversation on Matrix we would have to use the Marionette actor for now, and retrieve the location via the actor child.

This would only apply when the actors are actually in use. In case of using the framescript we would need an endpoint in listener for getting the current location.

Comment 7

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

Attachment: Bug 1663641 - [marionette] Don't return the current URL with "about:error" prefixes.

This regressed by the patch on bug 1658928, which changed the type of
URL Marionette returns. While before we were returning the visible
URL from the location bar, the new and broken behavior includes
"about:error" prefixes for broken navigation requests.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77a41f4b6fa0
[marionette] Don't return the current URL with "about:error" prefixes. r=marionette-reviewers,maja_zf

Comment 9

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/77a41f4b6fa0

Comment 10

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

Comment on attachment 9175227 [details]
Bug 1663641 - [marionette] Don't return the current URL with "about:error" prefixes.

Beta/Release Uplift Approval Request

• User impact if declined: Since bug 1658928 landed for Firefox 81 the WebDriver command WebDriver:GetCurrentURL returns an URL with a about:error prefix for erroneous navigations like invalid certs. This majorly impacts users of geckodriver via Selenium and can cause problems in external test suites.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch partly reverts former changes only, and makes use of the framescript again.

Here a try build with the grafted changeset:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=189607f98903f2816e8f42140025e04a6e076214

• String changes made/needed: no

Comment 11

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 9175227 [details]
Bug 1663641 - [marionette] Don't return the current URL with "about:error" prefixes.

81 is now on m-r

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9175227 [details]
Bug 1663641 - [marionette] Don't return the current URL with "about:error" prefixes.

Approved for 81.0rc2.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/1d4d5f89aff0