1351246 - (xpcshell:39978): Gdk-CRITICAL **: gdk_screen_get_root_window: assertion `GDK_IS_SCREEN (screen)' failed

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Jan Beich]

$ c++ -v
FreeBSD clang version 4.0.0 (tags/RELEASE_400/final 297347) (based on LLVM 4.0.0)
Target: x86_64-unknown-freebsd12.0
Thread model: posix
InstalledDir: /usr/bin

$ pkg info -x \^gtk \^glib
gtk-update-icon-cache-2.24.29
gtk2-2.24.29_3
gtk3-3.18.8_3
glib-2.46.2_4

$ ./mach bootstrap
$ ./mach build
$ ./mach package
[...]
resource://gre/modules/sdk/bootstrap.js
resource://gre/modules/sdk/system/Startup.js
Traceback (most recent call last):
  File "toolkit/mozapps/installer/packager.py", line 397, in <module>
    main()
  File "toolkit/mozapps/installer/packager.py", line 391, in main
    args.source, gre_path, base)
  File "toolkit/mozapps/installer/packager.py", line 165, in precompile_cache
    errors.fatal('Error while running startup cache precompilation')
  File "python/mozbuild/mozpack/errors.py", line 103, in fatal
    self._handle(self.FATAL, msg)
  File "python/mozbuild/mozpack/errors.py", line 98, in _handle
    raise ErrorMessage(msg)
mozpack.errors.ErrorMessage: Error: Error while running startup cache precompilation
gmake[3]: *** [toolkit/mozapps/installer/packager.mk:41: stage-package] Error 1
gmake[3]: Leaving directory 'obj-x86_64-unknown-freebsd12.0/browser/installer'
[...]
resource://gre/modules/sdk/bootstrap.js
resource://gre/modules/sdk/system/Startup.js
Process 9405 stopped
* thread #1, stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: libgdk-3.so.0`gdk_screen_get_root_window(screen=0x0000000000000000) at gdkscreen.c:695
   692  {
   693    g_return_val_if_fail (GDK_IS_SCREEN (screen), NULL);
   694
-> 695    return GDK_SCREEN_GET_CLASS (screen)->get_root_window (screen);
   696  }
   697
   698  /**
(lldb) bt
* thread #1, stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
  * frame #0: libgdk-3.so.0`gdk_screen_get_root_window(screen=0x0000000000000000) at gdkscreen.c:695
    frame #1: libgdk-3.so.0`gdk_get_default_root_window at gdkwindow.c:4798
    frame #2: libxul.so`mozilla::widget::ScreenHelperGTK::ScreenHelperGTK(this=0x000000081a99afa0) at ScreenHelperGTK.cpp:79
    frame #3: libxul.so`nsAppShell::Init(void) [inlined] _ZN7mozilla10MakeUniqueINS_6widget15ScreenHelperGTKEJEEENS_6detail14UniqueSelectorIT_E12SingleObjectEDpOT0_ at UniquePtr.h:680
    frame #4: libxul.so`nsAppShell::Init(this=0x000000081a940e00) at nsAppShell.cpp:173
    frame #5: libxul.so`nsAppShellInit(void) at nsAppShellSingleton.h:45
    frame #6: libxul.so`nsFactoryEntry::GetFactory(void) at nsComponentManager.cpp:814
    frame #7: libxul.so`nsFactoryEntry::GetFactory(this=0x00000008114f8c20) at nsComponentManager.cpp:1836
    frame #8: libxul.so`nsComponentManagerImpl::CreateInstance(this=0x00000008114943c0, aClass=<unavailable>, aDelegate=0x0000000000000000, aIID=<unavailable>, aResult=0x00007fffffffbb10) at nsComponentManager.cpp:1055
    frame #9: libxul.so`nsComponentManagerImpl::GetService(this=0x00000008114943c0, aClass=0x0000000805617de8, aIID=<unavailable>, aResult=0x00007fffffffbb80) at nsComponentManager.cpp:1300
    frame #10: libxul.so`nsGetServiceByCIDWithError::operator()(nsID const&, void**) const [inlined]CallGetService(aCID=<unavailable>) at nsComponentManagerUtils.cpp:56
    frame #11: libxul.so`nsGetServiceByCIDWithError::operator(this=0x00007fffffffbba8, aIID=<unavailable>, aInstancePtr=0x00007fffffffbb80)(nsID const&, void**) const at nsComponentManagerUtils.cpp:265
    frame #12: libxul.so`nsCOMPtr_base::assign_from_gs_cid_with_error(this=0x0000000819ecedc0, aGS=<unavailable>, aIID=<unavailable>) at nsCOMPtr.cpp:84
    frame #13: libxul.so`nsAppStartup::Init(void) [inlined] nsCOMPtr<nsIAppShell>::operator=(aRhs=<unavailable>) at nsCOMPtr.h:652
    frame #14: libxul.so`nsAppStartup::Init(this=0x0000000819eced90) at nsAppStartup.cpp:158
    frame #15: libxul.so`nsAppStartupConstructor(aOuter=<unavailable>, aIID=0x000000081158df50, aResult=0x00007fffffffbc70) at nsToolkitCompsModule.cpp:65
    frame #16: libxul.so`nsComponentManagerImpl::CreateInstance(this=<unavailable>, aClass=<unavailable>, aDelegate=0x0000000000000000, aIID=<unavailable>, aResult=0x00007fffffffbc70) at nsComponentManager.cpp:1057
    frame #17: libxul.so`nsComponentManagerImpl::GetService(this=0x00000008114943c0, aClass=0x000000081a94a630, aIID=<unavailable>, aResult=0x00007fffffffbcf0) at nsComponentManager.cpp:1300
    frame #18: libxul.so`JSContext* nsJSCID::GetService(this=0x000000081a99aea0, iidval=<unavailable>, cx=<unavailable>, optionalArgc=<unavailable>, retval=<unavailable>) at XPCJSID.cpp:695
    frame #19: libxul.so`NS_InvokeByIndex at xptcinvoke_asm_x86_64_unix.S:115
    frame #20: libxul.so`XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [inlined] CallMethodHelper::Invoke(void) at XPCWrappedNative.cpp:2010
    frame #21: libxul.so`XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [inlined] CallMethodHelper::Call(void) at XPCWrappedNative.cpp:1329
    frame #22: libxul.so`XPCWrappedNative::CallMethod(ccx=<unavailable>, mode=<unavailable>) at XPCWrappedNative.cpp:1296
    frame #23: libxul.so`XPC_WN_CallMethod(cx=<unavailable>, argc=1, vp=0x0000000814efd288) at XPCWrappedNativeJSOps.cpp:983
    frame #24: libxul.so`js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [inlined] js::CallJSNative(native=(libxul.so`XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) at XPCWrappedNativeJSOps.cpp:962))(JSContextunsigned intJS::Value*), JSContext*::CallArgs const&) at jscntxtinlines.h:282
    frame #25: libxul.so`js::InternalCallOrConstruct(cx=0x0000000811422800, args=0x00007fffffffc350,construct=<unavailable>) at Interpreter.cpp:454
    frame #26: libxul.so`Interpret(JSContext*, js::RunState&) [inlined] js::CallFromStack(cx=0x0000000811422800) at Interpreter.cpp:505
    frame #27: libxul.so`Interpret(cx=<unavailable>, state=<unavailable>) at Interpreter.cpp:2998
    frame #28: libxul.so`js::RunScript(cx=0x0000000811422800, state=0x00007fffffffc6a0) at Interpreter.cpp:394
    frame #29: libxul.so`JSObject& js::ExecuteKernel(cx=<unavailable>, script=<unavailable>, envChainArg=<unavailable>, newTargetValue=<unavailable>, evalInFrame=<unavailable>, result=<unavailable>) atInterpreter.cpp:683
    frame #30: libxul.so`JSObject& js::Execute(cx=0x0000000811422800, script=<unavailable>, envChainArg=<unavailable>, rval=0x0000000000000000) at Interpreter.cpp:715
    frame #31: libxul.so`JS_ExecuteScript(cx=0x0000000811422800, scriptArg=<unavailable>) at jsapi.cpp:4539
    frame #32: libxul.so`JS<JSScript*> mozJSComponentLoader::ObjectForLocation(this=0x00000008115d4900, aInfo=<unavailable>, aComponentFile=0x000000081a94a530, aObject=<unavailable>, aTableScript=<unavailable>, aLocation=0x000000081a8d3128, aPropagateExceptions=<unavailable>, aException=<unavailable>)at mozJSComponentLoader.cpp:969
    frame #33: libxul.so`JSContext* mozJSComponentLoader::ImportInto(this=<unavailable>, aLocation=<unavailable>, targetObj=<unavailable>, callercx=<unavailable>, vp=<unavailable>) at mozJSComponentLoader.cpp:1200
    frame #34: libxul.so`JSContext* mozJSComponentLoader::Import(this=<unavailable>, registryLocation=<unavailable>, targetValArg=<unavailable>, cx=0x0000000811422800, optionalArgc=<unavailable>, retval=<unavailable>) at mozJSComponentLoader.cpp:1076
    frame #35: libxul.so`JSContext* nsXPCComponents_Utils::Import(this=<unavailable>, registryLocation=0x000000081146c030, targetObj=JS::HandleValue @ r12, cx=0x0000000811422800, optionalArgc='\x01', retval=JS::MutableHandleValue @ r14) at XPCComponents.cpp:2504
    frame #36: libxul.so`NS_InvokeByIndex at xptcinvoke_asm_x86_64_unix.S:115
    frame #37: libxul.so`XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [inlined] CallMethodHelper::Invoke(void) at XPCWrappedNative.cpp:2010
    frame #38: libxul.so`XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [inlined] CallMethodHelper::Call(void) at XPCWrappedNative.cpp:1329
    frame #39: libxul.so`XPCWrappedNative::CallMethod(ccx=<unavailable>, mode=<unavailable>) at XPCWrappedNative.cpp:1296
    frame #40: libxul.so`XPC_WN_CallMethod(cx=<unavailable>, argc=2, vp=0x00007fffffffd238) at XPCWrappedNativeJSOps.cpp:983
    frame #41: 0x000019050efea2c4
    frame #42: 0x0000000814fc49c0
    frame #43: 0x000019050efd58a6
    frame #44: libxul.so`EnterBaseline(cx=0x000000081219e2e0, data=0xfffe00081219e2e0) at BaselineJIT.cpp:160
    frame #45: libxul.so`js::jit::EnterBaselineAtBranch(cx=0x0000000811422800, fp=0x0000000814efd140, pc=<unavailable>) at BaselineJIT.cpp:268
    frame #46: libxul.so`Interpret(cx=<unavailable>, state=<unavailable>) at Interpreter.cpp:1963
    frame #47: libxul.so`js::RunScript(cx=0x0000000811422800, state=0x00007fffffffdb60) at Interpreter.cpp:394
    frame #48: libxul.so`JSObject& js::ExecuteKernel(cx=<unavailable>, script=<unavailable>, envChainArg=<unavailable>, newTargetValue=<unavailable>, evalInFrame=<unavailable>, result=<unavailable>) atInterpreter.cpp:683
    frame #49: libxul.so`JSObject& js::Execute(cx=0x0000000811422800, script=<unavailable>, envChainArg=<unavailable>, rval=0x00007fffffffe0a0) at Interpreter.cpp:715
    frame #50: libxul.so`JS::Handle::ReadOnlyCompileOptions const& Evaluate(cx=0x0000000811422800, scopeKind=Global, env=<unavailable>, optionsArg=0x00007fffffffdfd0, srcBuf=0x00007fffffffdd80, rval=<unavailable>) at jsapi.cpp:4595
    frame #51: libxul.so`JS::Evaluate(cx=0x0000000811422800, options=0x00007fffffffdfd0, bytes=<unavailable>, length=<unavailable>, rval=JS::MutableHandleValue @ r14) at jsapi.cpp:4648
    frame #52: libxul.so`XRE_XPCShellMain(int, char**, char**, XREShellData const*) at XPCShellImpl.cpp:1139
    frame #53: libxul.so`XRE_XPCShellMain(argc=<unavailable>, argv=<unavailable>, envp=<unavailable>, aShellData=<unavailable>) at XPCShellImpl.cpp:1612
    frame #54: xpcshell`main(argc=<unavailable>, argv=<unavailable>, envp=0x00007fffffffe2b8) at xpcshell.cpp:68
    frame #55: xpcshell`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:72
(lldb) f 2
frame #2: libxul.so`mozilla::widget::ScreenHelperGTK::ScreenHelperGTK(this=0x000000081a99afa0) at ScreenHelperGTK.cpp:79
   76     , mNetWorkareaAtom(0)
   77   {
   78     MOZ_LOG(sScreenLog, LogLevel::Debug, ("ScreenHelperGTK created"));
-> 79     mRootWindow = gdk_get_default_root_window();
   80     if (!mRootWindow) {
   81       // Sometimes we don't initial X (e.g., xpcshell)
   82       MOZ_LOG(sScreenLog, LogLevel::Debug, ("mRootWindow is nullptr, running headless"));
(lldb) fr v
(mozilla::widget::ScreenHelperGTK *) this = 0x000000081a99afa0
(lldb) p *this
(mozilla::widget::ScreenHelperGTK) $1 = {
  mXineramalib = 0x0000000000000000
  mRootWindow = 0x0000000000000000
  mNetWorkareaAtom = 0
}
(lldb) f 4
frame #4: libxul.so`nsAppShell::Init(this=0x000000081a940e00) at nsAppShell.cpp:173
   170
   171      if (XRE_IsParentProcess()) {
   172          ScreenManager& screenManager = ScreenManager::GetSingleton();
-> 173          screenManager.SetHelper(mozilla::MakeUnique<ScreenHelperGTK>());
   174      }
   175
   176  #if MOZ_WIDGET_GTK == 3

Comment 1

[Jan Beich]

mozilla-central changeset 4be4367d022d is the first bad commit

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Is it FreeBSD specific? Are you running |./mach package| without X or headless? I cannot reproduce locally.

Karl, do you know what should I check to avoid this?

Comment 3

[Jan Beich]

(In reply to Jan Beich from comment #0)
> -> 79     mRootWindow = gdk_get_default_root_window();
>    80     if (!mRootWindow) {
>    81       // Sometimes we don't initial X (e.g., xpcshell)

Checking gdk_screen_get_default() appears to fix the crash.

(In reply to Kan-Ru Chen [:kanru] (UTC-4, EDT) from comment #2)
> Is it FreeBSD specific?

I'm not sure. It could be specific to my environment as well.

> Are you running |./mach package| without X or headless? I cannot reproduce locally.

No, I'm building from X11-connected terminal. DISPLAY is defined in environment.

Comment 4

[Jan Beich]

g_return_val_if_fail(0, NULL) is nop with -O or higher which means calling gdk_get_default_root_window() is unsafe without checking if screen is available.

Comment 5

[Karl Tomlinson (:karlt)]

g_return_val_if_fail() is for programming errors only.  It may not be a no-op if libgtk is configured with non-default options.

The programming error here may be in the design of gdk_get_default_root_window().

The workaround is to use gdk_screen_get_default() and gdk_screen_get_root_window().
The result of only the former need be null-checked.

gdk_screen_get_default() is already called in this method.

(Another thing this highlights is that the screen manager is now initialized on start-up, instead of lazily when required, but I don't know whether there is a good way to change that.)

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 1351246 - Check gdk_screen_get_default() for X.

Review commit: https://reviewboard.mozilla.org/r/124674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124674/

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8852442 [details]
Bug 1351246 - Check gdk_screen_get_default() for X.

https://reviewboard.mozilla.org/r/124674/#review127418

Comment 8

[Jan Beich]

Comment on attachment 8852442 [details]
Bug 1351246 - Check gdk_screen_get_default() for X.

https://reviewboard.mozilla.org/r/124674/#review127496

::: widget/gtk/ScreenHelperGTK.cpp:85
(Diff revision 1)
> -  if (!mRootWindow) {
> +  if (!defaultScreen) {
>      // Sometimes we don't initial X (e.g., xpcshell)
>      MOZ_LOG(sScreenLog, LogLevel::Debug, ("mRootWindow is nullptr, running headless"));
>      return;
>    }
> +  mRootWindow = gdk_get_default_root_window();

Maybe replace `gdk_get_default_root_window()` with `gdk_screen_get_root_window(defaultScreen)` to save an extra call to `gdk_screen_get_default()` from within Gtk.

Comment 9

[Jan Beich]

(In reply to Karl Tomlinson (:karlt) from comment #5)
> g_return_val_if_fail() is for programming errors only.  It may not be a
> no-op if libgtk is configured with non-default options.

Gtk+ passes -DG_DISABLE_CHECKS for --disable-debug builds which turns off g_return_val_if_fail() to avoid scaring users with warnings like:

  (xpcshell:73648): Gdk-CRITICAL **: gdk_screen_get_root_window: assertion 'GDK_IS_SCREEN (screen)' failed

https://git.gnome.org/browse/gtk+/diff/configure.in?id=fc7e845c8f

./mach packages shows a few more that maybe unrelated:

  (xpcshell:73648): GLib-GObject-WARNING **: invalid (NULL) pointer instance
  (xpcshell:73648): GLib-GObject-CRITICAL **: g_signal_handlers_disconnect_matched: assertion'G_TYPE_CHECK_INSTANCE (instance)' failed

Comment 10

[Jan Beich]

Comment on attachment 8852442 [details]
Bug 1351246 - Check gdk_screen_get_default() for X.

https://reviewboard.mozilla.org/r/124674/#review127506

::: widget/gtk/ScreenHelperGTK.cpp:109
(Diff revision 1)
>    RefreshScreens();
>  }
>  
>  ScreenHelperGTK::~ScreenHelperGTK()
>  {
>    g_signal_handlers_disconnect_by_func(gdk_screen_get_default(),

Can you add a similar check here for the 2nd assert in comment 9?

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #5)
> It may not be a no-op if libgtk is configured with non-default options.

Too many negatives there, sorry.  I meant it may be a no-op if libgtk is configured with non-default options.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8852442 [details]
Bug 1351246 - Check gdk_screen_get_default() for X.

https://reviewboard.mozilla.org/r/124674/#review127506

> Can you add a similar check here for the 2nd assert in comment 9?

Thanks.  Better to move it to the if (mRootWindow) block.

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8852442 [details]
Bug 1351246 - Check gdk_screen_get_default() for X.

https://reviewboard.mozilla.org/r/124674/#review127496

> Maybe replace `gdk_get_default_root_window()` with `gdk_screen_get_root_window(defaultScreen)` to save an extra call to `gdk_screen_get_default()` from within Gtk.

Yes, that was what I was thinking, but gdk_get_default_root_window() save passing one parameter, so I don't mind which.

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to Jan Beich from comment #9)
> (In reply to Karl Tomlinson (:karlt) from comment #5)
> > g_return_val_if_fail() is for programming errors only.  It may not be a
> > no-op if libgtk is configured with non-default options.
> 
> Gtk+ passes -DG_DISABLE_CHECKS for --disable-debug builds which turns off
> g_return_val_if_fail() to avoid scaring users with warnings like:
> 
>   (xpcshell:73648): Gdk-CRITICAL **: gdk_screen_get_root_window: assertion
> 'GDK_IS_SCREEN (screen)' failed

Those are usually there to protect against the subsequent code going wrong, and so instead of having scared users, users will be confused about broken apps with no messages.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8852442 [details]
Bug 1351246 - Check gdk_screen_get_default() for X.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124674/diff/1-2/

Comment 16

[Jan Beich]

Comment on attachment 8852442 [details]
Bug 1351246 - Check gdk_screen_get_default() for X.

Thank you. With the patch applied "./mach package" succeeds without a crash or gtk/glib assertions here.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fea86135263
Check gdk_screen_get_default() for X. r=karlt

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fea86135263