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: don't emit did-fail-load for MediaDocuments #37824

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

codebytere
Copy link
Member

Description of Change

Closes #37804.

Fixes an issue where successfully loaded media documents emit did-fail-load. We already throw out cancelled navigations in certain places - we want to address the case where it's incorrectly emitted for media documents.

Per spec: when we navigate to a media resource: the original request for the media resource—which resulted in a committed navigation—is simply discarded, while the media element created inside the MediaDocument then makes another new request for the same media resource.

We fix the issue by determining if the ostensible load failure originated from a media document and discard it if it is.

Checklist

Release Notes

Notes: Fixed an issue where successfully loaded media documents emitted did-fail-load.

@codebytere codebytere added semver/patch backwards-compatible bug fixes 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. target/24-x-y PR should also be added to the "24-x-y" branch. labels Apr 4, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 4, 2023
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Possible to have a test ?

@codebytere
Copy link
Member Author

@deepak1556 i thought about it and am not sure - the issue at hand is that the webContents never emits did-finish-load either. So we'd ultimately be checking for an event not being emitted, which isn't really possible to do because there's then nothing to hook on. We could, I suppose, hook onto did-stop-loading? Ideally, we should figure out how to emit did-finish-load correctly but at the moment i'm not quite sure how.

@deepak1556
Copy link
Member

What about listening for media-started-playing instead ?

@codebytere codebytere changed the title fix: don't emit did-fail-load for MediaDocuments fix: don't emit did-fail-load for MediaDocuments Apr 5, 2023
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Apr 5, 2023
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Thanks!

@codebytere codebytere merged commit 251e567 into main Apr 6, 2023
@codebytere codebytere deleted the fix-did-fail-load-media branch April 6, 2023 08:23
@release-clerk
Copy link

release-clerk bot commented Apr 6, 2023

Release Notes Persisted

Fixed an issue where successfully loaded media documents emitted did-fail-load.

@trop
Copy link
Contributor

trop bot commented Apr 6, 2023

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

@trop trop bot added the in-flight/22-x-y label Apr 6, 2023
@trop trop bot removed the target/22-x-y PR should also be added to the "22-x-y" branch. label Apr 6, 2023
@trop
Copy link
Contributor

trop bot commented Apr 6, 2023

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

@trop
Copy link
Contributor

trop bot commented Apr 6, 2023

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

@trop trop bot removed target/24-x-y PR should also be added to the "24-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. in-flight/22-x-y labels Apr 6, 2023
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]: webContents did-fail-load is triggered with -3 error code when streaming media
3 participants