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

Inherit webPreferences in windows opened via sandbox or nativeWindowOpen #9972

Merged
merged 13 commits into from Jul 17, 2017

Conversation

Projects
None yet
3 participants
@kevinsawicki
Contributor

kevinsawicki commented Jul 10, 2017

Previously both nativeWindowOpen and sandbox options did not support overriding webPreferences via new-window events. The preferences from the opener window were used instead when launching render processes for the new windows.

This pull request changes this behavior to set configured webPreferences when creating new windows with an existing webContents. These preferences are then used when the render process is launched for the webContents (if applicable).

  • Reset webContents web preferences with the configured options when creating a new window with an existing contents.
  • Track pending render process launches via RenderFrameHost instead of RenderProcessHost so that lookup via WebContents can be done later on.
  • Register sandbox as an inherited option in guest-window-manager.js so windows opened from sandboxed windows have it enabled by default.
  • Register nativeWindowOpen as an inherited option in guest-window-manager.js so windows opened from native window open windows have it enabled by default.
  • Add specs for inherited options behavior
  • Add specs new-window option overriding for sandbox/nativeWindowOpen windows.
  • Add spec for reload bug from #9928

Refs #9961
Closes #9928

auto pending_process = (*new_instance)->GetProcess();
pending_processes_[pending_process->GetID()] = current_process->GetID();
pending_processes_[pending_process->GetID()] =
content::WebContents::FromRenderFrameHost(render_frame_host);;

This comment has been minimized.

@kevinsawicki

kevinsawicki Jul 11, 2017

Contributor

@zcbenz I'm a little nervous about this change, I'm not sure if there are times when we would have a pending render process host but not a render frame host. Through my testing, it seemed like the render frame host for the proper web contents was passed in each time so it seemed okay to track the pending process to the originating contents.

Do you remember why initially this mapped process host to process host instead of using the frame host to track the web contents doing the navigation?

@kevinsawicki

kevinsawicki Jul 11, 2017

Contributor

@zcbenz I'm a little nervous about this change, I'm not sure if there are times when we would have a pending render process host but not a render frame host. Through my testing, it seemed like the render frame host for the proper web contents was passed in each time so it seemed okay to track the pending process to the originating contents.

Do you remember why initially this mapped process host to process host instead of using the frame host to track the web contents doing the navigation?

This comment has been minimized.

@zcbenz

zcbenz Jul 17, 2017

Contributor

I don't remember the details, Chromium frequently changes how renderer process restarts, so some assumptions were no longer valid. If this change passes the tests, I think we can confidently do it.

@zcbenz

zcbenz Jul 17, 2017

Contributor

I don't remember the details, Chromium frequently changes how renderer process restarts, so some assumptions were no longer valid. If this change passes the tests, I think we can confidently do it.

@kevinsawicki kevinsawicki requested a review from tarruda Jul 11, 2017

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Jul 11, 2017

Contributor

@tarruda Wanted to get your feedback on this since this does change the sandbox behavior so preferences can be configured per-window via the new-window event instead of using the preferences from the initial windows.

Seemed nice to have that flexibility given the number of webPreferences that you may want to configure on a per-window basis.

Contributor

kevinsawicki commented Jul 11, 2017

@tarruda Wanted to get your feedback on this since this does change the sandbox behavior so preferences can be configured per-window via the new-window event instead of using the preferences from the initial windows.

Seemed nice to have that flexibility given the number of webPreferences that you may want to configure on a per-window basis.

@kevinsawicki kevinsawicki requested a review from zcbenz Jul 11, 2017

@@ -78,19 +80,8 @@ const setupGuest = function (embedder, frameName, guest, options) {
embedder.send('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSED_' + guestId)
embedder.removeListener('render-view-deleted', closedByEmbedder)
}
if (!options.webPreferences.sandbox) {
// These events should only be handled when the guest window is opened by a
// non-sandboxed renderer for two reasons:

This comment has been minimized.

@kevinsawicki

kevinsawicki Jul 11, 2017

Contributor

@tarruda I removed this because I was seeing a spec failure once options.webPreferences.sandbox was inherited by default from the parent window (line 16).

It looks like it was going down this path previously when a sandboxed window was opening a new window so I wasn't sure if this code was still needed.

Is this check only needed for opening sandboxed windows opened from non-sandbox windows?

On master, if you open a sandbox window then do window.open() the listeners did appear to be registered since the options have no web preferences by default so options.webPreferences.sandbox is false.

@kevinsawicki

kevinsawicki Jul 11, 2017

Contributor

@tarruda I removed this because I was seeing a spec failure once options.webPreferences.sandbox was inherited by default from the parent window (line 16).

It looks like it was going down this path previously when a sandboxed window was opening a new window so I wasn't sure if this code was still needed.

Is this check only needed for opening sandboxed windows opened from non-sandbox windows?

On master, if you open a sandbox window then do window.open() the listeners did appear to be registered since the options have no web preferences by default so options.webPreferences.sandbox is false.

This comment has been minimized.

@tarruda

tarruda Jul 11, 2017

Contributor

Is this check only needed for opening sandboxed windows opened from non-sandbox windows?

I think it was related to opening sandboxed windows, regardless of the parent. Could also apply to nativeWindowOpen.

On master, if you open a sandbox window then do window.open() the listeners did appear to be registered since the options have no web preferences by default so options.webPreferences.sandbox is false.

Since this code was first added, there were some changes to allow sandbox reuse code from non-sandbox, so it is possible that this is obsolete.

If I remember correctly, you could reproduce the crash by closing a window created by window.open and waiting a bit for the garbage collector. If you can't reproduce this now, I guess it should be fine to remove it.

@tarruda

tarruda Jul 11, 2017

Contributor

Is this check only needed for opening sandboxed windows opened from non-sandbox windows?

I think it was related to opening sandboxed windows, regardless of the parent. Could also apply to nativeWindowOpen.

On master, if you open a sandbox window then do window.open() the listeners did appear to be registered since the options have no web preferences by default so options.webPreferences.sandbox is false.

Since this code was first added, there were some changes to allow sandbox reuse code from non-sandbox, so it is possible that this is obsolete.

If I remember correctly, you could reproduce the crash by closing a window created by window.open and waiting a bit for the garbage collector. If you can't reproduce this now, I guess it should be fine to remove it.

@tarruda

LGTM

@@ -78,19 +80,8 @@ const setupGuest = function (embedder, frameName, guest, options) {
embedder.send('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSED_' + guestId)
embedder.removeListener('render-view-deleted', closedByEmbedder)
}
if (!options.webPreferences.sandbox) {
// These events should only be handled when the guest window is opened by a
// non-sandboxed renderer for two reasons:

This comment has been minimized.

@tarruda

tarruda Jul 11, 2017

Contributor

Is this check only needed for opening sandboxed windows opened from non-sandbox windows?

I think it was related to opening sandboxed windows, regardless of the parent. Could also apply to nativeWindowOpen.

On master, if you open a sandbox window then do window.open() the listeners did appear to be registered since the options have no web preferences by default so options.webPreferences.sandbox is false.

Since this code was first added, there were some changes to allow sandbox reuse code from non-sandbox, so it is possible that this is obsolete.

If I remember correctly, you could reproduce the crash by closing a window created by window.open and waiting a bit for the garbage collector. If you can't reproduce this now, I guess it should be fine to remove it.

@tarruda

tarruda Jul 11, 2017

Contributor

Is this check only needed for opening sandboxed windows opened from non-sandbox windows?

I think it was related to opening sandboxed windows, regardless of the parent. Could also apply to nativeWindowOpen.

On master, if you open a sandbox window then do window.open() the listeners did appear to be registered since the options have no web preferences by default so options.webPreferences.sandbox is false.

Since this code was first added, there were some changes to allow sandbox reuse code from non-sandbox, so it is possible that this is obsolete.

If I remember correctly, you could reproduce the crash by closing a window created by window.open and waiting a bit for the garbage collector. If you can't reproduce this now, I guess it should be fine to remove it.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Jul 11, 2017

Contributor

@tarruda Wanted to get your feedback on this since this does change the sandbox behavior so preferences can be configured per-window via the new-window event instead of using the preferences from the initial windows.

Seemed nice to have that flexibility given the number of webPreferences that you may want to configure on a per-window basis.

Agreed, this is a nice improvement 👍

Contributor

tarruda commented Jul 11, 2017

@tarruda Wanted to get your feedback on this since this does change the sandbox behavior so preferences can be configured per-window via the new-window event instead of using the preferences from the initial windows.

Seemed nice to have that flexibility given the number of webPreferences that you may want to configure on a per-window basis.

Agreed, this is a nice improvement 👍

@zcbenz

zcbenz approved these changes Jul 17, 2017

👍

@kevinsawicki kevinsawicki changed the title from [WIP] Inherit webPreferences in windows opened via sandbox or nativeWindowOpen to Inherit webPreferences in windows opened via sandbox or nativeWindowOpen Jul 17, 2017

@kevinsawicki kevinsawicki merged commit 65fe703 into master Jul 17, 2017

6 of 8 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-linux-arm Build #7314489 succeeded in 90s
Details
electron-linux-ia32 Build #7314490 succeeded in 87s
Details
electron-linux-x64 Build #7314491 succeeded in 208s
Details
electron-mas-x64 Build #4596 succeeded in 9 min 49 sec
Details
electron-osx-x64 Build #4598 succeeded in 10 min
Details
electron-win-x64 Build #3575 succeeded in 10 min
Details

@kevinsawicki kevinsawicki deleted the merge-web-preferences branch Jul 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment