Closed Bug 1489229 Opened 3 years ago Closed 3 years ago
Crash in mozilla::net::ns
Bug 1489229 - crash on MOZ_RELEASE_ASSERT which checks that WPAD is not being called if not explicitly requested by the user prefs
46 bytes, text/x-phabricator-request
|Details | Review|
46 bytes, text/x-phabricator-request
|Details | Review|
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 firstname.lastname@example.org 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.
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
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.
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 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+
Pushed by email@example.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
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.
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
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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/cc56daee035f Return early from WPAD runnable when pref has been changed r=bagder
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.