Fix logic to check for proxy settings

The previous code tried to find out whether a user has set an
application proxy by checking the type of the applicationProxy.
This is wrong, because a system proxy will actually also change the
applicationProxy type.

Instead, we now rely on QNetworkProxyFactory::usesSystemConfiguration
to decide whether to use QtNetwork's application proxy, or Chromium's
logic for the system proxy. We also save the state of
QNetworkProxy::useSystemConfiguration to be able to track changes.

[ChangeLog][Networking] Fixed an issue where system proxy settings were
not picked up correctly.

Task-number: QTBUG-61910
Change-Id: I1d9af3f6006ba187266fe50c645f425a46632e41
Reviewed-by: Peter Varga <pvarga@inf.u-szeged.hu>

2 files changed, 63 insertions, 35 deletions

diff --git a/src/core/proxy_config_service_qt.cpp b/src/core/proxy_config_service_qt.cpp
index b7ed83624..491131e42 100644
--- a/src/core/proxy_config_service_qt.cpp
+++ b/src/core/proxy_config_service_qt.cpp
@@ -36,6 +36,9 @@
 ** $QT_END_LICENSE$
 **
 ****************************************************************************/
+
+
+//================ Based on ChromeProxyConfigService =======================
 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
@@ -67,11 +70,10 @@ net::ProxyServer ProxyConfigServiceQt::fromQNetworkProxy(const QNetworkProxy &qt
     return net::ProxyServer(proxyScheme, net::HostPortPair(qtProxy.hostName().toStdString(), qtProxy.port()));
 }
 
-//================ Based on ChromeProxyConfigService =======================
-
 ProxyConfigServiceQt::ProxyConfigServiceQt(std::unique_ptr<ProxyConfigService> baseService)
     : m_baseService(baseService.release()),
-      m_registeredObserver(false)
+      m_registeredObserver(false),
+      m_usesSystemConfiguration(false)
 {
 }
 
@@ -83,7 +85,6 @@ ProxyConfigServiceQt::~ProxyConfigServiceQt()
 
 void ProxyConfigServiceQt::AddObserver(net::ProxyConfigService::Observer *observer)
 {
-    RegisterObserver();
     m_observers.AddObserver(observer);
 }
 
@@ -94,28 +95,31 @@ void ProxyConfigServiceQt::RemoveObserver(net::ProxyConfigService::Observer *obs
 
 net::ProxyConfigService::ConfigAvailability ProxyConfigServiceQt::GetLatestProxyConfig(net::ProxyConfig *config)
 {
-    RegisterObserver();
-
-    // Ask the base service if available.
-    net::ProxyConfig systemConfig;
-    ConfigAvailability systemAvailability = net::ProxyConfigService::CONFIG_UNSET;
-    if (m_baseService.get())
-        systemAvailability = m_baseService->GetLatestProxyConfig(&systemConfig);
+#if QT_VERSION >= QT_VERSION_CHECK(5, 8, 0)
+    m_usesSystemConfiguration = QNetworkProxyFactory::usesSystemConfiguration();
+#endif
+    if (m_usesSystemConfiguration) {
+        // Use Chromium's base service to retrieve system settings
+        net::ProxyConfig systemConfig;
+        ConfigAvailability systemAvailability = net::ProxyConfigService::CONFIG_UNSET;
+        if (m_baseService.get())
+            systemAvailability = m_baseService->GetLatestProxyConfig(&systemConfig);
+        *config = systemConfig;
+        // make sure to get updates via OnProxyConfigChanged
+        RegisterObserver();
+        return systemAvailability;
+    }
 
+    // Use QNetworkProxy::applicationProxy settings
     const QNetworkProxy &qtProxy = QNetworkProxy::applicationProxy();
     if (qtProxy == m_qtApplicationProxy && !m_qtProxyConfig.proxy_rules().empty()) {
+        // no changes
         *config = m_qtProxyConfig;
         return CONFIG_VALID;
     }
+
     m_qtApplicationProxy = qtProxy;
     m_qtProxyConfig = net::ProxyConfig();
-#if QT_VERSION >= QT_VERSION_CHECK(5, 7, 0)
-    if (qtProxy.type() == QNetworkProxy::NoProxy
-            && QNetworkProxyFactory::usesSystemConfiguration()) {
-        *config = systemConfig;
-        return systemAvailability;
-    }
-#endif
 
     net::ProxyConfig::ProxyRules qtRules;
     net::ProxyServer server = fromQNetworkProxy(qtProxy);
@@ -145,31 +149,51 @@ net::ProxyConfigService::ConfigAvailability ProxyConfigServiceQt::GetLatestProxy
 
 void ProxyConfigServiceQt::OnLazyPoll()
 {
-    if (m_qtApplicationProxy != QNetworkProxy::applicationProxy()) {
-        net::ProxyConfig unusedConfig;
-        OnProxyConfigChanged(unusedConfig, CONFIG_VALID);
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+#if QT_VERSION >= QT_VERSION_CHECK(5, 8, 0)
+    // We need to update if
+    // - setUseSystemConfiguration() was called in between
+    // - user changed application proxy
+    if (m_usesSystemConfiguration != QNetworkProxyFactory::usesSystemConfiguration()
+        || (!m_usesSystemConfiguration && m_qtApplicationProxy != QNetworkProxy::applicationProxy())) {
+        Update();
+    } else if (m_usesSystemConfiguration) {
+        if (m_baseService.get())
+            m_baseService->OnLazyPoll();
     }
-    if (m_baseService.get())
-        m_baseService->OnLazyPoll();
+#else
+    if (m_qtApplicationProxy != QNetworkProxy::applicationProxy())
+        Update();
+#endif
 }
 
-
+// Called when the base service changed
 void ProxyConfigServiceQt::OnProxyConfigChanged(const net::ProxyConfig &config, ConfigAvailability availability)
 {
-    Q_UNUSED(config);
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    Q_UNUSED(config);
 
-    if (m_qtApplicationProxy != QNetworkProxy::applicationProxy()
-            || m_qtApplicationProxy.type() == QNetworkProxy::NoProxy) {
-        net::ProxyConfig actual_config;
-        availability = GetLatestProxyConfig(&actual_config);
-        if (availability == CONFIG_PENDING)
-            return;
-        for (net::ProxyConfigService::Observer &observer: m_observers)
-            observer.OnProxyConfigChanged(actual_config, availability);
-    }
+    if (!m_usesSystemConfiguration)
+        return;
+
+    Update();
+}
+
+// Update our observers
+void ProxyConfigServiceQt::Update()
+{
+    net::ProxyConfig actual_config;
+    ConfigAvailability availability = GetLatestProxyConfig(&actual_config);
+    if (availability == CONFIG_PENDING)
+        return;
+    for (net::ProxyConfigService::Observer &observer: m_observers)
+        observer.OnProxyConfigChanged(actual_config, availability);
 }
 
+// Register ourselves as observer of the base service.
+// This has to be done on the IO thread, and therefore cannot be done
+// in the constructor.
 void ProxyConfigServiceQt::RegisterObserver()
 {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
diff --git a/src/core/proxy_config_service_qt.h b/src/core/proxy_config_service_qt.h
index f2f9a2210..7be3289d0 100644
--- a/src/core/proxy_config_service_qt.h
+++ b/src/core/proxy_config_service_qt.h
@@ -69,13 +69,17 @@ private:
     void OnProxyConfigChanged(const net::ProxyConfig& config,
                               ConfigAvailability availability) override;
 
+    // Retrieve new proxy settings and notify observers.
+    void Update();
+
     // Makes sure that the observer registration with the base service is set up.
     void RegisterObserver();
 
     std::unique_ptr<net::ProxyConfigService> m_baseService;
     base::ObserverList<net::ProxyConfigService::Observer, true> m_observers;
 
-    // Keep the last QNetworkProxy::applicationProxy state around.
+    // Keep the last state around.
+    bool m_usesSystemConfiguration;
     QNetworkProxy m_qtApplicationProxy;
     net::ProxyConfig m_qtProxyConfig;