[Core Browser] Call stop on beforeunload event

[afharo]

Some plugins/services in Kibana may need to apply some persisting logic (to local storage or an unattended HTTP request) when the user leaves Kibana.

By definition, this piece of logic should be implemented in the stop method. However, we are only calling it when there's a Fatal Error:

kibana/src/core/public/core_system.ts

Lines 112 to 115 in 8539a91

	this.fatalErrors = new FatalErrorsService(rootDomElement, () => {
	// Stop Core before rendering any fatal errors into the DOM
	this.stop();
	});

We have some piece of logic handling the beforeunload event, but it only calls the leaveHandler of the current application:

kibana/src/core/public/application/application_service.tsx

Lines 402 to 413 in 9d5aca5

	private onBeforeUnload = (event: Event) => {
	const currentAppId = this.currentAppId$.value;
	if (currentAppId === undefined) {
	return;
	}
	const action = getLeaveAction(this.appInternalStates.get(currentAppId)?.leaveHandler);
	if (isConfirmAction(action)) {
	event.preventDefault();
	// some browsers accept a string return value being the message displayed
	event.returnValue = action.text as any;
	}
	};

This means UI services cannot properly handle their unload logic.

Should we call stop on this event if the leaveHandler is not a "confirm" action?

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[pgayvallet]

window.unbeforeunload is a tricky and legacy event, and its behavior may vary between browsers (yea, even in 2022). In addition, because the event was widely abused in the beginning of the web era, browsers fire-proofed themselves against it.

1. You can't really do anything async in there, given you can't be sure whether your code will be properly executed or not before the actual unload event.
2. On most browser, any handler attached to the event will be killed/skipped if it takes 'too much' time to execute.

The only valid use case this event is supposed to be used for is user confirmation when leaving a page with unsaved changes, even moz's usage notes states it.

Overall, there's no reliable way to make sure that some code will be executed in every scenario that could cause the page/tab/browser to terminate, which is why ihmo we shouldn't attach cleanup logic to it. The main risk I see by doing so is to have developer think their code will always be executed on page termination, when in fact, it's not.

[afharo]

You can't really do anything async in there, given you can't be sure whether your code will be properly executed or not before the actual unload event.

The sendBeacon API (or fetch with keepalive) allows us to trigger an unattended action that would be fine with this, wouldn't it?

On most browser, any handler attached to the event will be killed/skipped if it takes 'too much' time to execute.

Also, writing to the localStorage is a synchronous API that should not take long to execute and could be used in this event, couldn't it?

Probably, we can discuss an alternative event to hook into? The reason I think beforeunload is the one is because we need to NOT stop if the user cancels the App Leave.

The main risk I see by doing so is to have developers think their code will always be executed on page termination, when in fact, it's not.

FWIW, isn't that the same problem on the server? There are multiple use cases that might not call the stop method:

1. The server crashes
2. The server takes too long to gracefully shut down, so it's killed before actually calling all the stop methods.
3. The server is killed

To be clear: the main use case I'm thinking about at the moment is EBT on the browser. After some thorough testing, I noticed that pages that navigate via a full page refresh fail to report the click event to the navigation button. A clear example of that is our Login page: clicks to the login button fail to report because we refresh the entire page. The client has a shutdown mechanism that will flush the queue and send everything in it. But it's not called because we are not notifying about leaving the page.

We could implement this listener in the EBT client itself if we don't want to make it a core feature. What do you think?

[pgayvallet]

The sendBeacon API (or fetch with keepalive) allows us to trigger an unattended action that would be fine with this, wouldn't it?

Not always. See the example from mzdn: On mobile, you open a page, you go back to the home screen via home button, your browser app hibernates, then you close your browser app from the system menu, no events. On some non-mobile browsers, this works too: switch tab, close your browser, the non-active tabs will not necessarily trigger onbeforeunload. It's an implementation detail.

Probably, we can discuss an alternative event to hook into?

To my knowledge, this is the best event to hook into, but still a terrible one. That's the thing.

FWIW, isn't that the same problem on the server?

It totally is, and it's why I've been fighting against doing 'mandatory' cleanup operations during stop on the server-side too.

To be clear: the main use case I'm thinking about at the moment is EBT on the browser. After some thorough testing, I noticed that pages that navigate via a full page refresh fail to report the click event to the navigation button

I'm not against implementing it, and there probably isn't a better solution. My point is: we need to be aware that it is not a reliable way to execute something before a page reload (and that there is no reliable way)

[afharo]

My point is: we need to be aware that it is not a reliable way to execute something before a page reload (and that there is no reliable way)

I get and share that sentiment. Do you think it's best to listen to the event (and handle potential cancellations somehow) in the package instead of relying on Core to call the stop/shutdown method? This way we may not encourage others to rely on the stop call. WDYT?

[pgayvallet]

Do you think it's best to listen to the event (and handle potential cancellations somehow) in the package

Probably not... It's still probably better to have a single handler for the event, at least to handle cancelation as you said.

[lukeelmers]

I think it's worth taking a best-effort approach to calling stop before unload, even if it needs to carry the caveat that we don't guarantee it is bulletproof.

Some plugins/services in Kibana may need to apply some persisting logic (to local storage or an unattended HTTP request) when the user leaves Kibana.

Looks like we actually have documented this exact scenario as a use case for the stop lifecycle in the browser, whoops 😬 https://github.com/elastic/kibana/blob/main/dev_docs/key_concepts/kibana_platform_plugin_intro.mdx#lifecycle-methods

[pgayvallet]

So, I got a PR ready(#187938)...

However @jloleysens found out that the unload event is deprecated and planned for full desactivation: https://developer.chrome.com/docs/web-platform/deprecating-unload. They're encouraging to use visibilitychange and/or pagehide instead.

The thing is, those two events are quite different from what unload was doing (as unload was supposed to be called really only during page "termination", where unload/vischange fires when you switch tab, switch focus, restore your app's context on mobile and so on...)

As I was saying to JL, this is ihmo yet another example of why trying to have the exact same architecture on our browser-side and server-side "root" systems is probably a mistake: if the concept of "stopping" is quite concrete on the server, it is way less on the browser (especially on modern browsers, even more on modern mobile browsers), as closing your browser app, or putting the app on sleep, is not the same as killing the tab.

So all this to say: I think we do have a need for plugins to be able to "hook" into a "status change". One example is our telemetry that would want to be able to flush its events when the page's state changes. However, I don't think hooking into unload or beforeunload is the way to go.

Instead, we should accept that in real life scenario (non test, real user, real browser), the concept of "stopping" browser-side Kibana is abstract, and should either:

1. let the consumers that need to listen to page state change to it themselves (by manually listening to visibilitychange or pagehide)
2. expose a "pagestate" service from core that would be a wrapper on top of those APIs

WDYT?

[jloleysens]

Yeah I like this idea. I thought using "unload" while we can would be OK, but totally agree there is a better solution. If the primary use case is to flush telemetry then unload's "defined" behaviour seems like it could work in some cases, but be flaky at best.

Of your alternatives I like (1). Main benefit of (2) afaict is saving plugins/core 1 loc by exposing these events as observables which doesn't seem worth the lift imo.

Perhaps we should update the issue title too?

[pgayvallet]

@afharo did you have any particular use case in mind when you opened the issue?

[afharo]

@pgayvallet, yes, this one: https://github.com/elastic/kibana/blob/main/packages/core/analytics/core-analytics-browser-internal/src/analytics_service.ts#L79-L82

But I can imagine other use cases like aborting requests in Dashboard/Discover/Lens when closing the tab/window/leaving the page.

[jloleysens]

aborting requests in Dashboard/Discover/Lens

Would this be doable from the HTTP service? And, does the browser not do this already?

[afharo]

aborting requests in Dashboard/Discover/Lens

Would this be doable from the HTTP service?

Yeah... probably... but we still need a trigger for the HTTP service (similar to the Analytics service force-flushing the pending events before closing). That trigger can be the stop method or let each service subscribe to the events.

I'd rather rely on stop so we can do the heavy-lifting of handling the appropriate events in one single place.

And, does the browser not do this already?

Maybe? I don't know.

[pgayvallet]

I'd rather rely on stop so we can do the heavy-lifting of handling the appropriate events in one single place

Yeah, me too, but that's the thing, it seems we can't... unload will be removed, and we can't just call root.stop on visibility/pagestate change events, as it would fully kill kibana / delete the dom and would not restart our root when the page gets back to a visible state...

Which is why my question is mostly: Given the development cost will be way higher than initially anticipated, do we think we still need this in the short/mid term, or should we just postpone it for now.

[afharo]

I think postponing is fine. It's quite possible that all events are shipped by the time there's an attempt to close the tab.

And missing out on the edge cases is probably ok, bearing in mind the level of effort.

Ultimately, if we really need to force-flush events, we can call the flush API on onbeforeunload from the service itself.

I'll close this issue for now.