817568 - crash in ReadInternetOption @ StringCchCopyNW

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-e266e560-e4ed-4c58-91da-838e62121201 .
============================================================= 

Seen while looking at Firefox 18 data. Crash happens more frequently in FF 18.

More reports: https://crash-stats.mozilla.com/report/list?signature=StringCchCopyNW

High correlation to one addon, which appears to be http://www.dealply.com/

StringCchCopyNW|EXCEPTION_ACCESS_VIOLATION_READ (44 crashes)
     48% (21/44) vs.   3% (107/3766) {20a82645-c095-46ed-80e3-08825760534b} (Microsoft .NET Framework Assistant, http://www.windowsclient.net/)
     48% (21/44) vs.   3% (123/3766) {EB9394A3-4AD6-4918-9537-31A1FD8E8EDF}


Frame 	Module 	Signature 	Source
0 	wininet.dll 	StringCchCopyNW 	
1 	wininet.dll 	TransformMD5 	
2 	xul.dll 	ReadInternetOption 	toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp:91
3 	xul.dll 	nsWindowsSystemProxySettings::GetPACURI 	toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp:200
4 	xul.dll 	nsPACMan::ProcessPending 	netwerk/base/src/nsPACMan.cpp:526
5 	xul.dll 	nsPACMan::ProcessPendingQ 	netwerk/base/src/nsPACMan.cpp:489
6 	xul.dll 	nsPACMan::PostQuery 	netwerk/base/src/nsPACMan.cpp:334
7 	xul.dll 	PendingPACQuery::Run 	netwerk/base/src/nsPACMan.cpp:246
8 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:620
9 	xul.dll 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:258
10 	nspr4.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:395
11 	nspr4.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:90
12 	msvcr100.dll 	_callthreadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:314
13 	msvcr100.dll 	_threadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:292
14 	kernel32.dll 	BaseThreadStart 	
https://crash-stats.mozilla.com/report/list?signature=StringCchCopyNW

Comment 1

[Scoobidiver (away)]

(In reply to Marcia Knous [:marcia] from comment #0) 
> High correlation to one addon, which appears to be http://www.dealply.com/
> StringCchCopyNW|EXCEPTION_ACCESS_VIOLATION_READ (44 crashes)
>      48% (21/44) vs.   3% (107/3766) {20a82645-c095-46ed-80e3-08825760534b}
> (Microsoft .NET Framework Assistant, http://www.windowsclient.net/)
>      48% (21/44) vs.   3% (123/3766) {EB9394A3-4AD6-4918-9537-31A1FD8E8EDF}
63% of crashes happen with the same user (same install time).

66% of crashes are on Windows Server 2003 so likely corporate users.

It looks like bug 782034.

Comment 2

[Scoobidiver (away)]

It's #65 top browser crasher in 18.0.1 and #20 in 19.0b2 with many duplicates.

Comment 3

[Benjamin Smedberg]

mcmanus, this looks like a PAC bug which could hit us badly in release. Can you take an urgent look?

Comment 4

[Patrick McManus [:mcmanus]]

This is actually in :emk's OS code.. I think he had a prospective patch that didn't get backported until crash stats were looked at. emk?

Comment 5

[Masatoshi Kimura [:emk]]

Dup of bug 829518?

Comment 6

[Alex Keybl [:akeybl]]

If this is in fact a dupe of bug 829518 (where FF18 was affected), there's a good chance we'll wontfix for FF19. How do you plan to verify emk?

Comment 7

[Masatoshi Kimura [:emk]]

Verify what?
I'm fine uplifting bug 829518 to aurora and beta. It's safe enough for branches.

Comment 8

[Robert Kaiser]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> I'm fine uplifting bug 829518 to aurora and beta. It's safe enough for
> branches.

In that case, can you please request approval for that?


And also, I'm not sure that patch really helps the crash here, as it landed on the 14 but here are two crashes I just found on crash-stats that ahppened with the Nightly build from the 24th: bp-79685dd7-6011-4b48-8f04-516ad2130125 bp-ed155a53-1a3e-4bc5-91cf-3bdab2130125

Comment 9

[Masatoshi Kimura [:emk]]

> nsWindowsSystemProxySettings.cpp:72
It looks like I need to enclose broader range with __try...__except.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Enclose more API calls with __try...__except

Comment 11

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14c3c3913e3c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14c3c3913e3c

Comment 13

[Alex Keybl [:akeybl]]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> Verify what?

Verify that this is an issue that already impatcted FF18 (which it appears you already have). Given that, we'll maintain the status quo in FF19 and fix this for the first time in FF20. Please nominate for uplift.

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: Folded bug 829518 patches and a bug 817568 patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 563169
User impact if declined: High volume crash
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: none

Comment 15

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ed165fe28804

Comment 16

[Alex Keybl [:akeybl]]

Marking as fixed given comment 15.

Comment 17

[Benjamin Smedberg]

I am very disappointed in the patches in this bug and bug 829518. SEH from arbitrary crashes is not safe in code such as this, because it can leave structures in half-initialized states, silently leak memory, or even cause deadlocks if there are locked critical sections or other synchronization primitives.

See bug 854176 which suggests that this code should be rewritten using nsINetworkListManager.

Comment 18

[Masatoshi Kimura [:emk]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> I am very disappointed in the patches in this bug and bug 829518. SEH from
> arbitrary crashes is not safe in code such as this, because it can leave
> structures in half-initialized states, silently leak memory, or even cause
> deadlocks if there are locked critical sections or other synchronization
> primitives.

I just followed what plug-in module does of which you are a peer.
https://mxr.mozilla.org/mozilla-central/search?string=TRY_SAFE

> See bug 854176 which suggests that this code should be rewritten using
> nsINetworkListManager.

What is nsINetworkListManager?
https://mxr.mozilla.org/mozilla-central/search?string=nsINetworkListManager

Comment 19

[Benjamin Smedberg]

You'll note that the __try/__except handling was *removed* from plugins long ago, and currently those macros are just used for android reentry handling.

I meant of course INetworkListManager the Windows API mentioned in bug 854176.

Comment 20

[Patrick McManus [:mcmanus]]

we dont need this open with 854176