fix: illegal access errors with nodeIntegrationInSubFrames

[codebytere]

Description of Change

Closes #27995.

Fixes an issue where illegal access error would be thrown by the V8 isolate in some circumstances where an iframe was loaded and its source set later when Node.js integration is enabled in subframes. This was happening because if an iframe is created and a source is not set, the iframe loads about:blank and creates a script context for the same. We don't want to create a Node.js environment here because if the src is later set, the JS necessary to do that triggers illegal access errors when the initial about:blank Node.js environment is cleaned up.

See this comment: https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/render_frame_impl.h;l=870-892;drc=4b6001440a18740b76a1c63fa2a002cc941db394

cc @MarshallOfSound

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed an issue where illegal access error could be thrown when nodeIntegrationInSubFrames is enabled.

[deepak1556]

This is a unique case, ideally I wouldn't worry about setup and tear down of the node environment in a frame as it shouldn't affect the execution in other node environment. In this particular case the iframe being in the same process as the parent frame performing sync navigation shares the v8 isolate and hence the disallow execution scope impacts each other. I am fine with the current solution.

NB: looks like the new test fixtures are not tested, LGTM otherwise.

[codebytere]

@deepak1556 they're part of crash-cases - can you clarify what you mean by

looks like the new test fixtures are not tested

?

From macOS tests

[deepak1556]

they're part of crash-cases

Missed that, ignore my comment.

[miniak]

node ./script/lint.js && npm run lint:clang-format && npm run lint:docs
--- a/shell/renderer/electron_render_frame_observer.cc
+++ b/shell/renderer/electron_render_frame_observer.cc
@@ -162,7 +162,8 @@
   // https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/render_frame_impl.h;l=870-892;drc=4b6001440a18740b76a1c63fa2a002cc941db394
   GURL url = render_frame_->GetWebFrame()->GetDocument().Url();
   bool allow_node_in_sub_frames = prefs.node_integration_in_sub_frames;
-  if (allow_node_in_sub_frames && url.IsAboutBlank() && !render_frame_->IsMainFrame())
+  if (allow_node_in_sub_frames && url.IsAboutBlank() &&
+      !render_frame_->IsMainFrame())
     return false;
 
   if (prefs.context_isolation &&

[codebytere]

Test failures are unrelated.

[release-clerk]

Release Notes Persisted

Fixed an issue where illegal access error could be thrown when nodeIntegrationInSubFrames is enabled.

[trop]

I have automatically backported this PR to "12-x-y", please check out #29169

[trop]

I have automatically backported this PR to "13-x-y", please check out #29170

[y489436672]

@deepak1556 This problem still exists. electron@13.1.9 win7 or win10 This problem occurs when I open iframe when I download a file.

[y489436672]

webPreferences: {
webSecurity: false,
nodeIntegration: true,
contextIsolation: false,
enableRemoteModule: true,
allowRunningInsecureContent: true,
nodeIntegrationInSubFrames: true,
preload: path.resolve(global.__preloadPath, './preload.js'),
devTools: true
}

[mckravchyk]

@codebytere

The issue is with loading Node.js environment. Could this PR be improved to allow a pass for sandboxed renderers? Or would it also cause problems?

A sandboxed renderer won't have a Node.js environment initialized.

So we could use preload in about:blank iframes with webPreferences like this:

{
  nodeIntegration: false,
  sandbox: true,
  nodeIntegrationInSubFrames: true,
} 

This would partially fix #31313