1355097 - stylo: worker threads attempt to instantiate the preferences service off the main thread

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nathan Froyd [:froydnj]]

Seen while testing the parallel traversal testing patch:

[task 2017-04-10T14:13:00.212727Z] 14:13:00     INFO - Assertion failure: NS_IsMainThread(), at /home/worker/workspace/build/src/modules/libpref/Preferences.cpp:481
[task 2017-04-10T14:13:18.989306Z] 14:13:18     INFO - #01: mozilla::Preferences::GetInt [modules/libpref/Preferences.cpp:1431]
[task 2017-04-10T14:13:18.990352Z] 14:13:18     INFO - #02: mozilla::LangGroupFontPrefs::Initialize [layout/base/StaticPresData.cpp:129]
[task 2017-04-10T14:13:18.991035Z] 14:13:18     INFO - #03: Gecko_GetBaseSize [layout/style/ServoBindings.cpp:1607]
[task 2017-04-10T14:13:18.991186Z] 14:13:18     INFO - #04: style::gecko::wrapper::{{impl}}::get_size [servo/components/style/gecko/wrapper.rs:466]
[task 2017-04-10T14:13:18.992387Z] 14:13:18     INFO - #05: style::gecko_string_cache::Atom::with<closure,i32> [servo/components/style/gecko_string_cache/mod.rs:194]
[task 2017-04-10T14:13:18.992899Z] 14:13:18     INFO - #06: style::properties::longhands::font_size::{{impl}}::to_computed_value [servo/components/style/lib.rs:149]
[task 2017-04-10T14:13:18.993511Z] 14:13:18     INFO - #07: style::properties::longhands::font_size::{{impl}}::to_computed_value [servo/components/style/lib.rs:149]
[task 2017-04-10T14:13:18.994766Z] 14:13:18     INFO - #08: style::properties::longhands::font_size::cascade_property::{{closure}} [servo/components/style/lib.rs:149]
[task 2017-04-10T14:13:18.994890Z] 14:13:18     INFO - #09: style::properties::substitute_variables_font_size<closure> [servo/components/style/lib.rs:149]
[task 2017-04-10T14:13:18.996034Z] 14:13:18     INFO - #10: style::properties::longhands::font_size::cascade_property [servo/components/style/lib.rs:149]
[task 2017-04-10T14:13:18.996291Z] 14:13:18     INFO - #11: style::properties::apply_declarations<closure,core::iter::FlatMap<style::rule_tree::SelfAndAncestors, core::iter::FilterMap<core::iter::Rev<core::slice::Iter<(style::properties::PropertyDeclaration, style::properties::declaration_block::Importance)>>, closure>, closure>> [servo/components/style/lib.rs:149]
[task 2017-04-10T14:13:18.996993Z] 14:13:18     INFO - #12: style::properties::cascade [servo/components/style/lib.rs:149]
[task 2017-04-10T14:13:18.999609Z] 14:13:18     INFO - #13: style::matching::PrivateMatchMethods::cascade_with_rules<style::gecko::wrapper::GeckoElement> [servo/components/style/matching.rs:386]
[task 2017-04-10T14:13:19.001836Z] 14:13:19     INFO - #14: style::matching::PrivateMatchMethods::cascade_internal<style::gecko::wrapper::GeckoElement> [servo/components/style/matching.rs:414]
[task 2017-04-10T14:13:19.004035Z] 14:13:19     INFO - #15: style::matching::PrivateMatchMethods::cascade_primary_or_pseudo<style::gecko::wrapper::GeckoElement> [servo/components/style/matching.rs:432]
[task 2017-04-10T14:13:19.006198Z] 14:13:19     INFO - #16: style::matching::MatchMethods::match_and_cascade<style::gecko::wrapper::GeckoElement> [servo/components/style/matching.rs:708]
[task 2017-04-10T14:13:19.008290Z] 14:13:19     INFO - #17: style::traversal::compute_style<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> [servo/components/style/traversal.rs:710]
[task 2017-04-10T14:13:19.009937Z] 14:13:19     INFO - #18: style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> [servo/components/style/traversal.rs:584]
[task 2017-04-10T14:13:19.011328Z] 14:13:19     INFO - #19: style::gecko::traversal::{{impl}}::process_preorder [servo/components/style/gecko/traversal.rs:47]
[task 2017-04-10T14:13:19.014538Z] 14:13:19     INFO - #20: style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> [servo/components/style/parallel.rs:115]
[task 2017-04-10T14:13:19.016916Z] 14:13:19     INFO - #21: style::parallel::traverse_dom::{{closure}}::{{closure}}<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> [servo/components/style/parallel.rs:76]
[task 2017-04-10T14:13:19.019184Z] 14:13:19     INFO - #22: rayon::scope::{{impl}}::execute_job_closure::{{closure}}<closure> [third_party/rust/rayon/src/scope/mod.rs:314]
[task 2017-04-10T14:13:19.021437Z] 14:13:19     INFO - #23: std::panic::{{impl}}::call_once<(),closure> [/buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/panic.rs:297]
[task 2017-04-10T14:13:19.023708Z] 14:13:19     INFO - #24: std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> [/buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/panicking.rs:462]
[task 2017-04-10T14:13:19.025883Z] 14:13:19     INFO - #25: panic_abort::__rust_maybe_catch_panic [/buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libpanic_abort/lib.rs:42]

AFAICT, this is happening early in the child process (we only have 3 DOMWINDOW prints from the child process at this point), so it's totally conceivable that nothing has touched the preferences service yet.  Did we talk about adding an assert here and then decide it wasn't needed?

We may have to start the preferences service deliberately in nsLayoutStatics or somesuch, to ensure it's available for Stylo.

Comment 1

[Manish Goregaokar [:manishearth]]

IIRC that was a different assert we were thinking of, which was added?

Regardless, we should probably just call Preferences::InitStaticMembers() in PreTraverse() or perhaps nsLayoutStatics.

Comment 2

[Nathan Froyd [:froydnj]]

I thought we had talked about changing:

Preferences::InitStaticMembers()
{
#ifndef MOZ_B2G
  MOZ_ASSERT(NS_IsMainThread());
#endif

  if (!sShutdown && !sPreferences) {
    nsCOMPtr<nsIPrefService> prefService =
      do_GetService(NS_PREFSERVICE_CONTRACTID);
  }

into:

Preferences::InitStaticMembers()
{
  if (!sShutdown && !sPreferences) {
#ifndef MOZ_B2G
    MOZ_ASSERT(NS_IsMainThread());
#endif

    nsCOMPtr<nsIPrefService> prefService =
      do_GetService(NS_PREFSERVICE_CONTRACTID);
  }

or something like that...which might be all that's needed here.

Comment 3

[Nathan Froyd [:froydnj]]

Testing the obvious fix in:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27945cf0dc39c04a504a6e693824f9b64256f605

We can call InitStaticMembers on the main thread or during traversal, but the pref service must be instantiated on the main thread.

Comment 4

[Nathan Froyd [:froydnj]]

Testing with the proper platforms in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c42ad89c628496bd513b297b432b95691d0714e

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: adjust asserts in Preferences::InitStaticMembers to account for Stylo

Calls to this function on servo traversal threads are OK, but instantiating the
service must be done on the main thread.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: adjust asserts in Preferences::InitStaticMembers to account for Stylo

Here, let's try this without other gunk mixed in.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cc263a6e6d5
adjust asserts in Preferences::InitStaticMembers to account for Stylo; r=Manishearth

Comment 8

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/3cc263a6e6d5