Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread; r=bagder
authorLiang-Heng Chen <xeonchen@gmail.com>
Wed, 24 May 2017 14:36:53 +0800
changeset 413279 9a76c93fb1b5ff00ae7653433894958828b7194c
parent 413278 40c08de101f1413b58d7ceed08a3fcbd8654bbd1
child 413280 504c134cc674234582e42f4ba15803137f859737
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbagder
bugs1366133
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread; r=bagder MozReview-Commit-ID: FyiNM8KX0gk
netwerk/base/nsProtocolProxyService.cpp
netwerk/base/nsProtocolProxyService.h
toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp
--- a/netwerk/base/nsProtocolProxyService.cpp
+++ b/netwerk/base/nsProtocolProxyService.cpp
@@ -17,31 +17,33 @@
 #include "nsIChannel.h"
 #include "nsICancelable.h"
 #include "nsIDNSService.h"
 #include "nsPIDNSService.h"
 #include "nsIScriptSecurityManager.h"
 #include "nsIPrefService.h"
 #include "nsIPrefBranch.h"
 #include "nsThreadUtils.h"
+#include "nsQueryObject.h"
 #include "nsSOCKSIOLayer.h"
 #include "nsString.h"
 #include "nsNetUtil.h"
 #include "nsNetCID.h"
 #include "plstr.h"
 #include "prnetdb.h"
 #include "nsPACMan.h"
 #include "nsProxyRelease.h"
 #include "mozilla/Mutex.h"
 #include "mozilla/CondVar.h"
 #include "nsISystemProxySettings.h"
 #include "nsINetworkLinkService.h"
 #include "nsIHttpChannelInternal.h"
 #include "mozilla/Logging.h"
 #include "mozilla/Tokenizer.h"
+#include "mozilla/Unused.h"
 
 //----------------------------------------------------------------------------
 
 namespace mozilla {
 namespace net {
 
   extern const char kProxyType_HTTP[];
   extern const char kProxyType_HTTPS[];
@@ -303,16 +305,84 @@ private:
     nsCOMPtr<nsIProtocolProxyService>  mXPComPPS;
     nsCOMPtr<nsIChannel>               mChannel;
     nsCOMPtr<nsIProtocolProxyCallback> mCallback;
     nsCOMPtr<nsIProxyInfo>             mProxyInfo;
 };
 
 NS_IMPL_ISUPPORTS(nsAsyncResolveRequest, nsICancelable, nsIRunnable)
 
+// Bug 1366133: make GetPACURI off-main-thread since it may hang on Windows platform
+class AsyncGetPACURIRequest final : public nsIRunnable
+{
+public:
+    NS_DECL_THREADSAFE_ISUPPORTS
+
+    using CallbackFunc = nsresult(nsProtocolProxyService::*)(bool, bool, nsresult, const nsACString&);
+
+    AsyncGetPACURIRequest(nsProtocolProxyService* aService,
+                          CallbackFunc aCallback,
+                          nsISystemProxySettings* aSystemProxySettings,
+                          bool aMainThreadOnly,
+                          bool aForceReload,
+                          bool aResetPACThread)
+        : mIsMainThreadOnly(aMainThreadOnly)
+        , mService(aService)
+        , mServiceHolder(do_QueryObject(aService))
+        , mCallback(aCallback)
+        , mSystemProxySettings(aSystemProxySettings)
+        , mForceReload(aForceReload)
+        , mResetPACThread(aResetPACThread)
+    {
+        MOZ_ASSERT(NS_IsMainThread());
+        Unused << mIsMainThreadOnly;
+    }
+
+    NS_IMETHOD Run() override
+    {
+        MOZ_ASSERT(NS_IsMainThread() == mIsMainThreadOnly);
+
+        nsCString pacUri;
+        nsresult rv = mSystemProxySettings->GetPACURI(pacUri);
+
+        nsCOMPtr<nsIRunnable> event =
+            NewNonOwningCancelableRunnableMethod<bool,
+                                                 bool,
+                                                 nsresult,
+                                                 nsCString>("AsyncGetPACURIRequestCallback",
+                                                             mService,
+                                                             mCallback,
+                                                             mForceReload,
+                                                             mResetPACThread,
+                                                             rv,
+                                                             pacUri);
+
+        return NS_DispatchToMainThread(event);
+    }
+
+private:
+    ~AsyncGetPACURIRequest()
+    {
+        MOZ_ASSERT(NS_IsMainThread() == mIsMainThreadOnly);
+        NS_ReleaseOnMainThread(mServiceHolder.forget());
+    }
+
+    bool mIsMainThreadOnly;
+
+    nsProtocolProxyService* mService; // ref-count is hold by mServiceHolder
+    nsCOMPtr<nsIProtocolProxyService2> mServiceHolder;
+    CallbackFunc mCallback;
+    nsCOMPtr<nsISystemProxySettings> mSystemProxySettings;
+
+    bool mForceReload;
+    bool mResetPACThread;
+};
+
+NS_IMPL_ISUPPORTS(AsyncGetPACURIRequest, nsIRunnable)
+
 //----------------------------------------------------------------------------
 
 #define IS_ASCII_SPACE(_c) ((_c) == ' ' || (_c) == '\t')
 
 //
 // apply mask to address (zeros out excluded bits).
 //
 // NOTE: we do the byte swapping here to minimize overall swapping.
@@ -457,16 +527,18 @@ nsProtocolProxyService::Init()
     nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
     if (obs) {
         // register for shutdown notification so we can clean ourselves up
         // properly.
         obs->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
         obs->AddObserver(this, NS_NETWORK_LINK_TOPIC, false);
     }
 
+    NS_NewNamedThread("SysProxySetting", getter_AddRefs(mProxySettingThread));
+
     return NS_OK;
 }
 
 // ReloadNetworkPAC() checks if there's a non-networked PAC in use then avoids
 // to call ReloadPAC()
 nsresult
 nsProtocolProxyService::ReloadNetworkPAC()
 {
@@ -507,16 +579,64 @@ nsProtocolProxyService::ReloadNetworkPAC
         }
     } else if ((type == PROXYCONFIG_WPAD) || (type == PROXYCONFIG_SYSTEM)) {
         ReloadPAC();
     }
 
     return NS_OK;
 }
 
+nsresult
+nsProtocolProxyService::AsyncConfigureFromPAC(bool aForceReload,
+                                              bool aResetPACThread)
+{
+    MOZ_ASSERT(NS_IsMainThread());
+
+    if (NS_WARN_IF(!mProxySettingThread)) {
+        return NS_ERROR_NOT_INITIALIZED;
+    }
+
+    bool mainThreadOnly;
+    nsresult rv = mSystemProxySettings->GetMainThreadOnly(&mainThreadOnly);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+        return rv;
+    }
+
+    nsCOMPtr<nsIRunnable> req =
+        new AsyncGetPACURIRequest(this,
+                                  &nsProtocolProxyService::OnAsyncGetPACURI,
+                                  mSystemProxySettings,
+                                  mainThreadOnly,
+                                  aForceReload,
+                                  aResetPACThread);
+
+    if (mainThreadOnly) {
+        return req->Run();
+    }
+    return mProxySettingThread->Dispatch(req, nsIEventTarget::DISPATCH_NORMAL);
+}
+
+nsresult
+nsProtocolProxyService::OnAsyncGetPACURI(bool aForceReload,
+                                         bool aResetPACThread,
+                                         nsresult aResult,
+                                         const nsACString& aUri)
+{
+    MOZ_ASSERT(NS_IsMainThread());
+
+    if (aResetPACThread) {
+        ResetPACThread();
+    }
+
+    if (NS_SUCCEEDED(aResult) && !aUri.IsEmpty()) {
+        ConfigureFromPAC(PromiseFlatCString(aUri), aForceReload);
+    }
+
+    return NS_OK;
+}
 
 NS_IMETHODIMP
 nsProtocolProxyService::Observe(nsISupports     *aSubject,
                                 const char      *aTopic,
                                 const char16_t *aData)
 {
     if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
         mIsShutdown = true;
@@ -528,16 +648,21 @@ nsProtocolProxyService::Observe(nsISuppo
             delete mFilters;
             mFilters = nullptr;
         }
         if (mPACMan) {
             mPACMan->Shutdown();
             mPACMan = nullptr;
         }
 
+        if (mProxySettingThread) {
+            mProxySettingThread->Shutdown();
+            mProxySettingThread = nullptr;
+        }
+
         nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
         if (obs) {
             obs->RemoveObserver(this, NS_NETWORK_LINK_TOPIC);
             obs->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
         }
 
     } else if (strcmp(aTopic, NS_NETWORK_LINK_TOPIC) == 0) {
         nsCString converted = NS_ConvertUTF16toUTF8(aData);
@@ -679,17 +804,17 @@ nsProtocolProxyService::PrefsChanged(nsI
             // hosts's FQDN, stripping components until we hit a TLD.  Doing so
             // is dangerous in the face of an incomplete list of TLDs, and TLDs
             // get added over time.  We could consider doing only a single
             // substitution of the first component, if that proves to help
             // compatibility.
             tempString.AssignLiteral(WPAD_URL);
         } else if (mSystemProxySettings) {
             // Get System Proxy settings if available
-            mSystemProxySettings->GetPACURI(tempString);
+            AsyncConfigureFromPAC(false, false);
         }
         if (!tempString.IsEmpty())
             ConfigureFromPAC(tempString, false);
     }
 }
 
 bool
 nsProtocolProxyService::CanUseProxy(nsIURI *aURI, int32_t defaultPort)
@@ -1117,19 +1242,21 @@ nsProtocolProxyService::ReloadPAC()
         return NS_OK;
 
     nsXPIDLCString pacSpec;
     if (type == PROXYCONFIG_PAC)
         prefs->GetCharPref(PROXY_PREF("autoconfig_url"), getter_Copies(pacSpec));
     else if (type == PROXYCONFIG_WPAD)
         pacSpec.AssignLiteral(WPAD_URL);
     else if (type == PROXYCONFIG_SYSTEM) {
-        if (mSystemProxySettings)
-            mSystemProxySettings->GetPACURI(pacSpec);
-        ResetPACThread();
+        if (mSystemProxySettings) {
+            AsyncConfigureFromPAC(true, true);
+        } else {
+            ResetPACThread();
+        }
     }
 
     if (!pacSpec.IsEmpty())
         ConfigureFromPAC(pacSpec, true);
     return NS_OK;
 }
 
 // When sync interface is removed this can go away too
@@ -1810,16 +1937,19 @@ nsProtocolProxyService::Resolve_Internal
         *usePACThread = true;
         return NS_OK;
     }
 
     if (mSystemProxySettings && mProxyConfig == PROXYCONFIG_SYSTEM) {
         // If the system proxy setting implementation is not threadsafe (e.g
         // linux gconf), we'll do it inline here. Such implementations promise
         // not to block
+        // bug 1366133: this block uses GetPACURI & GetProxyForURI, which may
+        // hang on Windows platform. Fortunately, current implementation on
+        // Windows is not main thread only, so we are safe here.
 
         nsAutoCString PACURI;
         nsAutoCString pacString;
 
         if (NS_SUCCEEDED(mSystemProxySettings->GetPACURI(PACURI)) &&
             !PACURI.IsEmpty()) {
             // There is a PAC URI configured. If it is unchanged, then
             // just execute the PAC thread. If it is changed then load
--- a/netwerk/base/nsProtocolProxyService.h
+++ b/netwerk/base/nsProtocolProxyService.h
@@ -9,16 +9,17 @@
 #include "nsString.h"
 #include "nsCOMPtr.h"
 #include "nsAutoPtr.h"
 #include "nsTArray.h"
 #include "nsIProtocolProxyService2.h"
 #include "nsIProtocolProxyFilter.h"
 #include "nsIProxyInfo.h"
 #include "nsIObserver.h"
+#include "nsIThread.h"
 #include "nsDataHashtable.h"
 #include "nsHashKeys.h"
 #include "prio.h"
 #include "mozilla/Attributes.h"
 
 class nsIPrefBranch;
 class nsISystemProxySettings;
 
@@ -296,16 +297,22 @@ protected:
      */
     void MaybeDisableDNSPrefetch(nsIProxyInfo *aProxy);
 
 private:
     nsresult SetupPACThread();
     nsresult ResetPACThread();
     nsresult ReloadNetworkPAC();
 
+    nsresult AsyncConfigureFromPAC(bool aForceReload, bool aResetPACThread);
+    nsresult OnAsyncGetPACURI(bool aForceReload,
+                              bool aResetPACThread,
+                              nsresult aResult,
+                              const nsACString& aUri);
+
 public:
     // The Sun Forte compiler and others implement older versions of the
     // C++ standard's rules on access and nested classes.  These structs
     // need to be public in order to deal with those compilers.
 
     struct HostInfoIP {
         uint16_t   family;
         uint16_t   mask_len;
@@ -396,16 +403,17 @@ protected:
     int32_t                      mFailedProxyTimeout;
 
 private:
     nsresult AsyncResolveInternal(nsIChannel *channel, uint32_t flags,
                                   nsIProtocolProxyCallback *callback,
                                   nsICancelable **result,
                                   bool isSyncOK);
     bool                          mIsShutdown;
+    nsCOMPtr<nsIThread>           mProxySettingThread;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(nsProtocolProxyService, NS_PROTOCOL_PROXY_SERVICE_IMPL_CID)
 
 } // namespace net
 } // namespace mozilla
 
 #endif // !nsProtocolProxyService_h__
--- a/toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp
+++ b/toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp
@@ -11,16 +11,17 @@
 #include "mozilla/Attributes.h"
 #include "nsISystemProxySettings.h"
 #include "nsIServiceManager.h"
 #include "mozilla/ModuleUtils.h"
 #include "nsPrintfCString.h"
 #include "nsNetCID.h"
 #include "nsISupportsPrimitives.h"
 #include "nsIURI.h"
+#include "nsThreadUtils.h"
 #include "GeckoProfiler.h"
 #include "prnetdb.h"
 #include "ProxyUtils.h"
 
 class nsWindowsSystemProxySettings final : public nsISystemProxySettings
 {
 public:
     NS_DECL_THREADSAFE_ISUPPORTS
@@ -36,16 +37,19 @@ private:
     bool PatternMatch(const nsACString& aHost, const nsACString& aOverride);
 };
 
 NS_IMPL_ISUPPORTS(nsWindowsSystemProxySettings, nsISystemProxySettings)
 
 NS_IMETHODIMP
 nsWindowsSystemProxySettings::GetMainThreadOnly(bool *aMainThreadOnly)
 {
+  // bug 1366133: if you change this to main thread only, please handle
+  // nsProtocolProxyService::Resolve_Internal carefully to avoid hang on main
+  // thread.
   *aMainThreadOnly = false;
   return NS_OK;
 }
 
 
 nsresult
 nsWindowsSystemProxySettings::Init()
 {
@@ -64,16 +68,19 @@ static void SetProxyResultDirect(nsACStr
 {
     // For whatever reason, a proxy is not to be used.
     aResult.AssignASCII("DIRECT");
 }
 
 static nsresult ReadInternetOption(uint32_t aOption, uint32_t& aFlags,
                                    nsAString& aValue)
 {
+    // Bug 1366133: InternetGetConnectedStateExW() may cause hangs
+    MOZ_ASSERT(!NS_IsMainThread());
+
     DWORD connFlags = 0;
     WCHAR connName[RAS_MaxEntryName + 1];
     MOZ_SEH_TRY {
         InternetGetConnectedStateExW(&connFlags, connName,
                                      mozilla::ArrayLength(connName), 0);
     } MOZ_SEH_EXCEPT(EXCEPTION_EXECUTE_HANDLER) {
         return NS_ERROR_FAILURE;
     }