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: preloads and nodeIntegration in iframes #16425

Merged
merged 11 commits into from Jan 22, 2019

Conversation

Projects
None yet
4 participants
@MarshallOfSound
Copy link
Member

commented Jan 17, 2019

Description of Change

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

Checklist

Release Notes

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

@MarshallOfSound MarshallOfSound requested review from as code owners Jan 17, 2019

Show resolved Hide resolved atom/renderer/atom_renderer_client.cc Outdated
Show resolved Hide resolved atom/renderer/atom_sandboxed_renderer_client.cc Outdated
Show resolved Hide resolved docs/api/browser-window.md Outdated
Show resolved Hide resolved docs/api/browser-window.md Outdated
Show resolved Hide resolved docs/api/browser-window.md Outdated
Show resolved Hide resolved lib/browser/guest-window-manager.js Outdated
Show resolved Hide resolved lib/renderer/init.js Outdated
@@ -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 comment has been minimized.

Copy link
@nornagon

nornagon Jan 17, 2019

Contributor

this function can be replaced with the URL API

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

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Jan 18, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@nornagon

nornagon Jan 17, 2019

Contributor

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 comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Jan 18, 2019

Author Member

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 👍

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

This comment has been minimized.

Copy link
@nornagon

nornagon Jan 17, 2019

Contributor

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

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Jan 18, 2019

Author Member

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

zcbenz approved these changes Jan 21, 2019

Copy link
Member

left a comment

Nice work!

MarshallOfSound and others added some commits Jan 13, 2019

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
chore: apply suggestions from code review
Co-Authored-By: MarshallOfSound <samuel.r.attard@gmail.com>

@MarshallOfSound MarshallOfSound force-pushed the sub-frame-node-rebased branch from 1f3921f to ad0f072 Jan 22, 2019

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

const wrapEventWithReply = (event) => {

This comment has been minimized.

Copy link
@nornagon

nornagon Jan 22, 2019

Contributor

these still don't "wrap" a thing?

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Jan 22, 2019

Author Member

Ah right, will rename

@MarshallOfSound MarshallOfSound merged commit 58a6fe1 into master Jan 22, 2019

18 of 25 checks passed

electron-mas-testing Build #20190122.32 has failed
Details
electron-osx-testing Build #20190122.31 has failed
Details
appveyor: win-ia32-debug Waiting for AppVeyor build to complete
Details
appveyor: win-ia32-testing Waiting for AppVeyor build to complete
Details
appveyor: win-x64-debug Waiting for AppVeyor build to complete
Details
appveyor: win-x64-testing Waiting for AppVeyor build to complete
Details
Artifact Comparison Changes Detected
Details
Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
electron-lint Build #20190122.24 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Jan 22, 2019

Release Notes Persisted

Added support for running preload scripts and nodeIntegration in iframes

@MarshallOfSound MarshallOfSound deleted the sub-frame-node-rebased branch Jan 22, 2019

MarshallOfSound added a commit that referenced this pull request Jan 27, 2019

feat: preloads and nodeIntegration in iframes (#16425)
* 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

MarshallOfSound added a commit that referenced this pull request Jan 27, 2019

feat: preloads and nodeIntegration in iframes (#16425)
* 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
@CVertex

This comment has been minimized.

Copy link

commented on aadee6c Apr 24, 2019

Hey!

Is there a chance this will make it into v4.1.x?

Can't wait to see this feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.