693110 - crash nsWindowsShellService::GetShouldCheckDefaultBrowser

Status: RESOLVED FIXED

Description

[Makoto Kato [:m_kato]]

If Firefox cannot get prefs due to any error, firefox will crash.

This bug was filed from the Socorro interface and is 
report bp-49b7eb0f-bf45-4f81-b2d0-a199b2111008 .
============================================================= 
0 	browsercomps.dll 	nsWindowsShellService::GetShouldCheckDefaultBrowser 	browser/components/shell/src/nsWindowsShellService.cpp:455
1 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
2 	xul.dll 	XPC_WN_GetterSetter 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1678
3 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:630
4 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:680
5 	mozjs.dll 	js::InvokeGetterOrSetter 	js/src/jsinterp.cpp:717
6 	mozjs.dll 	js::Shape::get 	js/src/jsscopeinlines.h:279
7 	mozjs.dll 	js_GetPropertyHelper 	js/src/jsobj.cpp:5929
8 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:3535
9 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:585
10 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:648
11 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:680
12 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5124

Comment 1

[Brian R. Bondy [:bbondy]]

Created attachment 566077 [details] [diff] [review]
Patch for crash fix by error checking v1.

Comment 2

[Jim Mathies [:jimm]]

If we're going to overhaul this, can't we use the prefs helpers we have?

Also, rstrong should probably review this, don't believe I'm peer in here.

Comment 3

[Brian R. Bondy [:bbondy]]

> Also, rstrong should probably review this, don't believe I'm peer in here.

We were both added into peers for /browser Firefox module, so I think we can both do reviews here.

> If we're going to overhaul this, can't we use the prefs helpers we have?

Wasn't really an overhaul, just added error checking.  There are a couple of other uses of the old preferences too in the file that I wasn't going to touch as they already had proper error checking.  If you want though I can change everything to use the new preferences code.

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 566077 [details] [diff] [review]
Patch for crash fix by error checking v1.

Ok, sound reasoning. Let's fix the crash and file a follow up bug on migrating the whole file to the new preferences code. 

nit - 

> +  rv = prefs->GetBoolPref(PREF_CHECKDEFAULTBROWSER, aResult);
> +  return rv;

return prefs->...

Comment 5

[Brian R. Bondy [:bbondy]]

Created attachment 566232 [details] [diff] [review]
Patch for crash fix by error checking v2.

Fixed nit.

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d3804e9f2b65

Comment 6

[Ed Morley [:emorley]]

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

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

> file a follow up bug on
> migrating the whole file to the new preferences code. 

You won't forget?

Comment 8

[Brian R. Bondy [:bbondy]]

It was filed about an hour after that here: bug 693638

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks. We usually list followup bugs as dependencies.

Comment 10

[Brian R. Bondy [:bbondy]]

OK sounds good will do next time, thanks.