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: I guess it's esm #37535

Merged
merged 47 commits into from
Aug 31, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
f356b39
fix: allow ESM loads from within ASAR files
MarshallOfSound Mar 8, 2023
42773e5
fix: ensure that ESM entry points finish loading before app ready
MarshallOfSound Mar 8, 2023
42bc762
fix: allow loading ESM entrypoints via default_app
MarshallOfSound Mar 8, 2023
0b7a829
fix: allow ESM loading for renderer preloads
MarshallOfSound Mar 8, 2023
8390ab0
docs: document current known limitations of esm
MarshallOfSound Mar 8, 2023
7f0cf87
chore: add patches to support blending esm handlers
MarshallOfSound Mar 8, 2023
d89b339
refactor: use SetDefersLoading instead of JoinAppCode in renderers
MarshallOfSound Mar 9, 2023
7d93690
chore: add patch to expose SetDefersLoading
MarshallOfSound Mar 9, 2023
f149167
fix: use fileURLToPath instead of pathname
MarshallOfSound Mar 9, 2023
cca34de
chore: update per PR feedback
MarshallOfSound Mar 9, 2023
acf384c
fix: fs.exists/existsSync should never throw
MarshallOfSound Mar 9, 2023
6f20236
fix: convert path to file url before importing
MarshallOfSound Mar 9, 2023
2b16d4f
fix: oops
MarshallOfSound Mar 9, 2023
e179a92
fix: oops
MarshallOfSound Mar 9, 2023
d0601a3
Update docs/tutorial/esm-limitations.md
MarshallOfSound Mar 9, 2023
39d87af
windows...
MarshallOfSound Mar 9, 2023
8cf4bf9
windows...
MarshallOfSound Mar 9, 2023
799ab67
Merged in branch 'main' to fix mergability
electron-patch-conflict-fixer[bot] Mar 10, 2023
66a088f
chore: update patches
patchup[bot] Mar 10, 2023
6f49f89
spec: fix tests and document empty body edge case
MarshallOfSound Mar 10, 2023
329c36e
Apply suggestions from code review
MarshallOfSound Mar 13, 2023
62f9c28
spec: add tests for esm
MarshallOfSound Mar 14, 2023
5343b91
spec: windows
MarshallOfSound Mar 15, 2023
1f7bb22
chore: update per PR feedback
MarshallOfSound Mar 21, 2023
2b8dd6d
Merge remote-tracking branch 'origin/main' into engineering-science-m…
MarshallOfSound Mar 21, 2023
0094ed7
chore: update patches
MarshallOfSound Mar 21, 2023
98b65af
Update shell/common/node_bindings.h
MarshallOfSound Mar 21, 2023
7eddf99
Merged in branch 'main' to fix mergability
electron-patch-conflict-fixer[bot] Mar 31, 2023
c31fcc1
chore: update patches
patchup[bot] Mar 31, 2023
2c78e92
Merged in branch 'main' to fix mergability
electron-patch-conflict-fixer[bot] Apr 12, 2023
dc1cc2e
Merged in branch 'main' to fix mergability
electron-patch-conflict-fixer[bot] Apr 16, 2023
7ac8691
Merge remote-tracking branch 'origin/main' into engineering-science-m…
MarshallOfSound May 15, 2023
438ad9b
rebase
MarshallOfSound May 16, 2023
3da5be8
use cjs loader by default for preload scripts
MarshallOfSound May 17, 2023
b5da4ad
Merge remote-tracking branch 'origin/main' into engineering-science-m…
MarshallOfSound Jun 12, 2023
6a29c3d
Merge remote-tracking branch 'origin/main' into engineering-science-m…
MarshallOfSound Aug 28, 2023
b8601c8
chore: fix lint
MarshallOfSound Aug 29, 2023
37b02a7
chore: update patches
MarshallOfSound Aug 29, 2023
7f17796
chore: update patches
MarshallOfSound Aug 29, 2023
aa87ba1
chore: fix patches
MarshallOfSound Aug 29, 2023
0de8252
build: debug depshash
MarshallOfSound Aug 29, 2023
d74274e
?
MarshallOfSound Jun 22, 2023
ff86b12
Revert "build: debug depshash"
MarshallOfSound Aug 29, 2023
27adf25
chore: allow electron as builtin protocol in esm loader
MarshallOfSound Aug 30, 2023
8a71118
Revert "Revert "build: debug depshash""
MarshallOfSound Aug 30, 2023
c901f38
chore: fix esm doc
MarshallOfSound Aug 30, 2023
1bfa90c
chore: update node patches
MarshallOfSound Aug 30, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions default_app/default_app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,17 @@ async function createWindow (backgroundColor?: string) {
autoHideMenuBar: true,
backgroundColor,
webPreferences: {
preload: new URL('preload.js', import.meta.url).pathname,
preload: url.fileURLToPath(new URL('preload.js', import.meta.url)),
contextIsolation: true,
sandbox: false,
nodeIntegration: true
sandbox: true,
nodeIntegration: false
},
useContentSize: true,
show: false
};

if (process.platform === 'linux') {
options.icon = new URL('icon.png', import.meta.url).pathname;
options.icon = url.fileURLToPath(new URL('icon.png', import.meta.url));
}

mainWindow = new BrowserWindow(options);
Expand Down
2 changes: 1 addition & 1 deletion docs/tutorial/esm-limitations.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ This document serves to outline the limitations / differences between ESM in Ele

Sandboxed preload scripts are run as plain javascript without an ESM context. It is reccomended that preload scripts are bundled via something like `webpack` or `vite` for performance reasons regardless, so your preload script should just be a single file that doesn't need to use ESM imports. Loading the `electron` API is still done via `require('electron')`.

## Non context isolated renderers can't use Node.js ESM imports
## Non-context-isolated renderers can't use Node.js ESM imports
Copy link
Member

Choose a reason for hiding this comment

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

From the content of this section, it sounds like renderers can never import Node.js ESM modules, but that enabling contextIsolation can enable preload to load ESM modules. Should the title of this section be updated and/or merged with the one above?

Alternately, maybe the focus of these sections should be on the only configuration where it is possible to use ESM imports to import Node scripts, which seems to be inside a preload script with sandboxing disabled and contextIsolation enabled.

Final thought: if enabling ESM Node imports from preloads requires disabling an important security feature (sandboxing), is it worth supporting at all? Since none of this is already supported, it wouldn't be a regression to say "ESM imports are only supported in the Main process". Would that be a better (and simpler to document) line to draw?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there's a misunderstanding here (which is good it shows the docs need improvement). Lemme draw a table and clarify something.

In the renderer process there are two different "ESM" loaders. Node's and Blinks. There are also two different ways of doing ESM imports. The static import foo from 'bar' directive and the dynamic import('bar') method. With this information in hand the table below should make more sense

NodeJS Static Import NodeJS Dynamic Import Blink Static Import Blink Dynamic Import
Sandbox Enabled No (sandbox means no node) No (sandbox means no node) Yes, main world only Yes, main world only
Sandbox Disabled, Context Isolation Enabled Yes, preload script only Yes, preload script only Yes, main world only Yes, main world only
Sandbox Disabled, Context Isolation Disabled Yes, preload script only No Yes, main world only Yes, main world only

if enabling ESM Node imports from preloads requires disabling an important security feature (sandboxing), is it worth supporting at all?

I think the misunderstanding here is that enabling "ESM Node imports" by definitions requires disabling the sandbox because it requires enabling "Node" in the first place.

The only two edge cases (which I tried to document here) are:

  • Sandbox off, ctx isolation off: NodeJS dynamic import is not supported anywhere
  • Sandbox on: even blinks static and dynamic imports are not supported at all in preload scripts (in the same way that CJS isn't currently supported in these scripts either)


If your renderer process does not have `contextIsolation` enabled you can not `import` ESM files via the Node.js module loader. This means that you can't `import('fs')` or `import('./foo')`. If you want to be able to do so you must enable context isolation. This is because in the renderer Chromium's `import()` function takes precedence and without context isolation there is no way for Electron to know which loader to route the request to.

Expand Down
15 changes: 13 additions & 2 deletions lib/asar/fs-wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,13 @@ export const wrapFsWithAsar = (fs: Record<string, any>) => {

const { exists: nativeExists } = fs;
fs.exists = function exists (pathArgument: string, callback: any) {
const pathInfo = splitPath(pathArgument);
let pathInfo: ReturnType<typeof splitPath>;
try {
pathInfo = splitPath(pathArgument);
} catch {
nextTick(callback, [false]);
return;
}
if (!pathInfo.isAsar) return nativeExists(pathArgument, callback);
const { asarPath, filePath } = pathInfo;

Expand Down Expand Up @@ -423,7 +429,12 @@ export const wrapFsWithAsar = (fs: Record<string, any>) => {

const { existsSync } = fs;
fs.existsSync = (pathArgument: string) => {
const pathInfo = splitPath(pathArgument);
let pathInfo: ReturnType<typeof splitPath>;
try {
pathInfo = splitPath(pathArgument);
} catch {
return false;
}
if (!pathInfo.isAsar) return existsSync(pathArgument);
const { asarPath, filePath } = pathInfo;

Expand Down
21 changes: 8 additions & 13 deletions lib/renderer/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,24 +125,19 @@ if (nodeIntegration) {
const { appCodeLoaded } = process;
delete process.appCodeLoaded;

const { preloadPaths } = ipcRendererUtils.invokeSync(IPC_MESSAGES.BROWSER_NONSANDBOX_LOAD);
const preloadPromises: Promise<void>[] = [];
// Load the preload scripts.
for (const preloadScript of preloadPaths) {
// Finally load app's main.js and transfer control to C++.
const { preloadPaths } = ipcRendererUtils.invokeSync<{ preloadPaths: string[] }>(IPC_MESSAGES.BROWSER_NONSANDBOX_LOAD);
if (preloadPaths.length) {
const { loadESM } = __non_webpack_require__('internal/process/esm_loader');
preloadPromises.push(loadESM((esmLoader: any) => {
return esmLoader.import(preloadScript, undefined, Object.create(null)).catch((err: Error) => {

loadESM((esmLoader: any) => {
// Load the preload scripts.
return Promise.all(preloadPaths.map(preloadScript => esmLoader.import(preloadScript, undefined, Object.create(null)).catch((err: Error) => {
console.error(`Unable to load preload script: ${preloadScript}`);
console.error(err);

ipcRendererInternal.send(IPC_MESSAGES.BROWSER_PRELOAD_ERROR, preloadScript, err);
});
}));
}

if (preloadPromises.length) {
Promise.all(preloadPromises).then(() => appCodeLoaded!());
})));
}).then(() => appCodeLoaded!());
} else {
appCodeLoaded!();
}
1 change: 1 addition & 0 deletions patches/chromium/.patches
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,4 @@ chore_patch_out_profile_methods_in_profile_selections_cc.patch
fix_x11_window_restore_minimized_maximized_window.patch
chore_defer_usb_service_getdevices_request_until_usb_service_is.patch
refactor_expose_hostimportmoduledynamically_and.patch
feat_expose_documentloader_setdefersloading_on_webdocumentloader.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Samuel Attard <marshallofsound@electronjs.org>
Date: Thu, 9 Mar 2023 01:28:56 -0800
Subject: feat: expose DocumentLoader::SetDefersLoading on WebDocumentLoader

This allows embedders to call SetDefersLoading without reaching into blink internals. Electron uses this to defer page loading until the preload scripts have finished executing.
This might be upstreamable?

diff --git a/third_party/blink/public/web/web_document_loader.h b/third_party/blink/public/web/web_document_loader.h
index ff1948e649fffdc92a2db0e736c99bf4e8c06514..b165201b273c8fa9de8e66fe8ef7bfc28eee0850 100644
--- a/third_party/blink/public/web/web_document_loader.h
+++ b/third_party/blink/public/web/web_document_loader.h
@@ -38,6 +38,7 @@
#include "third_party/blink/public/platform/cross_variant_mojo_util.h"
#include "third_party/blink/public/platform/web_archive_info.h"
#include "third_party/blink/public/platform/web_common.h"
+#include "third_party/blink/public/platform/web_loader_freeze_mode.h"
#include "third_party/blink/public/platform/web_source_location.h"
#include "third_party/blink/public/web/web_navigation_type.h"

@@ -62,6 +63,8 @@ class BLINK_EXPORT WebDocumentLoader {
virtual ~ExtraData() = default;
};

+ virtual void SetDefersLoading(WebLoaderFreezeMode) = 0;
+
static bool WillLoadUrlAsEmpty(const WebURL&);

// Returns the http referrer of original request which initited this load.
diff --git a/third_party/blink/renderer/core/loader/document_loader.h b/third_party/blink/renderer/core/loader/document_loader.h
index f8839bb4ad9480daecd93b7aba4d4fef41aa1008..2f61bbbdc2dd6e76fda84eae2a7871c8cea28222 100644
--- a/third_party/blink/renderer/core/loader/document_loader.h
+++ b/third_party/blink/renderer/core/loader/document_loader.h
@@ -300,7 +300,7 @@ class CORE_EXPORT DocumentLoader : public GarbageCollected<DocumentLoader>,
absl::optional<scheduler::TaskAttributionId>
soft_navigation_heuristics_task_id);

- void SetDefersLoading(LoaderFreezeMode);
+ void SetDefersLoading(LoaderFreezeMode) override;

DocumentLoadTiming& GetTiming() { return document_load_timing_; }

8 changes: 4 additions & 4 deletions patches/node/chore_expose_importmoduledynamically_and.patch
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ it's own blended handler between node / blink.
Not upstremable.

diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js
index c4fe744be1c26c2306e843031192d88f977cd46c..badd978d58049c07716f3a1151da331d31506799 100644
index c4fe744be1c26c2306e843031192d88f977cd46c..f8218046cbdfbaeb0aaecd50676b6fcb37c8e4d7 100644
--- a/lib/internal/process/pre_execution.js
+++ b/lib/internal/process/pre_execution.js
@@ -575,7 +575,7 @@ function initializeESMLoader() {
// Create this WeakMap in js-land because V8 has no C++ API for WeakMap.
internalBinding('module_wrap').callbackMap = new SafeWeakMap();

- if (getEmbedderOptions().shouldNotRegisterESMLoader) return;
+ const setOnIsolate = !getEmbedderOptions().shouldNotRegisterESMLoader;
+ const shouldSetOnIsolate = !getEmbedderOptions().shouldNotRegisterESMLoader;

const {
setImportModuleDynamicallyCallback,
Expand All @@ -29,8 +29,8 @@ index c4fe744be1c26c2306e843031192d88f977cd46c..badd978d58049c07716f3a1151da331d
// track of for different ESM modules.
- setInitializeImportMetaObjectCallback(esm.initializeImportMetaObject);
- setImportModuleDynamicallyCallback(esm.importModuleDynamicallyCallback);
+ setInitializeImportMetaObjectCallback(esm.initializeImportMetaObject, setOnIsolate);
+ setImportModuleDynamicallyCallback(esm.importModuleDynamicallyCallback, setOnIsolate);
+ setInitializeImportMetaObjectCallback(esm.initializeImportMetaObject, shouldSetOnIsolate);
+ setImportModuleDynamicallyCallback(esm.importModuleDynamicallyCallback, shouldSetOnIsolate);

// Patch the vm module when --experimental-vm-modules is on.
// Please update the comments in vm.js when this block changes.
Expand Down
18 changes: 12 additions & 6 deletions shell/common/node_bindings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,8 @@ node::Environment* NodeBindings::CreateEnvironment(
v8::Handle<v8::Context> context,
node::MultiIsolatePlatform* platform,
std::vector<std::string> args,
std::vector<std::string> exec_args) {
std::vector<std::string> exec_args,
absl::optional<base::RepeatingCallback<void()>> on_app_code_ready) {
// Feed node the path to initialization script.
std::string process_type;
switch (browser_env_) {
Expand Down Expand Up @@ -677,17 +678,22 @@ node::Environment* NodeBindings::CreateEnvironment(

if (browser_env_ == BrowserEnvironment::kBrowser ||
browser_env_ == BrowserEnvironment::kRenderer) {
process.SetMethod("appCodeLoaded",
base::BindRepeating(&NodeBindings::SetAppCodeLoaded,
base::Unretained(this)));
if (on_app_code_ready) {
process.SetMethod("appCodeLoaded", std::move(*on_app_code_ready));
} else {
process.SetMethod("appCodeLoaded",
base::BindRepeating(&NodeBindings::SetAppCodeLoaded,
base::Unretained(this)));
}
}

return env;
}

node::Environment* NodeBindings::CreateEnvironment(
v8::Handle<v8::Context> context,
node::MultiIsolatePlatform* platform) {
node::MultiIsolatePlatform* platform,
absl::optional<base::RepeatingCallback<void()>> on_app_code_ready) {
#if BUILDFLAG(IS_WIN)
auto& electron_args = ElectronCommandLine::argv();
std::vector<std::string> args(electron_args.size());
Expand All @@ -696,7 +702,7 @@ node::Environment* NodeBindings::CreateEnvironment(
#else
auto args = ElectronCommandLine::argv();
#endif
return CreateEnvironment(context, platform, args, {});
return CreateEnvironment(context, platform, args, {}, on_app_code_ready);
}

void NodeBindings::LoadEnvironment(node::Environment* env) {
Expand Down
22 changes: 16 additions & 6 deletions shell/common/node_bindings.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
#include <vector>

#include "base/files/file_path.h"
#include "base/functional/callback.h"
#include "base/memory/weak_ptr.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "uv.h" // NOLINT(build/include_directory)
#include "v8/include/v8.h"

Expand Down Expand Up @@ -90,12 +92,18 @@ class NodeBindings {
void SetNodeCliFlags();

// Create the environment and load node.js.
node::Environment* CreateEnvironment(v8::Handle<v8::Context> context,
node::MultiIsolatePlatform* platform,
std::vector<std::string> args,
std::vector<std::string> exec_args);
node::Environment* CreateEnvironment(v8::Handle<v8::Context> context,
node::MultiIsolatePlatform* platform);
node::Environment* CreateEnvironment(
v8::Handle<v8::Context> context,
node::MultiIsolatePlatform* platform,
std::vector<std::string> args,
std::vector<std::string> exec_args,
absl::optional<base::RepeatingCallback<void()>> on_app_code_ready =
absl::nullopt);
node::Environment* CreateEnvironment(
v8::Handle<v8::Context> context,
node::MultiIsolatePlatform* platform,
absl::optional<base::RepeatingCallback<void()>> on_app_code_ready =
absl::nullopt);

// Load node.js in the environment.
void LoadEnvironment(node::Environment* env);
Expand Down Expand Up @@ -124,6 +132,8 @@ class NodeBindings {
NodeBindings(const NodeBindings&) = delete;
NodeBindings& operator=(const NodeBindings&) = delete;

// Blocks until app code is signaled to be loaded via |SetAppCodeLoaded|.
// Only has affect if called in the browser process
void JoinAppCode();

protected:
Expand Down
21 changes: 17 additions & 4 deletions shell/renderer/electron_renderer_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "third_party/blink/public/web/web_document.h"
#include "third_party/blink/public/web/web_local_frame.h"
#include "third_party/blink/renderer/core/execution_context/execution_context.h" // nogncheck
#include "third_party/blink/renderer/core/frame/web_local_frame_impl.h" // nogncheck

namespace electron {

Expand Down Expand Up @@ -61,6 +62,11 @@ void ElectronRendererClient::RunScriptsAtDocumentEnd(
"document-end");
}

void ElectronRendererClient::UndeferLoad(content::RenderFrame* render_frame) {
render_frame->GetWebFrame()->GetDocumentLoader()->SetDefersLoading(
blink::LoaderFreezeMode::kNone);
}

void ElectronRendererClient::DidCreateScriptContext(
v8::Handle<v8::Context> renderer_context,
content::RenderFrame* render_frame) {
Expand Down Expand Up @@ -88,8 +94,17 @@ void ElectronRendererClient::DidCreateScriptContext(
v8::Maybe<bool> initialized = node::InitializeContext(renderer_context);
CHECK(!initialized.IsNothing() && initialized.FromJust());

node::Environment* env =
node_bindings_->CreateEnvironment(renderer_context, nullptr);
// Before we load the node environment, let's tell blink to hold off on
// loading the body of this frame. We will undefer the load once the preload
// script has finished. This allows our preload script to run async (E.g.
// with ESM) without the preload being in a race
render_frame->GetWebFrame()->GetDocumentLoader()->SetDefersLoading(
blink::LoaderFreezeMode::kStrict);

node::Environment* env = node_bindings_->CreateEnvironment(
renderer_context, nullptr,
base::BindRepeating(&ElectronRendererClient::UndeferLoad,
base::Unretained(this), render_frame));

// If we have disabled the site instance overrides we should prevent loading
// any non-context aware native module.
Expand All @@ -115,8 +130,6 @@ void ElectronRendererClient::DidCreateScriptContext(
// Give the node loop a run to make sure everything is ready.
node_bindings_->StartPolling();
}

node_bindings_->JoinAppCode();
}

void ElectronRendererClient::WillReleaseScriptContext(
Expand Down
2 changes: 2 additions & 0 deletions shell/renderer/electron_renderer_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ class ElectronRendererClient : public RendererClientBase {
content::RenderFrame* render_frame) override;

private:
void UndeferLoad(content::RenderFrame* render_frame);

// content::ContentRendererClient:
void RenderFrameCreated(content::RenderFrame*) override;
void RunScriptsAtDocumentStart(content::RenderFrame* render_frame) override;
Expand Down