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

Run the Desktop app in a sandbox #7907

Merged
merged 4 commits into from Dec 19, 2018

Conversation

@dbkr
Copy link
Member

dbkr commented Dec 18, 2018

  • Turn off node integration in the electron renderer process
  • Enable the chromium sandbox to put the renderer into its own process
  • Expose just the ipc module with a preload script
  • Introduce a little IPC call wrapper so we can call into the
    renderer process and await on the result.
  • Use this in a bunch of places we previously used direct calls
    to electron modules.
  • Convert other uses of node, eg. use of process to derive the
    platform (just look at the user agent)
  • Strip out the desktopCapturer integration which doesn't appear
    to have ever worked (probably best to just wait until
    getDisplayMedia() is available in chrome at this point:
    #4880).

Requires matrix-org/matrix-react-sdk#2372

 * Turn off node integration in the electron renderer process
 * Enable the chromium sandbox to put the renderer into its own process
 * Expose just the ipc module with a preload script
 * Introduce a little IPC call wrapper so we can call into the
   renderer process and await on the result.
 * Use this in a bunch of places we previously used direct calls
   to electron modules.
 * Convert other uses of node, eg. use of process to derive the
   platform (just look at the user agent)
 * Strip out the desktopCapturer integration which doesn't appear
   to have ever worked (probably best to just wait until
   getDisplayMedia() is available in chrome at this point:
   #4880).
@dbkr dbkr requested a review from vector-im/riot-web Dec 18, 2018
dbkr added 2 commits Dec 18, 2018
@jryans jryans self-assigned this Dec 18, 2018
@jryans
jryans approved these changes Dec 18, 2018
Copy link
Member

jryans left a comment

Thanks, this looks good to me. 😁

// set the platform for react sdk (our Platform object automatically picks the right one)
PlatformPeg.set(new Platform());
// set the platform for react sdk
//if (navigator.userAgent.toLowerCase().indexOf('electron') > 0) {

This comment has been minimized.

Copy link
@jryans

jryans Dec 18, 2018

Member

Maybe remove this commented code?

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 19, 2018

Author Member

Oops, good spot.

@@ -108,11 +108,6 @@ module.exports = {
"matrix-js-sdk": path.resolve('./node_modules/matrix-js-sdk'),
},
},
externals: {

This comment has been minimized.

Copy link
@jryans

jryans Dec 18, 2018

Member

I am not sure I follow why this was removed... It seems unrelated to the rest of the changes. Is it something outdated that's no longer needed?

This comment has been minimized.

Copy link
@MTRNord

MTRNord Dec 18, 2018

Contributor

The webpack side doesn't import electron anymore from what I can see. Means that it doesn't need to be declared anymore in the webpack config. (webpack side == everything that actually is riot)

This comment has been minimized.

Copy link
@dbkr

dbkr Dec 19, 2018

Author Member

Yeah, it was just telling webpack to spit out a real 'require' into the bundle for electron rather than try to resolve it itself, but these are all gone now since they won't work in the renderer.

@dbkr dbkr merged commit b35903b into develop Dec 19, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@anoadragon453 anoadragon453 deleted the dbkr/electron_sandbox branch Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.