fix: lost window.opener after cross-origin navigation

[alexstrat]

Fixes #18032

Description of Change

With #15821, on cross-origin navigation, we use SiteInstance::CreateForURL to get the appropriate SiteInstance but in a new BrowsingInstance. It means we lose the relationship (by browsing instance) between the current and the new SiteInstance, and thus the window.opener is null in the opened window.

In this PR, I suggest using GetRelatedSiteInstance to keep the relationship.

In this PR, I introduce and use a SiteInstance::CreateRelatedSiteInstance that keeps the browsing instance relationship.

cc @ppontes @deepak1556

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are 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: Fixed window.opener null after cross-origin navigation

[codebytere]

not ok 842 webContents module zoom api cannot persist zoom level after navigation with webFrame
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  
  0.6 !== 0

failed on all platforms, so i believe it's a regression from this pr

[alexstrat]

Oddly, it looks like it makes the test-case about temporary zoom fail :/

My feeling is that GetRelatedSiteInstance is less restrictive than IsSameWebSite for some particular cases: it prefers keeping the same SiteInstance.
For instance, my guess is that from file://a to file://b, GetRelatedSiteInstance will keep the same SiteInstance while we were creating a new one previously.
If some tests (like the test-case about temporary zoom) were relying on file://a-> file://b to simulate cross-origin situations, they now are abusively failing.

Looking into GetRelatedSiteInstance implementation and will try a version of the temporary zoom test that uses other cross-origin simulations.

[alexstrat]

Given SiteInstance unittests, it seems that, by using GetRelatedSiteInstance:

• file://a and file://b will be in the same site instance
• http://localhost:123 and http://localhost:456 will be in the same site instance
• (obviously, http://host1.com and http://host2.com will be in different site instances)

So the tests relying on file://a -> file://b to simulate cross-site navigation should theoretically be updated 😬

What should I do? Update them or tweak so that we force a different site instance when switching between file://a and file://b?

[codebytere]

I would say that the second option would probably be optimal for the purposes of this PR: to update tests to reflect the behavior your PR is designed to create

[alexstrat]

@codebytere I'm not sure to understand your answer: you seem to recommend option 2 with the arguments of option 1 😀

• option 1: update tests that leverages file://a -> file://b as simulation of cross-origin navigation (for instance by using http://hotst1.com -> http://hotst2.com with interceptStringProtocol('http',..))
• option 2: change the patch to make sure that, with file:// protocol, we change theSiteInstance any time, while, with other protocols, we trust GetRelatedSiteInstance

After some thought, I lean towards option 1 because:

• the original intentions of the cross-origin tests were to test cross-origin navigation situations (http://host1.com to http://host2.com). The modifications of option 1 will make them closer to reality.
• option 2 will introduce a special behavior (more code) that differs from Chromium behavior (risks of inconstencies and confusion) whereas, AFAIK, there is no demand for such behavior (treat file://a and file://b as different SiteInstance)

But I'd like to get the opinion of @deepak1556 and @ppontes (who worked on the subject and on the concerned tests) beforehand. 🙏

[codebytere]

Whoops poor communication on my part: I intended option 1

i'll also defer to @deepak1556 and @ppontes though 🙇‍♀

[deepak1556]

Yeah the file scheme behavior difference we have from chrome is for legacy reasons, file doesn't have a origin concept so it doesn't make sense for file://a and file://b to have different site instances. IIRC the reason we went against this policy was because users were loading native modules using this scheme and we wanted to restart the renderer process for those navigations as well.

For fixing the test, yes option 1 is the right route. But I also want to decide on the file scheme behavior, whether we want to maintain this difference or align with browser behavior. There are obvious advantages this. /cc @zcbenz thoughts ?

[zcbenz]

But I also want to decide on the file scheme behavior, whether we want to maintain this difference or align with browser behavior. There are obvious advantages this. /cc @zcbenz thoughts ?

As long as the renderer process is still restarted for navigations, I'm good with either way, otherwise it would be a breaking change.

[alexstrat]

OK, I updated the failing test to test (more) real cross-origin navigations. However:

• I now have a doubt on the original intention of this test. I originally thought it was to test cross-origin navigation and not just navigation on the file:// protocol but I stumbled upon this test that seems to also test zoom persistence for cross-origin navigation.
• In reference to @zcbenz remark, I don't know how to verify that the renderer process is still restarted for navigations 🤷‍♂️
• I'm hoping this fix to land in 5.x release. So if there is anything is this PR that is too "breaking change", I'd prefer to rework it to remove breaking changes (or make 2 PRs with and without breaking changes).

[zcbenz]

After testing the change, I think it is breaking the purpose of frame_host_manager.patch, which is to force restarting renderer process for every navigation.

Most of the native Node.js modules can not be reloaded in the same process, they would just crash if you do so. So in Electron we have to force restarting renderer process for every navigation, otherwise any app using native modules would crash after navigation.

We implement this behavior by force creating a new site instance after doing navigations. And you can test whether this behavior is broken by printing process.pid in devtools, and refresh to see whether it changes.

For #18032 we should probably find another way to fix it instead of changing the behavior of force creating new site instances, in Electron window.opener is implemented with JS in lib/renderer/window-setup.ts so it should be possible to fix it JS.

If the only solution is to change the behavior of force creating new site instances, we should introduce a new option for it instead of changing the default behavior.

[MarshallOfSound]

in Electron window.opener is implemented with JS in lib/renderer/window-setup.ts so it should be possible to fix it JS.

FWIW this doesn't work with contextIsolation and a medium-long term goal for me is to remove this. We should be relying on nativeWindowOpen and find a way to fix this without JS tricks. If we fundamentally can't (which is the conclusion I'm coming to) then we should start seriously looking into how we can follow Chromiums process model and safely destruct/cleanup node environments.

[ppontes]

I'm afraid that the fix to this problem is not this simple. As @zcbenz has already mentioned, we need to force the renderer process to restart upon navigating, to properly restart Node. This is the reason Electron is forcing an independent site instance in this case. #15821 fixed the window.opener problem for sandboxed renderers only, where Node is disabled and therefore there is no need to restart the process. I agree with @MarshallOfSound in that we should find a way to move towards the Chromium process model while properly restarting Node without forcing full process restarts.

[alexstrat]

@zcbenz oh ok I see it now: indeed it does break the purpose of frame_host_manager.patch because SiteInstance:: GetRelatedSiteInstance will re-use an existing SiteInstance if one matches, while we were expecting to create one every-time.

Actually, I was originally looking for an equivalent of SiteInstance::CreateForURL that’d keep the BrowsingInstance but could not find it, so I had to use GetRelatedSiteInstance.
I will try to implement a SiteInstance::CreateRelatedSiteInstance as a patch and see if I it keeps the window.opener while forcing restarting the process. @ppontes would that work?

I +1 @MarshallOfSound opinion for the JS based implementation of window.opener, even more, because it will produce an incomplete implementation (what happens to the result of window.open upon the navigation of the opener?).

For anyone (maybe me) willing to dig the subject of safely destruct/cleanup node environments, what are the relevant issues/code bytes one can look into? I had the feeling that @MarshallOfSound made some good progress with #16425, didn’t you?

[ppontes]

@alexstrat, thanks for looking into this!

I will try to implement a SiteInstance::CreateRelatedSiteInstance as a patch.

With this approach we can end up with multiple site instances for the same site as part of the same browsing instance. Please ensure that this doesn't fail Chromium's expectations.

[alexstrat]

I implemented the SiteInstance::CreateRelatedSiteInstance and it works as it was expected in #18173 (comment). (I also reverted my changes on the zoom persistence tests and verified it still passes).

Regarding @ppontes 's remark, I'm still unsure of the consequences of having multiple site instances for the same site as part of the same browsing instance — except that it distances us from the 1 process-per-site model, from which we are already far anyway.

[alexstrat]

@zcbenz @ppontes I made the necessary changes. Do you think you could have a second look soon? 🙏

[ppontes]

@alexstrat, please reset the changes introduced in the last commit to unrelated patch files.
The code seems logical but while testing your branch I bumped into the fact that preload scripts are no longer executed when opening x-site popups from an non-sandboxed renderer with nativeWindowOpen === true. Can you please check this on your side?

[alexstrat]

Thanks for your time!

Regarding the changes to unrelated patch files, I have had a discussion on the Electron's Slack with @nornagon about this who advised me to keep them 🤷‍♂️:

that’s not unusual. it happens when roller-bot updates the DEPS version but doesn’t update the patches.
you can include the changes in your PR

Happy to remove it anyway! LMK

Regarding preload scripts non-execution in child windows with nativeWindowOpen, it is an intended behavior since #15213.
With my changes, because we now keep the opener reference, it's enforced even after cross-site navigation of the child window.
So, technically, it's intended behavior, and my changes fix cases that were not handled before.

However, the intention of #15213 was to make sure we don't have several node integrations in a single render process. Given the fact that cross-site navigation creates a new render process, we could implement this intention differently.
I think, though, it's outside the scope of this PR but LMK if you disagree, I'll make the necessary.

Note you can make sure preload-scripts are executed in child-windows with nodeIntegrationInSubFrames (I suggested to precise the documentation about it in #18156).

[ppontes]

Wasn't aware of such roller-bot issue, please then disregard my request to reset the changes.

Regarding the preload scripts, I understand your explanation but I'm a bit confused that nodeIntegration being disabled in child windows means preload scripts not running for those child windows, as that is not the case when nodeIntegration is disabled for a root window - the preload still runs and can do useful stuff even though require is then unavailable in the main world. Also, in sandbox renderers, nativeWindowOpen is implied true, nodeIntegration is implied false and the preload scripts are executed. But this seems to be another discussion. Approving from my side but still requesting a review from @deepak1556 who's knowledgeable in this area.

[zcbenz]

I have tried the new change but it still breaks the policy that navigations should restart the renderer process. It can be tested by open a process monitor and the reload the default app, and the PID should change for every reload.

This would be a breaking change if the policy is changed. Using Atom and VS Code as example, currently you can reload the window and everything would still work, with this change the app would crash after reloading, because they are using native modules.

We should of course find a better way than always reloading the renderer process, but before we get a better solution we have to stick to current way to ensure apps can work correctly.

[alexstrat]

@zcbenz I had understood these requirements and their implications and I was sure I made the necessary, so I was surprised and sorry to see that it does not work on your side. 😔

However, I tested with v6.0.0-beta.2 that the PID changes for every reloads of the default app and, on my side, I was not able to verify it.
My guess is that the recent addition of the sandbox flag to the default app changed this behavior you were used to seeing.

You can verify that my changes have no impact when sandbox flag is off: the render process is properly restarted.

[zcbenz]

Sorry I didn't notice the sandbox option, things work as expected after removing it!

[MarshallOfSound]

So here's my concern if I'm reading this thread correctly two things are true after this PR.

• window.opener is not null
• We are still restarting / getting a new renderer for navigations

This means we have somehow made cross-process bi-directional synchronous communication which is the definition of how deadlocks happen. I'd argue that for the benefit of all users we shouldn't be letting them do that. E.g. We only allow synchronous IPC in one direction in our ipc module, we shouldn't allow it here.

[alexstrat]

Rebase ✅
Ready to backport and merge, as far as I can tell 👐

[alexstrat]

@MarshallOfSound @deepak1556 can one of you merge this PR and start a backport, please?

[MarshallOfSound]

macOS failure is due to the PR coming from a fork. Approving for master 👍

[release-clerk]

Release Notes Persisted

Fixed window.opener null after cross-origin navigation

[trop]

A maintainer has manually backported this PR to "5-0-x", please check out #18614

[trop]

A maintainer has manually backported this PR to "6-0-x", please check out #18624

[alexstrat]

Backports for 5-0-x and 6-0-x are ready to review and merge.

[alexstrat]

@MarshallOfSound @deepak1556 could you approve the backport for electron 5 #18614 so that the fix gets released soon, please?