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.

Sign up for GitHub

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

Jump to bottom

Fix output buffering for cross-origin isolation #65701

Merged
merged 3 commits into from
Sep 28, 2024
Merged

Fix output buffering for cross-origin isolation #65701

merged 3 commits into from
Sep 28, 2024

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

@swissspidy
Fix output buffering for cross-origin isolation
779e7d9
@swissspidy
Fix wasm content type in rewrite rules
142cec4 
Improves compatibility with hosts who don't already serve wasm files with the correct mime type
@swissspidy
Remove unused param
d3947b3
@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
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 27, 2024 •
edited
Loading

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.

@swissspidy swissspidy mentioned this pull request Sep 27, 2024
Merged
felixarntz
felixarntz reviewed Sep 27, 2024
View reviewed changes
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
lib/experimental/media/load.php
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
huubl pushed a commit to huubl/gutenberg that referenced this pull request Oct 2, 2024
@swissspidy @huubl
Fix output buffering for cross-origin isolation (WordPress#65701)
35d2718
huubl added a commit to huubl/gutenberg that referenced this pull request Oct 2, 2024
@huubl
Revert "Fix output buffering for cross-origin isolation (WordPress#65701
18c79ae 
)"

This reverts commit 35d2718.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixarntz felixarntz felixarntz left review comments

@westonruter westonruter westonruter approved these changes

@adamsilverstein adamsilverstein Awaiting requested review from adamsilverstein

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey spacedmonkey is a code owner

Assignees
No one assigned
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
Milestone
Gutenberg 19.4
Development

Successfully merging this pull request may close these issues.
3 participants
@swissspidy @westonruter @felixarntz