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

multi: Update to electron v23.3.8 #3884

Merged
merged 1 commit into from Jun 30, 2023
Merged

Conversation

JoeGruffins
Copy link
Member

@JoeGruffins JoeGruffins commented Jun 25, 2023

This is needed for webusb support because it is the earliest release to include these changes electron/electron#36289

webusb is needed to use Ledger over usb without having to add anything to the "main_dev" layer.

nodeIntegration: false,
nodeIntegration: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

With this on false getting errors like:

VM4 sandbox_bundle:2 Unable to load preload script: /home/joe/git/decrediton/app/dist/wallet-preload.js
(anonymous)	@	VM4 sandbox_bundle:2
VM4 sandbox_bundle:2 Error: module not found: fs
    at preloadRequire (VM4 sandbox_bundle:2:82255)
    at Object.fs (<anonymous>:182057:18)
    at __webpack_require__ (<anonymous>:182411:42)
    at ./app/wallet/fs.js (<anonymous>:139438:34)
    at __webpack_require__ (<anonymous>:182411:42)
    at ./app/wallet-preload.js (<anonymous>:137577:34)
    at __webpack_require__ (<anonymous>:182411:42)
    at <anonymous>:182438:37
    at <anonymous>:182441:12
    at runPreloadScript (VM4 sandbox_bundle:2:83095)
bootstrap:28 Uncaught TypeError: Cannot read properties of undefined (reading 'blake256')
    at ./app/helpers/walletCryptoModule.js (walletCryptoModule.js:7:38)
    at __webpack_require__ (bootstrap:25:4)
    at fn (hot module replacement:63:1)
    at ./app/helpers/addresses.js (addresses.js:10:1)
    at __webpack_require__ (bootstrap:25:4)
    at fn (hot module replacement:63:1)
    at ./app/helpers/index.js (index.js:5:1)
    at __webpack_require__ (bootstrap:25:4)
    at fn (hot module replacement:63:1)
    at ./app/selectors.js (selectors.js:17:1)

And I guess this is not the change we want. Will try to find the correct solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, it seems that with electron v20 the sandbox value's default was changed from false to true. So, setting false, and I believe this is not a change from the previous behavior. https://www.electronjs.org/docs/latest/tutorial/sandbox

https://github.com/decred/decrediton/compare/e15e45658a20572520d2e9a8b2a0533d3297d4af..76047017969fc70e826335538c497a963dcdfd22

Copy link
Member

@matheusd matheusd Jun 29, 2023

Choose a reason for hiding this comment

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

The Preload Scripts section of the doc you linked to hints as to why you're getting the error:

[...] preload scripts attached to sandboxed renderers will still have a polyfilled subset of Node.js APIs available.

And notably, fs is not on the list of available modules. So in order to support sandbox mode, we'd have to move all references to fs (and any other unsupported node/electron modules) from the preload layer to the main_dev layer. It would have to be done carefully though, to avoid re-opening generic holes from which stuff in the renderer layer could use to escape to the main_dev layer.

It would be a worthwhile project to do, but I agree that disabling sandbox with the current impl as we're upgrading the electron version is the way to go now, and working to get sandbox: true would be a separate issue/pr.

Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

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

Tested on both linux and windows.

FYI @alexlyp, I had to update my windows build setup (msys2 version, node version, python version, VC build tools version) in order to get the native dcrwin32ipc module to build, and do the whole clean-yarn-rebuild natives dance to get it to compile, but I do have it syncing now.

@JoeGruffins any specific reason not to update to the latest (v25) electron version? Regardless, this is working as intended.

@JoeGruffins
Copy link
Member Author

JoeGruffins commented Jun 30, 2023

@JoeGruffins any specific reason not to update to the latest (v25) electron version? Regardless, this is working as intended.

The reason was that this is the lowest version that works, that is all. I was afraid that the further along the version, the more that may break. Would you like to update to v25?

@alexlyp
Copy link
Member

alexlyp commented Jun 30, 2023

Tested on both linux and windows.

FYI @alexlyp, I had to update my windows build setup (msys2 version, node version, python version, VC build tools version) in order to get the native dcrwin32ipc module to build, and do the whole clean-yarn-rebuild natives dance to get it to compile, but I do have it syncing now.

@JoeGruffins any specific reason not to update to the latest (v25) electron version? Regardless, this is working as intended.

Thanks for the headsup @matheusd

@alexlyp alexlyp merged commit 51c5c8c into decred:master Jun 30, 2023
1 check passed
alexlyp pushed a commit that referenced this pull request Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants