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: getUserMedia duplicate permissions call #36787

Merged
merged 2 commits into from
Jan 11, 2023
Merged

fix: getUserMedia duplicate permissions call #36787

merged 2 commits into from
Jan 11, 2023

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jan 4, 2023

Description of Change

Closes #36629.
Refs #34895.

Fixes an issue where the setPermissionRequestHandler callback would be invoked twice when using navigator.getUserMedia(...). This was happening because we didn't fully short-circuit all of the permission checks in MediaStreamDevicesController When a user called getUserMedia, Electron first does its own permissions check through RequestMediaAccessPermission, which resulted in the first callback being invoked. Then, when MediaStreamDevicesController::RequestPermissions was called, it would fall down to this line which fell down to ElectronPermissionManager::RequestPermissionsWithDetails and resulted in the second callback being called. We need to take previously-approved status into account when calling webrtc::MediaStreamDevicesController::RequestPermissions in order to fix this.

Checklist

Release Notes

Notes: Fixed an issue where the setPermissionRequestHandler callback would be invoked twice when using navigator.getUserMedia(...).

@codebytere codebytere requested a review from a team as a code owner January 4, 2023 09:08
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 4, 2023
@codebytere codebytere added target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. semver/patch backwards-compatible bug fixes labels Jan 4, 2023
@codebytere codebytere marked this pull request as draft January 4, 2023 09:19
@codebytere codebytere marked this pull request as ready for review January 4, 2023 10:17
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 5, 2023
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Odds on a test? This code seems a little bit awkward and easy to break during patch changes

@codebytere
Copy link
Member Author

codebytere commented Jan 9, 2023

@MarshallOfSound test added!

@codebytere codebytere merged commit f31826f into main Jan 11, 2023
@codebytere codebytere deleted the fix-duplicate branch January 11, 2023 10:55
@release-clerk
Copy link

release-clerk bot commented Jan 11, 2023

Release Notes Persisted

Fixed an issue where the setPermissionRequestHandler callback would be invoked twice when using navigator.getUserMedia(...).

@trop
Copy link
Contributor

trop bot commented Jan 11, 2023

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

@trop
Copy link
Contributor

trop bot commented Jan 11, 2023

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

@trop trop bot added merged/23-x-y PR was merged to the "23-x-y" branch. merged/22-x-y PR was merged to the "22-x-y" branch. and removed target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. in-flight/23-x-y labels Jan 11, 2023
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
* fix: getUserMedia duplicate permissions call

* test: add regression test
gecko19 pushed a commit to brightsign/electron that referenced this pull request Feb 28, 2023
* fix: getUserMedia duplicate permissions call

* test: add regression test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/22-x-y PR was merged to the "22-x-y" branch. merged/23-x-y PR was merged to the "23-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: getUserMedia triggers extra call to setPermissionRequestHandler handler with missing metadata
5 participants