Bug 1747497 - Allow only one PAC execution at a time, r=necko-reviewers,valentin

Differential Revision: https://phabricator.services.mozilla.com/D134849

netwerk/base/ProxyAutoConfig.cpp

@@ -257,26 +257,21 @@ bool ProxyAutoConfig::ResolveAddress(const nsCString& aHostName,
     }
   }
 
-  mWaitingForDNSResolve = true;
   // Spin the event loop of the pac thread until lookup is complete.
   // nsPACman is responsible for keeping a queue and only allowing
   // one PAC execution at a time even when it is called re-entrantly.
   SpinEventLoopUntil("ProxyAutoConfig::ResolveAddress"_ns, [&, helper, this]() {
     if (!helper->mRequest) {
-      mWaitingForDNSResolve = false;
       return true;
     }
     if (this->mShutdown) {
       NS_WARNING("mShutdown set with PAC request not cancelled");
       MOZ_ASSERT(NS_FAILED(helper->mStatus));
-      mWaitingForDNSResolve = false;
       return true;
     }
     return false;
   });
 
-  MaybeInvokeDNSResolveCallbacks();
-
   if (NS_FAILED(helper->mStatus)) {
     return false;
   }
@@ -612,31 +607,9 @@ nsresult ProxyAutoConfig::SetupJS() {
   mConcatenatedPACData.Truncate();
   mPACURI.Truncate();
 
-  MaybeInvokeDNSResolveCallbacks();
   return NS_OK;
 }
 
-void ProxyAutoConfig::MaybeInvokeDNSResolveCallbacks() {
-  if (mDNSResolveCallbacks.IsEmpty()) {
-    return;
-  }
-
-  // This function could be called in the middle of SetupJS(). If this is the
-  // case, we should wait until mJSContext is ready to use. Otherwise,
-  // GetProxyForURI() would fail.
-  if (!mJSContext || !mJSContext->IsOK()) {
-    return;
-  }
-
-  NS_DispatchToCurrentThread(
-      NS_NewRunnableFunction("InvokeDNSResolveCallback",
-                             [callbacks{std::move(mDNSResolveCallbacks)}]() {
-                               for (auto& callback : callbacks) {
-                                 callback();
-                               }
-                             }));
-}
-
 void ProxyAutoConfig::GetProxyForURIWithCallback(
     const nsCString& aTestURI, const nsCString& aTestHost,
     std::function<void(nsresult aStatus, const nsACString& aResult)>&&
@@ -739,7 +712,6 @@ void ProxyAutoConfig::Shutdown() {
   mShutdown = true;
   delete mJSContext;
   mJSContext = nullptr;
-  mDNSResolveCallbacks.Clear();
 }
 
 bool ProxyAutoConfig::SrcAddress(const NetAddr* remoteAddress,
@@ -930,7 +902,10 @@ nsresult RemoteProxyAutoConfig::ConfigurePAC(const nsCString& aPACURI,
 
 void RemoteProxyAutoConfig::Shutdown() { mProxyAutoConfigParent->Close(); }
 
-void RemoteProxyAutoConfig::GC() { Unused << mProxyAutoConfigParent->SendGC(); }
+void RemoteProxyAutoConfig::GC() {
+  // Do nothing. GC would be performed when there is not pending query in socket
+  // process.
+}
 
 void RemoteProxyAutoConfig::GetProxyForURIWithCallback(
     const nsCString& aTestURI, const nsCString& aTestHost,

netwerk/base/ProxyAutoConfig.h

@@ -104,11 +104,6 @@ class ProxyAutoConfig : public ProxyAutoConfigBase {
       std::function<void(nsresult aStatus, const nsACString& aResult)>&&
           aCallback) override;
 
-  bool WaitingForDNSResolve() const { return mWaitingForDNSResolve; }
-  void RegisterDNSResolveCallback(std::function<void(void)>&& aCallback) {
-    mDNSResolveCallbacks.AppendElement(std::move(aCallback));
-  }
-
  private:
   // allow 665ms for myipaddress dns queries. That's 95th percentile.
   const static unsigned int kTimeout = 665;
@@ -119,7 +114,6 @@ class ProxyAutoConfig : public ProxyAutoConfigBase {
   bool SrcAddress(const NetAddr* remoteAddress, nsCString& localAddress);
   bool MyIPAddressTryHost(const nsCString& hostName, unsigned int timeout,
                           const JS::CallArgs& aArgs, bool* aResult);
-  void MaybeInvokeDNSResolveCallbacks();
 
   JSContextWrapper* mJSContext{nullptr};
   bool mJSNeedsSetup{false};
@@ -127,12 +121,10 @@ class ProxyAutoConfig : public ProxyAutoConfigBase {
   nsCString mConcatenatedPACData;
   nsCString mPACURI;
   bool mIncludePath{false};
-  bool mWaitingForDNSResolve{false};
   uint32_t mExtraHeapSize{0};
   nsCString mRunningHost;
   nsCOMPtr<nsITimer> mTimer;
   nsCOMPtr<nsIEventTarget> mMainThreadEventTarget;
-  nsTArray<std::function<void(void)>> mDNSResolveCallbacks;
 };
 
 class RemoteProxyAutoConfig : public ProxyAutoConfigBase {

netwerk/ipc/PProxyAutoConfig.ipdl

@@ -14,8 +14,6 @@ child:
                      bool aIncludePath, uint32_t aExtraHeapSize);
   async GetProxyForURI(nsCString aTestURI, nsCString aTestHost)
     returns (nsresult aStatus, nsCString aResult);
-  async GC();
-
 };
 
 } // namespace net

netwerk/ipc/ProxyAutoConfigChild.cpp

@@ -19,7 +19,7 @@ static bool sThreadLocalSetup = false;
 static uint32_t sThreadLocalIndex = 0xdeadbeef;
 StaticRefPtr<nsIThread> ProxyAutoConfigChild::sPACThread;
 bool ProxyAutoConfigChild::sShutdownObserverRegistered = false;
-Atomic<uint32_t> ProxyAutoConfigChild::sLiveActorCount(0);
+static StaticRefPtr<ProxyAutoConfigChild> sActor;
 
 namespace {
 
@@ -45,6 +45,27 @@ ShutdownObserver::Observe(nsISupports* aSubject, const char* aTopic,
 
 }  // namespace
 
+// static
+void ProxyAutoConfigChild::BindProxyAutoConfigChild(
+    RefPtr<ProxyAutoConfigChild>&& aActor,
+    Endpoint<PProxyAutoConfigChild>&& aEndpoint) {
+  // We only allow one ProxyAutoConfigChild at a time, so we need to
+  // wait until the old one to be destroyed.
+  if (sActor) {
+    NS_DispatchToCurrentThread(NS_NewRunnableFunction(
+        "BindProxyAutoConfigChild",
+        [actor = std::move(aActor), endpoint = std::move(aEndpoint)]() mutable {
+          ProxyAutoConfigChild::BindProxyAutoConfigChild(std::move(actor),
+                                                         std::move(endpoint));
+        }));
+    return;
+  }
+
+  if (aEndpoint.Bind(aActor)) {
+    sActor = aActor;
+  }
+}
+
 // static
 bool ProxyAutoConfigChild::Create(Endpoint<PProxyAutoConfigChild>&& aEndpoint) {
   if (!sPACThread && !CreatePACThread()) {
@@ -66,22 +87,14 @@ bool ProxyAutoConfigChild::Create(Endpoint<PProxyAutoConfigChild>&& aEndpoint) {
   }
 
   RefPtr<ProxyAutoConfigChild> actor = new ProxyAutoConfigChild();
-  if (NS_FAILED(sPACThread->Dispatch(
-          NS_NewRunnableFunction("ProxyAutoConfigChild::ProxyAutoConfigChild",
-                                 [actor = std::move(actor),
-                                  endpoint = std::move(aEndpoint)]() mutable {
-                                   MOZ_ASSERT(endpoint.IsValid());
-
-                                   // Transfer ownership to PAC thread. If
-                                   // Bind() fails then we will release this
-                                   // reference in Destroy.
-                                   ProxyAutoConfigChild* actorTmp;
-                                   actor.forget(&actorTmp);
-
-                                   if (!endpoint.Bind(actorTmp)) {
-                                     actorTmp->Destroy();
-                                   }
-                                 })))) {
+  if (NS_FAILED(sPACThread->Dispatch(NS_NewRunnableFunction(
+          "ProxyAutoConfigChild::ProxyAutoConfigChild",
+          [actor = std::move(actor),
+           endpoint = std::move(aEndpoint)]() mutable {
+            MOZ_ASSERT(endpoint.IsValid());
+            ProxyAutoConfigChild::BindProxyAutoConfigChild(std::move(actor),
+                                                           std::move(endpoint));
+          })))) {
     NS_WARNING("Failed to dispatch runnable!");
     return false;
   }
@@ -115,7 +128,7 @@ void ProxyAutoConfigChild::ShutdownPACThread() {
   if (sPACThread) {
     // Wait until all actos are released.
     SpinEventLoopUntil("ProxyAutoConfigChild::ShutdownPACThread"_ns,
-                       [&]() { return !sLiveActorCount; });
+                       [&]() { return !sActor; });
 
     nsCOMPtr<nsIThread> thread = sPACThread.get();
     sPACThread = nullptr;
@@ -131,52 +144,74 @@ ProxyAutoConfigChild::ProxyAutoConfigChild()
   }
 
   mPAC->SetThreadLocalIndex(sThreadLocalIndex);
-  sLiveActorCount++;
 }
 
-ProxyAutoConfigChild::~ProxyAutoConfigChild() {
-  MOZ_ASSERT(NS_IsMainThread());
-  sLiveActorCount--;
-}
+ProxyAutoConfigChild::~ProxyAutoConfigChild() = default;
 
 mozilla::ipc::IPCResult ProxyAutoConfigChild::RecvConfigurePAC(
     const nsCString& aPACURI, const nsCString& aPACScriptData,
     const bool& aIncludePath, const uint32_t& aExtraHeapSize) {
   mPAC->ConfigurePAC(aPACURI, aPACScriptData, aIncludePath, aExtraHeapSize,
                      GetMainThreadSerialEventTarget());
+  mPACLoaded = true;
+  NS_DispatchToCurrentThread(
+      NewRunnableMethod("ProxyAutoConfigChild::ProcessPendingQ", this,
+                        &ProxyAutoConfigChild::ProcessPendingQ));
   return IPC_OK();
 }
 
+void ProxyAutoConfigChild::PendingQuery::Resolve(nsresult aStatus,
+                                                 const nsCString& aResult) {
+  mResolver(Tuple<const nsresult&, const nsCString&>(aStatus, aResult));
+}
+
 mozilla::ipc::IPCResult ProxyAutoConfigChild::RecvGetProxyForURI(
     const nsCString& aTestURI, const nsCString& aTestHost,
     GetProxyForURIResolver&& aResolver) {
-  RefPtr<ProxyAutoConfigChild> self = this;
-  auto callResolver = [self, testURI(aTestURI), testHost(aTestHost),
-                       resolver{std::move(aResolver)}]() {
-    if (!self->CanSend()) {
-      return;
-    }
-
-    nsCString result;
-    nsresult rv = self->mPAC->GetProxyForURI(testURI, testHost, result);
-    resolver(Tuple<const nsresult&, const nsCString&>(rv, result));
-  };
-  if (mPAC->WaitingForDNSResolve()) {
-    mPAC->RegisterDNSResolveCallback(
-        [resolverCallback{std::move(callResolver)}]() { resolverCallback(); });
-    return IPC_OK();
-  }
-
-  callResolver();
+  mPendingQ.insertBack(
+      new PendingQuery(aTestURI, aTestHost, std::move(aResolver)));
+  ProcessPendingQ();
   return IPC_OK();
 }
 
-mozilla::ipc::IPCResult ProxyAutoConfigChild::RecvGC() {
-  mPAC->GC();
-  return IPC_OK();
+void ProxyAutoConfigChild::ProcessPendingQ() {
+  while (ProcessPending()) {
+    ;
+  }
+
+  if (mShutdown) {
+    mPAC->Shutdown();
+  } else {
+    // do GC while the thread has nothing pending
+    mPAC->GC();
+  }
+}
+
+bool ProxyAutoConfigChild::ProcessPending() {
+  if (mPendingQ.isEmpty()) {
+    return false;
+  }
+
+  if (mInProgress || !mPACLoaded) {
+    return false;
+  }
+
+  if (mShutdown) {
+    return true;
+  }
+
+  mInProgress = true;
+  RefPtr<PendingQuery> query = mPendingQ.popFirst();
+  nsCString result;
+  nsresult rv = mPAC->GetProxyForURI(query->URI(), query->Host(), result);
+  query->Resolve(rv, result);
+  mInProgress = false;
+  return true;
 }
 
 void ProxyAutoConfigChild::ActorDestroy(ActorDestroyReason aWhy) {
+  mPendingQ.clear();
+  mShutdown = true;
   mPAC->Shutdown();
 
   // To avoid racing with the main thread, we need to dispatch
@@ -185,18 +220,6 @@ void ProxyAutoConfigChild::ActorDestroy(ActorDestroyReason aWhy) {
       "ProxyAutoConfigChild::Destroy", this, &ProxyAutoConfigChild::Destroy)));
 }
 
-void ProxyAutoConfigChild::Destroy() {
-  // May be called on any thread!
-  MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(NewNonOwningRunnableMethod(
-      "ProxyAutoConfigChild::MainThreadActorDestroy", this,
-      &ProxyAutoConfigChild::MainThreadActorDestroy)));
-}
-
-void ProxyAutoConfigChild::MainThreadActorDestroy() {
-  MOZ_ASSERT(NS_IsMainThread());
-
-  // This may be the last reference!
-  Release();
-}
+void ProxyAutoConfigChild::Destroy() { sActor = nullptr; }
 
 }  // namespace mozilla::net

netwerk/ipc/ProxyAutoConfigChild.h

@@ -6,6 +6,8 @@
 #ifndef ProxyAutoConfigChild_h__
 #define ProxyAutoConfigChild_h__
 
+#include <functional>
+#include "mozilla/LinkedList.h"
 #include "mozilla/net/PProxyAutoConfigChild.h"
 #include "mozilla/StaticPtr.h"
 #include "mozilla/UniquePtr.h"
@@ -34,15 +36,43 @@ class ProxyAutoConfigChild final : public PProxyAutoConfigChild {
       const nsCString& aTestURI, const nsCString& aTestHost,
       GetProxyForURIResolver&& aResolver);
 
-  mozilla::ipc::IPCResult RecvGC();
-
   void Destroy();
 
  private:
   virtual ~ProxyAutoConfigChild();
-  void MainThreadActorDestroy();
+  void ProcessPendingQ();
+  bool ProcessPending();
+  static void BindProxyAutoConfigChild(
+      RefPtr<ProxyAutoConfigChild>&& aActor,
+      Endpoint<PProxyAutoConfigChild>&& aEndpoint);
 
   UniquePtr<ProxyAutoConfig> mPAC;
+  bool mInProgress{false};
+  bool mPACLoaded{false};
+  bool mShutdown{false};
+
+  class PendingQuery final : public LinkedListElement<RefPtr<PendingQuery>> {
+   public:
+    NS_INLINE_DECL_REFCOUNTING(PendingQuery)
+
+    explicit PendingQuery(const nsACString& aTestURI,
+                          const nsACString& aTestHost,
+                          GetProxyForURIResolver&& aResolver)
+        : mURI(aTestURI), mHost(aTestHost), mResolver(std::move(aResolver)) {}
+
+    void Resolve(nsresult aStatus, const nsCString& aResult);
+    const nsCString& URI() const { return mURI; }
+    const nsCString& Host() const { return mHost; }
+
+   private:
+    ~PendingQuery() = default;
+
+    nsCString mURI;
+    nsCString mHost;
+    GetProxyForURIResolver mResolver;
+  };
+
+  LinkedList<RefPtr<PendingQuery>> mPendingQ;
 
   static StaticRefPtr<nsIThread> sPACThread;
   static bool sShutdownObserverRegistered;

netwerk/ipc/SocketProcessChild.cpp

@@ -135,17 +135,6 @@ bool SocketProcessChild::Init(base::ProcessId aParentPid,
     return false;
   }
 
-  if (StaticPrefs::network_proxy_parse_pac_on_socket_process()) {
-    // For parsing PAC.
-    const char* jsInitFailureReason = JS_InitWithFailureDiagnostic();
-    if (jsInitFailureReason) {
-      MOZ_CRASH_UNSAFE(jsInitFailureReason);
-    }
-    sInitializedJS = true;
-
-    xpc::SelfHostedShmem::GetSingleton();
-  }
-
   BackgroundChild::Startup();
   BackgroundChild::InitSocketStarter(this);
 
@@ -690,6 +679,17 @@ mozilla::ipc::IPCResult SocketProcessChild::RecvGetHttpConnectionData(
 
 mozilla::ipc::IPCResult SocketProcessChild::RecvInitProxyAutoConfigChild(
     Endpoint<PProxyAutoConfigChild>&& aEndpoint) {
+  // For parsing PAC.
+  if (!sInitializedJS) {
+    const char* jsInitFailureReason = JS_InitWithFailureDiagnostic();
+    if (jsInitFailureReason) {
+      MOZ_CRASH_UNSAFE(jsInitFailureReason);
+    }
+    sInitializedJS = true;
+
+    xpc::SelfHostedShmem::GetSingleton();
+  }
+
   Unused << ProxyAutoConfigChild::Create(std::move(aEndpoint));
   return IPC_OK();
 }

netwerk/test/unit/test_protocolproxyservice.js

@@ -23,6 +23,7 @@
 var ios = Services.io;
 var pps = Cc["@mozilla.org/network/protocol-proxy-service;1"].getService();
 var prefs = Services.prefs;
+var again = true;
 
 /**
  * Test nsIProtocolHandler that allows proxying, but doesn't allow HTTP
@@ -921,6 +922,7 @@ function run_failed_script_test() {
 }
 
 var directFilter;
+const TEST_URI = "http://127.0.0.1:7247/";
 
 function failed_script_callback(pi) {
   // we should go direct
@@ -937,7 +939,7 @@ function failed_script_callback(pi) {
   obs.addObserver(directFilterListener, "http-on-modify-request");
 
   var ssm = Services.scriptSecurityManager;
-  let uri = "http://127.0.0.1:7247";
+  let uri = TEST_URI;
   var chan = NetUtil.newChannel({
     uri,
     loadingPrincipal: ssm.createContentPrincipal(Services.io.newURI(uri), {}),
@@ -975,6 +977,10 @@ var directFilterListener = {
       subject instanceof Ci.nsIHttpChannel &&
       subject instanceof Ci.nsIProxiedChannel
     ) {
+      info("check proxy in observe uri=" + subject.URI.spec);
+      if (subject.URI.spec != TEST_URI) {
+        return;
+      }
       check_proxy(subject.proxyInfo, "http", "127.0.0.1", 7246, 0, 0, false);
       this.onModifyRequestCalled = true;
     }
@@ -1040,12 +1046,36 @@ function localhost_callback(pi) {
   Assert.equal(pi, null); // no proxy!
 
   prefs.setIntPref("network.proxy.type", 0);
-  do_test_finished();
+
+  // Due to the sandbox limitation on windows, we can't test this on windows.
+  if (mozinfo.socketprocess_networking && again && mozinfo.os != "win") {
+    info("run test again");
+    again = false;
+    cleanUp();
+    prefs.setBoolPref("network.proxy.parse_pac_on_socket_process", true);
+    run_filter_test();
+  } else {
+    cleanUp();
+    do_test_finished();
+  }
+}
+
+function cleanUp() {
+  prefs.clearUserPref("network.proxy.type");
+  prefs.clearUserPref("network.proxy.http");
+  prefs.clearUserPref("network.proxy.http_port");
+  prefs.clearUserPref("network.proxy.socks");
+  prefs.clearUserPref("network.proxy.socks_port");
+  prefs.clearUserPref("network.proxy.autoconfig_url");
+  prefs.clearUserPref("network.proxy.proxy_over_tls");
+  prefs.clearUserPref("network.proxy.no_proxies_on");
+  prefs.clearUserPref("network.proxy.parse_pac_on_socket_process");
 }
 
 function run_test() {
   register_test_protocol_handler();
 
+  prefs.setBoolPref("network.proxy.parse_pac_on_socket_process", false);
   // start of asynchronous test chain
   run_filter_test();
   do_test_pending();

netwerk/test/unit/test_trr_proxy.js

@@ -19,6 +19,7 @@ const dns = Cc["@mozilla.org/network/dns-service;1"].getService(
 
 registerCleanupFunction(async () => {
   Services.prefs.clearUserPref("network.proxy.type");
+  Services.prefs.clearUserPref("network.proxy.parse_pac_on_socket_process");
   trr_clear_prefs();
 });
 
@@ -47,7 +48,7 @@ const override = Cc["@mozilla.org/network/native-dns-override;1"].getService(
   Ci.nsINativeDNSResolverOverride
 );
 
-add_task(async function test_pac_dnsResolve() {
+async function do_test_pac_dnsResolve() {
   Services.prefs.setCharPref("network.trr.confirmationNS", "skip");
   Services.console.reset();
   // Create a console listener.
@@ -133,4 +134,27 @@ add_task(async function test_pac_dnsResolve() {
   await test_with("test3.com", 2, false);
   await test_with("test4.com", 3, false);
   await httpserv.stop();
+}
+
+add_task(async function test_pac_dnsResolve() {
+  Services.prefs.setBoolPref(
+    "network.proxy.parse_pac_on_socket_process",
+    false
+  );
+
+  await do_test_pac_dnsResolve();
+
+  // Due to the sandbox limitation on windows, we can't test this on windows.
+  if (mozinfo.socketprocess_networking && mozinfo.os != "win") {
+    info("run test again");
+    Services.prefs.clearUserPref("network.proxy.type");
+    trr_clear_prefs();
+    Services.prefs.setBoolPref(
+      "network.proxy.parse_pac_on_socket_process",
+      true
+    );
+    Services.prefs.setIntPref("network.proxy.type", 2);
+    Services.prefs.setIntPref("network.proxy.type", 0);
+    await do_test_pac_dnsResolve();
+  }
 });