1592628 - network.http.proxy.respect-be-conservative doesn't take effect on startup

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

From bug 1585236 comment 10, when using Firefox Private Network:

I've been testing with the network.http.proxy.respect-be-conservative pref set to false. The update check succeeds after I set it, but when Firefox is restarted it doesn't always take effect; sometimes I'll see the update check fail until I flip the pref back and forth again.

I was seeing this on FPN v11, and I'm still seeing it on v14. I think v15 will set respect-be-conservative false by itself, but I don't think that will fix this.

---

For completeness, in Nightly 72.0a1 (2019-10-30), Windows 64-bit:

1. Create and run a new profile (other instances of Nightly should be closed, only one of them will be able to do update checks)
2. Install FPN from https://private-network.firefox.com/
3. Click the FPN icon, click "Sign in" in the dropdown, sign in to a Firefox account with access to the FPN beta.
4. In about:config, set app.update.log true and network.http.proxy.respect-be-conservative false
5. Close Nightly.
6. Start Nightly.
7. Open the About dialog, which will trigger an update check.

This message will be logged to the browser console:

AUS:SVC Checker:onError - request.status: 2153394078
AUS:SVC Checker:onError - Getting secInfo failed.
AUS:SVC getStatusTextFromCode - transfer error: Update XML file malformed (200), default code: 200

Then after toggling respect-be-conservative true, then false:

AUS:SVC Checker:onLoad - request completed downloading document
AUS:SVC Checker:onLoad - Getting sslStatus failed.
AUS:SVC Checker:onLoad - number of updates available: 0

(this is the correct behavior)

I'm seeing this every time now, I'm not sure why I saw it only about 50% before. Possibly FPN wasn't activating soon enough; if I turn off FPN and restart, then turn FPN on and open About, the update checks succeeds, though this happens even with respect-be-conservative true.

Comment 1

[Dragana Damjanovic [:dragana]]

Honza, can you take a look? is it possible that we do not read the pref on the start?

Comment 2

[Honza Bambas (:mayhemer)]

Managed to repro and debug. We have a broader problem here. When we are called the first time from:

>	xul.dll!mozilla::net::nsHttpHandler::PrefsChanged(0x0000000000000000) Line 1328	C++
 	xul.dll!mozilla::net::nsHttpHandler::Init() Line 461	C++
 	xul.dll!mozilla::net::nsHttpHandler::GetInstance() Line 181	C++
 	xul.dll!mozCreateComponent<mozilla::net::nsHttpHandler>() Line 67	C++
 	xul.dll!mozilla::xpcom::CreateInstanceImpl(, , {...}, 0x000000ee311fd700) Line 0	C++
 	[Inline Frame] xul.dll!mozilla::xpcom::StaticModule::CreateInstance(, , ) Line 11364	C++
 	[Inline Frame] xul.dll!`anonymous namespace'::EntryWrapper::CreateInstance(, {...}, ) Line 221	C++
 	xul.dll!nsComponentManagerImpl::GetServiceLocked({...}, {...}, {...}, 0x000000ee311fd840) Line 1380	C++
 	xul.dll!nsComponentManagerImpl::GetServiceByContractID(0x00007ff9ebaea263, {...}, 0x000000ee311fd840) Line 1567	C++
 	[Inline Frame] xul.dll!CallGetService(, , ) Line 61	C++
 	[Inline Frame] xul.dll!nsGetServiceByContractID::operator()(, ) Line 243	C++
 	xul.dll!nsCOMPtr_base::assign_from_gs_contractid(, ) Line 82	C++
 	[Inline Frame] xul.dll!nsCOMPtr<nsIProtocolHandler>::nsCOMPtr() Line 607	C++
 	xul.dll!mozilla::net::nsHttpsHandler::Init() Line 2529	C++
 	xul.dll!mozCreateComponent<mozilla::net::nsHttpsHandler>() Line 73	C++
 	xul.dll!mozilla::xpcom::CreateInstanceImpl(, , {...}, 0x000000ee311fd9b0) Line 10444	C++
 	[Inline Frame] xul.dll!mozilla::xpcom::StaticModule::CreateInstance(, , ) Line 11364	C++
 	[Inline Frame] xul.dll!`anonymous namespace'::EntryWrapper::CreateInstance(, {...}, ) Line 221	C++
 	xul.dll!nsComponentManagerImpl::GetServiceLocked({...}, {...}, {...}, 0x000000ee311fdbd0) Line 1380	C++
 	xul.dll!nsComponentManagerImpl::GetServiceByContractID(0x000000ee311fdb2c, {...}, 0x000000ee311fdbd0) Line 1567	C++
 	[Inline Frame] xul.dll!CallGetService(, , 0x000000ee311fdbd0) Line 61	C++
 	[Inline Frame] xul.dll!CallGetService(, 0x000000ee311fdbd0) Line 73	C++
 	xul.dll!mozilla::net::nsIOService::GetProtocolHandler(0x000000ee311fdbf4, 0x000000ee311fdbd0) Line 717	C++
 	xul.dll!mozilla::net::nsIOService::ProtocolHasFlags(0x000002db02fe6820, 16, 0x000000ee311fdd37) Line 1505	C++
 	xul.dll!mozilla::net::nsIOService::URIChainHasFlags(0x000002db02fe6820, 16, 0x000000ee311fdd37) Line 1515	C++
 	[Inline Frame] xul.dll!NS_URIChainHasFlags(, , ) Line 2258	C++
 	xul.dll!mozilla::BasePrincipal::CreateContentPrincipal(0x000002db02fe6820, {...}, {...}) Line 580	C++
 	xul.dll!mozilla::BasePrincipal::CreateContentPrincipal(0x000002db02fe6820, {...}) Line 569	C++
 	xul.dll!`anonymous namespace'::GetPrincipalFromOrigin(, 0x000000ee311fdfb0) Line 233	C++
 	xul.dll!nsPermissionManager::_DoImport(0x000002db05d0a2e0, ) Line 2948	C++
 	xul.dll!nsPermissionManager::ImportDefaults() Line 2874	C++
 	xul.dll!nsPermissionManager::InitDB() Line 1688	C++
 	xul.dll!nsPermissionManager::Observe(, , ) Line 0	C++
 	xul.dll!nsObserverList::NotifyObservers(0x0000000000000000, 0x00007ff9ebb58790, 0x00007ff9eb88a4e0) Line 65	C++
 	xul.dll!nsObserverService::NotifyObservers(0x0000000000000000, 0x00007ff9ebb58790, 0x00007ff9eb88a4e0) Line 294	C++
 	xul.dll!nsXREDirProvider::DoStartup() Line 929	C++
 	xul.dll!XREMain::XRE_mainRun() Line 4401	C++
 	xul.dll!XREMain::XRE_main(, , {...}) Line 4721	C++

...we don't have mConnMgr assigned, because nsHttpHandler::Init() first reads prefs and only then creates the conn manager. This means that probably none of the preferences is sent to the socket thread initially, if ever!

This is a major problem and I wonder how we could miss this.

Note that the patch is definitely not a Release channel material, this may reveal bugs we even don't know we have and possibly create problems for users playing with preferences and having non-standard values of some of those we handle initially this way.

For Release we could send down just this new preference.

Comment 3

[Honza Bambas (:mayhemer)]

Filed bug 1595833 for the global scope issue.

Leaving this bug open for consideration whether we want to have a Beta/Release patch just for the affected preference (it's very simple and safe to do).

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you explain or estimate the impact on release users, to help figure out if this is something that should drive a 70.0.2 dot release?

Comment 5

[Honza Bambas (:mayhemer)]

I think the impact has been discussed on slack in #private-network-beta channel. The impact is that bug 1585236 is actually not fixed at all.

The patch I have in mind (to only correctly update this new preference at startup) is a one-line safe patch with zero impact on non-FPN users.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

OK then I'll take that as, no big pressure for a 70 dot release and hope this can wait for 71, Dec. 2nd.

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

Tracking for 71. Please keep in mind that we build our last beta tomorrow and next week is RC week, so if you want an uplift, it would be good to do it now so as to have time for QA to verify it before RC, thanks.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: Bug 1592628 - Propagate proxy.respect-be-conservative preference to the connection manager after startup correctly, r=dragana

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: Bug 1592628 - Propagate network.http.proxy.respect-be-conservative to the http connection manager at startup properly, r=kershaw

Comment 10

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

This seems to no longer be blocking updates as of FPN v15. I think this is because the addon sets the pref after a connection is made for FxA. It's probably still worth fixing but it may not demand such a high priority.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adc08e84bf76
Propagate network.http.proxy.respect-be-conservative to the http connection manager at startup properly, r=kershaw

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #10)

This seems to no longer be blocking updates as of FPN v15. I think this is because the addon sets the pref after a connection is made for FxA. It's probably still worth fixing but it may not demand such a high priority.

Good to know! I was about to fill an uplift request, but looks like I don't have to then?

Comment 13

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Sorry, I stand corrected, v15 doesn't (necessarily?) fix this. I tried with v14, then v15 in a fresh profile on release 70.0.1, it didn't help (though flipping the pref manually did work). The error just doesn't seem to occur on Nightly anymore, even with v14, until I slow things down with logs that I haven't had time to sift through yet. I guess some other net access is happening early enough that the update check can go along with it.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 9110312 [details]
Bug 1592628 - Propagate network.http.proxy.respect-be-conservative to the http connection manager at startup properly, r=kershaw

Beta/Release Uplift Approval Request

• User impact if declined: With FPN installed and turned on, after startup we are not able to check for updates. Users may get stuck on the version they are.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Please ask Adam Gashlin or see or https://bugzilla.mozilla.org/show_bug.cgi?id=1585236#c10.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): None actually. This is an isolated, simple, a pre-existing pattern using patch that only propagates the new preference value in question to the http connection manager to know that value; as it had to in the patch that introduced the preference... Nothing else has changed at all. This patch on its own has zero-impact on users w/o fpn and default settings.
• String changes made/needed:

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/adc08e84bf76

Comment 16

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9110312 [details]
Bug 1592628 - Propagate network.http.proxy.respect-be-conservative to the http connection manager at startup properly, r=kershaw

Given that this is a P1 for FPN and that the patch is limited uplift approved for the beta branch before RC.

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e3f0c3ea7df3

Comment 18

[Cristian Fogel [:cfogel]]

Confirmed issue with 70.0 on Windows 10.
Verified fix with :csasca for 71.0b11 and 72.a1(2019-11-26) on Ubuntu 18.04, Windows 10, macOS 10.15.