Crash in mozilla::net::nsPACMan::ConfigureWPAD

RESOLVED FIXED in Firefox 63

Status

()

defect
P2
critical
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: philipp, Assigned: valentin)

Tracking

({crash, regression})

63 Branch
mozilla64
All
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 fixed, firefox64 fixed)

Details

(Whiteboard: [necko-triaged], crash signature)

Attachments

(2 attachments)

Reporter

Description

9 months ago
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 pr_root@4.llvm.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

9 months ago
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.
Assignee: nobody → polly.shaw

Comment 2

9 months ago
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.
Priority: -- → P2
Whiteboard: [necko-triaged]

Comment 3

8 months ago
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.
Flags: needinfo?(valentin.gosu)
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.
Attachment #9007638 - Flags: review+

Comment 5

8 months ago
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/59ea9b4671d0
crash on MOZ_RELEASE_ASSERT which checks that WPAD is not being r=valentin
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.
Assignee: polly.shaw → valentin.gosu
Flags: needinfo?(valentin.gosu)
Keywords: leave-open
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)
Crash Signature: [@ mozilla::net::nsPACMan::ConfigureWPAD] → [@ mozilla::net::nsPACMan::ConfigureWPAD] [@ mozilla::net::nsPACMan::LoadPACFromURI]
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

8 months ago
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
Flags: needinfo?(valentin.gosu)
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.
(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.
Flags: needinfo?(valentin.gosu)
Keywords: leave-open

Comment 14

8 months ago
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc56daee035f
Return early from WPAD runnable when pref has been changed r=bagder

Comment 15

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cc56daee035f
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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:
Attachment #9014061 - Flags: approval-mozilla-beta?
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.
Attachment #9014061 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.