Skip to content

Commit

Permalink
Fix data race in NGPhysicalFragment
Browse files Browse the repository at this point in the history
TSan reports a race between
`blink::NGPhysicalFragment::SetChildrenInvalid()` and
`NGPhysicalFragment::Type` [1]. The `type_` is const, but is part of a
bitfield, and so reading it is racing with updates to other fields.
Fix by moving `type_` out of the bitfield.

[1] https://luci-milo.appspot.com/ui/inv/task-chromium-swarm.appspot.com-6541a5eaaa567e11/test-results?q=ExactID%3Aninja%3A%2F%2Fcontent%2Ftest%3Acontent_browsertests%2FNavigationControllerBrowserTest.ErrorPageReplacement%2FAll.RDCrashedFrame_BFCacheEnabled+VHash%3A9e749fb4b167a644

Change-Id: I129f36e4e56c8b88670c14e5b71dcc85025738f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4935821
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1209802}
  • Loading branch information
omerktz authored and Chromium LUCI CQ committed Oct 14, 2023
1 parent 16f02fb commit 70abb61
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ struct SameSizeAsNGPhysicalFragment
: public GarbageCollected<SameSizeAsNGPhysicalFragment> {
Member<void*> layout_object;
PhysicalSize size;
unsigned flags;
uint8_t flags[4];
Member<void*> members[3];
};

Expand Down Expand Up @@ -327,12 +327,12 @@ NGPhysicalFragment::NGPhysicalFragment(NGFragmentBuilder* builder,
unsigned sub_type)
: layout_object_(builder->layout_object_),
size_(ToPhysicalSize(builder->size_, builder->GetWritingMode())),
has_floating_descendants_for_paint_(false),
children_valid_(true),
type_(type),
sub_type_(sub_type),
style_variant_((unsigned)builder->style_variant_),
is_hidden_for_paint_(builder->is_hidden_for_paint_),
has_floating_descendants_for_paint_(false),
children_valid_(true),
is_opaque_(builder->is_opaque_),
is_block_in_inline_(builder->is_block_in_inline_),
is_line_for_parallel_flow_(builder->is_line_for_parallel_flow_),
Expand Down Expand Up @@ -420,6 +420,10 @@ NGPhysicalFragment::OutOfFlowData* NGPhysicalFragment::OutOfFlowDataFromBuilder(
NGPhysicalFragment::NGPhysicalFragment(const NGPhysicalFragment& other)
: layout_object_(other.layout_object_),
size_(other.size_),
type_(other.type_),
sub_type_(other.sub_type_),
style_variant_(other.style_variant_),
is_hidden_for_paint_(other.is_hidden_for_paint_),
has_floating_descendants_for_paint_(
other.has_floating_descendants_for_paint_),
has_adjoining_object_descendants_(
Expand All @@ -429,10 +433,6 @@ NGPhysicalFragment::NGPhysicalFragment(const NGPhysicalFragment& other)
children_valid_(other.children_valid_),
has_propagated_descendants_(other.has_propagated_descendants_),
has_hanging_(other.has_hanging_),
type_(other.type_),
sub_type_(other.sub_type_),
style_variant_(other.style_variant_),
is_hidden_for_paint_(other.is_hidden_for_paint_),
is_opaque_(other.is_opaque_),
is_block_in_inline_(other.is_block_in_inline_),
is_line_for_parallel_flow_(other.is_line_for_parallel_flow_),
Expand Down
60 changes: 32 additions & 28 deletions third_party/blink/renderer/core/layout/ng/ng_physical_fragment.h
Original file line number Diff line number Diff line change
Expand Up @@ -777,42 +777,46 @@ class CORE_EXPORT NGPhysicalFragment
Member<LayoutObject> layout_object_;
PhysicalSize size_;

unsigned has_floating_descendants_for_paint_ : 1;
unsigned has_adjoining_object_descendants_ : 1;
unsigned depends_on_percentage_block_size_ : 1;
mutable unsigned children_valid_ : 1;
const uint8_t type_ : 1; // NGFragmentType
const uint8_t sub_type_ : 4; // NGBoxType, NGTextType, or NGLineBoxType
const uint8_t style_variant_ : 2; // NGStyleVariant
const uint8_t is_hidden_for_paint_ : 1;
uint8_t : 0; // NOLINT, zero-length bitfield used to allow the compiler to
// split memory locations. If the above bitfields are part of
// the same memory location as the bitfields below, they will
// all be updated together, which will result in races.

uint8_t has_floating_descendants_for_paint_ : 1; // NOLINT
uint8_t has_adjoining_object_descendants_ : 1; // NOLINT
uint8_t depends_on_percentage_block_size_ : 1; // NOLINT
mutable uint8_t children_valid_ : 1; // NOLINT

// The following bitfields are only to be used by NGPhysicalLineBoxFragment
// (it's defined here to save memory, since that class has no bitfields).
unsigned has_propagated_descendants_ : 1;
unsigned has_hanging_ : 1;

const unsigned type_ : 1; // NGFragmentType
const unsigned sub_type_ : 4; // NGBoxType, NGTextType, or NGLineBoxType
const unsigned style_variant_ : 2; // NGStyleVariant
const unsigned is_hidden_for_paint_ : 1;
unsigned is_opaque_ : 1;
unsigned is_block_in_inline_ : 1;
unsigned is_line_for_parallel_flow_ : 1;
unsigned is_math_fraction_ : 1;
unsigned is_math_operator_ : 1;
unsigned may_have_descendant_above_block_start_ : 1;
uint8_t has_propagated_descendants_ : 1; // NOLINT
uint8_t has_hanging_ : 1; // NOLINT
uint8_t is_opaque_ : 1; // NOLINT
uint8_t is_block_in_inline_ : 1; // NOLINT
uint8_t is_line_for_parallel_flow_ : 1; // NOLINT
uint8_t is_math_fraction_ : 1; // NOLINT
uint8_t is_math_operator_ : 1; // NOLINT
uint8_t may_have_descendant_above_block_start_ : 1; // NOLINT

// The following are only used by NGPhysicalBoxFragment but are initialized
// for all types to allow methods using them to be inlined.
unsigned is_fieldset_container_ : 1;
unsigned is_table_ng_part_ : 1;
unsigned is_painted_atomically_ : 1;
unsigned has_collapsed_borders_ : 1;
unsigned has_first_baseline_ : 1;
unsigned has_last_baseline_ : 1;
unsigned use_last_baseline_for_inline_baseline_ : 1;
const unsigned has_fragmented_out_of_flow_data_ : 1;
const unsigned has_out_of_flow_fragment_child_ : 1;
const unsigned has_out_of_flow_in_fragmentainer_subtree_ : 1;
uint8_t is_fieldset_container_ : 1; // NOLINT
uint8_t is_table_ng_part_ : 1; // NOLINT
uint8_t is_painted_atomically_ : 1; // NOLINT
uint8_t has_collapsed_borders_ : 1; // NOLINT
uint8_t has_first_baseline_ : 1; // NOLINT
uint8_t has_last_baseline_ : 1; // NOLINT
uint8_t use_last_baseline_for_inline_baseline_ : 1; // NOLINT
const uint8_t has_fragmented_out_of_flow_data_ : 1; // NOLINT
const uint8_t has_out_of_flow_fragment_child_ : 1; // NOLINT
const uint8_t has_out_of_flow_in_fragmentainer_subtree_ : 1; // NOLINT

// The following are only used by NGPhysicalLineBoxFragment.
unsigned base_direction_ : 1; // TextDirection
uint8_t base_direction_ : 1; // NOLINT, TextDirection

Member<const PropagatedData> propagated_data_;
Member<const NGBreakToken> break_token_;
Expand Down

0 comments on commit 70abb61

Please sign in to comment.