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

feat: Add option to conditionally disable site instance patches #18396

Merged
merged 2 commits into from May 31, 2019

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented May 21, 2019

Description of Change

This adds a new app.allowRendererProcessReuse that allows us to dynamically (at runtime) disable the site instance override patches. The intention is to:

  • have this PR land in Electron 6
  • have the first deprecation warning to land in Electron 7 for non-context aware native modules
  • have a deprecation warning land in Electron 8 for the default value of app.allowRendererProcessReuse to switch
  • switch the default value of app.allowRendererProcessReuse in Electron 9
  • Deprecate the ability to change app.allowRendererProcessReuse in Electron 10
  • Remove the ability to change app.allowRendererProcessReuse in Electron 11

Yes you're reading that right, 5 major versions 👍 (60~ish weeks)

For more info see this Discussion Issue

Checklist

Still TODO

  • Find a way to prevent loading native modules that are not context aware / NAPI in the renderer process if this new mode is enabled. Required for this PR
  • Add a console.warn for loading native modules in the renderer process that are not context-aware or NAPI - This warning will ship in Electron 7

Release Notes

Notes: Added new app.allowRendererProcessReuse property that allows apps to disable the site instance overrides Electron has patched into Chromium. This can be used to prevent the automatic renderer process restarting that Electron currently does.

@MarshallOfSound MarshallOfSound requested a review from a team as a code owner May 21, 2019 23:41
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 21, 2019
@MarshallOfSound MarshallOfSound added target/6-0-x semver/minor backwards-compatible functionality labels May 22, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 22, 2019
DEPS Outdated
@@ -12,7 +12,7 @@ vars = {
'chromium_version':
'84c40395c741fa24ccbd9fc2c5828e2e97472952',
'node_version':
'a86a4a160dc520c61a602c949a32a1bc4c0fc633',
'b213b26f0fb642b95561c0f10039886607036d5a',
Copy link
Member

Choose a reason for hiding this comment

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

link to electron/nodejs pr?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tis just this commit --> electron/node@dee0db9

@MarshallOfSound MarshallOfSound force-pushed the conditionally-disable-site-instance-patches branch from 3901321 to 8c19b11 Compare May 24, 2019 19:54
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 in general, naming conventions could be made better :)

atom/browser/api/atom_api_app.cc Outdated Show resolved Hide resolved
atom/browser/api/atom_api_app.cc Outdated Show resolved Hide resolved
atom/browser/api/atom_api_app.cc Outdated Show resolved Hide resolved
atom/browser/api/atom_api_app.cc Outdated Show resolved Hide resolved
@MarshallOfSound MarshallOfSound force-pushed the conditionally-disable-site-instance-patches branch 2 times, most recently from 80e6965 to 90af9bc Compare May 31, 2019 00:21
…able of site instance overrides

spec: add tests for the new allowRendererProcessReuse property

feat: add console warnings / errors for loading non context-aware native modules
  * Only error if the patch is disabled
  * Warn all the time, this will ship in Electron 7
@MarshallOfSound MarshallOfSound force-pushed the conditionally-disable-site-instance-patches branch from 90af9bc to 0a952e2 Compare May 31, 2019 00:24
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, since its behind a flag don't see any issue with landing.

@deepak1556 deepak1556 requested a review from zcbenz May 31, 2019 22:17
@MarshallOfSound MarshallOfSound merged commit 87ae932 into master May 31, 2019
@release-clerk
Copy link

release-clerk bot commented May 31, 2019

Release Notes Persisted

Added new app.allowRendererProcessReuse property that allows apps to disable the site instance overrides Electron has patched into Chromium. This can be used to prevent the automatic renderer process restarting that Electron currently does.

@trop
Copy link
Contributor

trop bot commented May 31, 2019

I was unable to backport this PR to "6-0-x" cleanly;
you will need to perform this backport manually.

@MarshallOfSound MarshallOfSound deleted the conditionally-disable-site-instance-patches branch May 31, 2019 22:47
MarshallOfSound added a commit that referenced this pull request May 31, 2019
* chore: allow conditional disable of the site instance override patches at runtime

* feat: add app.allowRendererProcessReuse property to allow runtime disable of site instance overrides

spec: add tests for the new allowRendererProcessReuse property

feat: add console warnings / errors for loading non context-aware native modules
  * Only error if the patch is disabled
  * Warn all the time, this will ship in Electron 7
@trop
Copy link
Contributor

trop bot commented May 31, 2019

A maintainer has manually backported this PR to "6-0-x", please check out #18554

@trop trop bot added the in-flight/6-0-x label May 31, 2019
MarshallOfSound added a commit that referenced this pull request Jun 13, 2019
* chore: allow conditional disable of the site instance override patches at runtime

* feat: add app.allowRendererProcessReuse property to allow runtime disable of site instance overrides

spec: add tests for the new allowRendererProcessReuse property

feat: add console warnings / errors for loading non context-aware native modules
  * Only error if the patch is disabled
  * Warn all the time, this will ship in Electron 7
MarshallOfSound added a commit that referenced this pull request Jun 13, 2019
* feat: Add option to conditionally disable site instance patches (#18396)

* chore: allow conditional disable of the site instance override patches at runtime

* feat: add app.allowRendererProcessReuse property to allow runtime disable of site instance overrides

spec: add tests for the new allowRendererProcessReuse property

feat: add console warnings / errors for loading non context-aware native modules
  * Only error if the patch is disabled
  * Warn all the time, this will ship in Electron 7

* chore: do not warn in about context aware in v6

* chore: update patches
@sofianguy sofianguy added this to Fixed in 6.0.0-beta.8 in 6.1.x Jun 17, 2019
@396784797
Copy link

app.allowRendererProcessReuse = true ;pall on electron9.0.0 nodejs 12.17.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor backwards-compatible functionality
Projects
No open projects
6.1.x
Fixed in 6.0.0-beta.8
Development

Successfully merging this pull request may close these issues.

None yet

5 participants