2033769 - Ignored error in "WebDriver:Navigate" and "WebDriver:Refresh" when triggering the actual navigation fails

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

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

When trying to navigate to a page that would cause an error to be thrown within lazy.navigate.navigateTo() we currently completely miss this error because this method is marked async and we do not return the Promise here:

https://searchfox.org/firefox-main/rev/ad5f057320ecc6b934dfa1e3ec361f87712806cc/remote/marionette/driver.sys.mjs#2536

As such the code in lazy.navigate.waitForNavigationCompleted() is not going to wait for the Promise to be resolved and we silently ignore the error.

Solution should add a return statement to the code above to ensure we always return a value but also we can change lazy.navigate.navigateTo() to not be async.

Not sure how to actually trigger that situation given that we already filter out invalid URLs before we start to navigate. At least for now I can see it with my upcoming patch for bug 1579790.

Comment 1

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

The WebDriver:Refresh command is affected as well. But in both cases it's currently unlikely that this particular code path is hit.

Comment 2

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

Attachment: Bug 2033769 - [marionette] Fix "WebDriver:Navigate" and "WebDriver:Refresh" to not miss errors when triggering the actual navigation.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/fdf197f64ce8
https://hg.mozilla.org/integration/autoland/rev/c229b0bc7846
[marionette] Fix "WebDriver:Navigate" and "WebDriver:Refresh" to not miss errors when triggering the actual navigation. r=Sasha

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0cb86fb959fa
https://hg.mozilla.org/integration/autoland/rev/dcf7683ebfa3
Revert "Bug 2033769 - [marionette] Fix "WebDriver:Navigate" and "WebDriver:Refresh" to not miss errors when triggering the actual navigation. r=Sasha" for causing wp failures at /ink-overflow-001-print.html

Comment 5

[rperta]

Backed out for causing wp failures at /ink-overflow-001-print.html
Backout link
Push with failures
Failure log(s)

Comment 6

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

The problem here is an exception for the reload command:
https://treeherder.mozilla.org/logviewer?job_id=561810342&repo=autoland&task=CmsDOrpgSYORhH-R8fYGpQ.0&lineNumber=2855

[task 2026-04-22T19:41:52.888+00:00] 19:41:52     INFO - [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISHistory.reload]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://remote/content/marionette/navigate.sys.mjs :: navigate.refresh :: line 187"  data: no]
[task 2026-04-22T19:41:52.888+00:00] 19:41:52     INFO - navigate.refresh@chrome://remote/content/marionette/navigate.sys.mjs:187:36
[task 2026-04-22T19:41:52.888+00:00] 19:41:52     INFO - loadTestUrl/<@chrome://remote/content/marionette/reftest.sys.mjs:669:23

It's exactly that kind of error that we actually missed so far and it's good to see that tests started to fail. Beside the above mentioned tests a couple more wpt reftests are failing:

https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=c229b0bc7846d8dafc25dd237f4e15805e96b9af&selectedTaskRun=QxIMCdXTS4C_fzl-5uyeBg.0

What's failing is the call to browsingContext.sessionHistory.reload(flags) because that's what we call now when the session history is available since the patch from Julian on bug 2023917 landed. I wonder if calling browsingContext.reload(flags) would work.

Comment 7

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

Here a try build with only this patch triggered for the print reftests:
https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=38388

And another try build forcing the reload via browsingContext.reload():
https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=38389

Comment 8

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

The original call via browsingContext.reload() still works as the latest try builds show.

Julian pointed me to some extra checks that other components do before calling sessionHistory.reload() and I was trying to see if that could be a solution for us as well. I pushed a try build for just logging the index and count of the sessionHistory before calling reload:

https://treeherder.mozilla.org/logviewer?job_id=561985438&repo=try&task=C1HE-HjsRbeicyMOIntSTQ.0&lineNumber=12036-12039

As it can be seen the failure happens when count=0 and index=-1. Under this situation the following check fails:
https://searchfox.org/firefox-main/rev/986b2e474acca54ab469aca8d549b71cdcea33a5/docshell/shistory/nsSHistory.cpp#1705

Comment 9

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

Attachment: Bug 2033769 - [remote] Use sessionHistory.reload() only when history items exist.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a3e1548a205c
https://hg.mozilla.org/integration/autoland/rev/cec4e04a8ecf
[remote] Use sessionHistory.reload() only when history items exist. r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/03941e2efdcb
https://hg.mozilla.org/integration/autoland/rev/caeb115e161b
[marionette] Fix "WebDriver:Navigate" and "WebDriver:Refresh" to not miss errors when triggering the actual navigation. r=Sasha

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/42d2666083b2
https://hg.mozilla.org/integration/autoland/rev/40e1905e43e9
Revert "Bug 2033769 - [marionette] Fix "WebDriver:Navigate" and "WebDriver:Refresh" to not miss errors when triggering the actual navigation. r=Sasha" for causing wd failures @ reload/frame.py

Comment 12

[amarc]

Backed out for causing wd failures

• Backout link
• Push with failures
• Failure Log

Comment 13

[Julian Descottes [:jdescottes]]

I think we might want to try sessionHistory?.count && sessionHistory?.index >= 0 instead of sessionHistory?.count && sessionHistory?.index > 0. If the context is on its first URL sessionHistory.count === 1 and sessionHistory.index === 0, but we should still be able to use this in order to reload via session history.

Hopefully that also works fine for /ink-overflow-001-print.html

Comment 14

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

Thanks Julian. Yes, that makes total sense. Not sure why I used > 0 in this case. Nevertheless I do not understand why I'm not getting this affected wdspec test to fail locally while it's perma failing in CI.

Comment 15

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

I pushed a new try build for verification:
https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=39514

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a2fd58e47d66
https://hg.mozilla.org/integration/autoland/rev/0af9aa4ca303
[remote] Use sessionHistory.reload() only when history items exist. r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/e180b024c363
https://hg.mozilla.org/integration/autoland/rev/292ba1707ee0
[marionette] Fix "WebDriver:Navigate" and "WebDriver:Refresh" to not miss errors when triggering the actual navigation. r=Sasha

Comment 17

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/0af9aa4ca303
https://hg.mozilla.org/mozilla-central/rev/292ba1707ee0