Disconnect circuit on beforeunload event

[captainsafia]

Closes #19050.

In the issue above, the user originally reported that they would see our reconnect modal when navigating away from Firefox. They pointed at inconsistent behavior through browser versions. They'll always see the reconnect modal on Firefox but only on a few version of Chrome.

While debugging this, I realized that this is because our clean-up handler that is registered to the unload event is never invoked in Chrome.

The unload event is not emitted and, consequently, the async request sent to /disconnect via navigator.sendBeaconis never emitted.

It turns out that Chrome is discouraging developers from using the unload event in favor of its new page navigation APIs (ref). Chrome made some changes to the unload and beforeunload APIs around Chrome 80, I suspect that this is when the regression happened.

I tried triggered the clean-up on a few other events (like pagehide and visibilitychange) but ultimately settled on beforeunload because:

• It works on the widest set of browsers (validated on Chrome, Firefox, Safari, Edge)
• The pagehide event isn't triggered all the time in some browsers depending on when the subsequent page is loaded
• This visibilitychange is triggered for too many scenarios

[javiercn]

@captainsafia can onbeforeunload be cancelled?

[javiercn]

Looks great!

I assume you've done due diligence on testing it on various situations, so signing-off.

[captainsafia]

@captainsafia can onbeforeunload be cancelled?

Yes, if the user accepts the "Do you want to navigate away from this page?" prompt that the browser provides.

[SteveSandersonMS]

Awesome! Glad there was such a clean fix here!

Assuming there are no known drawbacks, this seems like a very strong patch candidate.

[captainsafia]

Made some modifications after testing a few more scenarios. Re-requesting reviews for this.

We register the same event handler on both the beforeunload and unload events. We track whether the /disconnect request has been successfully queued in the beforeunload. If not, we empty to send it again in the unload. We execute each event listener at most once.

[javiercn]

Made some modifications after testing a few more scenarios. Re-requesting reviews for this.

We register the same event handler on both the beforeunload and unload events. We track whether the /disconnect request has been successfully queued in the beforeunload. If not, we empty to send it again in the unload. We execute each event listener at most once.

What motivated this change? It would be good to keep a record for the future

[javiercn]

LGTM, interested in knowing the reason for subscribing to both events

[captainsafia]

LGTM, interested in knowing the reason for subscribing to both events

Chrome won't fire the event if the user hasn't interact with the page. This ends up happening in our CI tests because we call Browser.Close() without interacting with the page.

Firefox allows users to set a disable_beforeunload flag to disable those navigate away alerts. However, that disables beforeunload events and immediately emits the unload event.

[captainsafia]

@mkArtakMSFT Can you merge this?

[alexclarkmusa]

Just curious but... what sort of QA/unit-testing does a patch like this go through before it's unleashed on the public as a release? This appears to have caused a fairly significant/fundamental issue with the NavigationManager opening links and it seems like this should have shown up in some simple tests. I'm trying to make a case for Blazor at work and things like this really scare the business away from it.

Can there be a bit more diligence before things like this get pushed to the production release please?

[pranavkm]

@alexclarkmusa thank you for your feedback. Patches have a high bar for compatibility and go through a validation process before being released. We had tests that covered normal page navigations, but unfortunately we had a blind spot in our test \ scenario coverage for this particular use case. We will be reverting the patch shortly and we are filling the test hole. We are committed to making sure you're able to confidently pick up new patch releases without the risk of regression and we will be much more cautious making changes of this kind going forward.