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

chore: cherry-pick fix from chromium issue 1113227 #24997

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

danbrianwhite
Copy link

@danbrianwhite danbrianwhite commented Aug 17, 2020

Description of Change

Currently, when the underlying network connection breaks from changing network IP address, the underlying chromium network connection does not update with the new IP address for RTC. This causes all RTC connections to fail after this scenario happens. The only way to work around this issue is to completely close the tab or restart the electron application.

The Chromium team created a fix for this in the master branch and the git hash https://chromium.googlesource.com/chromium/src/+/72ac0aae53384a914ea11e4f71dfcb81cf53830b. This PR is to cherry pick this fix on top of the version of chromium used by Electron 9-x-y and then create a patch file.

The original conversation on the bug board can be followed at https://bugs.chromium.org/p/chromium/issues/detail?id=1113227.

Backport https://chromium.googlesource.com/chromium/src/+/72ac0aae53384a914ea11e4f71dfcb81cf53830b

Checklist

Release Notes

Notes: Resolve network issues that prevented RTC calls from being connected due to network IP address changes and ICE. (Chromium issue 1113227)

@danbrianwhite danbrianwhite requested a review from a team as a code owner August 17, 2020 16:21
@welcome
Copy link

welcome bot commented Aug 17, 2020

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 17, 2020
@danbrianwhite danbrianwhite changed the title fix: Resolve RTC issues - Reconnect P2P socket dispatcher if network service dies. chore: cherry-pick fix from chromium issue 1113227 Aug 17, 2020
@danbrianwhite
Copy link
Author

danbrianwhite commented Aug 17, 2020

Hello electron team. Please let me know if there are any steps or guidelines I missed in this PR. I'm also going to open a PR request against 10-x-y.

Currently this issue is impacting Symphony.com Electron application. We tested this patch on a local build of electron and saw it fixed the issue with RTC and network IP fix.

If there are any questions or concerns, please let me know. Any and all assistance is much appreciated.

Thanks!

deepak1556
deepak1556 previously approved these changes Aug 17, 2020
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.

LGTM based on https://bugs.chromium.org/p/chromium/issues/detail?id=1113227#c66. Thanks! feel free to create a backport for 10-x-y

@deepak1556 deepak1556 dismissed their stale review August 17, 2020 17:22

there is build failure in applying the patch, can you please fix it.

@deepak1556
Copy link
Member

deepak1556 commented Aug 17, 2020

Download https://630330-9384267-gh.circle-artifacts.com/0/patches/update-patches.patch and cd electron/ && git am --3way <downloaded patch>

@danbrianwhite
Copy link
Author

@deepak1556 thanks so much for the help. I ran the merge, squashed, and pushed. 🤞

@danbrianwhite
Copy link
Author

@deepak1556 I also created the backport for 10-x-y
#24998

@deepak1556 deepak1556 added the backport-check-skip Skip trop's backport validity checking label Aug 17, 2020
@deepak1556 deepak1556 requested a review from a team August 17, 2020 18:05
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.

Can you rebase to fix conflict, thanks!

@danbrianwhite
Copy link
Author

@deepak1556 - rebased and pushed. Thanks!

@codebytere codebytere merged commit cf321a9 into electron:9-x-y Aug 18, 2020
@welcome
Copy link

welcome bot commented Aug 18, 2020

Congrats on merging your first pull request! 🎉🎉🎉

@release-clerk
Copy link

release-clerk bot commented Aug 18, 2020

Release Notes Persisted

Resolve network issues that prevented RTC calls from being connected due to network IP address changes and ICE. (Chromium issue 1113227)

@danbrianwhite
Copy link
Author

Thank you so much all. The assistance is greatly appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9-x-y backport-check-skip Skip trop's backport validity checking new-pr 🌱 PR opened in the last 24 hours
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants