[Bug]: window reference from window.open does not have preload script executed

[t57ser]

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for a bug report that matches the one I want to file, without success.

Electron Version

17.0.0-beta.5

What operating system are you using?

Windows

Operating System Version

Windows 10 19042.985

What arch are you using?

x64

Last Known Working Electron version

14.1.1

Expected Behavior

var win = window.open("https://www.google.com");
win.hello

value should be accessible if defined in preload script

Actual Behavior

The value is undefined. If trying to manually access it later it will be defined, probably because the preload script is executed at a later time.

Testcase Gist URL

https://gist.github.com/t57ser/be7947cd051cdedf1e03c31100785042

Additional Information

No response

[t57ser]

Works with 14.2.3.
Does not work with 14.2.4
My guess is this caused it: #32057

[t57ser]

I am pretty sure that caused it, it fits the comment

electron/shell/renderer/electron_render_frame_observer.cc

Lines 93 to 95 in 94d4a07

	// We work around this issue by delaying the child window's initialization of
	// Node.js if this is the initial empty document, and only do it when the
	// acutal page has started to load.

Due to later initialization it is not available immidiately. @zcbenz
If there is not good fix for this I would suggest adding a config flag to control this behavior globally

[zcbenz]

Yeah the late initialization is intended, because we have to wait for the main process to pass options otherwise there will be security issues.

So we can not really fix this bug, and adding a new config seems to be the only option. But the config would be a renderer-wide flag to ignore checks from the main process and have new windows always inherit web preferences from their parents, it introduces security problems and I don't think we can get it approved.

[t57ser]

Is this only related to

electron/shell/renderer/electron_render_frame_observer.cc

Lines 90 to 92 in 94d4a07

	// This will end up initializing Node.js in the child window with wrong
	// WebPreferences, leads to problem that child window having node integration
	// while "nodeIntegration=no" is passed.

or are there more security concerns than nodeIntegration?
My assumption would be that security of such things should be in the hands of the developer (same as activating nodeIntegration in the first place), but I don't know all the implications of it.
Maybe a secure solution would be to have a flag that only allows instant initialization if nodeIntegration is set to false. In that case it would still not be possible to switch it for a child window but it at least allows having the instant initialization in case the parent is already secure.

I'm also in the process of trying out other possible workarounds but there doesnt seem to be anything due to its synchronous nature. The easiest thing to do would be to delay the return value of window.open in the parent but that would require a way to pause the renderer in some way. Currently this is only possible via ipcRenderer.sendSync which doesnt work if the child runs in the same domain since they will reuse the renderer and both will hang.

I'm surprised this was seen as a patch rather than a breaking change as this breaks all synchronous usage cases.
Currently it still works with about:blank but not by intention

electron/shell/renderer/electron_render_frame_observer.cc

Lines 99 to 104 in 94d4a07

	// FIXME(zcbenz): Chromium does not do any browser side navigation for
	// window.open('about:blank'), so there is no way to override WebPreferences
	// of it. We should not delay Node.js initialization as there will be no
	// further loadings.
	// Please check http://crbug.com/1215096 for updates which may help remove
	// this hack.

[zcbenz]

Hmm a switch that only works when nodeIntegration is turned off should have no security issues, but it is still a new API that for a corner case, and I think it would receive questions on why should such API be added instead of having the app working around the corner case.

Sorry my change brought a breaking change, but it is a security issue that has to be fixed, and I couldn't find any alternative solution.

[t57ser]

Currently it is needed for the use of various 3rd party apps.
A very simple use case:

var win = window.open("");
win.document.write(someHTML);
win.print();
win.close();

Here the print is overwritten in the preload to have custom print logic.
Another use case is the interception of window.open and window.close to notify the host / have custom logic.
For example this is needed to workaround the following issue: #28625
There are more use cases but these are just some simple examples.

I would be happy with any workaround or hack but at the moment I can not find any way to solve this.
The printing example still works at the moment as it navigates to about:blank but I am concerned how to solve this in the future once this is no longer supported.

Another workaround I tried was using
Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, milliseconds) before returning the window reference.
Though this throws the following: Uncaught TypeError: Atomics.wait cannot be called in this context.
As far as I know it would not work anyway as that too would block the child process if it runs under the same renderer.
I assume this goes for any webassembly based solution as well.
I don't think that there is any way to wait until the correct window reference exists in the parent.

Reading through the chromium issue again and

electron/shell/renderer/electron_render_frame_observer.cc

Lines 87 to 89 in 94d4a07

	// Normally the WebPreferences is overriden in browser before navigation,
	// but this behavior bypasses the browser side navigation and the child
	// window will get wrong WebPreferences in the initialization.

Does this imply that the behaviour will be fixed once chromium changes the initial navigation?
See: (https://bugs.chromium.org/p/chromium/issues/detail?id=1215096)

The synchronous about:blank commit for window.open() is no longer
  ignored. Since this is the last case of a navigation getting ignored,
  this means we don't ignore any committed navigations anymore. This
  also means the browser-side listeners (e.g. WebContentsObserver's
  navigation related functions) will now be notified of this navigation

[zcbenz]

Thanks for the examples. Since the preload option is parsed in the main process, the preload scripts set by setWindowOpenHandler API or passed in the feature string of window.open are not possible to run synchronously. So with an option to retreat back to old behavior, it can only run preload scripts inherited from the parent window, and its documentation would be very long to cover all the cases, I don't think it can get enough love to get passed.

I'll see if I can work out a decent API to cover your use case.

Does this imply that the behaviour will be fixed once chromium changes the initial navigation?

Yes I think so, but I believe Chromium would need a few refactorings to achieve it so it won't happen anytime soon.

[t57ser]

Thank you for the support. My biggest fear was the about:blank case anyway since those are the most important use cases.
If this won't break in the future but rather the behavior might get fixed instead, this is already great news.

I am aware of the preload script issue but this isn't a big problem since the preload script could check for the use case it runs in, meaning it should not be necessary to have different preload scripts for different use cases, but I understand that having a flag would cause confusion among users. Though this is already causing confusion for the about:blank case. See: #32136

Maybe the setWindowOpenHandler could throw a warning in case someone tries to modify properties that can not be modified at the time (like preload and nodeIntegration), since it should already be known if the navigation is going to about:blank.
Of course this behavior could also just be documented.
I would imagine that any api or flag would then just run the same path.

In any case thanks for the help, at least there is some hope that chromium will fix this in the future.

[github-actions]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

[github-actions]

This issue has been closed due to inactivity, and will not be monitored. If this is a bug and you can reproduce this issue on a supported version of Electron please open a new issue and include instructions for reproducing the issue.