1824200 - [tsan] data race initializing the SharedFTFace in gfxFontconfigFontEntry::GetFTFace

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

+++ This bug was initially created as a clone of Bug #1823365 +++

After fixing the originally-reported missing-glyph atlas issue, Tyson is still seeing another, unrelated data race with the same testcase; see bug 1823365 comment 11, and attachment 9324553 [details].

The race here involved the fontconfig backend and its management of the FreeType face:

WARNING: ThreadSanitizer: data race (pid=104338)
  Read of size 8 at 0x7b5400001d60 by thread T18:
    #0 RefPtr /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:97:27 (libxul.so+0x5917041) (BuildId: 9ee4bdacd12c56311ad459908123f344c17c0ace)
    #1 gfxFontconfigFontEntry::CreateFontInstance(gfxFontStyle const*) /builds/worker/checkouts/gecko/gfx/thebes/gfxFcPlatformFontList.cpp:859:31 (libxul.so+0x5917041)
    #2 gfxFontEntry::FindOrMakeFont(gfxFontStyle const*, gfxCharacterMap*) /builds/worker/checkouts/gecko/gfx/thebes/gfxFontEntry.cpp:266:22 (libxul.so+0x5947cfe) (BuildId: 9ee4bdacd12c56311ad459908123f344c17c0ace)

vs

  Previous write of size 8 at 0x7b5400001d60 by thread T20:
    #0 assign_assuming_AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:71:13 (libxul.so+0x59162e3) (BuildId: 9ee4bdacd12c56311ad459908123f344c17c0ace)
    #1 operator=<mozilla::gfx::SharedFTFace> /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:226:5 (libxul.so+0x59162e3)
    #2 gfxFontconfigFontEntry::GetFTFace() /builds/worker/checkouts/gecko/gfx/thebes/gfxFcPlatformFontList.cpp:941:13 (libxul.so+0x59162e3)
    #3 gfxFontconfigFontEntry::CreateFontInstance(gfxFontStyle const*) /builds/worker/checkouts/gecko/gfx/thebes/gfxFcPlatformFontList.cpp:859:31 (libxul.so+0x5917036) (BuildId: 9ee4bdacd12c56311ad459908123f344c17c0ace)
    #4 gfxFontEntry::FindOrMakeFont(gfxFontStyle const*, gfxCharacterMap*) /builds/worker/checkouts/gecko/gfx/thebes/gfxFontEntry.cpp:266:22 (libxul.so+0x5947cfe) (BuildId: 9ee4bdacd12c56311ad459908123f344c17c0ace)

I guess the root of the problem here is that the mFTFace and mFTFaceInitialized members of gfxFontconfigFontEntry are not guarded or atomic, so we have a problem if two threads call GetFTFace() on the same (not-yet-initialized) entry at the same time.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1824200 - Make mFTFace an atomic pointer. r=lsalzman

Comment 2

[Jonathan Kew [:jfkthame]]

Tyson, could you please test whether this patch resolves the race? I believe it should, but I haven't been able to verify locally because currently my tsan build is insta-failing with an issue somewhere inside GTK before it reaches any of this code.

(Maybe updating various components on my system will help....)

Comment 3

[Tyson Smith [:tsmith]]

Attachment: post-patch-tsan.txt

With the patch applied I see:

WARNING: ThreadSanitizer: data race (pid=769123)
  Read of size 1 at 0x7b5400001fee by thread T23:
    #0 gfxFontconfigFontEntry::HasVariations() mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:970:7 (libxul.so+0x61220a0) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #1 gfxFontconfigFontEntry::CreateFontInstance(gfxFontStyle const*) mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:869:7 (libxul.so+0x61211d3) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #2 gfxFontEntry::FindOrMakeFont(gfxFontStyle const*, gfxCharacterMap*) mozilla-central/gfx/thebes/gfxFontEntry.cpp:266:22 (libxul.so+0x6151e4e) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #3 gfxFontGroup::GetFontAt(int, unsigned int, bool*) mozilla-central/gfx/thebes/gfxTextRun.cpp:2061:16 (libxul.so+0x61e4e54) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #4 gfxFontGroup::GetFirstValidFont(unsigned int, mozilla::StyleGenericFontFamily*, bool*) mozilla-central/gfx/thebes/gfxTextRun.cpp:2316:12 (libxul.so+0x61e5f9b) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #5 mozilla::dom::CanvasRenderingContext2D::DrawOrMeasureText(nsTSubstring<char16_t> const&, float, float, mozilla::dom::Optional<double> const&, mozilla::dom::CanvasRenderingContext2D::TextDrawOperation, mozilla::ErrorResult&) mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:4304:46 (libxul.so+0x7ac434b) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #6 mozilla::dom::CanvasRenderingContext2D::FillText(nsTSubstring<char16_t> const&, double, double, mozilla::dom::Optional<double> const&, mozilla::ErrorResult&) mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:3864:37 (libxul.so+0x7ac39e3) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #7 mozilla::dom::OffscreenCanvasRenderingContext2D_Binding::fillText(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) mozilla-central/objdir-ff-tsan/dom/bindings/OffscreenCanvasRenderingContext2DBinding.cpp:3904:24 (libxul.so+0x6d02d08) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #8 bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) mozilla-central/dom/bindings/BindingUtils.cpp:3318:13 (libxul.so+0x79f7154) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #9 CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) mozilla-central/js/src/vm/Interpreter.cpp:459:13 (libxul.so+0xc96ad0e) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    ...

  Previous write of size 1 at 0x7b5400001fee by thread T20:
    #0 gfxFontconfigFontEntry::HasVariations() mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:973:29 (libxul.so+0x61220c0) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #1 gfxFontconfigFontEntry::CreateFontInstance(gfxFontStyle const*) mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:869:7 (libxul.so+0x61211d3) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #2 gfxFontEntry::FindOrMakeFont(gfxFontStyle const*, gfxCharacterMap*) mozilla-central/gfx/thebes/gfxFontEntry.cpp:266:22 (libxul.so+0x6151e4e) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #3 gfxFontGroup::GetFontAt(int, unsigned int, bool*) mozilla-central/gfx/thebes/gfxTextRun.cpp:2061:16 (libxul.so+0x61e4e54) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #4 gfxFontGroup::GetFirstValidFont(unsigned int, mozilla::StyleGenericFontFamily*, bool*) mozilla-central/gfx/thebes/gfxTextRun.cpp:2316:12 (libxul.so+0x61e5f9b) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #5 mozilla::dom::CanvasRenderingContext2D::DrawOrMeasureText(nsTSubstring<char16_t> const&, float, float, mozilla::dom::Optional<double> const&, mozilla::dom::CanvasRenderingContext2D::TextDrawOperation, mozilla::ErrorResult&) mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:4304:46 (libxul.so+0x7ac434b) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #6 mozilla::dom::CanvasRenderingContext2D::FillText(nsTSubstring<char16_t> const&, double, double, mozilla::dom::Optional<double> const&, mozilla::ErrorResult&) mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:3864:37 (libxul.so+0x7ac39e3) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #7 mozilla::dom::OffscreenCanvasRenderingContext2D_Binding::fillText(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) mozilla-central/objdir-ff-tsan/dom/bindings/OffscreenCanvasRenderingContext2DBinding.cpp:3904:24 (libxul.so+0x6d02d08) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #8 bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) mozilla-central/dom/bindings/BindingUtils.cpp:3318:13 (libxul.so+0x79f7154) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #9 CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) mozilla-central/js/src/vm/Interpreter.cpp:459:13 (libxul.so+0xc96ad0e) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    ...

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Tyson Smith [:tsmith] from comment #3)

Created attachment 9325057 [details]
post-patch-tsan.txt

With the patch applied I see:

WARNING: ThreadSanitizer: data race (pid=769123)
  Read of size 1 at 0x7b5400001fee by thread T23:
    #0 gfxFontconfigFontEntry::HasVariations() mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:970:7 (libxul.so+0x61220a0) (BuildId: a58f95ae75e917c5002f73f6198d7392)
    #1 gfxFontconfigFontEntry::CreateFontInstance(gfxFontStyle const*) mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:869:7 (libxul.so+0x61211d3) (BuildId: a58f95ae75e917c5002f73f6198d7392)

Cool, thanks -- that makes sense, and should be fixable. So the patch here does eliminate the race on the SharedFTFace, and now we're seeing a further issue.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1824200 - Also make the has-variations state atomic. r=lsalzman

Comment 6

[Jonathan Kew [:jfkthame]]

OK, the second patch should also fix the issue with HasVariations, I hope. When you have a chance, could you please verify (and let me know what we hit next, if anything!) -- Thanks!

Comment 7

[Tyson Smith [:tsmith]]

Attachment: post-patch-tsan-2.txt

Of course :)

Here is the next report I see:

WARNING: ThreadSanitizer: data race (pid=775797)
  Read of size 8 at 0x7b5400001ff0 by thread T21:
    #0 RefPtr<mozilla::detail::ThreadSafeWeakReference>::operator!() const /home/twsmith/code/mozilla-central/objdir-ff-tsan/dist/include/mozilla/RefPtr.h:350:36 (libxul.so+0x61209b6) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #1 mozilla::ThreadSafeWeakPtr<mozilla::gfx::UnscaledFontFontconfig>::getRefPtr() const /home/twsmith/code/mozilla-central/objdir-ff-tsan/dist/include/mozilla/ThreadSafeWeakPtr.h:277:9 (libxul.so+0x61209b6)
    #2 mozilla::ThreadSafeWeakPtr<mozilla::gfx::UnscaledFontFontconfig>::operator RefPtr<mozilla::gfx::UnscaledFontFontconfig>() const /home/twsmith/code/mozilla-central/objdir-ff-tsan/dist/include/mozilla/ThreadSafeWeakPtr.h:272:48 (libxul.so+0x61209b6)
    #3 gfxFontconfigFontEntry::UnscaledFontCache::Lookup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, unsigned int) /home/twsmith/code/mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:805:42 (libxul.so+0x61209b6)
    #4 gfxFontconfigFontEntry::CreateFontInstance(gfxFontStyle const*) /home/twsmith/code/mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:925:26 (libxul.so+0x6121903) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #5 gfxFontEntry::FindOrMakeFont(gfxFontStyle const*, gfxCharacterMap*) /home/twsmith/code/mozilla-central/gfx/thebes/gfxFontEntry.cpp:266:22 (libxul.so+0x6151b0e) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #6 gfxFontGroup::GetFontAt(int, unsigned int, bool*) /home/twsmith/code/mozilla-central/gfx/thebes/gfxTextRun.cpp:2061:16 (libxul.so+0x61e4b14) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #7 gfxFontGroup::GetFirstValidFont(unsigned int, mozilla::StyleGenericFontFamily*, bool*) /home/twsmith/code/mozilla-central/gfx/thebes/gfxTextRun.cpp:2316:12 (libxul.so+0x61e5c5b) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #8 mozilla::dom::CanvasRenderingContext2D::DrawOrMeasureText(nsTSubstring<char16_t> const&, float, float, mozilla::dom::Optional<double> const&, mozilla::dom::CanvasRenderingContext2D::TextDrawOperation, mozilla::ErrorResult&) /home/twsmith/code/mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:4304:46 (libxul.so+0x7ac400b) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #9 mozilla::dom::CanvasRenderingContext2D::FillText(nsTSubstring<char16_t> const&, double, double, mozilla::dom::Optional<double> const&, mozilla::ErrorResult&) /home/twsmith/code/mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:3864:37 (libxul.so+0x7ac36a3) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #10 mozilla::dom::OffscreenCanvasRenderingContext2D_Binding::fillText(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /home/twsmith/code/mozilla-central/objdir-ff-tsan/dom/bindings/OffscreenCanvasRenderingContext2DBinding.cpp:3904:24 (libxul.so+0x6d029c8) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
...

  Previous write of size 8 at 0x7b5400001ff0 by thread T20:
    #0 RefPtr<mozilla::detail::ThreadSafeWeakReference>::assign_assuming_AddRef(mozilla::detail::ThreadSafeWeakReference*) /home/twsmith/code/mozilla-central/objdir-ff-tsan/dist/include/mozilla/RefPtr.h:71:13 (libxul.so+0x6121d08) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #1 RefPtr<mozilla::detail::ThreadSafeWeakReference>& RefPtr<mozilla::detail::ThreadSafeWeakReference>::operator=<mozilla::detail::ThreadSafeWeakReference, void>(RefPtr<mozilla::detail::ThreadSafeWeakReference>&&) /home/twsmith/code/mozilla-central/objdir-ff-tsan/dist/include/mozilla/RefPtr.h:239:5 (libxul.so+0x6121d08)
    #2 mozilla::ThreadSafeWeakPtr<mozilla::gfx::UnscaledFontFontconfig>::operator=(mozilla::ThreadSafeWeakPtr<mozilla::gfx::UnscaledFontFontconfig>&&) /home/twsmith/code/mozilla-central/objdir-ff-tsan/dist/include/mozilla/ThreadSafeWeakPtr.h:212:68 (libxul.so+0x6121d08)
    #3 gfxFontconfigFontEntry::UnscaledFontCache::MoveToFront(unsigned long) /home/twsmith/code/mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:797:23 (libxul.so+0x6121d08)
    #4 gfxFontconfigFontEntry::UnscaledFontCache::Add(RefPtr<mozilla::gfx::UnscaledFontFontconfig> const&) /home/twsmith/code/mozilla-central/gfx/thebes/gfxFcPlatformFontList.h:157:7 (libxul.so+0x6121d08)
    #5 gfxFontconfigFontEntry::CreateFontInstance(gfxFontStyle const*) /home/twsmith/code/mozilla-central/gfx/thebes/gfxFcPlatformFontList.cpp:933:24 (libxul.so+0x6121d08)
    #6 gfxFontEntry::FindOrMakeFont(gfxFontStyle const*, gfxCharacterMap*) /home/twsmith/code/mozilla-central/gfx/thebes/gfxFontEntry.cpp:266:22 (libxul.so+0x6151b0e) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #7 gfxFontGroup::GetFontAt(int, unsigned int, bool*) /home/twsmith/code/mozilla-central/gfx/thebes/gfxTextRun.cpp:2061:16 (libxul.so+0x61e4b14) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #8 gfxFontGroup::GetFirstValidFont(unsigned int, mozilla::StyleGenericFontFamily*, bool*) /home/twsmith/code/mozilla-central/gfx/thebes/gfxTextRun.cpp:2316:12 (libxul.so+0x61e5c5b) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #9 mozilla::dom::CanvasRenderingContext2D::DrawOrMeasureText(nsTSubstring<char16_t> const&, float, float, mozilla::dom::Optional<double> const&, mozilla::dom::CanvasRenderingContext2D::TextDrawOperation, mozilla::ErrorResult&) /home/twsmith/code/mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:4304:46 (libxul.so+0x7ac400b) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
    #10 mozilla::dom::CanvasRenderingContext2D::FillText(nsTSubstring<char16_t> const&, double, double, mozilla::dom::Optional<double> const&, mozilla::ErrorResult&) /home/twsmith/code/mozilla-central/dom/canvas/CanvasRenderingContext2D.cpp:3864:37 (libxul.so+0x7ac36a3) (BuildId: c5de7b8857d52e0edd7d01649db45e11)
...

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1824200 - Lock around access to mUnscaledFontCache. r=lsalzman

Comment 9

[Jonathan Kew [:jfkthame]]

Same again :) Thanks!

Comment 10

[Tyson Smith [:tsmith]]

Success! No issues are reported with the three patches applied :)

Comment 11

[Jonathan Kew [:jfkthame]]

Updating status flags, not sure how they all got set to "fixed"!

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 9325039 [details]
Bug 1824200 - Make mFTFace an atomic pointer. r=lsalzman

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Probably not easy - data races detected by tsan, but it's unclear how they could be exploited
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 105+
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should be straightforward
• How likely is this patch to cause regressions; how much testing does it need?: low risk, just making more things atomic/locked
• Is Android affected?: No

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9325078 [details]
Bug 1824200 - Lock around access to mUnscaledFontCache. r=lsalzman

Approved to request uplift and land

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 9325039 [details]
Bug 1824200 - Make mFTFace an atomic pointer. r=lsalzman

Beta/Release Uplift Approval Request

• User impact if declined: Potential data races in fontconfig font backend, could result in crashes or maybe other issues.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: (No known STR to produce user-visible issues, only observed via tsan warnings.)
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just making some vars atomic, adding locking - no substantial logic changes.
• String changes made/needed: none
• Is Android affected?: No

Comment 15

[Julien Cristau [:jcristau]]

Make mFTFace an atomic pointer. r=lsalzman
https://hg.mozilla.org/mozilla-central/rev/5947eeacabce
Also make the has-variations state atomic. r=lsalzman
https://hg.mozilla.org/mozilla-central/rev/785d7c9a0341
Lock around access to mUnscaledFontCache. r=lsalzman
https://hg.mozilla.org/mozilla-central/rev/478e795d42d7

Comment 16

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

Comment on attachment 9325039 [details]
Bug 1824200 - Make mFTFace an atomic pointer. r=lsalzman

Approved for 112.0b9

Comment 17

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

https://hg.mozilla.org/releases/mozilla-beta/rev/170675359f57
https://hg.mozilla.org/releases/mozilla-beta/rev/a6b3b4299574
https://hg.mozilla.org/releases/mozilla-beta/rev/977a487ffc79

Comment 18

[Daniel Veditz [:dveditz]]

Since these races were found using a testcase on a bug bounty submission we are adding a bonus bounty for each.

Comment 19

[Tom Ritter [:tjr]]

Attachment: advisory.txt