1392272 - [Win] Optimize the way we resolve proxy settings

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P3)

Description

[Honza Bambas (:mayhemer)]

With the default prefs we do async proxy resolution for every channel.  Most users these days have Windows set to "no proxy" in the Windows internet settings.  We should detect changes by observing for either registry changes or doing some other magic and bypass proxy resolution completely when no proxy has been specified.

I can see 20+ ms (on a fast machine, in an opt build!) on EVERY channel.  This also includes main thread dispatch.

Not sure we could have this for 57, but would definitely be nice.

Loosely blocking CDP.

https://stackoverflow.com/questions/6192563/detect-windows-ie-proxy-settings-changes

Comment 1

[Honza Bambas (:mayhemer)]

One simple solution (some probability of regression prove..) could be to refresh the registry settings only when loading a top level document and not for sub-resources.

Comment 2

[Gary Chen [:xeonchen]]

Assign to myself, but maybe fix later, so keep necko-next.

Comment 3

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: Bug 1392272 - P1: [windows] Monitor system proxy changes, r=#necko

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Bug 1392272 - P2: [osx] Monitor system proxy changes, r=#necko

Depends on D127724

Comment 7

[Kershaw Chang [:kershaw]]

Add leave-open, since linux and android parts are not completed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c1c60a1f6ef
P1: [windows] Monitor system proxy changes, r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/931db25d14fd
P2: [osx] Monitor system proxy changes, r=necko-reviewers,dragana

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/1c1c60a1f6ef
https://hg.mozilla.org/mozilla-central/rev/931db25d14fd

Comment 10

[Mathew Hodson]

I think it would be better to close this bug since patches landed in Firefox 96 and open new bugs for Linux and Android.

Comment 11

[Kershaw Chang [:kershaw]]

(In reply to Mathew Hodson from comment #10)

I think it would be better to close this bug since patches landed in Firefox 96 and open new bugs for Linux and Android.

Note that the patches in this bug was reverted because of bug 1749501.
I'll file a new bug to track the necessary work.

Comment 12

[Markus Stange [:mstange]]

(In reply to Kershaw Chang [:kershaw] from comment #11)

I'll file a new bug to track the necessary work.

Bug 1907364 might be that new bug.

Comment 13

[Lars Eggert [:lars]]

I recently looked into this for Bug 2028356, and Claude came up with suggestions for how to fix this bug here, FYI:

Plan for Re-landing network.proxy.detect_system_proxy_changes (Bug 1392272)

Background

Kershaw's original patches (D127724, D127725) were reverted in Bug 1749501 due to a
one-line bug in MatchOverride that caused only the first proxy exception to be checked.
The approach itself — caching system proxy configuration to avoid per-request OS API calls
— is sound. This document describes what is needed to re-land the patches correctly.

---

Issue 1: The Regression Bug (Must Fix)

File: toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp

MatchOverride was refactored to use a ReadProxyRules + callback pattern.
ReadProxyRules initializes continueProcessing = false and relies on the callback to
set it true to continue iterating. The MatchOverride ruleHandler only set aContinue = false on match and never set it true on non-match, so iteration always stopped after
the first bypass rule.

Fix: One line in the MatchOverride ruleHandler:

auto ruleHandler = [&](const nsACString& aOverrideRule, bool& aContinue) {
    foundMatch = MatchOverrideInternal(aHost, aOverrideRule);
    aContinue = !foundMatch;  // was missing — loop never continued past first rule
};

---

Issue 2: nsIWindowsRegKey::HasChanged No Longer Exists (Must Fix)

File: toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp,
WindowsSystemProxySettingsAsync::ThreadFunc

The original patch used nsIWindowsRegKey::HasChanged in a background thread loop to
detect registry changes. This XPCOM interface no longer exists — it was replaced by
WinRegistry::KeyWatcher. The ThreadFunc approach also had a structural problem: even
when HasChanged existed, it performed a non-blocking poll
(WaitForSingleObject(..., 0)), meaning the loop would spin at 100% CPU on a dedicated
thread waiting for a registry change.

Fix: Remove the background thread entirely. Replace with WinRegistry::KeyWatcher,
exactly as WindowsInternetFunctionsWrapper already does in the same file:

// In WindowsSystemProxySettingsAsync::Init():
using namespace mozilla::widget::WinRegistry;
Key key(HKEY_CURRENT_USER,
        u"Software\\Microsoft\\Windows\\CurrentVersion\\Internet Settings"_ns,
        KeyMode::Notify);
if (key) {
    mKeyWatcher = MakeUnique<KeyWatcher>(
        std::move(key), GetCurrentSerialEventTarget(), [self = RefPtr{this}] {
            self->OnProxyConfigChangedInternal();
        });
}

This dispatches OnProxyConfigChangedInternal() on the event target whenever the
registry key changes — event-driven with no background thread, no polling, no busy-wait.
The ThreadFunc, mBackgroundThread, mTerminated, and the xpcom-shutdown-threads
observer can all be removed.

Initial load: Call OnProxyConfigChangedInternal() once synchronously from Init()
to populate the initial cache before returning.

---

Issue 3: Init Window Falls Back to Uninitialized State (Should Fix)

File: toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp

During the initialization window (before mInited = true), GetProxyForURI delegates
to the base class. With the KeyWatcher-based approach, Init() can call
OnProxyConfigChangedInternal() synchronously and set mInited = true before
returning, eliminating the fallback window entirely.

---

Issue 4: processProxyStr Duplicates Existing Proxy String Parsing (Should Fix)

File: toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp,
OnProxyConfigChangedInternal

The original patch contains a ~50-line hand-rolled tokenizer (processProxyStr lambda)
parsing Windows proxy strings like http=127.0.0.1:3128. This duplicates logic already
in nsProtocolProxyService::ExtractProxyInfo / ProcessPACString. Two independent
parsers for the same format will diverge.

Fix: Use ProcessPACString (or the existing GetProxyForURI formatting) rather than
re-implementing the parser. Alternatively, factor the Windows registry parsing into the
existing nsWindowsSystemProxySettings::GetProxyForURI and call it from
OnProxyConfigChangedInternal.

---

Issue 5: OSXSystemProxySettingsAsync::GetSystemWPADSetting Not Overridden (Should Fix)

File: toolkit/system/osxproxy/nsOSXSystemProxySettings.mm

OSXSystemProxySettingsAsync does not override GetSystemWPADSetting. The base class
always returns false. If macOS has WPAD configured, the async class will silently
ignore it. The WPAD state should be captured during OnProxyConfigChangedInternal and
returned here.

---

Issue 6: Tests (Must Add)

The original patches shipped with no tests. This is what allowed the regression to ship
undetected.

Test for MatchOverride with multiple bypass rules (regression test)

nsWindowsSystemProxySettings accepts a WindowsInternetFunctionsWrapper* as a
constructor argument. Subclass it with a mock that returns a specific bypass string:

class MockInternetFunctions : public WindowsInternetFunctionsWrapper {
  nsresult ReadInternetOption(uint32_t aOption, uint32_t& aFlags,
                              nsAString& aValue) override {
    aFlags = PROXY_TYPE_PROXY;
    if (aOption == INTERNET_PER_CONN_PROXY_SERVER) {
      aValue.AssignLiteral("proxy.example.com:8080");
    } else if (aOption == INTERNET_PER_CONN_PROXY_BYPASS) {
      aValue.AssignLiteral("*.first.com;*.second.com;*.third.com");
    }
    return NS_OK;
  }
};

Then in a gtest:

• GetProxyForURI("http://foo.first.com/", ...) → assert DIRECT (first rule)
• GetProxyForURI("http://foo.second.com/", ...) → assert DIRECT (second rule — this
  is the regression)
• GetProxyForURI("http://foo.third.com/", ...) → assert DIRECT (third rule)
• GetProxyForURI("http://other.example.com/", ...) → assert PROXY

Lives in toolkit/system/windowsproxy/tests/ as a gtest, #ifdef XP_WIN guarded.

Test for OnProxyConfigChangedInternal / async GetProxyForURI

With Init() loading synchronously (Issue 3 fix), the async class is immediately
testable:

auto mock = MakeRefPtr<MockInternetFunctions>();
// mock returns bypass list with multiple rules
auto settings = MakeRefPtr<WindowsSystemProxySettingsAsync>(mock);
settings->Init();  // populates mConfig synchronously

nsAutoCString result;
settings->GetProxyForURI("...", "http", "foo.second.com", 80, result);
ASSERT_STREQ(result.get(), "DIRECT");

macOS test for OSXSystemProxySettingsAsync

A test subclass can populate mConfig directly:

class TestOSXProxySettingsAsync : public OSXSystemProxySettingsAsync {
 public:
  void SetConfig(ProxyConfig&& aConfig) { mConfig = std::move(aConfig); }
};

Test: populate with bypass rules, call GetProxyForURI for hosts matching the second and
third rules. Lives in toolkit/system/osxproxy/tests/ under #ifdef XP_MACOSX.

---

Files to Modify

File	Change
toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp	Fix aContinue; replace ThreadFunc/HasChanged with KeyWatcher; synchronous init; remove processProxyStr
toolkit/system/windowsproxy/nsWindowsSystemProxySettings.h	Remove thread/observer members; add mKeyWatcher
toolkit/system/osxproxy/nsOSXSystemProxySettings.mm	Override GetSystemWPADSetting in async class
toolkit/system/windowsproxy/tests/	New gtests for MatchOverride with multiple rules and async class
toolkit/system/osxproxy/tests/	New tests for async bypass rule handling