Skip to content

Commit

Permalink
revert: "fix: window.open site instance should belong to same browsin…
Browse files Browse the repository at this point in the history 
…g instance (#15216)" (#15757)

This reverts commit 8f35198.
  • Loading branch information
@ppontes @alexeykuzmin
ppontes authored and alexeykuzmin committed Nov 20, 2018
1 parent 46c2953 commit 57d2ae1
Show file tree
Hide file tree
Showing 10 changed files with 190 additions and 333 deletions.
158 changes: 56 additions & 102 deletions atom/browser/atom_browser_client.cc
View file
Expand Up @@ -179,7 +179,7 @@ AtomBrowserClient::~AtomBrowserClient() {
content::WebContents* AtomBrowserClient::GetWebContentsFromProcessID(
int process_id) {
// If the process is a pending process, we should use the web contents
// for the frame host passed into RegisterPendingProcess.
// for the frame host passed into OverrideSiteInstanceForNavigation.
if (base::ContainsKey(pending_processes_, process_id))
return pending_processes_[process_id];

Expand All @@ -188,21 +188,17 @@ content::WebContents* AtomBrowserClient::GetWebContentsFromProcessID(
return WebContentsPreferences::GetWebContentsFromProcessID(process_id);
}

bool AtomBrowserClient::ShouldForceNewSiteInstance(
bool AtomBrowserClient::ShouldCreateNewSiteInstance(
content::RenderFrameHost* render_frame_host,
content::BrowserContext* browser_context,
content::SiteInstance* current_instance,
const GURL& url) const {
const GURL& url) {
if (url.SchemeIs(url::kJavaScriptScheme))
// "javacript:" scheme should always use same SiteInstance
return false;

int process_id = current_instance->GetProcess()->GetID();
if (IsRendererSandboxed(process_id)) {
// Renderer is sandboxed, delegate the decision to the content layer for all
// origins.
return false;
} else {
if (!IsRendererSandboxed(process_id)) {
if (!RendererUsesNativeWindowOpen(process_id)) {
// non-sandboxed renderers without native window.open should always create
// a new SiteInstance
Expand Down Expand Up @@ -234,65 +230,25 @@ void AtomBrowserClient::RemoveProcessPreferences(int process_id) {
process_preferences_.erase(process_id);
}

bool AtomBrowserClient::IsProcessObserved(int process_id) const {
bool AtomBrowserClient::IsProcessObserved(int process_id) {
return process_preferences_.find(process_id) != process_preferences_.end();
}

bool AtomBrowserClient::IsRendererSandboxed(int process_id) const {
bool AtomBrowserClient::IsRendererSandboxed(int process_id) {
auto it = process_preferences_.find(process_id);
return it != process_preferences_.end() && it->second.sandbox;
}

bool AtomBrowserClient::RendererUsesNativeWindowOpen(int process_id) const {
bool AtomBrowserClient::RendererUsesNativeWindowOpen(int process_id) {
auto it = process_preferences_.find(process_id);
return it != process_preferences_.end() && it->second.native_window_open;
}

bool AtomBrowserClient::RendererDisablesPopups(int process_id) const {
bool AtomBrowserClient::RendererDisablesPopups(int process_id) {
auto it = process_preferences_.find(process_id);
return it != process_preferences_.end() && it->second.disable_popups;
}

std::string AtomBrowserClient::GetAffinityPreference(
content::RenderFrameHost* rfh) const {
auto* web_contents = content::WebContents::FromRenderFrameHost(rfh);
auto* web_preferences = WebContentsPreferences::From(web_contents);
std::string affinity;
if (web_preferences &&
web_preferences->GetPreference("affinity", &affinity) &&
!affinity.empty()) {
affinity = base::ToLowerASCII(affinity);
}

return affinity;
}

content::SiteInstance* AtomBrowserClient::GetSiteInstanceFromAffinity(
content::BrowserContext* browser_context,
const GURL& url,
content::RenderFrameHost* rfh) const {
std::string affinity = GetAffinityPreference(rfh);
if (!affinity.empty()) {
auto iter = site_per_affinities_.find(affinity);
GURL dest_site = content::SiteInstance::GetSiteForURL(browser_context, url);
if (iter != site_per_affinities_.end() &&
IsSameWebSite(browser_context, iter->second->GetSiteURL(), dest_site)) {
return iter->second;
}
}

return nullptr;
}

void AtomBrowserClient::ConsiderSiteInstanceForAffinity(
content::RenderFrameHost* rfh,
content::SiteInstance* site_instance) {
std::string affinity = GetAffinityPreference(rfh);
if (!affinity.empty()) {
site_per_affinities_[affinity] = site_instance;
}
}

void AtomBrowserClient::RenderProcessWillLaunch(
content::RenderProcessHost* host,
service_manager::mojom::ServiceRequest* service_request) {
Expand Down Expand Up @@ -358,59 +314,62 @@ void AtomBrowserClient::OverrideWebkitPrefs(content::RenderViewHost* host,
web_preferences->OverrideWebkitPrefs(prefs);
}

content::ContentBrowserClient::SiteInstanceForNavigationType
AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation(
void AtomBrowserClient::OverrideSiteInstanceForNavigation(
content::RenderFrameHost* rfh,
content::BrowserContext* browser_context,
const GURL& url,
bool has_request_started,
content::SiteInstance** affinity_site_instance) const {
content::SiteInstance* candidate_instance,
content::SiteInstance** new_instance) {
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, rfh);
if (site_instance_from_affinity) {
*affinity_site_instance = site_instance_from_affinity;
return SiteInstanceForNavigationType::FORCE_AFFINITY;
return;
}

content::SiteInstance* current_instance = rfh->GetSiteInstance();
if (!ShouldForceNewSiteInstance(rfh, browser_context, current_instance,
url)) {
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_request_started) {
return SiteInstanceForNavigationType::FORCE_CURRENT;
}

return SiteInstanceForNavigationType::FORCE_CANDIDATE_OR_NEW;
}

void AtomBrowserClient::RegisterPendingSiteInstance(
content::RenderFrameHost* rfh,
content::SiteInstance* pending_site_instance) {
// Do we have an affinity site to manage?
ConsiderSiteInstanceForAffinity(rfh, pending_site_instance);
if (!ShouldCreateNewSiteInstance(rfh, browser_context, current_instance, url))
return;

// Remember the original web contents for the pending renderer process.
// Do we have an affinity site to manage ?
auto* web_contents = content::WebContents::FromRenderFrameHost(rfh);
auto* pending_process = pending_site_instance->GetProcess();
pending_processes_[pending_process->GetID()] = web_contents;
auto* web_preferences = WebContentsPreferences::From(web_contents);
std::string affinity;
if (web_preferences &&
web_preferences->GetPreference("affinity", &affinity) &&
!affinity.empty()) {
affinity = base::ToLowerASCII(affinity);
auto iter = site_per_affinities.find(affinity);
GURL dest_site = content::SiteInstance::GetSiteForURL(browser_context, url);
if (iter != site_per_affinities.end() &&
IsSameWebSite(browser_context, iter->second->GetSiteURL(), dest_site)) {
*new_instance = iter->second;
} else {
site_per_affinities[affinity] = candidate_instance;
*new_instance = candidate_instance;
// Remember the original web contents for the pending renderer process.
auto* pending_process = candidate_instance->GetProcess();
pending_processes_[pending_process->GetID()] = web_contents;
}
} else {
// OverrideSiteInstanceForNavigation 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 resuse 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_request_started) {
*new_instance = current_instance;
return;
}

*new_instance = candidate_instance;
// Remember the original web contents for the pending renderer process.
auto* pending_process = candidate_instance->GetProcess();
pending_processes_[pending_process->GetID()] = web_contents;
}
}

void AtomBrowserClient::AppendExtraCommandLineSwitches(
Expand Down Expand Up @@ -588,10 +547,10 @@ void AtomBrowserClient::SiteInstanceDeleting(
content::SiteInstance* site_instance) {
// We are storing weak_ptr, is it fundamental to maintain the map up-to-date
// when an instance is destroyed.
for (auto iter = site_per_affinities_.begin();
iter != site_per_affinities_.end(); ++iter) {
for (auto iter = site_per_affinities.begin();
iter != site_per_affinities.end(); ++iter) {
if (iter->second == site_instance) {
site_per_affinities_.erase(iter);
site_per_affinities.erase(iter);
break;
}
}
Expand Down Expand Up @@ -825,9 +784,4 @@ std::string AtomBrowserClient::GetApplicationLocale() {
return *g_application_locale;
}

bool AtomBrowserClient::ShouldEnableStrictSiteIsolation() {
// Enable site isolation. It is off by default in Chromium <= 69.
return true;
}

} // namespace atom
36 changes: 12 additions & 24 deletions atom/browser/atom_browser_client.h
View file
Expand Up @@ -64,9 +64,6 @@ class AtomBrowserClient : public content::ContentBrowserClient,
// content::ContentBrowserClient:
std::string GetApplicationLocale() override;

// content::ContentBrowserClient:
bool ShouldEnableStrictSiteIsolation() override;

protected:
void RenderProcessWillLaunch(
content::RenderProcessHost* host,
Expand All @@ -75,15 +72,13 @@ class AtomBrowserClient : public content::ContentBrowserClient,
CreateSpeechRecognitionManagerDelegate() override;
void OverrideWebkitPrefs(content::RenderViewHost* render_view_host,
content::WebPreferences* prefs) override;
SiteInstanceForNavigationType ShouldOverrideSiteInstanceForNavigation(
void OverrideSiteInstanceForNavigation(
content::RenderFrameHost* render_frame_host,
content::BrowserContext* browser_context,
const GURL& dest_url,
bool has_request_started,
content::SiteInstance** affinity_instance) const override;
void RegisterPendingSiteInstance(
content::RenderFrameHost* render_frame_host,
content::SiteInstance* pending_site_instance) override;
content::SiteInstance* candidate_instance,
content::SiteInstance** new_instance) override;
void AppendExtraCommandLineSwitches(base::CommandLine* command_line,
int child_process_id) override;
void DidCreatePpapiPlugin(content::BrowserPpapiHost* browser_host) override;
Expand Down Expand Up @@ -171,23 +166,16 @@ class AtomBrowserClient : public content::ContentBrowserClient,
bool disable_popups = false;
};

bool ShouldForceNewSiteInstance(content::RenderFrameHost* render_frame_host,
content::BrowserContext* browser_context,
content::SiteInstance* current_instance,
const GURL& dest_url) const;
bool ShouldCreateNewSiteInstance(content::RenderFrameHost* render_frame_host,
content::BrowserContext* browser_context,
content::SiteInstance* current_instance,
const GURL& dest_url);
void AddProcessPreferences(int process_id, ProcessPreferences prefs);
void RemoveProcessPreferences(int process_id);
bool IsProcessObserved(int process_id) const;
bool IsRendererSandboxed(int process_id) const;
bool RendererUsesNativeWindowOpen(int process_id) const;
bool RendererDisablesPopups(int process_id) const;
std::string GetAffinityPreference(content::RenderFrameHost* rfh) const;
content::SiteInstance* GetSiteInstanceFromAffinity(
content::BrowserContext* browser_context,
const GURL& url,
content::RenderFrameHost* rfh) const;
void ConsiderSiteInstanceForAffinity(content::RenderFrameHost* rfh,
content::SiteInstance* site_instance);
bool IsProcessObserved(int process_id);
bool IsRendererSandboxed(int process_id);
bool RendererUsesNativeWindowOpen(int process_id);
bool RendererDisablesPopups(int process_id);

// pending_render_process => web contents.
std::map<int, content::WebContents*> pending_processes_;
Expand All @@ -196,7 +184,7 @@ class AtomBrowserClient : public content::ContentBrowserClient,
std::map<int, base::ProcessId> render_process_host_pids_;

// list of site per affinity. weak_ptr to prevent instance locking
std::map<std::string, content::SiteInstance*> site_per_affinities_;
std::map<std::string, content::SiteInstance*> site_per_affinities;

std::unique_ptr<AtomResourceDispatcherHostDelegate>
resource_dispatcher_host_delegate_;
Expand Down
2 changes: 0 additions & 2 deletions atom/browser/atom_browser_main_parts.cc
View file
Expand Up @@ -211,8 +211,6 @@ void AtomBrowserMainParts::InitializeFeatureList() {
// Chromium drops support for the old sandbox implmentation.
disable_features += std::string(",") + features::kMacV2Sandbox.name;
#endif
disable_features +=
std::string(",") + features::kSpareRendererForSitePerProcess.name;
auto feature_list = std::make_unique<base::FeatureList>();
feature_list->InitializeFromCommandLine(enable_features, disable_features);
base::FeatureList::SetInstance(std::move(feature_list));
Expand Down
22 changes: 19 additions & 3 deletions lib/browser/guest-window-manager.js
View file
Expand Up @@ -118,12 +118,28 @@ const createGuest = function (embedder, url, referrer, frameName, options, postD
}

guest = new BrowserWindow(options)
if (!options.webContents) {
if (!options.webContents || url !== 'about:blank') {
// We should not call `loadURL` if the window was constructed from an
// existing webContents (window.open in a sandboxed renderer).
// existing webContents(window.open in a sandboxed renderer) and if the url
// is not 'about:blank'.
//
// Navigating to the url when creating the window from an existing
// webContents is not necessary (it will navigate there anyway).
// webContents would not be necessary(it will navigate there anyway), but
// apparently there's a bug that allows the child window to be scripted by
// the opener, even when the child window is from another origin.
//
// That's why the second condition(url !== "about:blank") is required: to
// force `OverrideSiteInstanceForNavigation` to be called and consequently
// spawn a new renderer if the new window is targeting a different origin.
//
// If the URL is "about:blank", then it is very likely that the opener just
// wants to synchronously script the popup, for example:
//
// let popup = window.open()
// popup.document.body.write('<h1>hello</h1>')
//
// The above code would not work if a navigation to "about:blank" is done
// here, since the window would be cleared of all changes in the next tick.
const loadOptions = {
httpReferrer: referrer
}
Expand Down

0 comments on commit 57d2ae1

Please sign in to comment.