Closed Bug 1467134 Opened Last year Closed Last year

assertions at startup due to use of non-atomic pref storage

Categories

(Core :: Preferences: Backend, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 --- fixed

People

(Reporter: jya, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(1 file)

Like:
* thread #57, name = 'StyleThread#7', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000103edcc8b XUL`mozilla::StaticPrefs::layout_css_font_variations_enabled() at StaticPrefList.h:278
    frame #1: 0x0000000103edd581 XUL`gfxFontEntry::GetVariationsForStyle(this=&0x13357eae0, aResult=0x000070000849dec8, aStyle=0x0000000137220080) at gfxFontEntry.cpp:1152
    frame #2: 0x0000000103e503b5 XUL`gfxMacFont::gfxMacFont(this=&0x1372330a0, aUnscaledFont=0x000070000849dfc0, aFontEntry=&0x13357eae0, aFontStyle=&0x137220080) at gfxMacFont.cpp:55
    frame #3: 0x0000000103e51aad XUL`gfxMacFont::gfxMacFont(this=&0x1372330a0, aUnscaledFont=0x000070000849dfc0, aFontEntry=&0x13357eae0, aFontStyle=&0x137220080) at gfxMacFont.cpp:42
    frame #4: 0x0000000103f51dd1 XUL`MacOSFontEntry::CreateFontInstance(this=&0x13357eae0, aFontStyle=&0x137220080) at gfxMacPlatformFontList.mm:283
    frame #5: 0x0000000103ed48db XUL`gfxFontEntry::FindOrMakeFont(this=&0x13357eae0, aStyle=&0x137220080, aUnicodeRangeMap=&0x0) at gfxFontEntry.cpp:258
    frame #6: 0x0000000103f24115 XUL`gfxFontGroup::GetFontAt(this=&0x137220050, i=0, aCh=32) at gfxTextRun.cpp:1922
    frame #7: 0x0000000103f24e8a XUL`gfxFontGroup::GetFirstValidFont(this=&0x137220050, aCh=32, aGeneric=&0x0) at gfxTextRun.cpp:2104
    frame #8: 0x00000001038bd36d XUL`nsFontMetrics::GetMetrics(this=&0x13720a550, aOrientation=eHorizontal) const at nsFontMetrics.cpp:169
    frame #9: 0x00000001038bd488 XUL`nsFontMetrics::GetMetrics(this=&0x13720a550) const at nsFontMetrics.h:244
    frame #10: 0x00000001038bd439 XUL`nsFontMetrics::XHeight(this=&0x13720a550) at nsFontMetrics.cpp:175
    frame #11: 0x000000010729daae XUL`::Gecko_GetFontMetrics(aPresContext=&0x133cbe800, aIsVertical=false, aFont=&0x14d8fc0d8, aFontSize=660, aUseUserFontSet=true) at ServoBindings.cpp:2579
    frame #12: 0x000000010d154f25 XUL`_$LT$style..gecko..wrapper..GeckoFontMetricsProvider$u20$as$u20$style..font_metrics..FontMetricsProvider$GT$::query::h22a33607c9f5aef3(self=&0x1386d2e20, font=&0x14d8fc0d8, font_size=Au(660), wm=WritingMode {
bits: '\0'

How did that pass mochitests and made it to central?
VARCACHE_PREF(
  "layout.css.font-variations.enabled",
   layout_css_font_variations_enabled,
  bool, true
)

is troublesome, as it's called from servo/components/style/font_face.rs which is expecting a bool
> How did that pass mochitests and made it to central?

Is it possible that the test hardware has few enough cores that stylo doesn't do off-main-thread stuff?

Anyway, I would assume changing that "bool" to Atomic<bool> would fix the assertion failure.  Does it?
Flags: needinfo?(jyavenard)
Flags: needinfo?(jyavenard)
Summary: Dozens of assertions at startup due to use of non-atomic pref storage → assertions at startup due to use of non-atomic pref storage
it doesn't. As I mentioned in comment 1, when doing so I get:

 1:47.46 error[E0308]: mismatched types
 1:47.46    --> servo/components/style/font_face.rs:326:13
 1:47.46     |
 1:47.46 326 |               mozilla::StaticPrefs_sVarCache_layout_css_font_variations_enabled
 1:47.46     |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected bool, found u32
 1:47.46 ...
 1:47.46 453 | / font_face_descriptors! {
 1:47.46 454 | |     mandatory descriptors = [
 1:47.46 455 | |         /// The name of this font face
 1:47.46 456 | |         "font-family" family / mFamily: FamilyName,
 1:47.47 ...   |
 1:47.47 485 | |     ]
 1:47.47 486 | | }
 1:47.47     | |_- in this macro invocation
 1:48.27 error: aborting due to previous error
 1:48.38 error: Could not compile `style`.

due to how Atomic are stored.

You'll also get errors on layout_css_font_display_enabled most likely
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> > How did that pass mochitests and made it to central?
> 
> Is it possible that the test hardware has few enough cores that stylo
> doesn't do off-main-thread stuff?

This seems bad; time was that we force-enabled stylo threads to ensure the multithreaded codepaths got some coverage on testing.  Are we accidentally force-not-enabling stylo threads?
Well, the good think is that that is "ok", because the main thread is blocked while stylo is running I'd think...

No idea about the hardware, we do force sequential stuff to run (that's the stylo-sequential platform)... We should probably have a force-parallel platform (as well? Or probably just replacing the sequential one is enough) after the traversal was made adaptative.
Atomic<bool> is implemented in terms of AtomicBase<uint32_t>, because that way
you don't need to depend on atomic 1-byte operations.  That means that the rust
bindgen sees it as a u32, not a bool.

It's a bit concerning that the rust code seems to be doing an unsynchronized
read here, but given this is a RelaxedAtomic, that's probably ok.
Attachment #8983826 - Flags: review?(emilio)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Does stylo use layout_css_font_display_enabled off the main thread?
Comment on attachment 8983826 [details] [diff] [review]
Use Atomic<bool> for the staticpref version of layout.css.font-variations.enabled

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

Yeah... At first I thought this code was main-thread only, but I forgot about the parallel parsing of stylesheets...

I agree that in practice I don't expect this to be an issue, but worth filing, maybe? We may need to special-case bindgen, but that seems fine.
Attachment #8983826 - Flags: review?(emilio) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> Does stylo use layout_css_font_display_enabled off the main thread?

It does when parsing in parallel... Pretty much all our prefs can be queried OMT in presence of parallel parsing. I can fix soon if you don't have the cycles to change all of them.
I should add that I typically runs it with e10s off...
> Does stylo use layout_css_font_display_enabled off the main thread?

I guess more precisely, does it call C++ code that then does the staticprefs access?

Looks like those can only happen from nsCSSProps::LookupFontDesc and nsFontFaceLoader::GetFontDisplay.  

LookupFontDesc is called only from CSSOM methods (which means mainthread-only).  nsFontFaceLoader::GetFontDisplay is called from various nsFontFaceLoader things all of which also look mainthread-only.  So I think we're good there in terms of hitting assertions.
with the attached patch (and fixing my new preferences) it starts okay now...
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6de156e08e8e
Use Atomic<bool> for the staticpref version of layout.css.font-variations.enabled.  r=emilio
https://hg.mozilla.org/mozilla-central/rev/6de156e08e8e
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Please request Beta approval on this when you get a chance.
this was a 62 only change
Flags: needinfo?(bzbarsky)
Yeah, this is trunk-only, no beta anything needed.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.