Add AutoDisallowEvents to PaintOpBuffer and fix up and remove Asserts

[Domiii]

• https://linear.app/replay/issue/RUN-2104#comment-de50f9b0
• Tests: Tests passed ✔

[bhackett1024]

There's a bunch of complex refcount class logic here mixed in with assorted asserts. Please factor these out into separate PRs, this makes the changes much more difficult to review.

In addition to the comments I have serious concerns about these refcounting changes. All these macros are poorly documented and many seem unnecessary, this makes it a lot harder to see what changes we're making to chromium, their effects and possible performance implications.

More generally, in the past we've tried to assert refcount changes but it quickly gets to be very gnarly, as this PR demonstrates. I'd like to better understand why we need these now, and why we can't treat the object destruction as non-deterministic and not worry about the refcount changes matching precisely when replaying. Can we discuss this at the next crash fix meeting?

[bhackett1024]

What is the value of assertWhenEventsDisallowed ? It is false in all the uses here. Also, the label is always non-null, why do we test for it (and compare it with zero which is confusing). All the branching here seems like it could be reduced to a single AssertMaybeEventsDisallowed call, which will be clearer and minimize overhead.

[bhackett1024]

It seems pretty weird to be putting a RecordReplayId() member on every single refcounted object, which almost always returns zero. This sort of pollution will make it harder to rely on compiler type errors to fix problems in the code.

[Domiii]

@bhackett1024:

There's a bunch of complex refcount class logic here mixed in with assorted asserts. Please factor these out into separate PRs, this makes the changes much more difficult to review.

In addition to the comments I have serious concerns about these refcounting changes. All these macros are poorly documented and many seem unnecessary, this makes it a lot harder to see what changes we're making to chromium, their effects and possible performance implications.

More generally, in the past we've tried to assert refcount changes but it quickly gets to be very gnarly, as this PR demonstrates. I'd like to better understand why we need these now

Yeah - I removed REPLAY_ASSERT_REF_COUNTS. We also discussed it internally and we agree with your assessment that these Asserts often won't lead to much. We need a better solution, and Chris will propose one soon.

why we can't treat the object destruction as non-deterministic and not worry about the refcount changes matching precisely when replaying

Ok, let's give it a try. If it does not work, we have something in the pipeline that might.

Can we discuss this at the next crash fix meeting?

Sure! Already jotted it down.