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: older systems crash when playing media files #32046

Merged
merged 3 commits into from Dec 16, 2021

Conversation

deermichel
Copy link
Contributor

@deermichel deermichel commented Nov 29, 2021

Description of Change

Closes #31762.

Regression caused by #31492. On Windows 7 & 8 / macOS 10.11/10.12, system_media_controls_ is nullptr and calling methods on it crashes the process. This PR wraps problematic statements in an if-clause like upstream Chromium does.

I manually checked that MediaMetadata still works on Windows 10 and above.

cc @codebytere @lyswhut @miniak

Checklist

Release Notes

Notes: Fixed crash when playing media files on Windows 7/8 or macOS 10.11/10.12.

@deermichel deermichel added semver/patch backwards-compatible bug fixes target/13-x-y labels Nov 29, 2021
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 29, 2021
@deermichel deermichel marked this pull request as ready for review November 29, 2021 17:03
@deermichel deermichel requested a review from a team as a code owner November 29, 2021 17:03
+ system_media_controls_.get());
+
}

+ // Directly listen for media key keypresses when using GlobalShortcuts.
+ media_keys_listener_ = ui::MediaKeysListener::Create(
+ this, ui::MediaKeysListener::Scope::kGlobal);
+ DCHECK(media_keys_listener_);
Copy link
Member

Choose a reason for hiding this comment

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

I think you've broken line endings here or something? This line is marked as changed when nothing has changed, can you clean up this diff so that only changed lines are committed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I tried a couple of things now, this is the cleanest version I get with e patches chromium. Some lines get shuffled because with the if-clause, git interprets changes a little bit different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I try to remove the + or line end at end of the file, I end up with a corrupt patch.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 30, 2021
@deermichel deermichel changed the title fix: win7 crash when playing media files fix: older systems crash when playing media files Dec 2, 2021
@indutny-signal
Copy link
Contributor

We'd really love to help this move forward. Please let me know if I can help with getting this merged in any way!

Thanks for the fix! ❤️

@zcbenz
Copy link
Member

zcbenz commented Dec 8, 2021

This PR requires review from @electron/wg-upgrades.

But just a notice that this organization won't have much activity this month, so even if it is merged we might have to wait until next month before it is released.

@gaodeng
Copy link

gaodeng commented Dec 11, 2021

any news?

@bruceauyeung
Copy link
Contributor

lately we encountered crashes when playing video files, looking forward for this !

@bruceauyeung
Copy link
Contributor

Is there any workround to avoid crash when play media files before this PR getting merged ?
Thanks !
@zcbenz @MarshallOfSound

@deermichel
Copy link
Contributor Author

@bruceauyeung downgrading to 15.3.0 / 14.2.0 / 13.6.0 works

@VerteDinde VerteDinde merged commit e942098 into main Dec 16, 2021
@VerteDinde VerteDinde deleted the deermichel/media-patch branch December 16, 2021 17:23
@trop
Copy link
Contributor

trop bot commented Dec 16, 2021

I have automatically backported this PR to "16-x-y", please check out #32215

@lyswhut
Copy link

lyswhut commented Jan 5, 2022

13.x seems to be still updating,
Can it be merged into 13.x?

@trop
Copy link
Contributor

trop bot commented Jan 5, 2022

@deermichel has manually backported this PR to "13-x-y", please check out #32348

@trop
Copy link
Contributor

trop bot commented Jan 5, 2022

@deermichel has manually backported this PR to "14-x-y", please check out #32349

t57ser pushed a commit to t57ser/electron that referenced this pull request Jan 25, 2022
* fix: win7 crash when playing media

* reset

* chore: update patches

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Windows 7/8 and macOS 10.11/10.12 crash when playing media files
8 participants