1357307 - startup crash in gfxPrefs::CMSMode() with session manager installed

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Cervantes Yu [:cyu] [:cervantes]]

This is found in m-c revision a374c3546993.
When the addon "Session Manager" is installed, the chrome process crashes on startup. gfxPrefs::CMSMode() is called before gfxPlatform is initialized.

STR:
* Create a clean profile and install session manager.
* Restart the browser and then the chrome process crashes.

Crash stack:
#0  gfxPrefs::CMSMode () at gfx/thebes/gfxPrefs.h:406
#1  gfxPlatform::GetCMSMode () at gfx/thebes/gfxPlatform.cpp:1815
#2  0x00007fffe96b8835 in nsXPLookAndFeel::GetColorImpl (this=0x7fffe274fdb0, aID=<optimized out>, aUseStandinsForNativeColors=<optimized out>, aResult=@0x7fffffff5140: 4290954696) at widget/nsXPLookAndFeel.cpp:823
#3  0x00007fffe96b8ac6 in mozilla::LookAndFeel::GetColor (aID=<optimized out>, aUseStandinsForNativeColors=<optimized out>, aResult=aResult@entry=0x7fffffff5140) at widget/nsXPLookAndFeel.cpp:929
#4  0x00007fffe98e17b3 in (anonymous namespace)::GetEnumColorValue (aIsChrome=<optimized out>, aKeyword=<optimized out>) at layout/style/nsCSSParser.cpp:6696
#5  (anonymous namespace)::CSSParserImpl::ParseColor (this=this@entry=0x7fffd8191000, aValue=...) at layout/style/nsCSSParser.cpp:6749
#6  0x00007fffe98da0ec in (anonymous namespace)::CSSParserImpl::ParseVariant (this=this@entry=0x7fffd8191000, aValue=..., aVariantMask=aVariantMask@entry=8, aKeywordTable=aKeywordTable@entry=0x0) at layout/style/nsCSSParser.cpp:7893
#7  0x00007fffe98f39f3 in (anonymous namespace)::CSSParserImpl::ParseBorderColors (aProperty=eCSSProperty__moz_border_top_colors, this=0x7fffd8191000) at layout/style/nsCSSParser.cpp:13499
#8  (anonymous namespace)::CSSParserImpl::ParsePropertyByFunction (this=this@entry=0x7fffd8191000, aPropID=aPropID@entry=eCSSProperty__moz_border_top_colors) at layout/style/nsCSSParser.cpp:11654
#9  0x00007fffe98f5642 in (anonymous namespace)::CSSParserImpl::ParseProperty (this=this@entry=0x7fffd8191000, aPropID=aPropID@entry=eCSSProperty__moz_border_top_colors) at layout/style/nsCSSParser.cpp:11426
#10 0x00007fffe98f7aa7 in (anonymous namespace)::CSSParserImpl::ParseDeclaration (this=this@entry=0x7fffd8191000, aDeclaration=aDeclaration@entry=0x7fffd8e2f200, aFlags=aFlags@entry=3, aChanged=aChanged@entry=0x7fffffff5b57, aContext=aContext@entry=(anonymous namespace)::CSSParserImpl::eCSSContext_General, aMustCallValueAppended=true) at layout/style/nsCSSParser.cpp:7260
#11 0x00007fffe98f8529 in (anonymous namespace)::CSSParserImpl::ParseDeclarationBlock (this=this@entry=0x7fffd8191000, aFlags=aFlags@entry=3, aContext=aContext@entry=(anonymous namespace)::CSSParserImpl::eCSSContext_General) at layout/style/nsCSSParser.cpp:6664
#12 0x00007fffe98fc72a in (anonymous namespace)::CSSParserImpl::ParseRuleSet (this=this@entry=0x7fffd8191000, aAppendFunc=aAppendFunc@entry=0x7fffe98a6420 <(anonymous namespace)::AppendRuleToSheet(mozilla::css::Rule*, void*)>, aData=aData@entry=0x7fffd8191000, aInsideBraces=aInsideBraces@entry=false) at layout/style/nsCSSParser.cpp:5406
#13 0x00007fffe98fda6f in (anonymous namespace)::CSSParserImpl::ParseSheet (this=0x7fffd8191000, aInput=u"#sessionmanager-menu menuitem[_id=\"windows\"],\r\n#sessionmanager-appmenu menuitem[_id=\"windows\"],\r\n#sessionmanager-toolbar menuitem[_id=\"windows\"],\r\n#sessionmanager-undo menuitem[_id=\"windows\"],\r\n#sessi"..., aSheetURI=aSheetURI@entry=0x7fffd7e20150, aBaseURI=aBaseURI@entry=0x7fffd7e20150, aSheetPrincipal=0x7fffe3fa8f60, aLineNumber=aLineNumber@entry=1, aReusableSheets=0x0) at layout/style/nsCSSParser.cpp:1762
#14 0x00007fffe98fdc0d in nsCSSParser::ParseSheet (this=this@entry=0x7fffffff5e00, aInput=u"#sessionmanager-menu menuitem[_id=\"windows\"],\r\n#sessionmanager-appmenu menuitem[_id=\"windows\"],\r\n#sessionmanager-toolbar menuitem[_id=\"windows\"],\r\n#sessionmanager-undo menuitem[_id=\"windows\"],\r\n#sessi"..., aSheetURI=aSheetURI@entry=0x7fffd7e20150, aBaseURI=aBaseURI@entry=0x7fffd7e20150, aSheetPrincipal=<optimized out>, aLineNumber=aLineNumber@entry=1, aReusableSheets=0x0) at layout/style/nsCSSParser.cpp:17923
#15 0x00007fffe985faa0 in mozilla::css::Loader::ParseSheet (this=0x7fffd7f5d540, aInput=u"#sessionmanager-menu menuitem[_id=\"windows\"],\r\n#sessionmanager-appmenu menuitem[_id=\"windows\"],\r\n#sessionmanager-toolbar menuitem[_id=\"windows\"],\r\n#sessionmanager-undo menuitem[_id=\"windows\"],\r\n#sessi"..., aLoadData=aLoadData@entry=0x7fffd7f5e080, aCompleted=@0x7fffffff5f50: false) at layout/style/Loader.cpp:1777
#16 0x00007fffe98617f8 in mozilla::css::SheetLoadData::OnStreamComplete (this=0x7fffd7f5e080, aLoader=<optimized out>, aContext=<optimized out>, aStatus=<optimized out>, aBuffer=u"#sessionmanager-menu menuitem[_id=\"windows\"],\r\n#sessionmanager-appmenu menuitem[_id=\"windows\"],\r\n#sessionmanager-toolbar menuitem[_id=\"windows\"],\r\n#sessionmanager-undo menuitem[_id=\"windows\"],\r\n#sessi"...) at layout/style/Loader.cpp:998
#17 0x00007fffe6b4e4bc in nsUnicharStreamLoader::OnStopRequest (this=0x7fffd80f6a00, aRequest=<optimized out>, aContext=<optimized out>, aStatus=<optimized out>) at netwerk/base/nsUnicharStreamLoader.cpp:97
#18 0x00007fffe7fb4897 in nsSyncLoadService::PushSyncStreamToListener (aIn=0x7fffd8e40b48, aListener=0x7fffd80f6a00, aChannel=0x7fffd8183b00) at dom/base/nsSyncLoadService.cpp:393
#19 0x00007fffe985c56a in mozilla::css::Loader::LoadSheet (this=this@entry=0x7fffd7f5d540, aLoadData=aLoadData@entry=0x7fffd7f5e080, aSheetState=<optimized out>, aIsPreload=aIsPreload@entry=false) at layout/style/Loader.cpp:1550
#20 0x00007fffe985ea97 in mozilla::css::Loader::InternalLoadNonDocumentSheet (this=this@entry=0x7fffd7f5d540, aURL=aURL@entry=0x7fffd7e20150, aIsPreload=aIsPreload@entry=false, aParsingMode=aParsingMode@entry=mozilla::css::eUserSheetFeatures, aUseSystemPrincipal=aUseSystemPrincipal@entry=true, aOriginPrincipal=aOriginPrincipal@entry=0x0, aCharset=<empty_buffer> "", aSheet=0x7fffffff6630, aObserver=0x0, aCORSMode=mozilla::CORS_NONE, aReferrerPolicy=mozilla::net::RP_Unset, aIntegrity=<empty_buffer> u"") at layout/style/Loader.cpp:2446
#21 0x00007fffe985eecf in mozilla::css::Loader::LoadSheetSync (this=this@entry=0x7fffd7f5d540, aURL=aURL@entry=0x7fffd7e20150, aParsingMode=aParsingMode@entry=mozilla::css::eUserSheetFeatures, aUseSystemPrincipal=aUseSystemPrincipal@entry=true, aSheet=aSheet@entry=0x7fffffff6630) at layout/style/Loader.cpp:2333
#22 0x00007fffe9a83567 in LoadSheet (aType=mozilla::StyleBackendType::Gecko, aResult=0x7fffffff6630, aParsingMode=mozilla::css::eUserSheetFeatures, aURI=0x7fffd7e20150) at layout/base/nsStyleSheetService.cpp:223
#23 nsStyleSheetService::LoadAndRegisterSheetInternal (this=this@entry=0x7fffd8128a00, aSheetURI=aSheetURI@entry=0x7fffd7e20150, aSheetType=1) at layout/base/nsStyleSheetService.cpp:256
#24 0x00007fffe9a8cd29 in nsStyleSheetService::LoadAndRegisterSheet (this=0x7fffd8128a00, aSheetURI=0x7fffd7e20150, aSheetType=1) at layout/base/nsStyleSheetService.cpp:176
#25 0x00007fffe6a290f6 in NS_InvokeByIndex () at xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:106
#26 0x00007fffe76f42a9 in CallMethodHelper::Invoke (this=0x7fffffff68a0) at js/xpconnect/src/XPCWrappedNative.cpp:2010
#27 CallMethodHelper::Call (this=0x7fffffff68a0) at js/xpconnect/src/XPCWrappedNative.cpp:1329
#28 XPCWrappedNative::CallMethod (ccx=..., mode=mode@entry=XPCWrappedNative::CALL_METHOD) at js/xpconnect/src/XPCWrappedNative.cpp:1296
#29 0x00007fffe76f852d in XPC_WN_CallMethod (cx=<optimized out>, argc=<optimized out>, vp=0x7fffdbaba5a0) at js/xpconnect/src/XPCWrappedNativeJSOps.cpp:983
#30 0x00007fffeb039bcf in js::CallJSNative (cx=cx@entry=0x7fffe27e6000, native=0x7fffe76f82a0 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:291
#31 0x00007fffeb02f613 in js::InternalCallOrConstruct (cx=0x7fffe27e6000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:455
#32 0x00007fffeb02199b in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:506
#33 Interpret (cx=0x7fffe27e6000, state=...) at js/src/vm/Interpreter.cpp:3010
#34 0x00007fffeb02f1c2 in js::RunScript (cx=0x7fffe27e6000, state=...) at js/src/vm/Interpreter.cpp:395
#35 0x00007fffeb02f747 in js::InternalCallOrConstruct (cx=cx@entry=0x7fffe27e6000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:473
#36 0x00007fffeb02fa28 in InternalCall (cx=cx@entry=0x7fffe27e6000, args=...) at js/src/vm/Interpreter.cpp:500
#37 0x00007fffeb02fb5d in js::Call (cx=cx@entry=0x7fffe27e6000, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:519
#38 0x00007fffeb51c819 in js::Wrapper::call (this=this@entry=0x7fffed783400 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x7fffe27e6000, proxy=..., proxy@entry=..., args=...) at js/src/proxy/Wrapper.cpp:165
#39 0x00007fffeb507292 in js::CrossCompartmentWrapper::call (this=0x7fffed783400 <js::CrossCompartmentWrapper::singleton>, cx=0x7fffe27e6000, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:353
#40 0x00007fffeb514a48 in js::Proxy::call (cx=cx@entry=0x7fffe27e6000, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:464
#41 0x00007fffeb514b3c in js::proxy_Call (cx=0x7fffe27e6000, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:716
#42 0x00007fffeb039bcf in js::CallJSNative (cx=cx@entry=0x7fffe27e6000, native=0x7fffeb514ac0 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:291
#43 0x00007fffeb02f93d in js::InternalCallOrConstruct (cx=cx@entry=0x7fffe27e6000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:437
#44 0x00007fffeb02fa28 in InternalCall (cx=cx@entry=0x7fffe27e6000, args=...) at js/src/vm/Interpreter.cpp:500
#45 0x00007fffeb02fb2a in js::CallFromStack (cx=cx@entry=0x7fffe27e6000, args=...) at js/src/vm/Interpreter.cpp:506
#46 0x00007fffeb0fa5f6 in js::jit::DoCallFallback (cx=0x7fffe27e6000, frame=0x7fffffff7d08, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffff7c70, res=...) at js/src/jit/BaselineIC.cpp:2452
#47 0x00001d5dba482f6e in ?? ()
#48 0x00007fffffff7c98 in ?? ()
#49 0x00007fffffff7c28 in ?? ()
#50 0x0000000000000000 in ?? ()
(gdb) p gfxPrefs::sInstance
$1 = (gfxPrefs *) 0x0

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

More info about the crash:
- This is on debug build.
- Release build doesn't have this crash. I think gfxPrefs::CMSMode() is called in debug-only code.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #1)
> ...
> - Release build doesn't have this crash. I think gfxPrefs::CMSMode() is
> called in debug-only code.

That would be surprising.

It just means that the prefs aren't initialized when we call this method.  They will self-initialize, which is why this doesn't crash in non-debug.  However, we prefer to explicitly initialize gfxPrefs, to ensure that this is done on the main thread.  If we were to just lazy initialize, making sure it's on the main thread becomes more difficult.

In other words, it's still a problem, even though it's debug only crash.  Given that there is a new path for initialization, with this add-on, we should probably just explicitly call gfxPrefs::GetSingleton() in nsXPLookAndFeel.cpp, in a place where we know we're on the main thread.

Comment 3

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1357307-Make-sure-gfxPrefs-instance-is-created-b.patch

(In reply to Milan Sreckovic [:milan] from comment #2)

> In other words, it's still a problem, even though it's debug only crash. 
> Given that there is a new path for initialization, with this add-on, we
> should probably just explicitly call gfxPrefs::GetSingleton() in
> nsXPLookAndFeel.cpp, in a place where we know we're on the main thread.

I think you are right from looked into the backtrace of crash point. 
With this add-on. a new initial patch caused Gecko call the static function GetCMSMode() in [1] before gfxPlatform was created.

[1]: http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/widget/nsXPLookAndFeel.cpp#823

The attached patch intends to get this Singleton instance before using it. Could you please have a review? Really thanks.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8859904 [details] [diff] [review]
0001-Bug-1357307-Make-sure-gfxPrefs-instance-is-created-b.patch

Review of attachment 8859904 [details] [diff] [review]:
-----------------------------------------------------------------

great, thanks.  I would add MOZ_ASSERT(NS_IsMainThread()) just before the call as well.

Comment 5

[Vincent Liu[:vliu]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&tochange=23f8f980b8453737e90ce3fb226d9bed8e27df49&fromchange=737cb4827d9e2ab839a9ffa930905e323ee93e37&selectedJob=93249610

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2fe5e3f78c
Make sure gfxPrefs instance is created before accessing it. r=milan

Comment 7

[Iris Hsiao [:ihsiao]]

sorry had to back this out for assertion failure at nsXPLookAndFeel.cpp like https://treeherder.mozilla.org/logviewer.html#?job_id=93648187&repo=mozilla-inbound&lineNumber=1844

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6efe7915f97b
Backed out changeset 9f2fe5e3f78c for assertion failure at nsXPLookAndFeel.cpp

Comment 9

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1357307-Make-sure-gfxPrefs-instance-is-created-b.patch

Hi Milan,
Since the patch hit the mainthread assert check on linux64-style build, I would rewrite the patch and put gfxPrefs::GetSingleton() in nsXPLookAndFeel::Init(). In it, it already had mainthread assert check in [1] so it should be fine. Could you please have a review for the patch? Really thanks

[1]: http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/widget/nsXPLookAndFeel.cpp#446

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8861288 [details] [diff] [review]
0001-Bug-1357307-Make-sure-gfxPrefs-instance-is-created-b.patch

Review of attachment 8861288 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this will work; we can't call gfxPrefs before the preference system has been initialized, and this is probably too soon for it, judging by the comments in the code.  Welcome to the fun of main thread preferences :)

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Bobby, has stylo work relaxed some of the "preferences must be initialized and set on main thread only"?  This feels like it's stylo related, and this line within Preferences::InitStaticMembers:
 MOZ_ASSERT(NS_IsMainThread() || mozilla::ServoStyleSet::IsInServoTraversal())
makes me think things aren't as they used to be?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Milan Sreckovic [:milan] from comment #11)
> Bobby, has stylo work relaxed some of the "preferences must be initialized
> and set on main thread only"?  This feels like it's stylo related, and this
> line within Preferences::InitStaticMembers:
>  MOZ_ASSERT(NS_IsMainThread() ||
> mozilla::ServoStyleSet::IsInServoTraversal())
> makes me think things aren't as they used to be?

Stylo is allowed to read prefs (but not write to anything) from its worker threads, because the main thread is blocked during the style traversal. That's why you'll see a few relaxed asserts of the form above.

Let me know if any of that doesn't make sense. I'm not sure how that impacts the crash here.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Reading off main thread is fine; it was more the initialization, where https://dxr.mozilla.org/mozilla-central/source/modules/libpref/Preferences.cpp#482 suggests that Preferences::InitStaticMembers is OK off main thread as long as "IsInServoTraversal" is OK.

As for the connection - nothing solid right now, making sure we know if something has changed.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Milan Sreckovic [:milan] from comment #13)
> Reading off main thread is fine; it was more the initialization, where
> https://dxr.mozilla.org/mozilla-central/source/modules/libpref/Preferences.
> cpp#482 suggests that Preferences::InitStaticMembers is OK off main thread
> as long as "IsInServoTraversal" is OK.

It's a "initialize if not already initialized" thing, and the actual branch where we do lazy initialization asserts main thread. I agree it would be better if this code were to use explicit initialization (though pref initialization is hairy, so I understand why Nathan did it that way).

Comment 15

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1357307-Make-sure-gfxPrefs-instance-is-created-b.patch

Hi Milan,

Do you think the attached patch is fine to fix the issue?

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8863634 [details] [diff] [review]
0001-Bug-1357307-Make-sure-gfxPrefs-instance-is-created-b.patch

Review of attachment 8863634 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder if we even need the GetSingleton() call, but it doesn't hurt.

Comment 17

[Vincent Liu[:vliu]]

try:https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c386649316f465996d1edac681b802ce66a1e93

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33df8c04309c
Make sure gfxPrefs instance is created before accessing it. r=milan

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33df8c04309c