1489229 - Crash in mozilla::net::nsPACMan::ConfigureWPAD

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-4f3fff95-5d0c-4a50-afd6-02c790180906.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsresult mozilla::net::nsPACMan::ConfigureWPAD netwerk/base/nsPACMan.cpp:601
1 xul.dll mozilla::net::ExecutePACThreadAction::Run netwerk/base/nsPACMan.cpp:324
2 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1161
3 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519
4 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:364
5 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:318
6 xul.dll static void nsThread::ThreadFunc xpcom/threads/nsThread.cpp:464
7 nss3.dll static void _PR_NativeRunThread nsprpub/pr/src/threads/combined/pruthr.c:397
8 nss3.dll [e-mail].3753308998558175368 nsprpub/pr/src/md/windows/w95thred.c:137
9 ucrtbase.dll thread_start<unsigned int > 

=============================================================
this crash signature is newly showing up in firefox 63 with MOZ_RELEASE_ASSERT(mProxyConfigType == nsIProtocolProxyService::PROXYCONFIG_WPAD) (WPAD is being executed when not selected by user) from bug 356831.
so far those crashes are happening in rather low volume though.

Comment 1

[polly.shaw]

Not quite sure what the source of this is, but here are some theories

1. nsPACMan::LoadPACFromURI is being called with an empty string when WPAD is not preffed (although my scrutiny of NSProtocolProxyService hasn't revealed where this might be happening)
2. In nsPACMan::StartLoading, the call to GetNetworkProxyTypeFromPref is failing, and the error is being swallowed
3. There is a race condition with StartLoading being called before mAutoDetect is set
4. there is some sort of race condition with the user changing their pref.

In the absence of an actual diagnosis and fix, I am thinking of putting in a patch which 
a) puts a similar assertion to the one causing this crash in nsPACMan::LoadPACFromURI, when it's called on the main thread. This will provide a stack trace for if theory 1 is the cause.
b) puts in a warning and stops execution if there is an error getting the pref in nsPACMan::StartLoading
c) sets mAutodetect before StartLoading is initiated.

Comment 2

[polly.shaw]

Attachment: Bug 1489229 - crash on MOZ_RELEASE_ASSERT which checks that WPAD is not being called if not explicitly requested by the user prefs

The author did not isolate and fix the cause of the assertion failure,
but put in further diagnostics.The author did not isolate and fix the cause of the assertion failure,
but put in further diagnostics.
* an additional assertion was put in on the main thread (which if
triggered would reveal the stack trace)
* in one place where a previously a failure to read the network.proxy.type pref
was ignored, execution of WPAD is now halted with a warning.

Besides these improved diagnostics, in nsPACMan::LoadPACFromURI where an
asynchronous call was made to nsPACMan::StartLoading *before* the preconditions
for this call are set up was changed to be the other way around. The author
suspects that the previous code may have led to race conditions when
LoadPACFromURI was not called from the main thread, although it is not obvious
that this would have caused such a crash.

Comment 3

[Jason Duell]

Low volume, so hopefully we can wait for Valentin to get back from PTO (end of Sept) to look at this since he knows WPAD.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9007638 [details]
Bug 1489229 - crash on MOZ_RELEASE_ASSERT which checks that WPAD is not being
called if not explicitly requested by the user prefs

Valentin Gosu [:valentin] has approved the revision.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59ea9b4671d0
crash on MOZ_RELEASE_ASSERT which checks that WPAD is not being r=valentin

Comment 6

[Valentin Gosu [:valentin] (he/him)]

I just landed Polly's patch to add some diagnostics and I'll be following up if that tracks down the reason for the crash.

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/59ea9b4671d0

Comment 8

[Calixte Denizet (:calixte)]

There is a crash hitting the added assert:
https://crash-stats.mozilla.com/report/index/d76dab18-131e-4b6e-a1ba-77ddd0181002

The crash reason is:
MOZ_RELEASE_ASSERT(mProxyConfigType == nsIProtocolProxyService::PROXYCONFIG_WPAD) (WPAD is being executed when not selected by user)

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Thanks for pointing out the report, Calixte!

I noticed in the report that the URL is about:preferences... which meant it's quite likely that the crash happened when changing proxy settings.
I managed to reproduce it once on linux, just by changing the settings. I had a harder time to reproduce on Windows, but I finally managed to get some decent STR:
1. Set a page to reload every second (I used urlreload.com to do so)
2. Go to about:config - network.proxy.type
3. Switch between type = 4 to type = 2 a few times.

I think the problem is that we use pacURL = "" as a hint to do autodetect. When changing to type=2 the URL will be empty, mAutoDetect = true (from earlier), and the pref will be different from PROXYCONFIG_WPAD, leading to the crash.

Comment 10

[Jason Duell]

Valentin--any idea how hard this is to fix, and if we could get it into 63?

Small volume crash, so not huge priority if it's hard or a risky code change

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1489229 - Return early from WPAD runnable when pref has been changed r=bagder!

Bug 356831 added a release assert that when performing WPAD the network.proxy.type pref is always 4 (PROXYCONFIG_WPAD).
However, when changing the pref, the runnable is scheduled to run before the PAC thread is shutdown, so it will see the pref that is not PROXYCONFIG_WPAD, while attempting to do WPAD. To avoid this situation, we simply exit early when we detect the wrong pref value.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15045d1278788e357a9b9ac1bdaf3d997e7cda8d

Comment 13

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jason Duell [:jduell] (Regression Engineering Owner for 64; needinfo me) from comment #10)
> Valentin--any idea how hard this is to fix, and if we could get it into 63?
> Small volume crash, so not huge priority if it's hard or a risky code change

It's an easy fix and not risky. I'll land it and nominate it to be uplifted.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc56daee035f
Return early from WPAD runnable when pref has been changed r=bagder

Comment 15

[Narcis Beleuzu [:NarcisB]]

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

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9014061 [details]
Bug 1489229 - Return early from WPAD runnable when pref has been changed r=bagder!

Note that attachment 9007638 [details] needs to be uplifted along with this one.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 356831

User impact if declined: Chance of crashes when changing the proxy settings in about:preferences

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The patch only affects users with the proxy settings set to autodetect.
It is a relatively simple patch, that removes a release assert and turns in into an early return if the condition doesn't match.

I have verified in nightly by looking at the logs that the code functions correctly.

String changes made/needed:

Comment 17

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

Comment on attachment 9014061 [details]
Bug 1489229 - Return early from WPAD runnable when pref has been changed r=bagder!

Crash fix and low risk, uplift approved for 63 beta 13, thanks.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6375a647b5b7
https://hg.mozilla.org/releases/mozilla-beta/rev/dd37ab60c111