1922677 - Prevent new navigations triggered from abort handlers while an ongoing navigation is in progress

Status: ASSIGNED

(Core :: DOM: Navigation, defect, P2)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: server.js

The attached server will wait 1 second before responding to any requests. If you make a status file containing some text and serve the attached index.html it will give different results in different browsers during refresh.

• Firefox calls onabort on the XHR
• Chrome seems to silently drop the XHR
• Safari calls onerror on the XHR

This behaviour difference was observable on battle.net in bug 1919308. If you refresh the 2fa page in Firefox it will sometime redirect to the login page because onabort calls the jquery error handler in the following code:

	checkStatus: function() {
		$.ajax({
			method: 'GET',
			timeout: 30000,
			url: '/login/authenticator/status?check-' + (Math.random() + '') * 100000000000000000,
			beforeSend: function (){
				history.replaceState
					? history.replaceState(null, null, window.location.href.split('#')[0])
					: window.location.hash = '';
			},
			success: function (data) {
				var state = data.two_factor_state;

				if (state == "ACCEPTED" || state == "REJECTED") {
					pushButtonAuthenticator.isStatusCheckComplete = true;
					// Unschedule statusTimer on a successful response that will end up redirecting away from this view.
					clearTimeout(pushButtonAuthenticator.statusTimer);
					var queryString = '?';
					if (window.location.href.indexOf('?') !== -1) {
						queryString = '&';
					}
					window.location.href = window.location.href + queryString + (Math.random() + '') * 100000000000000000;
				}  else {
					pushButtonAuthenticator.statusTimer = setTimeout(function() {
						return pushButtonAuthenticator.checkStatus();
					}, pushButtonAuthenticator.statusFrequency);
				}
			},
			error: function() {
				// Prevent late responses from causing a redirect back to login after accepting/rejecting
				if (pushButtonAuthenticator.isStatusCheckComplete) {
					return;
				}

				// Only redirect if the user agent is still on the authenticator view.
				if (window.location.pathname.indexOf("/authenticator") > -1) {
					window.location.href =
						"/login/?app=" + pushButtonAuthenticator.getParam('app') +
						"&ref=" + pushButtonAuthenticator.getParam('ref');
				}
			}
		})
	},

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: index.html

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

This impacts a major site webcompat issue

Comment 3

[Valentin Gosu [:valentin]]

From my research I think Firefox is doing the right thing by calling onabort when the XHR is cancelled due to the page navigating/refreshing.
However, what doesn't make a lot of sense is for the onabort handler to be able to override the location from the handler - in bug 1919308 comment 3 - if I'm understanding the problem correctly.

Comment 4

[Valentin Gosu [:valentin]]

Attachment: nav.html - Firefox blocks location changes during unload as expected

Comment 5

[Valentin Gosu [:valentin]]

Attachment: index.html

Running this testcase allong with the server in attachment 9428936 [details] and refreshing the page will have different behaviour in Chrome vs Firefox.
Chrome will refresh and end up on the same URL.
Firefox calls onabort and ends up navigating to http://example.com
Safari calls onerror and ends up navigating to http://example.org

Comment 6

[Valentin Gosu [:valentin]]

I did a small try push to see if this works
https://treeherder.mozilla.org/jobs?repo=try&revision=813425d96e3b97e98063d6b7c053b280291f3a7b

Comment 7

[Valentin Gosu [:valentin]]

Attachment: WIP: Bug 1922677 - Don't allow navigations during nsDocLoader::Stop

Comment 8

[Valentin Gosu [:valentin]]

Olli, any feedback regarding attachment 9496667 [details] ?
Not entirely sure this is the correct approach.
try auto looks green https://treeherder.mozilla.org/jobs?repo=try&revision=813425d96e3b97e98063d6b7c053b280291f3a7b
Anne also said this is problematic (onunload not being allowed to navigate, but onabort being called during unload is allowed to), and he'd appreciate some WPTs.
I'll try to post those too.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

How does the patch work if one just calls window.stop() during pageload?
But otherwise, looks quite reasonable.

Comment 10

[Valentin Gosu [:valentin]]

Attachment: file_test_navigation_handlers.html

Comment 11

[Valentin Gosu [:valentin]]

Unfortunately my simple patch didn't really solve the issue, but I also found that the problem isn't limited to just abort handlers.

IMO when we navigate the docshell, we should make it ignore any other LoadURI calls that don't come from the browser or from user interaction.
I've found in attachment 9531993 [details] that the page handlers can even override user navigations via the URL bar which I don't think is right.
Not only can beforeunload navigate the page to a different location than expected, but even when some navigations are blocked (like in the unload event handler), one can still use setTimeout(0) to change the location after the restriction is lifted.

I think this seeds a more hollistic fix, and I'm probably not the best one to come up with it. Olli, could you take a look?

Comment 12

[Valentin Gosu [:valentin]]

See comment 11

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

After discussing with farre, it seems like we're missing https://html.spec.whatwg.org/#:~:text=If%20navigable%27s%20ongoing%20navigation%20is%20%22traversal%22%2C%20then

Comment 14

[Valentin Gosu [:valentin]]

Hi folks, would it be possible to prioritize this bug?
It seems we're seeing more and more web-compat issues steming from it, and it would be great to avoid them.

Comment 15

[Andreas Farre [:farre]]

I'll take it to the next DOM Core meeting. It would be nice if we could do what smaug mentions in comment 13, and I think that we can shoe-horn this in to be a part of the work we need to do to fix replace loads for iframes. Or maybe not, but it would still be nice to handle ongoing navigation tracking as spec does.

Comment 16

[Andreas Farre [:farre]]

I'm looking at the test in comment 11. Chrome does exactly what we do insofar that it gets into a reload loop. How they manager to break it I don't know, but we hit our rate limit in BrowsingContext::Navigate, which makes the last one throw and the second to last finish.

@zcorpan, @smaug: has it been the rate limiter all along?

Comment 17

[Andreas Farre [:farre]]

I'm trying to see if rate limiting everything works, but removing the exception if limit is hit. This would align us with Chrome.

Comment 18

[Andreas Farre [:farre]]

OK, I think this is actually two bugs:

1. Rate limiting throwing with Gecko. Chromium were willing to switch to doing this, but never followed through, and as long as the situation is this way we'll misbehave on poorly constructed web-pages. This corresponds to bug 2011368 and bug 2021531. Poorly, in the way that what has been constructed is a navigation trigger loop:

1) Trigger navigation
2) On navigation triggered goto 1

2. onAbort firing when navigating away. This is bug 2004615.

Comment 19

[Andreas Farre [:farre]]

Attachment: Bug 1922677 - Rate limit every navigation. r=smaug!,zcorpan!

Comment 20

[Andreas Farre [:farre]]

More try