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

add lots about why this is ok #8

Merged
merged 8 commits into from
May 9, 2022
Merged

add lots about why this is ok #8

merged 8 commits into from
May 9, 2022

Conversation

fergald
Copy link
Owner

@fergald fergald commented Apr 25, 2022

@domenic I've tried to summarize our arguments for why blocking unload is OK. I have not managed to get a good wording for the conclusion.


We propose that this API only be made available
when we believe that migration paths are available for all uses of unload
and sufficient time has passed for that migration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit vague and I'm unclear how I would read it as another browser vendor... Are there specific APIs other browsers should be sure to ship before shipping this one? (That seems unfortunate if so.)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The beacon API, mentioned at the top could be a prerequisite. That said, even the people who proposed the beacon API were able to move to pagehide (they had concerns but it turned out to be doable). We know that metrics providers, ads and maybe others use unload in the same way.

I've rephrased this to say that there are no known blockers but there are some ergonomics issues.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I've removed this section and added some more comments on the beacon API.

explainers/permissions-policy-unload.md Outdated Show resolved Hide resolved
- implementations typically allot a time budget for running unload handlers
which may cause some not to complete or not to run at all

This means that if preventing uload handlers from running is a problem
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might either rephrase this, or add an additional sentence, along the lines of "This means that unload handlers are already not a guaranteed capability that subframes can depend on, and placing some control over this nondeterminism with the parent frame does not make the problem worse".

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

What remains are the cases where the embedder
accidentally breaks something an iframe
or (hypothetically) intentionally exploits the iframe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a third major argument is about setting a new default/treating unload as a powerful non-default capability. Something like:

  • This is a path toward eventually flipping the default so that unload handlers are only possible if the top frame explicitly grants subframes the ability to use them. That is, making unload handlers similar to other user- or page-disruptive features like geolocation, autoplay, cross-origin-isolated, or document-domain.
  • For now, and for the forseeable future, we plan to keep unload handlers available by default. But letting pages enforce this on themselves and their subframes to protect user experience is valuable in the meantime.

It might be worth looking at previous discussions for document-domain and cross-origin-isolated (which feel rather similar): whatwg/html#4170 w3c/webappsec-permissions-policy#241 whatwg/html#5435

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this but then didn't try to express it. When I started thinking about this, it led me down the path of

  • 1 switch to flip the default, so that unload handlers don't run
  • another switch to re-enable them and we make it very explicit so that it's easy to report on, e.g. make it a header, so you don't have to analyse the page's JS.

I think that is a gentler path but also more complex. I had some idea that it would look like, Permissions-Policy: unload-default() and then a subframe does Permissions-Policy: unload(self) to flip it back on. There are problems there though because if the main frame also does Permissions-Policy: unload() then the subframe cannot undo that. It doesn't fit in the existing PP framework (I think). Maybe reenabling it is a DP not a PP?

I didn't include it because I'd prefer the simpler path if we can get it. If you think the double switch path is the right one then I can make a larger update.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we generally do these things by spreading out the switches over time instead of over multiple configuration points. I.e.:

  • Time 0: everyone has access to the powerful feature always
  • Time 1: everyone has access to the powerful feature by default; parent frames can disallow child frames. (In Permissions Policy terms, this is default allowlist of "*". This is where sync-xhr is.)
  • Time 2: only parent frames have access to the powerful feature by default; they can delegate to children using allow="". (In Permissions Policy terms, this is default allowlist of "self". This is where most powerful features that are not on a deprecation path end up.)
  • Time 3: nobody has access to the powerful feature by default, but parent frames can opt-in using the Permissions Policy header and if they do so they can delegate to child frames using allow="".
  • Time 4: nobody has access to the powerful feature ever

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, what I was proposing was a step in between 0 and 1 where subframes could rescue themselves even if the parent disagrees. I will not suggest that but I will update it to suggest that 4 is where we want to end up (we are not strongly aiming for that but if unload remains a problem we would consider it).

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I did here is point out that sync-xhr and document-domain almost certainly caused code not to run too and that worked out OK. If you make a stronger case, I'll happily include it. In particular if there are evils of unload that really extend beyond BFCache that I've missed.

### Conclusion

We believe that the danger posed by embedders maliciously disabling unload handlers in iframes is minimal
and not in the same class as an ability to disable other APIs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add something affirmative about how this trades off against allowing well-behaved outer frames to protect the user against bad experiences (e.g. slow-to-unload pages and broken bfcache), and how on balance we believe the small risks here are outweighed by the potential upside.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately anything that's slow to unload can just be slow to pagehide, they happen at the same timing and block the same things (in chrome at least). So I think bfcache is the win we can claim.

I've updated this.

Copy link
Contributor

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fergald fergald merged commit 4b84b90 into master May 9, 2022
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

Successfully merging this pull request may close these issues.

2 participants