fix: always use new site instance for a new navigation.

[ppontes]

In non-sandboxed renderers, always use a new site instance when starting a new navigation. This prevents the same process from being reused by consecutive navigations in the same frame if the subsequent navigation happens before the preload script for the first navigation finishes.

Fixes #17576.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• 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: Removed the possibility of a preload script being executed twice for the same process in quickly succeeding navigations in the same frame.

[ckerr]

Requesting feedback from @MarshallOfSound as this might be relevant to https://hackmd.io/rBd-NL4nTDWgv5GLISqu2g ?

[deepak1556]

I see the issue, I would also suggest to make SiteInstanceForNavigationType::FORCE_NEW as the default instead of SiteInstanceForNavigationType::FORCE_CANDIDATE_OR_NEW. There is not much of a use case for SiteInstanceForNavigationType::FORCE_CANDIDATE_OR_NEW in non sandboxed scenario, its causing more problems than the performance benefits that were supposed to be gained from it, like #18624. I am in favor of removing it, which would also let us possibly revert https://github.com/electron/electron/blob/master/atom/browser/feature_list.cc#L26

Can you check if there is a visiable time difference in loading a renderer with/without this change. Thanks!

[ppontes]

@ckerr, this is definitely relevant to the document you linked and I fully support the direction described in that document. This PR is just a small tweak in our current solution to remove the pain of node modules unexpectedly failing to load, while we move in the direction of context-aware node modules.

[ppontes]

@deepak1556 , doesn't your suggestion conflict with this comment?

  // In the default mode we should reuse the same site instance until the
  // request commits otherwise it will get destroyed.

[deepak1556]

Sorry I didn't write the comment clearly, it simply meant in non-sandboxed mode when the site instance is forced by us, try to maintain the same instance for a request over successive calls to this function so that the request won't be destroyed. It has nothing to do with candidate site instances. Those site instances are created by speculative render frame hosts which are pre spawned hosts maybe leftover from previous renderer sometimes, thats an optimization to reduce cold startup time. I was trying to use the same for our non-sandboxed renderers, but clearly that has led to this problem. My suggestion here is to completely eliminate the use of candidate site instances for non sandboxed renderers.

content::ContentBrowserClient::SiteInstanceForNavigationType
AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation(
    content::RenderFrameHost* current_rfh,
    content::BrowserContext* browser_context,
    const GURL& url,
    bool has_response_started,
    content::SiteInstance** affinity_site_instance) const {
  if (g_suppress_renderer_process_restart) {
    g_suppress_renderer_process_restart = false;
    return SiteInstanceForNavigationType::ASK_CHROMIUM;
  }
  // Do we have an affinity site to manage ?
  content::SiteInstance* site_instance_from_affinity =
      GetSiteInstanceFromAffinity(browser_context, url, current_rfh);
  if (site_instance_from_affinity) {
    *affinity_site_instance = site_instance_from_affinity;
    return SiteInstanceForNavigationType::FORCE_AFFINITY;
  }
  if (!ShouldForceNewSiteInstance(current_rfh, browser_context,
                                  url, has_response_started)) {
    return SiteInstanceForNavigationType::ASK_CHROMIUM;
  }
  // ShouldOverrideSiteInstanceForNavigation will be called more than once
  // during a navigation (currently twice, on request and when it's about
  // to commit in the renderer), look at
  // RenderFrameHostManager::GetFrameHostForNavigation.
  // In the default mode we should reuse the same site instance until the
  // request commits otherwise it will get destroyed. Currently there is no
  // unique lifetime tracker for a navigation request during site instance
  // creation. We check for the state of the request, which should be one of
  // (WAITING_FOR_RENDERER_RESPONSE, STARTED, RESPONSE_STARTED, FAILED) along
  // with the availability of a speculative render frame host.
  if (has_response_started) {
    return SiteInstanceForNavigationType::FORCE_CURRENT;
  }

  return SiteInstanceForNavigationType::FORCE_NEW;
}

[ppontes]

@deepak1556 Makes sense, thanks for explaining. Modified the change so that candidate site instances are no longer considered.

[ppontes]

I'll be without internet access for 3 weeks starting tomorrow. If there's any interest in getting this merged earlier, please feel free to take ownership @deepak1556 , @miniak. Thanks.

[ppontes]

@deepak1556 Discarding the candidate site instance breaks some unit tests. Had to revert to the original fix.

not ok 606 renderer nodeIntegrationInSubFrames with webview on should load preload scripts in top level iframes
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec\api-subframe-spec.js)
      at Test.Runnable._timeoutError (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:440:10)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:251:24
[5100:0621/134046.653:ERROR:process_win.cc(168)] Unable to terminate process: Access is denied. (0x5)
[5100:0621/134047.824:ERROR:process_win.cc(168)] Unable to terminate process: Access is denied. (0x5)
not ok 607 renderer nodeIntegrationInSubFrames with webview on should load preload scripts in nested iframes
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec\api-subframe-spec.js)
      at Test.Runnable._timeoutError (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:440:10)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:251:24
[5100:0621/134116.781:ERROR:process_win.cc(168)] Unable to terminate process: Access is denied. (0x5)
[5100:0621/134116.781:ERROR:process_win.cc(168)] Unable to terminate process: Access is denied. (0x5)
[5100:0621/134117.951:ERROR:process_win.cc(168)] Unable to terminate process: Access is denied. (0x5)
not ok 608 renderer nodeIntegrationInSubFrames with webview on should correctly reply to the main frame with using event.reply
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec\api-subframe-spec.js)
      at Test.Runnable._timeoutError (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:440:10)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:251:24
[5100:0621/134146.917:ERROR:process_win.cc(168)] Unable to terminate process: Access is denied. (0x5)
[5100:0621/134146.917:ERROR:process_win.cc(168)] Unable to terminate process: Access is denied. (0x5)
[5100:0621/134148.091:ERROR:process_win.cc(168)] Unable to terminate process: Access is denied. (0x5)
not ok 609 renderer nodeIntegrationInSubFrames with webview on should correctly reply to the sub-frames with using event.reply
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec\api-subframe-spec.js)
      at Test.Runnable._timeoutError (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:440:10)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:251:24
[5100:0621/134217.055:ERROR:platform_handle_in_transit.cc(34)] DuplicateHandle failed: Access is denied. (0x5)

[miniak]

@deepak1556 I've rebased it on latest master, resolved conflicts and fixed some clang-format issues. How do we get this integrated?

[release-clerk]

Release Notes Persisted

Removed the possibility of a preload script being executed twice for the same process in quickly succeeding navigations in the same frame.

[deepak1556]

I think we need manual backport for other suppoted release lines.

[trop]

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

[trop]

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

[trop]

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