Daniel Cheng (Gerrit)

unread,

Jun 28, 2024, 12:19:30 PM (5 days ago) Jun 28

to Daniel Cheng, Fr, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, andysjl...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, navigation...@chromium.org, nwoked...@chromium.org, rhalava...@chromium.org, steimel+...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from Fr

New activity on the change

Open in Gerrit

Related details

Attention is currently required from:

Fr

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Fr (Gerrit)

unread,

Jul 1, 2024, 2:34:55 AM (2 days ago) Jul 1

to Daniel Cheng, Kevin McNee, James Maclean, Chromium LUCI CQ, chromium...@chromium.org, dullweb...@chromium.org, ffred...@chromium.org, msrame...@chromium.org, tluk+...@chromium.org, alexmo...@chromium.org, andysjl...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, navigation...@chromium.org, nwoked...@chromium.org, rhalava...@chromium.org, steimel+...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from Daniel Cheng

Fr voted and added 3 comments

Votes added by Fr

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Fr . resolved

LGTM with nits

@libe...@chromium.org may also be interested in this change.

File chrome/browser/picture_in_picture/auto_pip_setting_overlay_view.h

Line 77, Patchset 3 (Latest): DCHECK(!delegate_);

Fr . unresolved

I thought we were supposed to use `CHECK` instead of `DCHECK` nowadays?

Line 75, Patchset 3 (Latest): void set_delegate(Delegate* delegate) {

Fr . unresolved

Nit: Should this be `SetDelegate`?

Open in Gerrit

Related details

Attention is currently required from:

Daniel Cheng

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Daniel Cheng (Gerrit)

unread,

Jul 1, 2024, 11:59:10 AM (2 days ago) Jul 1

to Daniel Cheng, Fr, Kevin McNee, James Maclean, Chromium LUCI CQ, chromium...@chromium.org, dullweb...@chromium.org, ffred...@chromium.org, msrame...@chromium.org, tluk+...@chromium.org, alexmo...@chromium.org, andysjl...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, navigation...@chromium.org, nwoked...@chromium.org, rhalava...@chromium.org, steimel+...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from Fr

Daniel Cheng added 2 comments

File chrome/browser/picture_in_picture/auto_pip_setting_overlay_view.h

Line 77, Patchset 3 (Latest): DCHECK(!delegate_);

Fr . unresolved

I thought we were supposed to use `CHECK` instead of `DCHECK` nowadays?

Daniel Cheng

I don't think the guidance in https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md is very clear about this point. I can change it if you or Frank prefer CHECK.

Line 75, Patchset 3 (Latest): void set_delegate(Delegate* delegate) {

Fr . unresolved

Nit: Should this be `SetDelegate`?

Daniel Cheng

I left this named using getter style naming, because it's essentially a getter. But if you or Frank prefer, I can rename it.

Open in Gerrit

Related details

Attention is currently required from:

Fr

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Fr (Gerrit)

unread,

Jul 2, 2024, 4:29:32 AM (yesterday) Jul 2

to Daniel Cheng, Kevin McNee, James Maclean, Chromium LUCI CQ, chromium...@chromium.org, dullweb...@chromium.org, ffred...@chromium.org, msrame...@chromium.org, tluk+...@chromium.org, alexmo...@chromium.org, andysjl...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, navigation...@chromium.org, nwoked...@chromium.org, rhalava...@chromium.org, steimel+...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from Daniel Cheng

Fr voted and added 2 comments

Votes added by Fr

Code-Review

+1

2 comments

File chrome/browser/picture_in_picture/auto_pip_setting_overlay_view.h

Line 77, Patchset 3: DCHECK(!delegate_);

Fr . unresolved

I thought we were supposed to use `CHECK` instead of `DCHECK` nowadays?

Daniel Cheng

I don't think the guidance in https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md is very clear about this point. I can change it if you or Frank prefer CHECK.

Fr

This document says "Prefer CHECK() and NOTREACHED_NORETURN() as they ensure that if an invariant fails, the program does not continue in an unexpected state, and we hear about the failure either through a test failure or a crash report".

Having said that, Frank is OOO until next week. I'm fine landing it as is and come back to it if Frank prefers CHECK.

Line 75, Patchset 3: void set_delegate(Delegate* delegate) {

Fr . unresolved

Nit: Should this be `SetDelegate`?

Daniel Cheng

I left this named using getter style naming, because it's essentially a getter. But if you or Frank prefer, I can rename it.

Fr

I don't feel strongly about it. I tend to prefer consistency in this case but as said below. We can land as is and revisit if Frank feels like it.

Open in Gerrit

Related details

Attention is currently required from:

Daniel Cheng

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Daniel Cheng (Gerrit)

unread,

1:00 AM (5 hours ago) 1:00 AM

to Daniel Cheng, Enterprise Policy Reviews, Fr, Kevin McNee, James Maclean, Chromium LUCI CQ, chromium...@chromium.org, dullweb...@chromium.org, ffred...@chromium.org, msrame...@chromium.org, tluk+...@chromium.org, alexmo...@chromium.org, andysjl...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, navigation...@chromium.org, nwoked...@chromium.org, rhalava...@chromium.org, steimel+...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Daniel Cheng added 2 comments

File chrome/browser/picture_in_picture/auto_pip_setting_overlay_view.h

Line 77, Patchset 3: DCHECK(!delegate_);

Fr . resolved

I thought we were supposed to use `CHECK` instead of `DCHECK` nowadays?

Daniel Cheng

I don't think the guidance in https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md is very clear about this point. I can change it if you or Frank prefer CHECK.

Fr

This document says "Prefer CHECK() and NOTREACHED_NORETURN() as they ensure that if an invariant fails, the program does not continue in an unexpected state, and we hear about the failure either through a test failure or a crash report".
Having said that, Frank is OOO until next week. I'm fine landing it as is and come back to it if Frank prefers CHECK.

Daniel Cheng

I changed this to a CHECK(), though I combined them and added a comment for clarity.

Line 75, Patchset 3: void set_delegate(Delegate* delegate) {

Fr . resolved

Nit: Should this be `SetDelegate`?

Daniel Cheng

I left this named using getter style naming, because it's essentially a getter. But if you or Frank prefer, I can rename it.

Fr

I don't feel strongly about it. I tend to prefer consistency in this case but as said below. We can land as is and revisit if Frank feels like it.

Daniel Cheng

Acknowledged

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Daniel Cheng (Gerrit)

unread,

1:02 AM (5 hours ago) 1:02 AM

to Daniel Cheng, Enterprise Policy Reviews, Fr, Kevin McNee, James Maclean, Chromium LUCI CQ, chromium...@chromium.org, dullweb...@chromium.org, ffred...@chromium.org, msrame...@chromium.org, tluk+...@chromium.org, alexmo...@chromium.org, andysjl...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, navigation...@chromium.org, nwoked...@chromium.org, rhalava...@chromium.org, steimel+...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Daniel Cheng voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

2:05 AM (4 hours ago) 2:05 AM

to Daniel Cheng, Enterprise Policy Reviews, Fr, Kevin McNee, James Maclean, chromium...@chromium.org, dullweb...@chromium.org, ffred...@chromium.org, msrame...@chromium.org, tluk+...@chromium.org, alexmo...@chromium.org, andysjl...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, navigation...@chromium.org, nwoked...@chromium.org, rhalava...@chromium.org, steimel+...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: chrome/browser/picture_in_picture/auto_pip_setting_overlay_view.h
Insertions: 4, Deletions: 5.

The diff is too large to show. Please review the diff.
```

Change information

Commit message:

[MSan] Fix use-after-dtor in VideoOverlayWindowViews

VideoOverlayWindowViews observes the overlay view (if any). If the
overlay view is manually removed, VideoOverlayWindowViews unregisters
itself as an observer. However, if the VideoOverlayWindowViews is
destroyed without manually removing the overlay view, it never
unregisters itself. This results in a MSan use-after-dtor error, since:

- the overlay view is a child view
- destroying a view triggers observer calls
- but child views are not destroyed until the View base class is
  destroyed
- and the ViewObserver base class is destroyed before the View base
  class

While it would be easy to simply destroy the View base class before the
ViewObserver base class, dispatching observations at destruction is
pointless, since it will never call VideoOverlayWindowViews's
implementation.

Using ScopedObservation here is also a bit awkward; ScopedObservation
itself needs to store a pointer to the observee, which means
VideoOverlayWindowViews ends up with three (!!) pointers to the overlay
view: the raw_ptr, the ScopedObservation (as a ViewObserver), and the
ScopedObservation (as an AutoPipSettingOverlayViewObserver).

It is possible to simply get the pointer to the overlay view out of the
ScopedObservation, but the manual removal of the overlay view remains
awkward.

Instead, just add a conditional RemoveObserver() call in the destructor.
As a bonus, convert the ObserverList in AutoPipSettingOverlayView to
just use the delegate pattern: a given AutoPipSettingOverlayView only
ever has a single AutoPipSettingOverlayViewObserver at a time. This
makes it easier to unregister the ViewObserver, since the RemoveObserver
call no longer needs to be qualified.

Bug: 40222690

Change-Id: I322f8876e8dcd6d5de0d83176b295ec2b710f29c

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5668018

Commit-Queue: Daniel Cheng <dch...@chromium.org>

Reviewed-by: Fr <beaufort...@gmail.com>

Cr-Commit-Position: refs/heads/main@{#1322594}

Files:

M chrome/browser/picture_in_picture/auto_pip_setting_overlay_view.cc
M chrome/browser/picture_in_picture/auto_pip_setting_overlay_view.h
M chrome/browser/picture_in_picture/auto_pip_setting_overlay_view_unittest.cc
M chrome/browser/ui/views/overlay/video_overlay_window_views.cc
M chrome/browser/ui/views/overlay/video_overlay_window_views.h

Change size: M

Delta: 5 files changed, 40 insertions(+), 52 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Fr

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[MSan] Fix use-after-dtor in VideoOverlayWindowViews [chromium/src : main]

Daniel Cheng (Gerrit)

New activity on the change

Related details

Fr (Gerrit)

Fr voted and added 3 comments

Votes added by Fr

3 comments

Related details

Daniel Cheng (Gerrit)

Daniel Cheng added 2 comments

Related details

Fr (Gerrit)

Fr voted and added 2 comments

Votes added by Fr

2 comments

Related details

Daniel Cheng (Gerrit)

Daniel Cheng added 2 comments

Related details

Daniel Cheng (Gerrit)

Daniel Cheng voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information