Skip to content

Conversation

@Domiii
Copy link

@Domiii Domiii commented Jun 27, 2024

@Domiii Domiii requested review from klochek and toshok June 27, 2024 16:32
@Domiii Domiii self-assigned this Jun 27, 2024
@Domiii Domiii force-pushed the dominik/tt-1480-assert-more-counter-code branch from 5310396 to 59ad771 Compare June 27, 2024 17:05
@Domiii Domiii removed request for klochek and toshok July 4, 2024 13:46
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.

@Domiii Domiii closed this Jul 9, 2024
@Domiii Domiii deleted the dominik/tt-1480-assert-more-counter-code branch July 9, 2024 18:23
@Domiii Domiii restored the dominik/tt-1480-assert-more-counter-code branch July 9, 2024 18:23
@Domiii Domiii reopened this Jul 9, 2024
@Domiii
Copy link
Author

Domiii commented Jul 9, 2024

I closed the wrong thing...

@Domiii Domiii requested a review from toshok July 9, 2024 18:35
Copy link

@toshok toshok left a comment

Choose a reason for hiding this comment

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

either a naming problem or a logic problem (guessing the former) in the heap_observer_set.h change. either way, a little confused there.

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?

CHECK(iteration_state_ & kAllowingRemoval);
if (recordreplay::AreEventsDisallowed() &&
recordreplay::IsRecordingOrReplaying("leak-references", "HeapObserverSet")) {
return;
Copy link

@toshok toshok Jul 10, 2024

Choose a reason for hiding this comment

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

🤔 at "leak-references". this does more than leak/keep-alive: the observer stays around in the list of observers to notify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants