Browse Source

Enable plznavigate aka browser side navigation (#12535)

* enable plznavigate code path

* AtomBrowserClient::GetGeolocationApiKey returns the right default

* use IsLoadingToDifferentDocument to identify top level navigation in mainFrame

* use candidate site instance when available

* spec: don't test httpReferrer option for file origin

* update libcc ref

* affinity: only group same site in this mode

* plznavigate: don't emit did-get-response-details event for blob scheme
Robo 7 years ago
parent
commit
65e8199a93

+ 0 - 8
atom/app/atom_main_delegate.cc

@@ -128,11 +128,6 @@ bool AtomMainDelegate::BasicStartupComplete(int* exit_code) {
 void AtomMainDelegate::PreSandboxStartup() {
   brightray::MainDelegate::PreSandboxStartup();
 
-  // Set google API key.
-  std::unique_ptr<base::Environment> env(base::Environment::Create());
-  if (!env->HasVar("GOOGLE_API_KEY"))
-    env->SetVar("GOOGLE_API_KEY", GOOGLEAPIS_API_KEY);
-
   auto command_line = base::CommandLine::ForCurrentProcess();
   std::string process_type = command_line->GetSwitchValueASCII(
       ::switches::kProcessType);
@@ -152,9 +147,6 @@ void AtomMainDelegate::PreSandboxStartup() {
     }
   }
 
-  // TODO(deepak1556): Fix and re-enable the plznavigation code path.
-  command_line->AppendSwitch(::switches::kDisableBrowserSideNavigation);
-
   // Allow file:// URIs to read other file:// URIs by default.
   command_line->AppendSwitch(::switches::kAllowFileAccessFromFiles);
 

+ 9 - 5
atom/browser/api/atom_api_web_contents.cc

@@ -849,6 +849,14 @@ void WebContents::DidStopLoading() {
 
 void WebContents::DidGetResourceResponseStart(
     const content::ResourceRequestDetails& details) {
+  // Plznavigate is using blob URLs to deliver the body
+  // of the main resource to the renderer process. This
+  // gets better in the future with kNavigationMojoResponse
+  // feature, which replaces this mechanism with Mojo Datapipe.
+  if (details.url.SchemeIsBlob() &&
+      (details.resource_type == content::RESOURCE_TYPE_MAIN_FRAME ||
+       details.resource_type == content::RESOURCE_TYPE_SUB_FRAME))
+    return;
   Emit("did-get-response-details",
        details.socket_address.IsEmpty(),
        details.url,
@@ -1155,11 +1163,7 @@ bool WebContents::IsLoading() const {
 }
 
 bool WebContents::IsLoadingMainFrame() const {
-  // Comparing site instances works because Electron always creates a new site
-  // instance when navigating, regardless of origin. See AtomBrowserClient.
-  return (web_contents()->GetLastCommittedURL().is_empty() ||
-          web_contents()->GetSiteInstance() !=
-          web_contents()->GetPendingSiteInstance()) && IsLoading();
+  return web_contents()->IsLoadingToDifferentDocument();
 }
 
 bool WebContents::IsWaitingForResponse() const {

+ 38 - 35
atom/browser/atom_browser_client.cc

@@ -64,7 +64,16 @@ bool g_suppress_renderer_process_restart = false;
 // Custom schemes to be registered to handle service worker.
 std::string g_custom_service_worker_schemes = "";
 
-void Noop(scoped_refptr<content::SiteInstance>) {
+bool IsSameWebSite(content::BrowserContext* browser_context,
+                   const GURL& src_url,
+                   const GURL& dest_url) {
+  return content::SiteInstance::IsSameWebSite(browser_context, src_url,
+                                              dest_url) ||
+         // `IsSameWebSite` doesn't seem to work for some URIs such as `file:`,
+         // handle these scenarios by comparing only the site as defined by
+         // `GetSiteForURL`.
+         content::SiteInstance::GetSiteForURL(browser_context, dest_url) ==
+             src_url;
 }
 
 }  // namespace
@@ -125,12 +134,7 @@ bool AtomBrowserClient::ShouldCreateNewSiteInstance(
 
   // Create new a SiteInstance if navigating to a different site.
   auto src_url = current_instance->GetSiteURL();
-  return
-      !content::SiteInstance::IsSameWebSite(browser_context, src_url, url) &&
-      // `IsSameWebSite` doesn't seem to work for some URIs such as `file:`,
-      // handle these scenarios by comparing only the site as defined by
-      // `GetSiteForURL`.
-      content::SiteInstance::GetSiteForURL(browser_context, url) != src_url;
+  return !IsSameWebSite(browser_context, src_url, url);
 }
 
 void AtomBrowserClient::AddProcessPreferences(
@@ -218,20 +222,19 @@ void AtomBrowserClient::OverrideWebkitPrefs(
 void AtomBrowserClient::OverrideSiteInstanceForNavigation(
     content::RenderFrameHost* rfh,
     content::BrowserContext* browser_context,
-    content::SiteInstance* current_instance,
     const GURL& url,
+    bool has_request_started,
+    content::SiteInstance* candidate_instance,
     content::SiteInstance** new_instance) {
   if (g_suppress_renderer_process_restart) {
     g_suppress_renderer_process_restart = false;
     return;
   }
 
+  content::SiteInstance* current_instance = rfh->GetSiteInstance();
   if (!ShouldCreateNewSiteInstance(rfh, browser_context, current_instance, url))
     return;
 
-  bool is_new_instance = true;
-  scoped_refptr<content::SiteInstance> site_instance;
-
   // Do we have an affinity site to manage ?
   std::string affinity;
   auto* web_contents = content::WebContents::FromRenderFrameHost(rfh);
@@ -241,35 +244,35 @@ void AtomBrowserClient::OverrideSiteInstanceForNavigation(
       !affinity.empty()) {
     affinity = base::ToLowerASCII(affinity);
     auto iter = site_per_affinities.find(affinity);
-    if (iter != site_per_affinities.end()) {
-      site_instance = iter->second;
-      is_new_instance = false;
+    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 {
-      // We must not provide the url.
-      // This site is "isolated" and must not be taken into account
-      // when Chromium looking at a candidate for an url.
-      site_instance = content::SiteInstance::Create(
-          browser_context);
-      site_per_affinities[affinity] = site_instance.get();
+      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 {
-    site_instance = content::SiteInstance::CreateForURL(
-        browser_context,
-        url);
-  }
-  *new_instance = site_instance.get();
-
-  if (is_new_instance) {
-    // Make sure the |site_instance| is not freed
-    // when this function returns.
-    // FIXME(zcbenz): We should adjust
-    // OverrideSiteInstanceForNavigation's interface to solve this.
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE,
-        base::Bind(&Noop, base::RetainedRef(site_instance)));
+    // 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 = site_instance->GetProcess();
+    auto pending_process = candidate_instance->GetProcess();
     pending_processes_[pending_process->GetID()] = web_contents;
   }
 }

+ 2 - 1
atom/browser/atom_browser_client.h

@@ -56,8 +56,9 @@ class AtomBrowserClient : public brightray::BrowserClient,
   void OverrideSiteInstanceForNavigation(
       content::RenderFrameHost* render_frame_host,
       content::BrowserContext* browser_context,
-      content::SiteInstance* current_instance,
       const GURL& dest_url,
+      bool has_request_started,
+      content::SiteInstance* candidate_instance,
       content::SiteInstance** new_instance) override;
   void AppendExtraCommandLineSwitches(base::CommandLine* command_line,
                                       int child_process_id) override;

+ 11 - 9
spec/webview-spec.js

@@ -285,15 +285,17 @@ describe('<webview> tag', function () {
   describe('httpreferrer attribute', () => {
     it('sets the referrer url', (done) => {
       const referrer = 'http://github.com/'
-      const listener = (e) => {
-        assert.equal(e.message, referrer)
-        webview.removeEventListener('console-message', listener)
+      const server = http.createServer((req, res) => {
+        res.end()
+        server.close()
+        assert.equal(req.headers.referer, referrer)
         done()
-      }
-      webview.addEventListener('console-message', listener)
-      webview.setAttribute('httpreferrer', referrer)
-      webview.src = `file://${fixtures}/pages/referrer.html`
-      document.body.appendChild(webview)
+      }).listen(0, '127.0.0.1', () => {
+        const port = server.address().port
+        webview.setAttribute('httpreferrer', referrer)
+        webview.src = 'http://127.0.0.1:' + port
+        document.body.appendChild(webview)
+      })
     })
   })
 
@@ -643,7 +645,7 @@ describe('<webview> tag', function () {
     })
   })
 
-  describe('setDevToolsWebCotnents() API', () => {
+  describe('setDevToolsWebContents() API', () => {
     it('sets webContents of webview as devtools', (done) => {
       const webview2 = new WebView()
       webview2.addEventListener('did-attach', () => {

+ 1 - 1
vendor/libchromiumcontent

@@ -1 +1 @@
-Subproject commit 76a718409b7b859c6b708492e89aab0389fe447f
+Subproject commit f1fc5cfb3d17d73687574b6ee4636dd1a9dbb7b5