Skip to content

fix: window.open causing occasional Node.js crashes

trop / Backportable? - 24-x-y completed Jun 21, 2023 in 46s

Backport Failed

This PR was checked and could not be automatically backported to "24-x-y" cleanly

Details

Failed Diff:

diff --cc shell/common/node_bindings.h
index 1f559f6e39,a980ce4e08..0000000000
--- a/shell/common/node_bindings.h
+++ b/shell/common/node_bindings.h
@@@ -170,11 -172,11 +170,16 @@@ class NodeBindings 
    // Semaphore to wait for main loop in the embed thread.
    uv_sem_t embed_sem_;
  
++<<<<<<< HEAD
 +  // Environment that to wrap the uv loop.
 +  node::Environment* uv_env_ = nullptr;
++=======
+   // Environment that wraps the uv loop.
+   raw_ptr<node::Environment> uv_env_ = nullptr;
++>>>>>>> fix: window.open causing occasional Node.js crashes
  
    // Isolate data used in creating the environment
 -  raw_ptr<node::IsolateData> isolate_data_ = nullptr;
 +  node::IsolateData* isolate_data_ = nullptr;
  
    base::WeakPtrFactory<NodeBindings> weak_factory_{this};
  };
diff --cc shell/renderer/electron_renderer_client.cc
index bc9b38cef7,d2272fb45b..0000000000
--- a/shell/renderer/electron_renderer_client.cc
+++ b/shell/renderer/electron_renderer_client.cc
@@@ -76,7 -101,7 +102,11 @@@ void ElectronRendererClient::DidCreateS
  
    if (!node_integration_initialized_) {
      node_integration_initialized_ = true;
++<<<<<<< HEAD
 +    node_bindings_->Initialize();
++=======
+     node_bindings_->Initialize(context);
++>>>>>>> fix: window.open causing occasional Node.js crashes
      node_bindings_->PrepareEmbedThread();
    }
  
@@@ -194,16 -195,13 +200,26 @@@ void ElectronRendererClient::WillDestro
  }
  
  node::Environment* ElectronRendererClient::GetEnvironment(
++<<<<<<< HEAD
 +    content::RenderFrame* render_frame) const {
 +  if (injected_frames_.find(render_frame) == injected_frames_.end())
 +    return nullptr;
 +  v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
 +  auto context =
 +      GetContext(render_frame->GetWebFrame(), v8::Isolate::GetCurrent());
 +  node::Environment* env = node::Environment::GetCurrent(context);
 +  if (environments_.find(env) == environments_.end())
 +    return nullptr;
 +  return env;
++=======
+     content::RenderFrame* render_frame,
+     v8::Isolate* isolate) const {
+   if (!base::Contains(injected_frames_, render_frame))
+     return nullptr;
+ 
+   auto* env = node::Environment::GetCurrent(isolate);
+   return (env && env == env_) ? env : nullptr;
++>>>>>>> fix: window.open causing occasional Node.js crashes
  }
  
  }  // namespace electron
diff --cc spec/chromium-spec.ts
index d8e02350d8,499b515028..0000000000
--- a/spec/chromium-spec.ts
+++ b/spec/chromium-spec.ts
@@@ -1111,8 -1114,31 +1111,36 @@@ describe('chromium features', () => 
        expect(frameName).to.equal('__proto__');
      });
  
++<<<<<<< HEAD
 +    // TODO(nornagon): I'm not sure this ... ever was correct?
 +    it.skip('inherit options of parent window', async () => {
++=======
+     it('works when used in conjunction with the vm module', async () => {
+       const w = new BrowserWindow({
+         show: false,
+         webPreferences: {
+           nodeIntegration: true,
+           contextIsolation: false
+         }
+       });
+ 
+       await w.loadFile(path.resolve(__dirname, 'fixtures', 'blank.html'));
+ 
+       const { contextObject } = await w.webContents.executeJavaScript(`(async () => {
+         const vm = require('node:vm');
+         const contextObject = { count: 1, type: 'gecko' };
+         window.open('');
+         vm.runInNewContext('count += 1; type = "chameleon";', contextObject);
+         console.log(contextObject);
+         return { contextObject };
+       })()`);
+ 
+       expect(contextObject).to.deep.equal({ count: 2, type: 'chameleon' });
+     });
+ 
+     // FIXME(nornagon): I'm not sure this ... ever was correct?
+     xit('inherit options of parent window', async () => {
++>>>>>>> fix: window.open causing occasional Node.js crashes
        const w = new BrowserWindow({ show: false, width: 123, height: 456 });
        w.loadFile(path.resolve(__dirname, 'fixtures', 'blank.html'));
        const url = `file://${fixturesPath}/pages/window-open-size.html`;

Annotations

Check failure on line 174 in shell/common/node_bindings.h

See this annotation in the file changed.

@trop trop / Backportable? - 24-x-y

shell/common/node_bindings.h#L173-L174

Patch Conflict
Raw output
++<<<<<<< HEAD
 +  // Environment that to wrap the uv loop.
 +  node::Environment* uv_env_ = nullptr;
++=======
+   // Environment that wraps the uv loop.
+   raw_ptr<node::Environment> uv_env_ = nullptr;
++>>>>>>> fix: window.open causing occasional Node.js crashes

Check failure on line 105 in shell/renderer/electron_renderer_client.cc

See this annotation in the file changed.

@trop trop / Backportable? - 24-x-y

shell/renderer/electron_renderer_client.cc#L105

Patch Conflict
Raw output
++<<<<<<< HEAD
 +    node_bindings_->Initialize();
++=======
+     node_bindings_->Initialize(context);
++>>>>>>> fix: window.open causing occasional Node.js crashes

Check failure on line 212 in shell/renderer/electron_renderer_client.cc

See this annotation in the file changed.

@trop trop / Backportable? - 24-x-y

shell/renderer/electron_renderer_client.cc#L203-L212

Patch Conflict
Raw output
++<<<<<<< HEAD
 +    content::RenderFrame* render_frame) const {
 +  if (injected_frames_.find(render_frame) == injected_frames_.end())
 +    return nullptr;
 +  v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
 +  auto context =
 +      GetContext(render_frame->GetWebFrame(), v8::Isolate::GetCurrent());
 +  node::Environment* env = node::Environment::GetCurrent(context);
 +  if (environments_.find(env) == environments_.end())
 +    return nullptr;
 +  return env;
++=======
+     content::RenderFrame* render_frame,
+     v8::Isolate* isolate) const {
+   if (!base::Contains(injected_frames_, render_frame))
+     return nullptr;
+ 
+   auto* env = node::Environment::GetCurrent(isolate);
+   return (env && env == env_) ? env : nullptr;
++>>>>>>> fix: window.open causing occasional Node.js crashes

Check failure on line 1115 in spec/chromium-spec.ts

See this annotation in the file changed.

@trop trop / Backportable? - 24-x-y

spec/chromium-spec.ts#L1114-L1115

Patch Conflict
Raw output
++<<<<<<< HEAD
 +    // TODO(nornagon): I'm not sure this ... ever was correct?
 +    it.skip('inherit options of parent window', async () => {
++=======
+     it('works when used in conjunction with the vm module', async () => {
+       const w = new BrowserWindow({
+         show: false,
+         webPreferences: {
+           nodeIntegration: true,
+           contextIsolation: false
+         }
+       });
+ 
+       await w.loadFile(path.resolve(__dirname, 'fixtures', 'blank.html'));
+ 
+       const { contextObject } = await w.webContents.executeJavaScript(`(async () => {
+         const vm = require('node:vm');
+         const contextObject = { count: 1, type: 'gecko' };
+         window.open('');
+         vm.runInNewContext('count += 1; type = "chameleon";', contextObject);
+         console.log(contextObject);
+         return { contextObject };
+       })()`);
+ 
+       expect(contextObject).to.deep.equal({ count: 2, type: 'chameleon' });
+     });
+ 
+     // FIXME(nornagon): I'm not sure this ... ever was correct?
+     xit('inherit options of parent window', async () => {
++>>>>>>> fix: window.open causing occasional Node.js crashes