Handling beforeunload / pending state when moving to FROZEN & DISCARDED

[spanicker]

Filling in some context from companion email thread:
There are 2 types of usecases here for handling of pending state during FROZEN / STOPPED (note recent rename here):

1. The background page hasn't finished some action eg. in the midst of persisting user state
   onfreeze alone works fine for this case, there is no need for beforeunload. the app should finish persisting the state in onfreeze, we plan to support waitUntil to make this reliable.

2. The background page has pending user state: eg. unsaved edits in a photo editor
   The app cannot really persist this state as "user state", because the user has not committed the changes, they are pending edits - that the user would have to decide whether to commit or throw away.

Options for addressing case 2:
a. If the page has a beforeunload handler present then do NOT move it to FROZEN and consequently DISCARDED. This is what bfcache does today. The downside is that very large number of pages have the handler 80+% -- so this opts out bulk of pages from ever going to FROZEN or DISCARDED.

b. Regardless of beforeunload handler move the page to FROZEN.
Later if the page is being considered for DISCARDED then run beforeunload at that time. If beforeunload returns string (i.e. has pending state) then do NOT move the tab to DISCARDED.
This would be effective -- only 0.03% of beforeunload calls actually return string - indicating pending state. The downside is that the page would need to be woken up to run beforeunload.

c. [@ojanvafai's suggestion] When moving the page to FROZEN - proactively run beforeunload and note if it returns string i.e. has pending state. Then move the page to FROZEN.
Later if the page is being considered for DISCARDED then use this info to decide: if there is pending state then don't discard and remain in FROZEN.
This addresses the downside of b. -- no need to wake up the page after it is in FROZEN, and it can be directly DISCARDED.
The issue here is that running beforeunload at this point could cause some compat issues and will potentially mess up analytics. Running beforeunload before onfreeze could be surprising for web developers.

We should probably try out Option c. and if that doesn't work then fallback to Option b.
\cc @smaug---- @ojanvafai @philipwalton @fmeawad

[smaug----]

I don't understand c. We'd run beforeunload even if we might not unload anything. How would we resume such page?

[spanicker]

Right, there is certainly some compat risk here as pages may be running code in beforeunload (analytics for sure) assuming that unload will follow, and resuming such a page can cause problems.
The idea was to run an experiment and see the extent of the breakage.
In theory though, the purpose of beforeunload is to enable checking for pending user state and if so show a modal dialog to warn the user. The fact that apps are doing a bunch of other types of work in beforeunload is an unintended consequence of the fact that they are allowed to run script (and a lot of it) so it's a last chance to do a bunch of work.
WDYT?

[smaug----]

But how do you resume beforeunloaded page? Just resume and hope the page works?
Conceptually feels rather wrong.

And "if there is pending state then don't discard and remain in FROZEN." Well, UA may need to discard if there is OOM. Better to discard non-visible page than the currently visible page.

There is also the option d) that one would just unload the page. If it wants to handle frozen state in some sane way, it would need to add event listener for 'freeze' event.
(Not sure this is any better option, but at least it wouldn't be too surprising.)

[spanicker]

But how do you resume beforeunloaded page? Just resume and hope the page works?
Conceptually feels rather wrong.

Yes I agree this feels strange conceptually as it is bending the meaning of "beforeunload".
@ojanvafai any thoughts?

And "if there is pending state then don't discard and remain in FROZEN." Well, UA may need to discard if there is OOM. Better to discard non-visible page than the currently visible page.

Yes -- it is always the case that under extreme pressure beforeunload cannot be respected and all bets are off, this already happens today. However if the browser is "proactively" discarding and the resource situation is not extreme then it can choose to discard background pages without pending state (from beforeunload) and avoid discarding ones with pending state.

There is also the option d) that one would just unload the page. If it wants to handle frozen state in some sane way, it would need to add event listener for 'freeze' event.

You mean pages containing beforeunload handler (or even in general?) cannot be frozen -- unless there is explicit freeze event handler -- and can only be discarded by default?
This seems a bit extreme to me, as that means freezing pages is essentially an "opt-in" (majority ~80+% pages contain beforeunload). However browsers (eg. safari, edge) already freeze pages today (without any callbacks), and want to do so much more aggressively (eg. chrome). Also freezing is generally less disruptive (less perceivable) for users than discarding as it does not need a full page load on revisit.
So not sure this is a palatable option as it really limits browsers' ability to freeze pages in a spec compliant manner.

[philipwalton]

I like option (d) if we couple it with new option (e) where we implement a new, declarative way for apps to indicate pending state.

This would gives apps that fear data loss for their users a strong incentive to implement the new API (which is better than only being able to discard apps that opt in, since few likely will).

[ojanvafai]

@smaug---- I actually feel like assuming you can resume after a beforeunload handler is conceptually right. The hard part is whether it's pragmatically deployable. Beforeunload is for showing the beforeunload dialog if there's unsaved state. If your doing page unloading work, it should happen in an unload handler, not a beforeunload handler. Otherwise, why do we have both events?

I think pages (mostly) already have to be resilient to the page continuing execution after beforeunload handlers have fired if the beforeunload dialog pops up and the user says to stay on the page. As far as the page is concerned it can't tell the difference between the browser showing a dialog and the user dismissing it and the browser ignoring the dialog and continuing page execution later.

It's definitely likely that beforeunload handlers are being used with no return value where people should be using unload handlers though, so there's some compat risk. I guess I'm hoping beforeunload handlers doing destructive teardown will be rare enough that for us to ask that small number of sites to switch over to unload handlers, which should generally be a very easy change for them to make.

Having a declarative way to indicate pending state seems like a fine fallback to me, but it seems considerably easier to get the small number of sites incorrectly using beforeunload to change behavior than the large number of sites correctly using beforeunload to change.

[spanicker]

Thanks Ojan, I think I agree that we should experiment with option c.
Although sadly recent data suggests that some third parties are doing a TON of work in beforeunload (up to 2 secs) as they can reliably do this work there, as opposed to unload.
So it would be bit unfortunate to do all this work at onfreeze time, but worth trying out still.

BTW removing sync xhr in beforeunload helps some, but not entirely, as they do a LOT of CPU work)

[ojanvafai]

That specific data doesn't worry me too much. On the scale of things, doing 2 seconds of work isn't that much...we don't rapidly freeze/thaw pages. I mean...it's frustrating and awful, but not that bad given that freezing doesn't happen in the critical tab discarding scenarios, but rather in scenarios where the machine isn't severely overloaded.

And those are third party libraries that already have to be somewhat resilient to beforeunload being called multiple times since they don't know if the page will have a beforeunload handler that returns a string.