feat: preloads and nodeIntegration in iframes #16425

MarshallOfSound · 2019-01-17T05:46:13Z

Description of Change

This adds support for running preload scripts and nodeIntegration in sub-frames (iframes)

Checklist

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

Release Notes

Notes: Added support for running preload scripts and nodeIntegration in iframes

atom/renderer/atom_renderer_client.cc

atom/renderer/atom_sandboxed_renderer_client.cc

docs/api/browser-window.md

lib/browser/guest-window-manager.js

lib/renderer/init.js

nornagon · 2019-01-17T23:20:45Z

lib/renderer/window-setup.js

@@ -26,7 +26,7 @@
 const { defineProperty, defineProperties } = Object

 // Helper function to resolve relative url.
-const a = window.top.document.createElement('a')
+const a = window.document.createElement('a')
 const resolveURL = function (url) {
  a.href = url
  return a.href


this function can be replaced with the URL API

const resolveURL = function(url) { return new URL(url, location.href).href }

I'll follow up with this and other clean up referenced above. I want to keep this down to just the new code

nornagon · 2019-01-17T23:26:41Z

spec/api-subframe-spec.js

+describe('renderer nodeSupportInSubFrames', () => {
+  const generateTests = (sandboxEnabled) => {
+    describe(`with sandbox ${sandboxEnabled ? 'enabled' : 'disabled'}`, () => {
+      let w


instead of this shared global, perhaps we could do something like

const withWindow = async (f) => { const w = new BrowserWindow(...) await f(w) await closeWindow(w) } // later it('something', async () => { await withWindow(async (w) => { // ... }) })

Or perhaps there's a nice way to do something like

it('something', withWindow(async () => { // ... }))

mainly I don't like that the window is closed in the test after. That could result in test failures relating to teardown being misattributed.

This is our current pattern, I like the idea of withWindow but I'd rather do that as a sweeping change rather than file-by-file for consitency 👍

nornagon · 2019-01-17T23:28:21Z

spec/api-subframe-spec.js

+      it('should load preload scripts in nested iframes', async () => {
+        const detailsPromise = emittedNTimes(remote.ipcMain, 'preload-ran', 3)
+        w.loadFile(path.resolve(__dirname, 'fixtures/sub-frames/frame-with-frame-container.html'))
+        const [event1, event2, event3] = await detailsPromise


Would it make sense to refactor these tests to fail rather than timeout if the preload doesn't get loaded?

Yes, but that's hard to do without complicated IPC. A timeout in this case seemed the simplest (unfortunately) unless you have a good idea here

zcbenz

Nice work!

This feature has delibrately been built / implemented in such a way that it has minimum impact on existing apps / code-paths. Without enabling the new "nodeSupportInSubFrames" option basically none of this new code will be hit. The things that I believe need extra scrutiny are: * Introduction of `event.reply` for IPC events and usage of `event.reply` instead of `event.sender.send()` * Usage of `node::FreeEnvironment(env)` when the new option is enabled in order to avoid memory leaks. I have tested this quite a bit and haven't managed to cause a crash but it is still feature flagged behind the "nodeSupportInSubFrames" flag to avoid potential impact. Closes #10569 Closes #10401 Closes #11868 Closes #12505 Closes #14035

Co-Authored-By: MarshallOfSound <samuel.r.attard@gmail.com>

nornagon · 2019-01-22T18:29:10Z

lib/browser/api/web-contents.js

@@ -330,6 +342,22 @@ WebContents.prototype.loadFile = function (filePath, options = {}) {
  }))
 }

+const wrapEventWithReply = (event) => {


these still don't "wrap" a thing?

Ah right, will rename

release-clerk · 2019-01-22T19:24:49Z

Release Notes Persisted

Added support for running preload scripts and nodeIntegration in iframes

* feat: add support for node / preloads in subframes This feature has delibrately been built / implemented in such a way that it has minimum impact on existing apps / code-paths. Without enabling the new "nodeSupportInSubFrames" option basically none of this new code will be hit. The things that I believe need extra scrutiny are: * Introduction of `event.reply` for IPC events and usage of `event.reply` instead of `event.sender.send()` * Usage of `node::FreeEnvironment(env)` when the new option is enabled in order to avoid memory leaks. I have tested this quite a bit and haven't managed to cause a crash but it is still feature flagged behind the "nodeSupportInSubFrames" flag to avoid potential impact. Closes #10569 Closes #10401 Closes #11868 Closes #12505 Closes #14035 * feat: add support preloads in subframes for sandboxed renderers * spec: add tests for new nodeSupportInSubFrames option * spec: fix specs for .reply and ._replyInternal for internal messages * chore: revert change to use flag instead of environment set size * chore: clean up subframe impl * chore: apply suggestions from code review Co-Authored-By: MarshallOfSound <samuel.r.attard@gmail.com> * chore: clean up reply usage * chore: fix TS docs generation * chore: cleanup after rebase * chore: rename wrap to add in event fns

miniak · 2019-06-15T18:02:47Z

Is there a chance this will make it into v4.1.x?
@CVertex highly unlikely

vladimiry · 2019-07-31T15:43:05Z

Could the event.reply call potentially end up with Object has been destroyed: sender-like error? If so will the event.sender.isDestroyed() be a proper check?

vladimiry · 2019-08-02T07:40:34Z

I answer the question myself.

Here is the reply code:

electron/lib/browser/api/web-contents.js

Lines 287 to 289 in e78b902

    
           event.reply = (...args) => { 
        
             event.sender.sendToFrame(event.frameId, ...args) 
        
           }

So yes event.sender.isDestroyed() check is needed since reply internally accesses the event.sender.

Here is how the error looks if there is no event.sender.isDestroyed() check:

Error: Object has been destroyed
    at WebContents.sendToFrame (/opt/ElectronMail/resources/electron.asar/browser/api/web-contents.js:138:17)
    at Object.event.reply (/opt/ElectronMail/resources/electron.asar/browser/api/web-contents.js:301:22)

MarshallOfSound requested review from a team January 17, 2019 05:46

nornagon requested changes Jan 17, 2019

View reviewed changes

qazbnm456 mentioned this pull request Jan 20, 2019

Session wide preload script LulumiProject/lulumi-browser#24

Open

zcbenz approved these changes Jan 21, 2019

View reviewed changes

MarshallOfSound and others added 10 commits January 22, 2019 09:04

feat: add support preloads in subframes for sandboxed renderers

939dd8d

spec: add tests for new nodeSupportInSubFrames option

94e0944

spec: fix specs for .reply and ._replyInternal for internal messages

aa18611

chore: revert change to use flag instead of environment set size

44e4943

chore: clean up subframe impl

5842618

chore: apply suggestions from code review

eaa44c5

Co-Authored-By: MarshallOfSound <samuel.r.attard@gmail.com>

chore: clean up reply usage

bb2926e

chore: fix TS docs generation

64310cd

chore: cleanup after rebase

ad0f072

MarshallOfSound force-pushed the sub-frame-node-rebased branch from 1f3921f to ad0f072 Compare January 22, 2019 17:08

nornagon reviewed Jan 22, 2019

View reviewed changes

chore: rename wrap to add in event fns

cd00a36

nornagon approved these changes Jan 22, 2019

View reviewed changes

MarshallOfSound merged commit 58a6fe1 into master Jan 22, 2019

MarshallOfSound deleted the sub-frame-node-rebased branch January 22, 2019 19:24

miniak mentioned this pull request Jan 23, 2019

fix: check process.isMainFrame in sandboxed_renderer/init.js #16500

Merged

3 tasks

dansteen mentioned this pull request Feb 6, 2019

Support new nodeIntegrationInSubFrames in webviews #16780

Closed

samuelmaddock mentioned this pull request Feb 7, 2019

Sub frames should get an isolated context when node is enabled in sub frames #16804

Closed

This was referenced May 6, 2019

Preload script not run with nativeWindowOpen=true and window.open('') #18070

Closed

fix: lost window.opener after cross-origin navigation #18173

Merged

andreasdj mentioned this pull request Oct 6, 2020

Feature request: nodeIntegrationInSubFrames for preload scripts #22582

Open

3 tasks

Obi-Dann mentioned this pull request Oct 9, 2020

Support Node / Preload scripts in iframes or workers #12505

Closed

jkae mentioned this pull request Nov 18, 2020

fix(cookies): use sameSite option, default to 'lax' ngageoint/opensphere-electron#29

Merged

rhysbrettbowen mentioned this pull request Nov 29, 2023

[Feature Request]: Enable preloads in iframes without node integration #40646

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: preloads and nodeIntegration in iframes #16425

feat: preloads and nodeIntegration in iframes #16425

MarshallOfSound commented Jan 17, 2019

nornagon Jan 17, 2019

MarshallOfSound Jan 18, 2019

nornagon Jan 17, 2019

MarshallOfSound Jan 18, 2019

nornagon Jan 17, 2019

MarshallOfSound Jan 18, 2019

zcbenz left a comment

nornagon Jan 22, 2019

MarshallOfSound Jan 22, 2019

release-clerk bot commented Jan 22, 2019

miniak commented Jun 15, 2019

vladimiry commented Jul 31, 2019 •

edited

vladimiry commented Aug 2, 2019 •

edited

feat: preloads and nodeIntegration in iframes #16425

feat: preloads and nodeIntegration in iframes #16425

Conversation

MarshallOfSound commented Jan 17, 2019

Description of Change

Checklist

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcbenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

release-clerk bot commented Jan 22, 2019

miniak commented Jun 15, 2019

vladimiry commented Jul 31, 2019 • edited

vladimiry commented Aug 2, 2019 • edited

vladimiry commented Jul 31, 2019 •

edited

vladimiry commented Aug 2, 2019 •

edited