1463884 - Assertion failure: NS_IsMainThread() || sServoFFILock->LockedForWritingByCurrentThread()

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jean-Yves Avenard [:jya]]

Using current central on Linux x86_64:

at the start I get an assertion:

Assertion failure: NS_IsMainThread() || sServoFFILock->LockedForWritingByCurrentThread(), at /home/jyavenard/Work/Mozilla/mozilla-central/layout/style/ServoBindings.cpp:126
#01: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x5999e60]
#02: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x10ea096]
#03: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x10e9fd6]
#04: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2b96efe]
#05: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2b970c9]
#06: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2ad0efb]
#07: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2ad0c18]
#08: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2af3cb9]
#09: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x26e05bc]
#10: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x10423b5]
#11: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x1069cbc]
#12: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x1a51207]
#13: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x198be25]
#14: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x198bda5]
#15: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x198bd7d]
#16: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x103fd18]
#17: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libnspr4.so +0x42a05]
#18: ???[/lib/x86_64-linux-gnu/libpthread.so.0 +0x76db]
#19: clone[/lib/x86_64-linux-gnu/libc.so.6 +0x12188f]
#20: ??? (???:???)

Program /home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/firefox (pid = 53999) received signal 11.
Stack:

Comment 1

[Emilio Cobos Álvarez (:emilio)]

What's the stack?

Comment 2

[Jean-Yves Avenard [:jya]]

The stack displayed in the console is what I get. There are no symbols resolved.

Likely related, E10s is deactivated. 300s after the assertion occurs, Firefox will close.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Could you attach gdb or something and try to get a stack with symbols? I'm pretty concerned (and surprised to some extent) about this assertion firing, but I can't repro at all on https://hg.mozilla.org/mozilla-central/rev/47e81ea1ef10189ef210867934bf36e14cf223dc with e10s either enabled or disabled.

Do you happen to have of any particular user style / font-related prefs / whatnot?

Comment 4

[Jean-Yves Avenard [:jya]]

I run central in a debugger, the assertion is showing up in the console, it doesn't trigger gdb and my current debugging session, it's like a new process is spawned anyway, even with e10s disabled.
The console displays a message that things will shut-down in 300. It then continues to run for a little while and everything ends 300s later

It's ubuntu recently updated to 18.04

Comment 5

[Jean-Yves Avenard [:jya]]

Ok...

To reproduce the bug I do:
./mach run -P Development
where Development is a profile that has browser.tabs.remote.autostart set to false (disabling e10s)

I then go into the Preferences -> boom you see in the launching terminal:
Assertion failure: NS_IsMainThread() || sServoFFILock->LockedForWritingByCurrentThread(), at /home/jyavenard/Work/Mozilla/mozilla-central/layout/style/ServoBindings.cpp:126
#01: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x5999e60]
#02: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x10ea096]
#03: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x10e9fd6]
#04: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2b96efe]
#05: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2b970c9]
#06: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2ad0efb]
#07: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2ad0c18]
#08: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2af3cb9]
#09: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x26e05bc]
#10: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x10423b5]
#11: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x1069cbc]
#12: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x1a51207]
#13: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x198be25]
#14: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x198bda5]
#15: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x198bd7d]
#16: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x103fd18]
#17: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libnspr4.so +0x42a05]
#18: ???[/lib/x86_64-linux-gnu/libpthread.so.0 +0x76db]
#19: clone[/lib/x86_64-linux-gnu/libc.so.6 +0x12188f]
#20: ??? (???:???)

Program /home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/firefox (pid = 60787) received signal 11.
Stack:
#01: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x901309b]
#02: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x8fc861d]
#03: ???[/lib/x86_64-linux-gnu/libpthread.so.0 +0x12890]
#04: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x5999e6e]
#05: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x10ea096]
#06: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x10e9fd6]
#07: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2b96efe]
#08: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2b970c9]
#09: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2ad0efb]
#10: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2ad0c18]
#11: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x2af3cb9]
#12: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x26e05bc]
#13: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x10423b5]
#14: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x1069cbc]
#15: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x1a51207]
#16: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x198be25]
#17: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x198bda5]
#18: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x198bd7d]
#19: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libxul.so +0x103fd18]
#20: ???[/home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/libnspr4.so +0x42a05]
#21: ???[/lib/x86_64-linux-gnu/libpthread.so.0 +0x76db]
#22: clone[/lib/x86_64-linux-gnu/libc.so.6 +0x12188f]
#23: ??? (???:???)
Sleeping for 300 seconds.

during this time, I can perfectly run FF just fine, it is then killed after 300s with the message "sleeping done" being displayed.

There's a message like:
Type 'gdb /home/jyavenard/Work/Mozilla/obj-ff-dbg/dist/bin/firefox 60787' to attach your debugger to this thread.

which I did, but the backtrace showing in that gdb session is for the main process, and nothing show about ServoBindings.cpp

Comment 6

[Jean-Yves Avenard [:jya]]

ah got one:

#0  0x00007fffe2b36e6e in AssertIsMainThreadOrServoLangFontPrefsCacheLocked() () at /home/jyavenard/Work/Mozilla/mozilla-central/layout/style/ServoBindings.cpp:126
#1  0x00007fffde287096 in nsLanguageAtomService::GetLanguageGroup(nsAtom*, bool*) (this=0x7fffdaa1c430, aLanguage=0x7fffb4757be0, aNeedsToCache=0x0) at /home/jyavenard/Work/Mozilla/mozilla-central/intl/locale/nsLanguageAtomService.cpp:99
#2  0x00007fffde286fd6 in nsLanguageAtomService::LookupLanguage(nsTSubstring<char> const&) (this=0x7fffdaa1c430, aLanguage=...) at /home/jyavenard/Work/Mozilla/mozilla-central/intl/locale/nsLanguageAtomService.cpp:46
#3  0x00007fffdfd33efe in gfxPlatformFontList::TryLangForGroup(nsTSubstring<char> const&, nsAtom*, nsTSubstring<char>&) (this=0x7fffdb974800, aOSLang=..., aLangGroup=0x7fffe83df50c <mozilla::detail::gGkAtoms+64332>, aFcLang=...) at /home/jyavenard/Work/Mozilla/mozilla-central/gfx/thebes/gfxPlatformFontList.cpp:1510
#4  0x00007fffdfd340c9 in gfxPlatformFontList::GetSampleLangForGroup(nsAtom*, nsTSubstring<char>&, bool) (this=0x7fffdb974800, aLanguage=0x7fffe83df50c <mozilla::detail::gGkAtoms+64332>, aLangStr=..., aCheckEnvironment=true) at /home/jyavenard/Work/Mozilla/mozilla-central/gfx/thebes/gfxPlatformFontList.cpp:1554
#5  0x00007fffdfc6defb in GetSystemFontList(nsTArray<nsTString<char16_t> >&, nsAtom*) (aListOfFonts=..., aLangGroup=0x7fffe83df50c <mozilla::detail::gGkAtoms+64332>) at /home/jyavenard/Work/Mozilla/mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:1762
#6  0x00007fffdfc6dc18 in gfxFcPlatformFontList::GetFontList(nsAtom*, nsTSubstring<char> const&, nsTArray<nsTString<char16_t> >&) (this=0x7fffdb974800, aLangGroup=0x7fffe83df50c <mozilla::detail::gGkAtoms+64332>, aGenericFamily=..., aListOfFonts=...) at /home/jyavenard/Work/Mozilla/mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:1800
#7  0x00007fffdfc90cb9 in gfxPlatformGtk::GetFontList(nsAtom*, nsTSubstring<char> const&, nsTArray<nsTString<char16_t> >&) (this=0x7fffdaa48d00, aLangGroup=0x7fffe83df50c <mozilla::detail::gGkAtoms+64332>, aGenericFamily=..., aListOfFonts=...) at /home/jyavenard/Work/Mozilla/mozilla-central/gfx/thebes/gfxPlatformGtk.cpp:180
#8  0x00007fffdf87d5bc in EnumerateFontsTask::Run() (this=0x7fffb3f48660) at /home/jyavenard/Work/Mozilla/mozilla-central/gfx/src/nsThebesFontEnumerator.cpp:152
#9  0x00007fffde1df3b5 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7fffc2dcce20, aMayWait=true, aResult=0x7fffaf7fbc3e) at /home/jyavenard/Work/Mozilla/mozilla-central/xpcom/threads/nsThread.cpp:1090
#10 0x00007fffde206cbc in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7fffc2dcce20, aMayWait=true) at /home/jyavenard/Work/Mozilla/mozilla-central/xpcom/threads/nsThreadUtils.cpp:519
#11 0x00007fffdebee207 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (this=0x7fffba4f3c00, aDelegate=0x7fffb3a64c80) at /home/jyavenard/Work/Mozilla/mozilla-central/ipc/glue/MessagePump.cpp:364
#12 0x00007fffdeb28e25 in MessageLoop::RunInternal() (this=0x7fffb3a64c80) at /home/jyavenard/Work/Mozilla/mozilla-central/ipc/chromium/src/base/message_loop.cc:326
#13 0x00007fffdeb28da5 in MessageLoop::RunHandler() (this=0x7fffb3a64c80) at /home/jyavenard/Work/Mozilla/mozilla-central/ipc/chromium/src/base/message_loop.cc:319
#14 0x00007fffdeb28d7d in MessageLoop::Run() (this=0x7fffb3a64c80) at /home/jyavenard/Work/Mozilla/mozilla-central/ipc/chromium/src/base/message_loop.cc:299
#15 0x00007fffde1dcd18 in nsThread::ThreadFunc(void*) (aArg=0x7fffffff0288) at /home/jyavenard/Work/Mozilla/mozilla-central/xpcom/threads/nsThread.cpp:425
#16 0x00007ffff67eba05 in _pt_root (arg=0x7fffb1464ca0) at /home/jyavenard/Work/Mozilla/mozilla-central/nsprpub/pr/src/pthreads/ptthread.c:201
#17 0x00007ffff7bbd6db in start_thread (arg=0x7fffaf7fc700) at pthread_create.c:463
#18 0x00007ffff6d9e88f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Comment 7

[Jean-Yves Avenard [:jya]]

this is from the "FontEnumThread"

Comment 8

[Emilio Cobos Álvarez (:emilio)]

I have no idea what that thread is, it's definitely not related to style, but whoever is doing that is doing something pretty unsafe... Jonathan, any ideas about how to fix this? This is unsafe.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

This is only related to about:preferences at least, which is good.

Comment 10

[Jonathan Kew [:jfkthame]]

Yeah, this arose because when Preferences was reorganized such that the Fonts menu appears on the main (default) prefs page, there was a concern that enumerating all the available fonts was taking a long time and blocking the initial display of the page.

So in bug 1375978, the enumeration of the fonts was moved to a separate thread, so that it doesn't block display of the prefs page (we just populate the Fonts menu later, once the list is ready).

On macOS and Windows, I believe that's safe (we dealt with a couple of thread-safety concerns back in bug 1375978), but it turns out that on Linux we still have a problem, because in order to set up the most appropriate FC_LANG for calling fontconfig, we use the LanguageAtomService, which may want to cache a lang->langGroup mapping in a hashtable.

Not sure yet what's the best way forward here. I think the most trivial "fix" might be to add a boolean 'false' as third parameter to the GetSampleLangForGroup call at:

https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/gfx/thebes/gfxFcPlatformFontList.cpp#1762

Jean-Yves, could you try that and confirm whether it avoids the assertion? (If not, I may have misunderstood the relevant control flow that's happening here.) Thanks!

Comment 11

[Jean-Yves Avenard [:jya]]

I should add that the defaut Font entry in the preference is always empty on this linux box. So the moving off the main thread wasn't really successful. Surprised that it wasn't picked up earlier.

Otherwise, adding False as 3rd parameter to https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/gfx/thebes/gfxFcPlatformFontList.cpp#1762
does fix the issue, *and* the default font entry is now populated as expected.

Comment 12

[Daniel Veditz [:dveditz]]

Calling this sec-low because it doesn't sound web triggerable (about:preferences is the only caller we found)

Comment 13

[Cameron McCormack (:heycam)]

Is the main thread blocked while the font enumeration happens?  If not, then it seems like we should be acquiring the lock mentioned in the assertion, otherwise style worker threads could be touching the same objects.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Move methods only used by fontconfig backend from gfxPlatformFontList to the fontconfig subclass. (No functional change.)

This is a preliminary patch that just moves the relevant methods from gfxPlatformFontList to its linux/fontconfig-based subclass, as that is the only user of these methods so there's no reason for them to live in the cross-platform base class.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Make gfxFcPlatformFontList::TryLangForGroup safe for off-main-thread use

And this actually fixes the problem, by avoiding the unsafe cache usage when called from a non-main, non-servo-traversal thread.

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #13)
> Is the main thread blocked while the font enumeration happens?  If not, then
> it seems like we should be acquiring the lock mentioned in the assertion,
> otherwise style worker threads could be touching the same objects.

No, AFAIK the main thread isn't blocked; so the patch above avoids the problem by simply not using the langGroup cache if we're on another thread. As this is only used to populate the Font menu in about:preferences (and only on Linux), I didn't think the slight performance penalty of bypassing the cache here would matter.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980685 [details] [diff] [review]
patch 1 - Move methods only used by fontconfig backend from gfxPlatformFontList to the fontconfig subclass. (No functional change.)

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

Looks good, thank you :)

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8980686 [details] [diff] [review]
patch 2 - Make gfxFcPlatformFontList::TryLangForGroup safe for off-main-thread use

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

::: layout/style/ServoUtils.h
@@ +33,5 @@
>    MOZ_ASSERT(sInServoTraversal || NS_IsMainThread());
>    return sInServoTraversal;
>  }
> +
> +inline bool IsInServoTraversalWithoutMainThreadAsserion()

Oops, typo.... obviously that should be ...Assertion() with a 't' in it!

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980686 [details] [diff] [review]
patch 2 - Make gfxFcPlatformFontList::TryLangForGroup safe for off-main-thread use

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

Looks reasonable, thanks for fixing this :)

I'm still somewhat concerned about all the complexity of the font-related code that we call OMT, and wish we could simplify it somehow... but that's not the point of this bug or patch in any case.

::: layout/style/ServoUtils.h
@@ +38,5 @@
> +{
> +  // This is for cases when we _only_ want to query whether we're in Servo
> +  // traversal, without asserting in the case of other non-main-thread callers.
> +  // Use this only if the calling code is safe to use off the main thread.
> +  return sInServoTraversal;

This looks somewhat easy to abuse, but yeah, can't think of anything better. Looks fine :)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/695a21084f8f
https://hg.mozilla.org/mozilla-central/rev/d48abd83fc7c

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Hmm, actually I was wrong when reviewing this. Given this thread can run concurrently during the servo traversal, this can take the racy path incorrectly.

I'll revert the second part of this bug and reopen.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

To clarify, the second part, which I reverted in:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/ea82a59465a9

has the issue that a style thread may be running (and thus using the cache with the lock held) and this other font enumerator thread may end up in this block of code and IsInServoTraversalWithoutMainThreadAssertion() will return true as well (since it's a static variable).

So IsInServoTraversalWithoutMainThreadAssertion is not really useful, and we need to think of something else, either storing something in TLS to properly identify a Servo thread or font enumerator thread, or just expose the lock somewhere, or thread down the information of whether to use the cache somehow as an argument.

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 8980686 [details] [diff] [review]
patch 2 - Make gfxFcPlatformFontList::TryLangForGroup safe for off-main-thread use

Yeah, that's a problem.... what we really wanted to know there was something like IsServoTraversalThread, not just to check IsInServoTraversal (globally). But I guess that doesn't currently exist.

Probably the simplest solution is for the GetSystemFontList caller of GetSampleLangForGroup to pass in a flag saying that we should avoid using the langGroup cache.

Comment 24

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Remove always-true argument aCheckEnvironment from gfxFcPlatformFontList::GetSampleLangForGroup

This is just cleanup, removing the aCheckEnvironment parameter here because no-one changes it from its default.

Comment 25

[Jonathan Kew [:jfkthame]]

Attachment: patch 3 - Make gfxFcPlatformFontList::TryLangForGroup safe for off-main-thread use when called from GetSystemFontList

And this should actually fix the bug (for real, this time).

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8981023 [details] [diff] [review]
patch 3 - Make gfxFcPlatformFontList::TryLangForGroup safe for off-main-thread use when called from GetSystemFontList

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

Sorry for the lag, didn't want to review this by night :)

Looks good, thanks a lot for fixing.

Comment 27

[Jonathan Kew [:jfkthame]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> Sorry for the lag, didn't want to review this by night :)

No problem, I (probably) wouldn't have landed it while I was sleeping anyhow. ;)

Thanks for catching the issues here and making sure we fix it properly.

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb7865ac41994d7fe0e90694e07ecdfe7486a85
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a238a8591587be826418452275f7b2e7a6364f5

https://hg.mozilla.org/mozilla-central/rev/aeb7865ac419
https://hg.mozilla.org/mozilla-central/rev/7a238a859158

Comment 29

[Jean-Yves Avenard [:jya]]

confirmed as fixed.

Comment 30

[Jonathan Kew [:jfkthame]]

Comment on attachment 8981023 [details] [diff] [review]
patch 3 - Make gfxFcPlatformFontList::TryLangForGroup safe for off-main-thread use when called from GetSystemFontList

Although this is sec-low (because it only applies to opening about:preferences, it's not a web-exposed vulnerability), I think we should take it on beta-61 and esr-60 so that Linux users aren't left with unsafe behavior (e.g. possible heap corruption) when opening Preferences.

Request is for the three patches as a set. Patches 1 and 2 here are purely cleanup to make the actual fix clearer; they involve no actual change to behavior. (Alternatively, we could backport only patch 3, but without the preliminary cleanup it would be easier to make a mistake in it.)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Minimal-risk fix that avoids exposing Linux users to a hashtable race condition that could result in....well, it's hard to say exactly what might go wrong as a result. Crashing would probably be the best-case outcome; subtle corruption could be worse.

User impact if declined: thread-unsafe hashtable usage when opening about:preferences on Linux, could result in crash or other corruption

Fix Landed on Version: 62

Risk to taking this patch (and alternatives if risky): minimal, this just ensures we skip use of the hashtable from the font-enumeration thread

String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1375978

[User impact if declined]: thread-safety violation when opening Preferences on Linux; could result in crash or other corruption

[Is this code covered by automated tests?]: no

[Has the fix been verified in Nightly?]: yes

[Needs manual test from QE? If yes, steps to reproduce]: yes - on Linux, with the LANGUAGE env variable set, open about:preferences using a DEBUG build; assertion should NOT be triggered

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: no

[Why is the change risky/not risky?]: minimal behavior change, just avoids use of an unsafe cache from the about:preferences font-enumeration thread

[String changes made/needed]: none

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8981023 [details] [diff] [review]
patch 3 - Make gfxFcPlatformFontList::TryLangForGroup safe for off-main-thread use when called from GetSystemFontList

Avoids possibly-exploitable crashes when Linux users open the preferences page. Approved for 61.0b10 and ESR 60.1.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4fcab7ec6dd0
https://hg.mozilla.org/releases/mozilla-beta/rev/27f627cf7883
https://hg.mozilla.org/releases/mozilla-beta/rev/a3142138b6c9

https://hg.mozilla.org/releases/mozilla-esr60/rev/cbe2ef9e7e4a
https://hg.mozilla.org/releases/mozilla-esr60/rev/5755188b40c5
https://hg.mozilla.org/releases/mozilla-esr60/rev/42be6252c737