Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Disallow running unload handlers #39

Open
rakina opened this issue Sep 8, 2021 · 10 comments
Open

Proposal: Disallow running unload handlers #39

rakina opened this issue Sep 8, 2021 · 10 comments

Comments

@rakina
Copy link
Member

rakina commented Sep 8, 2021

If a document specifies Document-Policy: unload=?0, the unload event will not be dispatched for that document.

This is useful because the existence of an unload handler in a page will make it be not eligible for BFCache on some browsers (e.g. Desktop Chrome and Desktop Firefox), impacting its performance. With this header, the page can guarantee that the unload handler won't be a problem for BFCache.
(Note that a page will need to ensure its iframes won't run unload handlers too if it wants to be BFCacheable, which it can do through setting Require-Document-Policy: unload=?0)

Additionally the unload event is already very unreliable (it won't be run in various cases on mobile platforms, and also won't run if the page gets BFCached on Android Chrome, Android Firefox, and Safari (+ probably other WebKit-based browsers), and so it's pretty much a footgun and we considered deprecating it. The header allows web-pages to see what happens if unload never runs in their pages (to prepare for migration), have control over their code and even code they can't necessarily change (e.g. third party libraries), and also ensure that they'll never add any usage of unload in the future (in favor of the alternatives like pagehide/visibilitychange).

cc @domenic @annevk @smaug---- @cdumez @clelland

@smaug----
Copy link

It isn't clear to me what Require-Document-Policy does. Would iframes without Document-Policy: unload=?0 fail to load?

Is the idea with the header that the risk of using bfcache for pages with unload event listeners is moved from browsers to whoever has access to set the headers?

@rakina
Copy link
Member Author

rakina commented Sep 13, 2021

It isn't clear to me what Require-Document-Policy does. Would iframes without Document-Policy: unload=?0 fail to load?

I think so (at least that's my reading of the explainer). It does seem rather strong, but the main frame site can of course just choose to not require it on its iframes if it knows they don't have unload handler already.

Another alternative to that is probably make a stronger requirement like Document-Policy: unload-on-iframes=?0 which allows main frames to affect whether the iframes can run their unload handlers or not, but I'm not sure if that's a good idea, especially if the iframes are cross-origin.

Is the idea with the header that the risk of using bfcache for pages with unload event listeners is moved from browsers to whoever has access to set the headers?

Yeah basically it's kinda hard to determine the risk of not running unload handlers on desktop platforms, and even though we've encouraged people to migrate off of unload for a while, it's still very widely used and are blocking pages that would've otherwise be eligible for BFCache.

With this header, those pages can easily become BFCache-eligible, or even just experiment locally and see what goes wrong if their unload handler doesn't run (which is actually not that rare, especially when BFCache is involved). This also gives more motivation to migrate (due to reliability & performance improvements) and future-proofs the site from adding more unload handler.

(cc @chikamune-cr who's working on a more complete explainer for this)

@smaug----
Copy link

Does Chrome warn in the web console about use of unload event listener?

@rakina
Copy link
Member Author

rakina commented Sep 13, 2021

Does Chrome warn in the web console about use of unload event listener?

We currently only have a Lighthouse audit warning for it, but we're planning to surface it more through DevTools etc. In order to make the warning more useful, we're also working on adding migration guides with suggestions for common use cases, and also prepare other things that makes it easier to respond to the warning, including this API.

BTW we've worked with internal customers that are interested in making their pages BFCacheable and they find this functionality pretty valuable, especially since their site's codebase is large and it's hard to drive migration without something like this.

@smaug----
Copy link

smaug---- commented Sep 13, 2021

This does feel rather error prone though. Basically let whoever has access to tweak headers possibly breaks web pages in a new way.

How do those internal customers know they aren't breaking their sites?

@rakina
Copy link
Member Author

rakina commented Sep 13, 2021

This does feel rather error prone though. Basically let whoever has access to tweak headers possibly breaks web pages in a new way.

You mean like things on the network, or just the site's server? I guess the same argument applies for other Document-Policy and Feature-Policy/Permissions-Policy values such as sync-xhr. I'm not really sure if this is something we should worry about, maybe @clelland can comment.

How do those internal customers know they aren't breaking their sites?

They know because their sites still work when BFCached on Android Chrome (and probably Android Firefox and Safari), which already won't run unload. If they didn't know that (e.g. their page isn't actually BFCached), they would probably experiment with this API and find out.

@annevk
Copy link

annevk commented Sep 13, 2021

I'm a bit hesitant to discuss new features as long as #26 isn't sorted, but the way sites would find is by using the reporting functionality. You can configure this policy such that it isn't enforced, but instead sends the server a JSON resource with some information about usage. (Very similar to CSP and other APIs.)

@fergald
Copy link

fergald commented Sep 13, 2021

To me, the key ability this gives it to prevent re-introduction of a bad practice once it has been removed. Using it to disable unload handlers without removing them is more questionable but I don't see how to give one ability without the other. Various CSPs have exactly that problem.

On large teams, it's very easy for someone to add an unload handler (even unknowingly via a library). Headers tend to be controlled much more centrally and it's generally not easy for someone to casually or accidentally add a header.

@clelland
Copy link
Collaborator

maybe @clelland can comment.

I would agree with @annevk here that report-only mode is a good way to tell whether or not a configuration change is going to negatively affect a site; using that, you can work to remove the unload handlers until you stop getting reports, and then, as @fergald says, switch to enforcement mode to ensure that no new uses creep in.

Basically let whoever has access to tweak headers possibly breaks web pages in a new way.

I think this is a property of the network now: you can do that pretty easily with CSP, more subtly with the various Access-Control-* headers, less subtly with Location or Authenticate; besides this header.

@fergald
Copy link

fergald commented Feb 9, 2022

I think we will withdraw this proposal and shift to the permissions policy verion unless someone sees value in doing both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants