feat: migrate protocol module to NetworkService (Part 9) (#18374)

* Compare final data instead of url

The behavior of did-finish-load and getURL has changed for redirects when
using NetworkService, so the test fails for NetworkService.

Comparing the finally received data makes the test more reliable.

* Implement intercept APIs

* Setting mimeType should set "content-type" header

* Passing no argument should not throw JS error

* Don't access api namespace in ProxyingURLLoaderFactory

* No need to create AtomURLLoaderFactory every time

* No use of weak factory

atom/browser/api/atom_api_protocol_ns.cc

@@ -5,6 +5,7 @@
 #include "atom/browser/api/atom_api_protocol_ns.h"
 
 #include <memory>
+#include <utility>
 
 #include "atom/browser/atom_browser_context.h"
 #include "atom/common/native_mate_converters/net_converter.h"
@@ -37,8 +38,6 @@ std::string ErrorCodeToString(ProtocolError error) {
   }
 }
 
-void Noop() {}
-
 }  // namespace
 
 ProtocolNS::ProtocolNS(v8::Isolate* isolate,
@@ -82,15 +81,36 @@ bool ProtocolNS::IsProtocolRegistered(const std::string& scheme) {
   return base::ContainsKey(handlers_, scheme);
 }
 
+ProtocolError ProtocolNS::InterceptProtocol(ProtocolType type,
+                                            const std::string& scheme,
+                                            const ProtocolHandler& handler) {
+  ProtocolError error = ProtocolError::OK;
+  if (!base::ContainsKey(intercept_handlers_, scheme))
+    intercept_handlers_[scheme] = std::make_pair(type, handler);
+  else
+    error = ProtocolError::INTERCEPTED;
+  return error;
+}
+
 void ProtocolNS::UninterceptProtocol(const std::string& scheme,
                                      mate::Arguments* args) {
-  HandleOptionalCallback(args, ProtocolError::NOT_INTERCEPTED);
+  ProtocolError error = ProtocolError::OK;
+  if (base::ContainsKey(intercept_handlers_, scheme))
+    intercept_handlers_.erase(scheme);
+  else
+    error = ProtocolError::NOT_INTERCEPTED;
+  HandleOptionalCallback(args, error);
+}
+
+bool ProtocolNS::IsProtocolIntercepted(const std::string& scheme) {
+  return base::ContainsKey(intercept_handlers_, scheme);
 }
 
 v8::Local<v8::Promise> ProtocolNS::IsProtocolHandled(
     const std::string& scheme) {
   util::Promise promise(isolate());
   promise.Resolve(IsProtocolRegistered(scheme) ||
+                  IsProtocolIntercepted(scheme) ||
                   // The |isProtocolHandled| should return true for builtin
                   // schemes, however with NetworkService it is impossible to
                   // know which schemes are registered until a real network
@@ -141,12 +161,20 @@ void ProtocolNS::BuildPrototype(v8::Isolate* isolate,
       .SetMethod("unregisterProtocol", &ProtocolNS::UnregisterProtocol)
       .SetMethod("isProtocolRegistered", &ProtocolNS::IsProtocolRegistered)
       .SetMethod("isProtocolHandled", &ProtocolNS::IsProtocolHandled)
-      .SetMethod("interceptStringProtocol", &Noop)
+      .SetMethod("interceptStringProtocol",
-      .SetMethod("interceptBufferProtocol", &Noop)
+                 &ProtocolNS::InterceptProtocolFor<ProtocolType::kString>)
-      .SetMethod("interceptFileProtocol", &Noop)
+      .SetMethod("interceptBufferProtocol",
-      .SetMethod("interceptHttpProtocol", &Noop)
+                 &ProtocolNS::InterceptProtocolFor<ProtocolType::kBuffer>)
-      .SetMethod("interceptStreamProtocol", &Noop)
+      .SetMethod("interceptFileProtocol",
-      .SetMethod("uninterceptProtocol", &ProtocolNS::UninterceptProtocol);
+                 &ProtocolNS::InterceptProtocolFor<ProtocolType::kFile>)
+      .SetMethod("interceptHttpProtocol",
+                 &ProtocolNS::InterceptProtocolFor<ProtocolType::kHttp>)
+      .SetMethod("interceptStreamProtocol",
+                 &ProtocolNS::InterceptProtocolFor<ProtocolType::kStream>)
+      .SetMethod("interceptProtocol",
+                 &ProtocolNS::InterceptProtocolFor<ProtocolType::kFree>)
+      .SetMethod("uninterceptProtocol", &ProtocolNS::UninterceptProtocol)
+      .SetMethod("isProtocolIntercepted", &ProtocolNS::IsProtocolIntercepted);
 }
 
 }  // namespace api

atom/browser/api/atom_api_protocol_ns.h

@@ -5,9 +5,7 @@
 #ifndef ATOM_BROWSER_API_ATOM_API_PROTOCOL_NS_H_
 #define ATOM_BROWSER_API_ATOM_API_PROTOCOL_NS_H_
 
-#include <map>
 #include <string>
-#include <utility>
 
 #include "atom/browser/api/trackable_object.h"
 #include "atom/browser/net/atom_url_loader_factory.h"
@@ -43,6 +41,8 @@ class ProtocolNS : public mate::TrackableObject<ProtocolNS> {
   void RegisterURLLoaderFactories(
       content::ContentBrowserClient::NonNetworkURLLoaderFactoryMap* factories);
 
+  const HandlersMap& intercept_handlers() const { return intercept_handlers_; }
+
  private:
   ProtocolNS(v8::Isolate* isolate, AtomBrowserContext* browser_context);
   ~ProtocolNS() override;
@@ -56,7 +56,12 @@ class ProtocolNS : public mate::TrackableObject<ProtocolNS> {
                                  const ProtocolHandler& handler);
   void UnregisterProtocol(const std::string& scheme, mate::Arguments* args);
   bool IsProtocolRegistered(const std::string& scheme);
+
+  ProtocolError InterceptProtocol(ProtocolType type,
+                                  const std::string& scheme,
+                                  const ProtocolHandler& handler);
   void UninterceptProtocol(const std::string& scheme, mate::Arguments* args);
+  bool IsProtocolIntercepted(const std::string& scheme);
 
   // Old async version of IsProtocolRegistered.
   v8::Local<v8::Promise> IsProtocolHandled(const std::string& scheme);
@@ -68,12 +73,18 @@ class ProtocolNS : public mate::TrackableObject<ProtocolNS> {
                            mate::Arguments* args) {
     HandleOptionalCallback(args, RegisterProtocol(type, scheme, handler));
   }
+  template <ProtocolType type>
+  void InterceptProtocolFor(const std::string& scheme,
+                            const ProtocolHandler& handler,
+                            mate::Arguments* args) {
+    HandleOptionalCallback(args, InterceptProtocol(type, scheme, handler));
+  }
 
   // Be compatible with old interface, which accepts optional callback.
   void HandleOptionalCallback(mate::Arguments* args, ProtocolError error);
 
-  // scheme => (type, handler).
+  HandlersMap handlers_;
-  std::map<std::string, std::pair<ProtocolType, ProtocolHandler>> handlers_;
+  HandlersMap intercept_handlers_;
 };
 
 }  // namespace api

atom/browser/atom_browser_client.cc

@@ -969,13 +969,16 @@ bool AtomBrowserClient::WillCreateURLLoaderFactory(
     bool* bypass_redirect_checks) {
   content::WebContents* web_contents =
       content::WebContents::FromRenderFrameHost(frame_host);
-  if (!web_contents)
+  api::ProtocolNS* protocol = api::ProtocolNS::FromWrappedClass(
+      v8::Isolate::GetCurrent(), web_contents->GetBrowserContext());
+  if (!protocol)
     return false;
 
   auto proxied_request = std::move(*factory_request);
   network::mojom::URLLoaderFactoryPtrInfo target_factory_info;
   *factory_request = mojo::MakeRequest(&target_factory_info);
-  new ProxyingURLLoaderFactory(std::move(proxied_request),
+  new ProxyingURLLoaderFactory(protocol->intercept_handlers(),
+                               std::move(proxied_request),
                                std::move(target_factory_info));
   return true;
 }

atom/browser/net/atom_url_loader_factory.cc

@@ -100,6 +100,10 @@ network::ResourceResponseHead ToResponseHead(const mate::Dictionary& dict) {
       "HTTP/1.1 %d %s", status_code,
       net::GetHttpReasonPhrase(static_cast<net::HttpStatusCode>(status_code))));
 
+  dict.Get("charset", &head.charset);
+  bool has_mime_type = dict.Get("mimeType", &head.mime_type);
+  bool has_content_type = false;
+
   base::DictionaryValue headers;
   if (dict.Get("headers", &headers)) {
     for (const auto& iter : headers.DictItems()) {
@@ -117,12 +121,17 @@ network::ResourceResponseHead ToResponseHead(const mate::Dictionary& dict) {
       }
       // Some apps are passing content-type via headers, which is not accepted
       // in NetworkService.
-      if (iter.first == "content-type" && iter.second.is_string())
+      if (iter.first == "content-type" && iter.second.is_string()) {
         head.mime_type = iter.second.GetString();
+        has_content_type = true;
+      }
     }
   }
-  dict.Get("mimeType", &head.mime_type);
+
-  dict.Get("charset", &head.charset);
+  // Setting |head.mime_type| does not automatically set the "content-type"
+  // header in NetworkService.
+  if (has_mime_type && !has_content_type)
+    head.headers->AddHeader("content-type: " + head.mime_type);
   return head;
 }
 
@@ -185,8 +194,18 @@ void AtomURLLoaderFactory::StartLoading(
     network::mojom::URLLoaderClientPtr client,
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
     ProtocolType type,
-    v8::Local<v8::Value> response,
     mate::Arguments* args) {
+  // Send network error when there is no argument passed.
+  //
+  // Note that we should not throw JS error in the callback no matter what is
+  // passed, to keep compatibility with old code.
+  v8::Local<v8::Value> response;
+  if (!args->GetNext(&response)) {
+    client->OnComplete(
+        network::URLLoaderCompletionStatus(net::ERR_NOT_IMPLEMENTED));
+    return;
+  }
+
   // Parse {error} object.
   mate::Dictionary dict = ToDict(args->isolate(), response);
   if (!dict.IsEmpty()) {
@@ -224,16 +243,12 @@ void AtomURLLoaderFactory::StartLoading(
       break;
     case ProtocolType::kFree:
       ProtocolType type;
-      v8::Local<v8::Value> extra_arg;
+      if (!mate::ConvertFromV8(args->isolate(), response, &type)) {
-      if (!mate::ConvertFromV8(args->isolate(), response, &type) ||
-          !args->GetNext(&extra_arg)) {
         client->OnComplete(network::URLLoaderCompletionStatus(net::ERR_FAILED));
-        args->ThrowError("Invalid args, must pass (type, options)");
         return;
       }
       StartLoading(std::move(loader), routing_id, request_id, options, request,
-                   std::move(client), traffic_annotation, type, extra_arg,
+                   std::move(client), traffic_annotation, type, args);
-                   args);
       break;
   }
 }

atom/browser/net/atom_url_loader_factory.h

@@ -5,7 +5,9 @@
 #ifndef ATOM_BROWSER_NET_ATOM_URL_LOADER_FACTORY_H_
 #define ATOM_BROWSER_NET_ATOM_URL_LOADER_FACTORY_H_
 
+#include <map>
 #include <string>
+#include <utility>
 
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "native_mate/dictionary.h"
@@ -24,11 +26,14 @@ enum class ProtocolType {
   kFree,  // special type for returning arbitrary type of response.
 };
 
-using StartLoadingCallback =
+using StartLoadingCallback = base::OnceCallback<void(mate::Arguments*)>;
-    base::OnceCallback<void(v8::Local<v8::Value>, mate::Arguments*)>;
 using ProtocolHandler =
     base::Callback<void(const network::ResourceRequest&, StartLoadingCallback)>;
 
+// scheme => (type, handler).
+using HandlersMap =
+    std::map<std::string, std::pair<ProtocolType, ProtocolHandler>>;
+
 // Implementation of URLLoaderFactory.
 class AtomURLLoaderFactory : public network::mojom::URLLoaderFactory {
  public:
@@ -46,7 +51,6 @@ class AtomURLLoaderFactory : public network::mojom::URLLoaderFactory {
                                 traffic_annotation) override;
   void Clone(network::mojom::URLLoaderFactoryRequest request) override;
 
- private:
   static void StartLoading(
       network::mojom::URLLoaderRequest loader,
       int32_t routing_id,
@@ -56,8 +60,9 @@ class AtomURLLoaderFactory : public network::mojom::URLLoaderFactory {
       network::mojom::URLLoaderClientPtr client,
       const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
       ProtocolType type,
-      v8::Local<v8::Value> response,
       mate::Arguments* args);
+
+ private:
   static void StartLoadingBuffer(network::mojom::URLLoaderClientPtr client,
                                  const mate::Dictionary& dict);
   static void StartLoadingString(network::mojom::URLLoaderClientPtr client,

atom/browser/net/proxying_url_loader_factory.cc

@@ -7,13 +7,16 @@
 #include <utility>
 
 #include "atom/browser/net/asar/asar_url_loader.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "services/network/public/mojom/url_loader.mojom.h"
 
 namespace atom {
 
 ProxyingURLLoaderFactory::ProxyingURLLoaderFactory(
+    const HandlersMap& handlers,
     network::mojom::URLLoaderFactoryRequest loader_request,
     network::mojom::URLLoaderFactoryPtrInfo target_factory_info)
-    : weak_factory_(this) {
+    : handlers_(handlers) {
   target_factory_.Bind(std::move(target_factory_info));
   target_factory_.set_connection_error_handler(base::BindOnce(
       &ProxyingURLLoaderFactory::OnTargetFactoryError, base::Unretained(this)));
@@ -32,6 +35,18 @@ void ProxyingURLLoaderFactory::CreateLoaderAndStart(
     const network::ResourceRequest& request,
     network::mojom::URLLoaderClientPtr client,
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
+  // Check if user has intercepted this scheme.
+  auto it = handlers_.find(request.url.scheme());
+  if (it != handlers_.end()) {
+    // <scheme, <type, handler>>
+    it->second.second.Run(
+        request, base::BindOnce(&AtomURLLoaderFactory::StartLoading,
+                                std::move(loader), routing_id, request_id,
+                                options, request, std::move(client),
+                                traffic_annotation, it->second.first));
+    return;
+  }
+
   // Intercept file:// protocol to support asar archives.
   if (request.url.SchemeIsFile()) {
     asar::CreateAsarURLLoader(request, std::move(loader), std::move(client),

atom/browser/net/proxying_url_loader_factory.h

@@ -5,16 +5,14 @@
 #ifndef ATOM_BROWSER_NET_PROXYING_URL_LOADER_FACTORY_H_
 #define ATOM_BROWSER_NET_PROXYING_URL_LOADER_FACTORY_H_
 
-#include "mojo/public/cpp/bindings/binding.h"
+#include "atom/browser/net/atom_url_loader_factory.h"
-#include "mojo/public/cpp/bindings/binding_set.h"
-#include "services/network/public/mojom/url_loader.mojom.h"
-#include "services/network/public/mojom/url_loader_factory.mojom.h"
 
 namespace atom {
 
 class ProxyingURLLoaderFactory : public network::mojom::URLLoaderFactory {
  public:
   ProxyingURLLoaderFactory(
+      const HandlersMap& handlers,
       network::mojom::URLLoaderFactoryRequest loader_request,
       network::mojom::URLLoaderFactoryPtrInfo target_factory_info);
   ~ProxyingURLLoaderFactory() override;
@@ -34,11 +32,18 @@ class ProxyingURLLoaderFactory : public network::mojom::URLLoaderFactory {
   void OnTargetFactoryError();
   void OnProxyBindingError();
 
+  // This is passed from api::ProtocolNS.
+  //
+  // The ProtocolNS instance lives through the lifetime of BrowserContenxt,
+  // which is guarenteed to cover the lifetime of URLLoaderFactory, so the
+  // reference is guarenteed to be valid.
+  //
+  // In this way we can avoid using code from api namespace in this file.
+  const HandlersMap& handlers_;
+
   mojo::BindingSet<network::mojom::URLLoaderFactory> proxy_bindings_;
   network::mojom::URLLoaderFactoryPtr target_factory_;
 
-  base::WeakPtrFactory<ProxyingURLLoaderFactory> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(ProxyingURLLoaderFactory);
 };
 

spec/api-protocol-spec.js

@@ -310,8 +310,8 @@ describe('protocol module', () => {
       const handler = (request, callback) => callback({ url: redirectURL })
       await registerHttpProtocol(protocolName, handler)
 
-      await contents.loadURL(url)
+      const r = await ajax(url)
-      expect(contents.getURL()).to.equal(url)
+      expect(r.data).to.equal(text)
     })
 
     it('can access request headers', (done) => {