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

Fix output buffering for cross-origin isolation #65701

Merged
merged 3 commits into from
Sep 28, 2024
Merged

Conversation

swissspidy
Copy link
Member

What?

This is a quick follow-up to #64650 with two simple fixes:

  1. Ensures the HTTP headers for cross-origin isolation are correctly sent in the output buffer callback
  2. Adds some hardening to ensure wasm files are served with the correct mime type (if not already done by the host anyway)
    • Actual wasm files will be added in follow-up PRs.

Note: this is only applicable when the client-side media processing experiment is enabled.

Why?

See #61447

How?

Testing Instructions

N/A

Testing Instructions for Keyboard

Screenshots or screencast

Improves compatibility with hosts who don't already serve wasm files with the correct mime type
@swissspidy swissspidy added [Type] Enhancement A suggestion for improvement. [Feature] Media Anything that impacts the experience of managing media No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core labels Sep 27, 2024
Copy link

github-actions bot commented Sep 27, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Two questions, but overall LGTM.

lib/experimental/media/load.php Show resolved Hide resolved
Comment on lines -240 to -241
// Only send the header when the buffer is not being cleaned.
if ( ( $phase & PHP_OUTPUT_HANDLER_CLEAN ) === 0 ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why this is no longer relevant?

I wouldn't have asked if it hadn't been here before, but why was it there before and is now being removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it was there before because I thought I needed it, but turns out it doesn't work, so I just removed it.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, the PL plugin includes this: WordPress/performance#1443

And the Optimization Detective plugin: WordPress/performance#1317

In my testing it worked, although I wrestled with this one for awhile to replicate the scenario where it would be relevant.

Copy link
Member Author

Choose a reason for hiding this comment

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

IDK what's different here, but this change here fixes the issue for me and would unblock further work, so I'd appreciate an approval

@swissspidy swissspidy merged commit 88d60e4 into trunk Sep 28, 2024
78 of 79 checks passed
@swissspidy swissspidy deleted the fix/mexp-php-changes branch September 28, 2024 18:51
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media Anything that impacts the experience of managing media No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants