Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assert access checks + more UseCounter code #1291

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ vars = {
# Three lines of non-changing comments so that
# the commit queue can handle CLs rolling V8
# and whatever else without interference from each other.
'v8_revision': '1a0e6a82c5f5cb43513b781e027569896b60bcb4',
'v8_revision': '4902428d14d7b4244afbbdae1fbfdfa282eeb5b7',
# Three lines of non-changing comments so that
# the commit queue can handle CLs rolling swarming_client
# and whatever else without interference from each other.
Expand Down
2 changes: 1 addition & 1 deletion REPLAY_BACKEND_REV
Original file line number Diff line number Diff line change
@@ -1 +1 @@
6c7e9991dcf557721c0566c2c0d68ac94f2a5b10
8d5f91e301c3d248654f3158db03fd9098eca8bb
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ class MojoPageTimingSender : public PageTimingSender {
const absl::optional<blink::MobileFriendliness>& mobile_friendliness,
uint32_t soft_navigation_count) override {
DCHECK(page_load_metrics_);
mojo::internal::AutoRecordReplayAssertBufferAllocations assertsEnabled("TT-366-1124");
page_load_metrics_->UpdateTiming(
limited_sending_mode_ ? CreatePageLoadTiming() : timing->Clone(),
metadata->Clone(), new_features, std::move(resources),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,16 @@ bool UseCounterFeatureTracker::Test(const UseCounterFeature& feature) const {

bool UseCounterFeatureTracker::TestAndSet(const UseCounterFeature& feature) {
bool has_record = Test(feature);
REPLAY_ASSERT("[TT-366-1456] UseCounterFeatureTracker::TestAndSet %d %d %u",
has_record,
feature.type(),
feature.value());
REPLAY_ASSERT_IF(
// [Rebase-Check] This might change with a rebase.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what exactly might change? can the answer go in the comment?

// Ignore Cross-origin access check (WebFeature::kCrossOriginPropertyAccess)
// because it looks like an allowed divergence.
(int)feature.type() != 0 || ((int)feature.value() != 1977 &&
((int)feature.value() < 4115 || (int)feature.value() > 4144)),
"[TT-366-1480] UseCounterFeatureTracker::TestAndSet %d %d %u",
has_record,
feature.type(),
feature.value());
Set(feature, true);
return has_record;
}
Expand Down
2 changes: 2 additions & 0 deletions third_party/blink/renderer/core/css/css_value.h
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,8 @@ class CORE_EXPORT CSSValue : public GarbageCollected<CSSValue> {
void TraceAfterDispatch(blink::Visitor* visitor) const {}
void Trace(Visitor*) const;

uint8_t ReplayGetClassType() const { return class_type_; }

protected:
enum ClassType {
kNumericLiteralClass,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,6 @@ KURL CSSParserContext::CompleteURL(const String& url) const {
}

void CSSParserContext::Count(WebFeature feature) const {
REPLAY_ASSERT("[TT-366-1467] CSSParserContext::Count feat %d %d",
IsUseCounterRecordingEnabled(),
feature);
if (IsUseCounterRecordingEnabled())
document_->CountUse(feature);
}
Expand All @@ -242,7 +239,7 @@ void CSSParserContext::CountDeprecation(WebFeature feature) const {
}

void CSSParserContext::Count(CSSParserMode mode, CSSPropertyID property) const {
REPLAY_ASSERT("[TT-366-1467] CSSParserContext::Count prop %d %d %d",
REPLAY_ASSERT_MAYBE_EVENTS_DISALLOWED("[TT-366-1480] CSSParserContext::Count prop %d %d %d",
IsUseCounterRecordingEnabled(),
(int)mode,
property);
Expand Down
2 changes: 2 additions & 0 deletions third_party/blink/renderer/core/css/parser/css_parser_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,8 @@ StyleRuleBase* CSSParserImpl::ConsumeAtRule(CSSParserTokenStream& stream,
StyleRuleBase* CSSParserImpl::ConsumeQualifiedRule(
CSSParserTokenStream& stream,
AllowedRulesType allowed_rules) {
REPLAY_ASSERT("[TT-366-1480] CSSParserImpl::ConsumeQualifiedRule %d",
allowed_rules);
if (allowed_rules <= kRegularRules) {
return ConsumeStyleRule(stream);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,9 @@ ArenaUniquePtr<CSSParserSelector> CSSSelectorParser::ConsumeRelativeSelector(
ArenaUniquePtr<CSSParserSelector> CSSSelectorParser::ConsumeComplexSelector(
CSSParserTokenRange& range) {
ArenaUniquePtr<CSSParserSelector> selector = ConsumeCompoundSelector(range);
REPLAY_ASSERT("[TT-366-1480] CSSSelectorParser::ConsumeComplexSelector %u %d",
range.size(),
!!selector);
if (!selector)
return nullptr;

Expand Down
4 changes: 2 additions & 2 deletions third_party/blink/renderer/core/css/resolver/font_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ void FontBuilder::CreateFont(ComputedStyle& style,
const ComputedStyle* parent_style) {
DCHECK(document_);

recordreplay::Assert("[RUN-1436-2237] FontBuilder::CreateFont A %u", flags_);
REPLAY_ASSERT("[TT-366-1480] FontBuilder::CreateFont A %u", flags_);
if (!flags_)
return;

Expand All @@ -496,7 +496,7 @@ void FontBuilder::CreateFont(ComputedStyle& style,
FontSelector* font_selector = ComputeFontSelector(style);
UpdateAdjustedSize(description, style, font_selector);

recordreplay::Assert("[RUN-1436-2237] FontBuilder::CreateFont B %d",
REPLAY_ASSERT("[TT-366-1480] FontBuilder::CreateFont B %d",
!!font_selector);
style.SetFontInternal(Font(description, font_selector));
flags_ = 0;
Expand Down
11 changes: 6 additions & 5 deletions third_party/blink/renderer/core/css/resolver/match_result.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,33 +68,34 @@ void MatchResult::AddMatchedProperties(
void MatchResult::FinishAddingUARules() {
DCHECK_EQ(current_origin_, CascadeOrigin::kUserAgent);
current_origin_ = CascadeOrigin::kUser;
REPLAY_ASSERT("[RUN-2424-3053] FinishAddingUARules");
REPLAY_ASSERT("[TT-366-1480] FinishAddingUARules");
}

void MatchResult::FinishAddingUserRules() {
DCHECK_EQ(current_origin_, CascadeOrigin::kUser);
current_origin_ = CascadeOrigin::kAuthorPresentationalHint;
REPLAY_ASSERT("[RUN-2424-3053] FinishAddingUserRules");
REPLAY_ASSERT("[TT-366-1480] FinishAddingUserRules");
}

void MatchResult::FinishAddingPresentationalHints() {
DCHECK_EQ(current_origin_, CascadeOrigin::kAuthorPresentationalHint);
current_origin_ = CascadeOrigin::kAuthor;
REPLAY_ASSERT("[RUN-2424-3053] FinishAddingPresentationalHints");
REPLAY_ASSERT("[TT-366-1480] FinishAddingPresentationalHints");
}

void MatchResult::FinishAddingAuthorRulesForTreeScope(
const TreeScope& tree_scope) {
DCHECK_EQ(current_origin_, CascadeOrigin::kAuthor);
tree_scopes_.push_back(&tree_scope);
current_tree_order_ = base::ClampAdd(current_tree_order_, 1);
REPLAY_ASSERT("[RUN-2424-3053] MatchResult::FinishAddingAuthorRulesForTreeScope %u",
REPLAY_ASSERT("[TT-366-1480] MatchResult::FinishAddingAuthorRulesForTreeScope %d %u",
tree_scope.RecordReplayId(),
tree_scopes_.size()
);
}

void MatchResult::Reset() {
REPLAY_ASSERT("[RUN-2424-3053] MatchResult::Reset %u",
REPLAY_ASSERT("[TT-366-1480] MatchResult::Reset %u",
(unsigned)current_tree_order_
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,8 @@ FontDescription::FamilyDescription StyleBuilderConverterBase::ConvertFontFamily(
const CSSValue& value,
FontBuilder* font_builder,
const Document* document_for_count) {
REPLAY_ASSERT("[TT-366-1480] StyleBuilderConverterBase::ConvertFontFamily %s",
value.CssText().Utf8().c_str());
FontDescription::FamilyDescription desc(FontDescription::kNoFamily);

if (const auto* system_font =
Expand Down
18 changes: 18 additions & 0 deletions third_party/blink/renderer/core/css/resolver/style_cascade.cc
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ void StyleCascade::AddInterpolations(const ActiveInterpolationsMap* map,
}

void StyleCascade::Apply(CascadeFilter filter) {
REPLAY_ASSERT("[TT-366-1480] StyleCascade::Apply %d", (int)generation_);
AnalyzeIfNeeded();

CascadeResolver resolver(filter, ++generation_);
Expand Down Expand Up @@ -637,6 +638,12 @@ void StyleCascade::LookupAndApply(const CSSProperty& property,
DCHECK(!resolver.IsLocked(property));

CascadePriority* priority = map_.Find(name);

REPLAY_ASSERT("[TT-366-1480] StyleCascade::LookupAndApply %d %d %d %d",
priority ? (int)priority->GetPosition() : -1,
priority ? (int)priority->GetGeneration() : -1,
priority ? (int)priority->GetOrigin() : -1,
name.Id());
if (!priority)
return;

Expand All @@ -660,6 +667,11 @@ void StyleCascade::LookupAndApplyValue(const CSSProperty& property,
void StyleCascade::LookupAndApplyDeclaration(const CSSProperty& property,
CascadePriority* priority,
CascadeResolver& resolver) {
REPLAY_ASSERT("[TT-366-1480] StyleCascade::LookupAndApplyDeclaration %d %d %d",
property.PropertyID(),
(int)priority->GetGeneration(),
(int)resolver.generation_
);
if (priority->GetGeneration() >= resolver.generation_) {
// Already applied this generation.
return;
Expand Down Expand Up @@ -787,6 +799,12 @@ const CSSValue* StyleCascade::Resolve(const CSSProperty& property,
const CSSValue* StyleCascade::ResolveSubstitutions(const CSSProperty& property,
const CSSValue& value,
CascadeResolver& resolver) {
REPLAY_ASSERT("[TT-366-1480] StyleCascade::ResolveSubstitutions %d %d %d %d %d",
property.PropertyID(),
(int)value.ReplayGetClassType(),
!!DynamicTo<CSSCustomPropertyDeclaration>(value),
!!DynamicTo<CSSVariableReferenceValue>(value),
!!DynamicTo<cssvalue::CSSPendingSubstitutionValue>(value));
if (const auto* v = DynamicTo<CSSCustomPropertyDeclaration>(value))
return ResolveCustomProperty(property, *v, resolver);
if (const auto* v = DynamicTo<CSSVariableReferenceValue>(value))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1874,6 +1874,9 @@ StyleResolver::CacheSuccess StyleResolver::ApplyMatchedCache(
const MatchResult& match_result) {
const Element& element = state.GetElement();

REPLAY_ASSERT("[TT-366-1480] StyleResolver::ApplyMatchedCache %d",
element.RecordReplayId());

MatchedPropertiesCache::Key key(match_result);

bool is_inherited_cache_hit = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,8 @@ const CSSValue& StyleResolverState::ResolveLightDarkPair(
}

void StyleResolverState::UpdateFont() {
recordreplay::Assert("[RUN-1436-2226] StyleResolverState::UpdateFont %d",
GetElement().RecordReplayId());
REPLAY_ASSERT("[TT-366-1480] StyleResolverState::UpdateFont %d",
GetElement().RecordReplayId());
GetFontBuilder().CreateFont(StyleRef(), ParentStyle());
SetConversionFontSizes(
CSSToLengthConversionData::FontSizes(Style(), RootElementStyle()));
Expand Down
7 changes: 3 additions & 4 deletions third_party/blink/renderer/core/dom/document.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8749,9 +8749,6 @@ void Document::CountUse(mojom::WebFeature feature) const {
}

void Document::CountUse(mojom::WebFeature feature) {
REPLAY_ASSERT("[TT-366-1467] Document::CountUse %d %d",
!!execution_context_,
feature);
if (execution_context_)
execution_context_->CountUse(feature);
}
Expand All @@ -8762,7 +8759,9 @@ void Document::CountDeprecation(mojom::WebFeature feature) {
}

void Document::CountProperty(CSSPropertyID property) const {
REPLAY_ASSERT("[TT-366-1467] Document::CountProperty %d %d",
REPLAY_ASSERT_MAYBE_EVENTS_DISALLOWED("[TT-366-1480] Document::CountProperty %d %d %d %d",
domWindow() ? domWindow()->RecordReplayId() : -1,
GetFrame() ? GetFrame()->RecordReplayId() : -1,
!!Loader(),
property);
if (DocumentLoader* loader = Loader()) {
Expand Down
5 changes: 5 additions & 0 deletions third_party/blink/renderer/core/dom/element.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2294,6 +2294,11 @@ void Element::setAttribute(const QualifiedName& name,
DISABLE_CFI_PERF
void Element::AttributeChanged(const AttributeModificationParams& params) {
const QualifiedName& name = params.name;
REPLAY_ASSERT("[TT-366-1480] Element::AttributeChanged %d %s=%s",
params.reason,
name.LocalName().Utf8().c_str(),
params.new_value.Utf8().c_str()
);
if (name == html_names::kSlotAttr && params.old_value != params.new_value) {
if (ShadowRoot* root = ShadowRootOfParent())
root->DidChangeHostChildSlotName(params.old_value, params.new_value);
Expand Down
3 changes: 0 additions & 3 deletions third_party/blink/renderer/core/frame/local_dom_window.cc
Original file line number Diff line number Diff line change
Expand Up @@ -725,9 +725,6 @@ void LocalDOMWindow::AddInspectorIssue(AuditsIssue issue) {
}

void LocalDOMWindow::CountUse(mojom::WebFeature feature) {
REPLAY_ASSERT("[TT-366-1467] LocalDOMWindow::CountUse %d %d",
!!GetFrame(),
GetFrame() ? !!GetFrame()->Loader().GetDocumentLoader() : -1);
if (!GetFrame())
return;
if (auto* loader = GetFrame()->Loader().GetDocumentLoader())
Expand Down
6 changes: 0 additions & 6 deletions third_party/blink/renderer/core/frame/use_counter_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -179,12 +179,6 @@ void UseCounterImpl::Count(const UseCounterFeature& feature,
if (recordreplay::AreEventsDisallowed("UseCounterImpl::Count"))
return;

REPLAY_ASSERT("[TT-366-1467] UseCounterImpl::Count %d %u %d %d",
!!source_frame,
mute_count_,
feature.type(),
feature.value());

if (!source_frame)
return;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,10 @@ void HTMLTreeBuilder::ConstructTree(AtomicHTMLToken* token) {
else
ProcessToken(token);

REPLAY_ASSERT("[TT-366-1480] HTMLTreeBuilder::ConstructTree %d %d",
parser_->IsDetached(),
tree_.HasPendingTasks());

if (parser_->IsDetached())
return;

Expand Down
74 changes: 2 additions & 72 deletions third_party/blink/renderer/platform/context_lifecycle_notifier.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,96 +23,26 @@ bool ContextLifecycleNotifier::IsContextDestroyed() const {
void ContextLifecycleNotifier::AddContextLifecycleObserver(
ContextLifecycleObserver* observer) {
observers_.AddObserver(observer);

if (recordreplay::IsRecordingOrReplaying("values") && recordreplay::IsReplaying())
replay_observers_.push_back(observer);
}

void ContextLifecycleNotifier::RemoveContextLifecycleObserver(
ContextLifecycleObserver* observer) {
DCHECK(observers_.HasObserver(observer));
observers_.RemoveObserver(observer);

for (wtf_size_t i = 0; i < replay_observers_.size(); i++) {
if (replay_observers_[i] == observer) {
replay_observers_.EraseAt(i);
break;
}
}
}

void ContextLifecycleNotifier::NotifyContextDestroyed() {
context_destroyed_ = true;

ScriptForbiddenScope forbid_script;
HeapVector<Member<ContextLifecycleObserver>> observers;
observers_.ForEachObserver([&](ContextLifecycleObserver* observer) {
observers.push_back(observer);
observers_.ForEachObserver([](ContextLifecycleObserver* observer) {
Copy link
Author

@Domiii Domiii Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: Undid outdated changes. This looks like a very old version of assuring determinism of a HeapObserverSet. I already fixed most of that inside HeapObserverSet itself a long time ago. I added the additional "leaking on Remove" part down below.

observer->NotifyContextDestroyed();
});
observers_.Clear();

std::sort(observers.begin(), observers.end(),
recordreplay::CompareMemberByPointerId<Member<ContextLifecycleObserver>>());

// When replaying, notify the same observers in the same order which were
// notified when recording. Because of the use of weak pointers in the
// HeapObserverSet the set contents can vary, so we manually record/replay
// the objects which should be notified. The replay_observers_ vector holds
// strong references on the observers when replaying so none of the observers
// we need to notify should already be collected.
if (recordreplay::IsRecordingOrReplaying("values", "ContextLifecycleNotifier::NotifyContextDestroyed") &&
recordreplay::FeatureEnabled("pointer-ids") &&
!recordreplay::AreEventsDisallowed()) {
size_t num_observers = recordreplay::RecordReplayValue("NotifyContextDestroyed NumObservers", observers.size());
int* observer_ids = new int[num_observers];

if (recordreplay::IsRecording()) {
for (wtf_size_t i = 0; i < observers.size(); i++) {
int id = recordreplay::PointerId(observers[i]);
CHECK(id);
observer_ids[i] = id;
}
}

recordreplay::RecordReplayBytes("ContextLifecycleNotifier::NotifyContextDestroyed ObserverIds",
observer_ids, num_observers * sizeof(int));

if (recordreplay::IsReplaying()) {
HeapVector<Member<ContextLifecycleObserver>> new_observers;
for (ContextLifecycleObserver* observer : observers) {
int id = recordreplay::PointerId(observer);
CHECK(id);
bool found = false;
for (wtf_size_t i = 0; i < num_observers; i++) {
if (observer_ids[i] == id) {
found = true;
break;
}
}
if (found)
new_observers.push_back(observer);
}

observers = std::move(new_observers);
}

delete[] observer_ids;
}

for (ContextLifecycleObserver* observer : observers) {
if (!recordreplay::AreEventsDisallowed()) {
recordreplay::Assert("ContextLifecycleNotifier::NotifyContextDestroyed #1 %d",
recordreplay::PointerId(observer));
}
observer->NotifyContextDestroyed();
}

replay_observers_.clear();
}

void ContextLifecycleNotifier::Trace(Visitor* visitor) const {
visitor->Trace(observers_);
visitor->Trace(replay_observers_);
}

} // namespace blink
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,6 @@ class PLATFORM_EXPORT ContextLifecycleNotifier : public GarbageCollectedMixin {
private:
HeapObserverSet<ContextLifecycleObserver> observers_;
bool context_destroyed_ = false;

// When replaying, strong references are held on all observers, which are
// cleared out after the context is destroyed. This ensures we can notify
// the same observers when replaying as when originally recording
HeapVector<Member<ContextLifecycleObserver>> replay_observers_;
};

} // namespace blink
Expand Down
Loading