[fonts] Disable concurrent tracing for FontCacheKey in ASAN builds [chromium/src : main]

0 views
Skip to first unread message

Michael Lippautz (Gerrit)

unread,
Jun 10, 2024, 11:29:20 AMJun 10
to Omer Katz, Kentaro Hara, AyeAye, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, oilpan-rev...@chromium.org, kouhe...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org
Attention needed from Nico Weber and Omer Katz

Michael Lippautz voted and added 1 comment

Votes added by Michael Lippautz

Commit-Queue+1

1 comment

Patchset-level comments
File-level comment, Patchset 4 (Latest):
Michael Lippautz . resolved

Omer, could you please review the GC changes?

Open in Gerrit

Related details

Attention is currently required from:
  • Nico Weber
  • Omer Katz
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7f192ee6b12ab75aa6e7188d58665bca52d827bd
Gerrit-Change-Number: 5614971
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
Gerrit-Reviewer: Omer Katz <omer...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Nico Weber <tha...@chromium.org>
Gerrit-Attention: Omer Katz <omer...@chromium.org>
Gerrit-Comment-Date: Mon, 10 Jun 2024 15:29:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Nico Weber (Gerrit)

unread,
Jun 10, 2024, 10:06:11 PMJun 10
to Michael Lippautz, Omer Katz, Kentaro Hara, AyeAye, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, oilpan-rev...@chromium.org, kouhe...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org
Attention needed from Michael Lippautz and Omer Katz

Nico Weber added 1 comment

Patchset-level comments
File-level comment, Patchset 6 (Latest):
Nico Weber . resolved

I sent a try job with this and the libc++ roll patched in: https://chromium-review.googlesource.com/c/chromium/src/+/5614917

Before: 9 test suite failures and 1000 failed tests: https://ci.chromium.org/ui/p/chromium/builders/try/linux_chromium_asan_rel_ng/1975737/test-results?sortby=&groupby=

After: 3 test suite failures and 10 failed tests: https://ci.chromium.org/ui/p/chromium/builders/try/linux_chromium_asan_rel_ng/1976797/test-results?sortby=&groupby=

That's a huge improvement.

But the first 2 of the 10 failed tests (I didn't look at more) have a fairly similar stack to what's fixed here:


```
==386739==ERROR: AddressSanitizer: container-overflow on address 0x7eec001caea9 at pc 0x5f7cfa75163e bp 0x7ffcd5263750 sp 0x7ffcd5262f10
WRITE of size 24 at 0x7eec001caea9 thread T0 (interactive_ui_)
    #0 0x5f7cfa75163d in __asan_memcpy /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:63:3
    #1 0x5f7cfa78d394 in std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>::basic_string(std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>&&) third_party/libc++/src/include/string:975:9
    #2 0x5f7d28582ea9 in FontFaceCreationParams third_party/blink/renderer/platform/fonts/font_face_creation_params.h:48:7
    #3 0x5f7d28582ea9 in FontCacheKey third_party/blink/renderer/platform/fonts/font_cache_key.h:55:8
    #4 0x5f7d28582ea9 in KeyValuePair third_party/blink/renderer/platform/wtf/key_value_pair.h:37:8
    #5 0x5f7d28582ea9 in WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>* WTF::ConstructTraits<WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>, WTF::HashMapValueTraits<WTF::HashTraits<blink::FontCacheKey>, WTF::HashTraits<cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>>, blink::HeapAllocator>::Construct<WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>>(void*, WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>&&) third_party/blink/renderer/platform/wtf/construct_traits.h:27:9
    ...
    #14 0x5f7d2852c1dd in blink::FontCache::GetFontData(blink::FontDescription const&, WTF::AtomicString const&, blink::AlternateFontName) third_party/blink/renderer/platform/fonts/font_cache.cc:182:47
    #15 0x5f7d28561940 in blink::FontFallbackList::GetFontData(blink::FontDescription const&) third_party/blink/renderer/platform/fonts/font_fallback_list.cc:163:33
    ```

Looks like that's now about inserting the element into a hashmap and no longer about GC, so it might be somewhat unrelated to this CL. But it's at least similar. (…but possibly something for a different CL.)

Open in Gerrit

Related details

Attention is currently required from:
  • Michael Lippautz
  • Omer Katz
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7f192ee6b12ab75aa6e7188d58665bca52d827bd
Gerrit-Change-Number: 5614971
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
Gerrit-Reviewer: Omer Katz <omer...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
Gerrit-Attention: Omer Katz <omer...@chromium.org>
Gerrit-Comment-Date: Tue, 11 Jun 2024 02:06:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Nico Weber (Gerrit)

unread,
Jun 10, 2024, 10:09:59 PMJun 10
to Michael Lippautz, Omer Katz, Kentaro Hara, AyeAye, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, oilpan-rev...@chromium.org, kouhe...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org
Attention needed from Michael Lippautz and Omer Katz

Nico Weber added 1 comment

Patchset-level comments
Nico Weber . resolved

I sent a try job with this and the libc++ roll patched in: https://chromium-review.googlesource.com/c/chromium/src/+/5614917

Before: 9 test suite failures and 1000 failed tests: https://ci.chromium.org/ui/p/chromium/builders/try/linux_chromium_asan_rel_ng/1975737/test-results?sortby=&groupby=

After: 3 test suite failures and 10 failed tests: https://ci.chromium.org/ui/p/chromium/builders/try/linux_chromium_asan_rel_ng/1976797/test-results?sortby=&groupby=

That's a huge improvement.

But the first 2 of the 10 failed tests (I didn't look at more) have a fairly similar stack to what's fixed here:


```
==386739==ERROR: AddressSanitizer: container-overflow on address 0x7eec001caea9 at pc 0x5f7cfa75163e bp 0x7ffcd5263750 sp 0x7ffcd5262f10
WRITE of size 24 at 0x7eec001caea9 thread T0 (interactive_ui_)
    #0 0x5f7cfa75163d in __asan_memcpy /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:63:3
    #1 0x5f7cfa78d394 in std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>::basic_string(std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>&&) third_party/libc++/src/include/string:975:9
    #2 0x5f7d28582ea9 in FontFaceCreationParams third_party/blink/renderer/platform/fonts/font_face_creation_params.h:48:7
    #3 0x5f7d28582ea9 in FontCacheKey third_party/blink/renderer/platform/fonts/font_cache_key.h:55:8
    #4 0x5f7d28582ea9 in KeyValuePair third_party/blink/renderer/platform/wtf/key_value_pair.h:37:8
    #5 0x5f7d28582ea9 in WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>* WTF::ConstructTraits<WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>, WTF::HashMapValueTraits<WTF::HashTraits<blink::FontCacheKey>, WTF::HashTraits<cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>>, blink::HeapAllocator>::Construct<WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>>(void*, WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>&&) third_party/blink/renderer/platform/wtf/construct_traits.h:27:9
    ...
    #14 0x5f7d2852c1dd in blink::FontCache::GetFontData(blink::FontDescription const&, WTF::AtomicString const&, blink::AlternateFontName) third_party/blink/renderer/platform/fonts/font_cache.cc:182:47
    #15 0x5f7d28561940 in blink::FontFallbackList::GetFontData(blink::FontDescription const&) third_party/blink/renderer/platform/fonts/font_fallback_list.cc:163:33
    ```

Looks like that's now about inserting the element into a hashmap and no longer about GC, so it might be somewhat unrelated to this CL. But it's at least similar. (…but possibly something for a different CL.)

Nico Weber

Looks like 9 of the remaining 10 failures have this stack (MediaNotificationDeviceProviderTest.MaybeRemoveDefaultDeviceDoesNotRemoveDefaultDevice is the one that doesn't).

Gerrit-Comment-Date: Tue, 11 Jun 2024 02:09:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Weber <tha...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Michael Lippautz (Gerrit)

unread,
Jun 11, 2024, 2:12:11 AMJun 11
to Omer Katz, Kentaro Hara, AyeAye, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, oilpan-rev...@chromium.org, kouhe...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org
Attention needed from Nico Weber and Omer Katz

Michael Lippautz added 1 comment

Patchset-level comments
Nico Weber . resolved

I sent a try job with this and the libc++ roll patched in: https://chromium-review.googlesource.com/c/chromium/src/+/5614917

Before: 9 test suite failures and 1000 failed tests: https://ci.chromium.org/ui/p/chromium/builders/try/linux_chromium_asan_rel_ng/1975737/test-results?sortby=&groupby=

After: 3 test suite failures and 10 failed tests: https://ci.chromium.org/ui/p/chromium/builders/try/linux_chromium_asan_rel_ng/1976797/test-results?sortby=&groupby=

That's a huge improvement.

But the first 2 of the 10 failed tests (I didn't look at more) have a fairly similar stack to what's fixed here:


```
==386739==ERROR: AddressSanitizer: container-overflow on address 0x7eec001caea9 at pc 0x5f7cfa75163e bp 0x7ffcd5263750 sp 0x7ffcd5262f10
WRITE of size 24 at 0x7eec001caea9 thread T0 (interactive_ui_)
    #0 0x5f7cfa75163d in __asan_memcpy /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:63:3
    #1 0x5f7cfa78d394 in std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>::basic_string(std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>&&) third_party/libc++/src/include/string:975:9
    #2 0x5f7d28582ea9 in FontFaceCreationParams third_party/blink/renderer/platform/fonts/font_face_creation_params.h:48:7
    #3 0x5f7d28582ea9 in FontCacheKey third_party/blink/renderer/platform/fonts/font_cache_key.h:55:8
    #4 0x5f7d28582ea9 in KeyValuePair third_party/blink/renderer/platform/wtf/key_value_pair.h:37:8
    #5 0x5f7d28582ea9 in WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>* WTF::ConstructTraits<WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>, WTF::HashMapValueTraits<WTF::HashTraits<blink::FontCacheKey>, WTF::HashTraits<cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>>, blink::HeapAllocator>::Construct<WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>>(void*, WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>&&) third_party/blink/renderer/platform/wtf/construct_traits.h:27:9
    ...
    #14 0x5f7d2852c1dd in blink::FontCache::GetFontData(blink::FontDescription const&, WTF::AtomicString const&, blink::AlternateFontName) third_party/blink/renderer/platform/fonts/font_cache.cc:182:47
    #15 0x5f7d28561940 in blink::FontFallbackList::GetFontData(blink::FontDescription const&) third_party/blink/renderer/platform/fonts/font_fallback_list.cc:163:33
    ```

Looks like that's now about inserting the element into a hashmap and no longer about GC, so it might be somewhat unrelated to this CL. But it's at least similar. (…but possibly something for a different CL.)

Nico Weber

Looks like 9 of the remaining 10 failures have this stack (MediaNotificationDeviceProviderTest.MaybeRemoveDefaultDeviceDoesNotRemoveDefaultDevice is the one that doesn't).

Michael Lippautz

The failure is unrelated to GC but indeed related to hashmap insertion.

Open in Gerrit

Related details

Attention is currently required from:
  • Nico Weber
  • Omer Katz
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7f192ee6b12ab75aa6e7188d58665bca52d827bd
Gerrit-Change-Number: 5614971
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
Gerrit-Reviewer: Omer Katz <omer...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Nico Weber <tha...@chromium.org>
Gerrit-Attention: Omer Katz <omer...@chromium.org>
Gerrit-Comment-Date: Tue, 11 Jun 2024 06:12:00 +0000
satisfied_requirement
unsatisfied_requirement
open
diffy

Omer Katz (Gerrit)

unread,
Jun 11, 2024, 3:46:48 AMJun 11
to Michael Lippautz, Kentaro Hara, AyeAye, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, oilpan-rev...@chromium.org, kouhe...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org
Attention needed from Michael Lippautz and Nico Weber

Omer Katz voted and added 1 comment

Votes added by Omer Katz

Code-Review+1

1 comment

Patchset-level comments
Omer Katz . resolved

lgtm

Open in Gerrit

Related details

Attention is currently required from:
  • Michael Lippautz
  • Nico Weber
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7f192ee6b12ab75aa6e7188d58665bca52d827bd
Gerrit-Change-Number: 5614971
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
Gerrit-Reviewer: Omer Katz <omer...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
Gerrit-Attention: Nico Weber <tha...@chromium.org>
Gerrit-Comment-Date: Tue, 11 Jun 2024 07:46:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Michael Lippautz (Gerrit)

unread,
Jun 11, 2024, 4:20:14 AMJun 11
to Omer Katz, Kentaro Hara, AyeAye, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, oilpan-rev...@chromium.org, kouhe...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org
Attention needed from Nico Weber

Michael Lippautz voted Commit-Queue+2

Commit-Queue+2
Open in Gerrit

Related details

Attention is currently required from:
  • Nico Weber
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7f192ee6b12ab75aa6e7188d58665bca52d827bd
Gerrit-Change-Number: 5614971
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
Gerrit-Reviewer: Omer Katz <omer...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Nico Weber <tha...@chromium.org>
Gerrit-Comment-Date: Tue, 11 Jun 2024 08:20:02 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Chromium LUCI CQ (Gerrit)

unread,
Jun 11, 2024, 6:52:55 AMJun 11
to Michael Lippautz, Omer Katz, Kentaro Hara, AyeAye, Nico Weber, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, oilpan-rev...@chromium.org, kouhe...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:
[fonts] Disable concurrent tracing for FontCacheKey in ASAN builds

`FontCacheKey` contains an `std::string` which contains poisoned
metadata for detecting buffer overflows in short strings. Copying this
string as part of `KeyValuePairExtractor` will thus trigger ASAN
warnings. Just don't use concurrent tracing when running with ASAN.

This is even true if `kCanTraceConcurrently` is false as we always
used the same infrastructure for simplicity.

Side note: The better way of implementing this would be to define a
token that can be extracted explicitly from a key and users of the
traits need to implement. This token could then be checked against
what we need (e.g.  being empty or deleted). This requires a larger
effort in more types though.
Bug: 346174906
Change-Id: I7f192ee6b12ab75aa6e7188d58665bca52d827bd
Commit-Queue: Michael Lippautz <mlip...@chromium.org>
Reviewed-by: Omer Katz <omer...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1313289}
Files:
  • M third_party/blink/renderer/platform/fonts/font_cache_key.h
  • M third_party/blink/renderer/platform/heap/collection_support/heap_hash_table_backing.h
Change size: S
Delta: 2 files changed, 29 insertions(+), 8 deletions(-)
Branch: refs/heads/main
Submit Requirements:
  • requirement satisfiedCode-Review: +1 by Omer Katz
Open in Gerrit
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: merged
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7f192ee6b12ab75aa6e7188d58665bca52d827bd
Gerrit-Change-Number: 5614971
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
Gerrit-Reviewer: Omer Katz <omer...@chromium.org>
open
diffy
satisfied_requirement

Michael Lippautz (Gerrit)

unread,
Jun 11, 2024, 7:22:45 AMJun 11
to Chromium LUCI CQ, Omer Katz, Kentaro Hara, AyeAye, Nico Weber, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, oilpan-rev...@chromium.org, kouhe...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org

Michael Lippautz added 1 comment

Patchset-level comments
Nico Weber . resolved

I sent a try job with this and the libc++ roll patched in: https://chromium-review.googlesource.com/c/chromium/src/+/5614917

Before: 9 test suite failures and 1000 failed tests: https://ci.chromium.org/ui/p/chromium/builders/try/linux_chromium_asan_rel_ng/1975737/test-results?sortby=&groupby=

After: 3 test suite failures and 10 failed tests: https://ci.chromium.org/ui/p/chromium/builders/try/linux_chromium_asan_rel_ng/1976797/test-results?sortby=&groupby=

That's a huge improvement.

But the first 2 of the 10 failed tests (I didn't look at more) have a fairly similar stack to what's fixed here:


```
==386739==ERROR: AddressSanitizer: container-overflow on address 0x7eec001caea9 at pc 0x5f7cfa75163e bp 0x7ffcd5263750 sp 0x7ffcd5262f10
WRITE of size 24 at 0x7eec001caea9 thread T0 (interactive_ui_)
    #0 0x5f7cfa75163d in __asan_memcpy /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:63:3
    #1 0x5f7cfa78d394 in std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>::basic_string(std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>&&) third_party/libc++/src/include/string:975:9
    #2 0x5f7d28582ea9 in FontFaceCreationParams third_party/blink/renderer/platform/fonts/font_face_creation_params.h:48:7
    #3 0x5f7d28582ea9 in FontCacheKey third_party/blink/renderer/platform/fonts/font_cache_key.h:55:8
    #4 0x5f7d28582ea9 in KeyValuePair third_party/blink/renderer/platform/wtf/key_value_pair.h:37:8
    #5 0x5f7d28582ea9 in WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>* WTF::ConstructTraits<WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>, WTF::HashMapValueTraits<WTF::HashTraits<blink::FontCacheKey>, WTF::HashTraits<cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>>, blink::HeapAllocator>::Construct<WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>>(void*, WTF::KeyValuePair<blink::FontCacheKey, cppgc::internal::BasicMember<blink::FontPlatformData const, cppgc::internal::WeakMemberTag, cppgc::internal::DijkstraWriteBarrierPolicy, cppgc::internal::DisabledCheckingPolicy, cppgc::internal::CompressedPointer>>&&) third_party/blink/renderer/platform/wtf/construct_traits.h:27:9
    ...
    #14 0x5f7d2852c1dd in blink::FontCache::GetFontData(blink::FontDescription const&, WTF::AtomicString const&, blink::AlternateFontName) third_party/blink/renderer/platform/fonts/font_cache.cc:182:47
    #15 0x5f7d28561940 in blink::FontFallbackList::GetFontData(blink::FontDescription const&) third_party/blink/renderer/platform/fonts/font_fallback_list.cc:163:33
    ```

Looks like that's now about inserting the element into a hashmap and no longer about GC, so it might be somewhat unrelated to this CL. But it's at least similar. (…but possibly something for a different CL.)

Nico Weber

Looks like 9 of the remaining 10 failures have this stack (MediaNotificationDeviceProviderTest.MaybeRemoveDefaultDeviceDoesNotRemoveDefaultDevice is the one that doesn't).

Michael Lippautz

The failure is unrelated to GC but indeed related to hashmap insertion.

Michael Lippautz

The other crashes just look like copy construction of std::string (?) Is the feature maybe not working such cases?

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7f192ee6b12ab75aa6e7188d58665bca52d827bd
Gerrit-Change-Number: 5614971
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
Gerrit-Reviewer: Omer Katz <omer...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Comment-Date: Tue, 11 Jun 2024 11:22:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Michael Lippautz <mlip...@chromium.org>
Comment-In-Reply-To: Nico Weber <tha...@chromium.org>
satisfied_requirement
open
diffy

Nico Weber (Gerrit)

unread,
Jun 11, 2024, 7:49:09 AMJun 11
to Chromium LUCI CQ, Michael Lippautz, Omer Katz, Kentaro Hara, AyeAye, Nico Weber, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, oilpan-rev...@chromium.org, kouhe...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org

Nico Weber added 1 comment

Patchset-level comments
Nico Weber

I'd imagine that we'd have significantly more failing tests if copy construction just didn't work 😊

I'm guessing it's somehow related to having a std::string in a HashTable key. This might be the only instance of that.

I'll put this comment on the bug and will keep looking.

Thanks for this fix!

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7f192ee6b12ab75aa6e7188d58665bca52d827bd
Gerrit-Change-Number: 5614971
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
Gerrit-Reviewer: Omer Katz <omer...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Comment-Date: Tue, 11 Jun 2024 11:49:01 +0000
satisfied_requirement
open
diffy
Reply all
Reply to author
Forward
0 new messages