1800295 - Shutdown handling of the Viaduct singleton seems bogus

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Jens Stutte [:jstutte]]

I find frequently in my log files:

NS_ENSURE_TRUE(inst) failed: StaticComponents.cpp:xxxx

and following the line in my build leads me to:

    case ModuleID::Anonymous325: {
      RefPtr<mozilla::Viaduct> inst = mozilla::Viaduct::GetSingleton();
      NS_ENSURE_TRUE(inst, NS_ERROR_OUT_OF_MEMORY);

which brings me to Viaduct::GetSingleton where I read:

already_AddRefed<Viaduct> Viaduct::GetSingleton() {
  if (gViaduct) {
    return do_AddRef(gViaduct);
  }

  gViaduct = new Viaduct();
  ClearOnShutdown(&gViaduct);
  return do_AddRef(gViaduct);
}

ClearOnShutdown has the nice side effect to null out the pointer if we are already in/beyond its phase (by default ShutdownPhase::XPCOMShutdownFinal). This is correct in the sense that we should not deliver a new instance such late, but callers seem to not expect this to ever return nullptr. I see two things here:

1. Make it more explicit inside Viaduct::GetSingleton() for which shutdown phase we expect to cease working. We can use an AppShutdown::IsInOrBeyond check for this. Note that it might make sense to not return an instance earlier than we clear it.
2. Check the call sites for proper handling. I can see it used only by bookmarks, IIUC. Nika, the tricky part here is that the message is printed by the generated code, so we might want to tweak/remove that message and leave it to the constructor to complain? We can and should also understand why bookmark's JS runs this late in the first place, though.

Edit: As Nika says below, I mixed up some link here, it seems.

Comment 1

[Jens Stutte [:jstutte]]

ni? to nika for the generated code piece. It might just be ok as is as we get at least a hint something is wrong as seen here.

Comment 2

[Jens Stutte [:jstutte]]

(In reply to Jens Stutte [:jstutte] from comment #0)

1. Make it more explicit inside Viaduct::GetSingleton() for which shutdown phase we expect to cease working. We can use an AppShutdown::IsInOrBeyond check for this. Note that it might make sense to not return an instance earlier than we clear it.

Note that this is meant to also avoid to do the new Viaduct() at all. See bug 1709184 comment 50.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Jens Stutte [:jstutte] from comment #1)

ni? to nika for the generated code piece. It might just be ok as is as we get at least a hint something is wrong as seen here.

I'm open to emitting a more-useful error message from the XPCOM constructor there, but I don't think it's a good idea to remove the message, as there are many instances of code like this, and having an automatically generated warning that something is wrong could help us with situations like this.

The only place which constructs the @mozilla.org/toolkit/viaduct;1 contract ID appears to be glean code, which constructs it in setup_viaduct(): https://searchfox.org/mozilla-central/rev/219df29d0fb5d8928ae41bba4a605046de411cf0/toolkit/components/glean/src/init/mod.rs#217. This caller does validate the return value, although it is using create_instance for some reason, re-invoking GetSingleton() every time, rather than using get_service for some reason? This feels like it was probably caused by a misunderstanding in how xpcom services work when the viaduct crate was introduced, and may be the primary cause of the weird error spam.

I'm not sure what you were referring to by saying it was only used by bookmarks, as the link seems unrelated.

Overall, though, the Viaduct type seems very odd. It is an XPCOM interface which doesn't need to exist, as it just wraps a single Atomic<bool> mInitialized to control whether to call the static viaduct initializer (which is another thing implemented in rust). The entire setup with having a singleton gViaduct could be avoided by instead having the atomic be static so that the Viaduct object doesn't need to be persisted at all. We could also remove the need for the XPCOM interface in that case.

I think the best way forward here is probably to remove gViaduct completely, along with the relevant interfaces, and replace it with something more straightforward which perhaps avoids the multi-language hop completely. Doing so should be quite simple so I'll throw up a patch.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1800295 - Delete the Viaduct XPCOM interface, r=chutten!

This XPCOM interface was being initialized and used, and was unnecessary.
This patch removes it completely to simplify things, initializing
viaduct during xpcom startup instead. This is done rather than keeping
it lazy and tied to FOG startup, as the implementation just sets a
static atomic to a function pointer.

This makes no changes to anything which actually uses viaduct, which is
done through the previously mentioned static atomic.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cea753e22a8e
Delete the Viaduct XPCOM interface, r=chutten,markh

Comment 6

[Iulian Moraru]

Backed out for causing Viaduct related build bustages.

Push with failures

Failure log

Backout link

[task 2022-11-14T22:58:01.793Z] 22:58:01     INFO -  gmake[4]: Entering directory '/builds/worker/workspace/obj-build/layout/build'
[task 2022-11-14T22:58:01.798Z] 22:58:01     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -Qunused-arguments -std=gnu++17 --target=aarch64-linux-android --rtlib=libgcc -o Unified_cpp_layout_build0.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -ftrivial-auto-var-init=pattern -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/layout/build -I/builds/worker/workspace/obj-build/layout/build -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/workspace/obj-build/security/rlbox -I/builds/worker/checkouts/gecko/layout/base -I/builds/worker/checkouts/gecko/layout/forms -I/builds/worker/checkouts/gecko/layout/generic -I/builds/worker/checkouts/gecko/layout/inspector -I/builds/worker/checkouts/gecko/layout/mathml -I/builds/worker/checkouts/gecko/layout/painting -I/builds/worker/checkouts/gecko/layout/style -I/builds/worker/checkouts/gecko/layout/tables -I/builds/worker/checkouts/gecko/layout/xul -I/builds/worker/checkouts/gecko/caps -I/builds/worker/checkouts/gecko/docshell/base -I/builds/worker/checkouts/gecko/dom/audiochannel -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/checkouts/gecko/dom/bindings -I/builds/worker/checkouts/gecko/dom/canvas -I/builds/worker/checkouts/gecko/dom/filesystem -I/builds/worker/checkouts/gecko/dom/geolocation -I/builds/worker/checkouts/gecko/dom/html -I/builds/worker/checkouts/gecko/dom/jsurl -I/builds/worker/checkouts/gecko/dom/media -I/builds/worker/checkouts/gecko/dom/offline -I/builds/worker/checkouts/gecko/dom/storage -I/builds/worker/checkouts/gecko/dom/xslt/base -I/builds/worker/checkouts/gecko/dom/xslt/xml -I/builds/worker/checkouts/gecko/dom/xslt/xpath -I/builds/worker/checkouts/gecko/dom/xslt/xslt -I/builds/worker/checkouts/gecko/dom/xul -I/builds/worker/checkouts/gecko/editor/composer -I/builds/worker/checkouts/gecko/js/xpconnect/loader -I/builds/worker/checkouts/gecko/js/xpconnect/src -I/builds/worker/checkouts/gecko/netwerk/base -I/builds/worker/checkouts/gecko/netwerk/cookie -I/builds/worker/checkouts/gecko/parser/htmlparser -I/builds/worker/checkouts/gecko/toolkit/components/sessionstore -I/builds/worker/checkouts/gecko/view -I/builds/worker/checkouts/gecko/dom/system -I/builds/worker/checkouts/gecko/dom/system/android -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include/aarch64-linux-android -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include --gcc-toolchain=/builds/worker/fetches/android-ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64 -D__ANDROID_API__=21 -fno-sized-deallocation -fno-aligned-new -fno-short-enums -fno-exceptions -fcrash-diagnostics-dir=/builds/worker/artifacts -stdlib=libstdc++ -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++/include -I/builds/worker/fetches/android-ndk/sources/android/support/include -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++abi/include -fno-exceptions -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pipe -gdwarf-4 -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Oz -mno-outline -fno-omit-frame-pointer -funwind-tables -Werror -Wall -Wbitfield-enum-conversion -Wdeprecated-this-capture -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wenum-compare-conditional -Wenum-float-conversion -Wno-ambiguous-reversed-operator -Wno-error=deprecated -Wno-error=deprecated-anon-enum-enum-conversion -Wno-error=deprecated-enum-enum-conversion -Wno-error=deprecated-pragma -Wno-error=deprecated-this-capture -Wno-error=deprecated-volatile -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-error=atomic-alignment -Wno-error=deprecated-builtins -Wformat -Wformat-security -Wno-psabi -Wthread-safety -Wno-unknown-warning-option -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/Unified_cpp_layout_build0.o.pp   Unified_cpp_layout_build0.cpp
[task 2022-11-14T22:58:01.799Z] 22:58:01     INFO -  In file included from Unified_cpp_layout_build0.cpp:20:
[task 2022-11-14T22:58:01.799Z] 22:58:01     INFO -  /builds/worker/checkouts/gecko/layout/build/nsLayoutStatics.cpp:93:10: fatal error: 'mozilla/Viaduct.h' file not found
[task 2022-11-14T22:58:01.799Z] 22:58:01     INFO -  #include "mozilla/Viaduct.h"
[task 2022-11-14T22:58:01.799Z] 22:58:01     INFO -           ^~~~~~~~~~~~~~~~~~~
[task 2022-11-14T22:58:01.799Z] 22:58:01     INFO -  1 error generated.
[task 2022-11-14T22:58:01.800Z] 22:58:01    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:669: Unified_cpp_layout_build0.o] Error 1
[task 2022-11-14T22:58:01.800Z] 22:58:01     INFO -  gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/layout/build'
[task 2022-11-14T22:58:01.800Z] 22:58:01     INFO -  gmake[4]: Target 'target-objects' not remade because of errors.
[task 2022-11-14T22:58:01.801Z] 22:58:01    ERROR -  gmake[3]: *** [/builds/worker/checkouts/gecko/config/recurse.mk:72: layout/build/target-objects] Error 2
[task 2022-11-14T22:58:01.801Z] 22:58:01     INFO -  gmake[4]: Entering directory '/builds/worker/workspace/obj-build/layout/mathml'

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1628068

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/942100b2dbbd
Delete the Viaduct XPCOM interface, r=chutten,markh

Comment 9

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/942100b2dbbd

Comment 10

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/942100b2dbbd