+tapted@

Looks like there was a CHECK added to ~ObserverList recently.  Any thoughts?

The CHECK makes logic that would cause a guaranteed UAF crash chrome instead. 

  bool IsMarkedForRemoval() const {
    // If |weak_ptr_| was invalidated then this attempt to iterate over the
    // pointer is a UAF. Tip: If it's unclear where the `delete` occurred, try
    // adding CHECK(!IsInObserverList()) to the ~CheckedObserver() (destructor)
    // override. However, note that this is not always a bug: a destroyed
    // observer can exist in an ObserverList so long as nothing iterates over
    // the ObserverList before the list itself is destroyed.
    CHECK(!weak_ptr_.WasInvalidated());
    return weak_ptr_ == nullptr;
  }

There's a tip to diagnose further, which should be straightfoward since there's a repro.

Note there's also a Windows crash - 100% of crashes have ViewsCastDialog enabled for this query - 
 https://crash.corp.google.com/browse?q=expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27base%3A%3Ainternal%3A%3ACheckedObserverAdapter%3A%3AIsMarkedForRemoval%27&stbtiq=&reportid=&index=0


See Issue 880197

Here's the culprit,

[41752:775:0914/112105.252216:FATAL:widget.cc(50)] Check failed: !IsInObserverList().
0   libbase.dylib                       0x000000010ac3ec5e base::debug::StackTrace::StackTrace(unsigned long) + 174
1   libbase.dylib                       0x000000010ac3ed1d base::debug::StackTrace::StackTrace(unsigned long) + 29
2   libbase.dylib                       0x000000010a81ae8c base::debug::StackTrace::StackTrace() + 28
3   libbase.dylib                       0x000000010a896bac logging::LogMessage::~LogMessage() + 460
4   libbase.dylib                       0x000000010a8947a5 logging::LogMessage::~LogMessage() + 21
5   libviews.dylib                      0x0000000147c17eb7 views::WidgetObserver::~WidgetObserver() + 199
6   libchrome_dll.dylib                 0x0000000119a7af48 media_router::MediaRouterDialogControllerViews::~MediaRouterDialogControllerViews() + 312
7   libchrome_dll.dylib                 0x0000000119a7afa5 media_router::MediaRouterDialogControllerViews::~MediaRouterDialogControllerViews() + 21
8   libchrome_dll.dylib                 0x0000000119a7b009 media_router::MediaRouterDialogControllerViews::~MediaRouterDialogControllerViews() + 25
9   libbase.dylib                       0x000000010aa5cfa3 std::__1::pair<void const* const, std::__1::unique_ptr<base::SupportsUserData::Data, std::__1::default_delete<base::SupportsUserData::Data> > >::~pair() + 179
<!-- snip -->
23  libbase.dylib                       0x000000010aa5cc22 base::SupportsUserData::~SupportsUserData() + 418
24  libcontent.dylib                    0x000000012bf1bd63 content::WebContents::~WebContents() + 35
25  libcontent.dylib                    0x000000012bf1b6aa content::WebContentsImpl::~WebContentsImpl() + 10794
26  libcontent.dylib                    0x000000012bf1bd95 content::WebContentsImpl::~WebContentsImpl() + 21
27  libcontent.dylib                    0x000000012bf1beb9 content::WebContentsImpl::~WebContentsImpl() + 25
28  libchrome_dll.dylib                 0x000000011929757b TabStripModel::SendDetachWebContentsNotifications(TabStripModel::DetachNotifications*) + 4251
29  libchrome_dll.dylib                 0x00000001192a23a3 TabStripModel::CloseWebContentses(base::span<content::WebContents* const, 18446744073709551615ul>, unsigned int) + 4195
30  libchrome_dll.dylib                 0x000000011929a702 TabStripModel::InternalCloseTabs(base::span<content::WebContents* const, 18446744073709551615ul>, unsigned int) + 658
31  libchrome_dll.dylib                 0x000000011929a311 TabStripModel::CloseAllTabs() + 865
32  libchrome_dll.dylib                 0x00000001191400d1 Browser::OnWindowClosing() + 385

That's

 MediaRouterDialogControllerViews::~MediaRouterDialogControllerViews() {
   Reset();
   if (CastDialogView::GetCurrentDialogWidget())
     CastDialogView::GetCurrentDialogWidget()->RemoveObserver(this);
 }


void MediaRouterDialogControllerViews::CreateMediaRouterDialog() {} adds an observer unconditionally, so the condition there looks incorrect.

In this case, you can probably use a base::ScopedObserver, making sure to do a Remove() in an override of WidgetObserver::OnWidgetDestroying() to handle the case where the observed widget is destroyed first.


Alternatively, note that CastDialogView is already a WidgetObserver -- there might be a way to have CastDialogView call a method on MediaRouterDialogControllerViews directly so you don't have multiple observers.

(note https://chromium-review.googlesource.com/c/chromium/src/+/1226472 is the CL I used to get that stack. Don't forget to add test coverage when fixing :).

> This also repros on M70.

I'm not sure when you're planning to launch. The fix might need a merge.

We're not launching until M71 at the soonest.  Thanks for all the debugging help!

I have a WIP fix up.  The unittest crashes, though, as I'm missing some object setup for the views dialog.  I'll send for review when I've addressed that.

https://chromium-review.googlesource.com/c/chromium/src/+/1226360

The patch referenced in my previous comment removes the observer in OnWidgetDestroyed as you suggested, but I still get a crash in the same codepath on browser shutdown (see below).

Is OnWidgetDestroyed called on shutdown?  I can do some experimentation, but will need to set this aside soon to work on other items.

[104588:104588:0918/113219.573779:FATAL:widget.cc(50)] Check failed: !IsInObserverList(). 
#0 0x7fc82131ffbd base::debug::StackTrace::StackTrace()
#1 0x7fc8210269ac base::debug::StackTrace::StackTrace()
#2 0x7fc8210963fa logging::LogMessage::~LogMessage()
#3 0x7fc80d3b0b16 views::WidgetObserver::~WidgetObserver()
#4 0x55afc7b11ca3 media_router::MediaRouterDialogControllerViews::~MediaRouterDialogControllerViews()
#5 0x55afc7b11d29 media_router::MediaRouterDialogControllerViews::~MediaRouterDialogControllerViews()
#6 0x7fc8211c9e3b std::__1::pair<>::~pair()
#7 0x7fc8211c9d25 std::__1::__tree<>::destroy()
#8 0x7fc8211c9c9e std::__1::__tree<>::destroy()
#9 0x7fc8211c9cb8 std::__1::__tree<>::destroy()
#10 0x7fc8211c9c9e std::__1::__tree<>::destroy()
#11 0x7fc8211c9cb8 std::__1::__tree<>::destroy()
#12 0x7fc8211c9cb8 std::__1::__tree<>::destroy()
#13 0x7fc8211c9cb8 std::__1::__tree<>::destroy()
#14 0x7fc8211c9c3f std::__1::__tree<>::~__tree()
#15 0x7fc8211c9be5 std::__1::map<>::~map()
#16 0x7fc8211c9a13 base::SupportsUserData::~SupportsUserData()
#17 0x7fc81cc3dfa3 content::WebContents::~WebContents()
#18 0x7fc81cc0988b content::WebContentsImpl::~WebContentsImpl()
#19 0x7fc81cc09c69 content::WebContentsImpl::~WebContentsImpl()
#20 0x55afc76c920b TabStripModel::SendDetachWebContentsNotifications()
#21 0x55afc76d3012 TabStripModel::CloseWebContentses()
#22 0x55afc76cbde1 TabStripModel::InternalCloseTabs()
#23 0x55afc76cbb2a TabStripModel::CloseAllTabs()
#24 0x55afc760cdb4 Browser::OnWindowClosing()

I added some logging, in one shutdown OnWidgetDestroyed() was called and in another it was not.  So I suspect there is a race condition with the destruction of MediaRouterDialogControllerViews and CastDialogView.  We may need to handle either destruction order here.

Latest patch fixes the crashes rooted in the MRDCV, but there are still shutdown crashes elsewhere on Linux unrelated to the Cast dialog.

[116264:116264:0918/120401.580266:FATAL:widget.cc(50)] Check failed: !IsInObserverList(). 
#0 0x7fa1cdbdcfbd base::debug::StackTrace::StackTrace()
#1 0x7fa1cd8e39ac base::debug::StackTrace::StackTrace()
#2 0x7fa1cd9533fa logging::LogMessage::~LogMessage()
#3 0x7fa1b9c6db16 views::WidgetObserver::~WidgetObserver()
#4 0x556050bcc051 BrowserView::~BrowserView()
#5 0x556050bcc2a9 BrowserView::~BrowserView()
#6 0x7fa1b9c4c648 views::View::~View()
#7 0x7fa1b9c89160 views::NonClientView::~NonClientView()
#8 0x7fa1b9c89229 views::NonClientView::~NonClientView()
#9 0x7fa1b9c4f689 views::View::DoRemoveChildView()
#10 0x7fa1b9c5063d views::View::RemoveAllChildViews()
#11 0x7fa1b9c6980a views::internal::RootView::~RootView()
#12 0x556050bebf49 BrowserRootView::~BrowserRootView()
#13 0x556050bec079 BrowserRootView::~BrowserRootView()
#14 0x7fa1b9c6f405 views::Widget::DestroyRootView()
#15 0x7fa1b9c6edca views::Widget::~Widget()
#16 0x556050bea4c7 BrowserFrame::~BrowserFrame()
#17 0x556050bea569 BrowserFrame::~BrowserFrame()
#18 0x7fa1b9caeabb views::DesktopNativeWidgetAura::~DesktopNativeWidgetAura()
#19 0x556050f50d3a DesktopBrowserFrameAura::~DesktopBrowserFrameAura()
#20 0x556050c6c924 DesktopBrowserFrameAuraX11::~DesktopBrowserFrameAuraX11()
#21 0x556050c6ca29 DesktopBrowserFrameAuraX11::~DesktopBrowserFrameAuraX11()
#22 0x7fa1b9cb0bbb views::DesktopNativeWidgetAura::OnHostClosed()
#23 0x556050f50f80 DesktopBrowserFrameAura::OnHostClosed()
#24 0x7fa1b9cdc2de views::DesktopWindowTreeHostX11::CloseNow()
#25 0x556050f51b81 BrowserDesktopWindowTreeHostX11::CloseNow()
<snip>

This can be reproduced by starting the browser, and exiting using a keyboard shortcut or window manager shortcut.

#13: That crash is because MRDCV is leaving an Observer registered in Widget when the Widget is being destructed - perhaps MRDCV needs to observe WidgetObserver::OnWidgetDestroying and unobserve the Widget?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7e075800d373ed375b6c1b51ce7661f471cd7b0c

commit 7e075800d373ed375b6c1b51ce7661f471cd7b0c
Author: mark a. foltz <mfoltz@chromium.org>
Date: Tue Sep 18 20:44:41 2018

[Cast Harmony] Fix UAF in MediaRouterDialogControllerViews.

MRDCV may outlive the CastDialogView widget, in which case it should remove itself as an
observer on widget destruction.

Bug:  883976 
Change-Id: I511909a679da9d6f6639713726f3589e1723ee56
Reviewed-on: https://chromium-review.googlesource.com/1226360
Commit-Queue: mark a. foltz <mfoltz@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592183}
[modify] https://crrev.com/7e075800d373ed375b6c1b51ce7661f471cd7b0c/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.cc
[modify] https://crrev.com/7e075800d373ed375b6c1b51ce7661f471cd7b0c/chrome/browser/ui/views/media_router/media_router_dialog_controller_views.h
[add] https://crrev.com/7e075800d373ed375b6c1b51ce7661f471cd7b0c/chrome/browser/ui/views/media_router/media_router_dialog_controller_views_browsertest.cc
[modify] https://crrev.com/7e075800d373ed375b6c1b51ce7661f471cd7b0c/chrome/test/BUILD.gn

The crash in C#13 happened without instantiating a MRDCV, and the fix I submitted has MRDCV remove itself as an observer in OnWidgetDestroying.

There must be some other observer relationship that is not being cleaned up.  Since it's easily reproducible using tapted@'s patch in C#7 it shouldn't be too hard to figure out.

dbbrooks@, please verify in tomorrow's canary.

wrt #c13 - this is probably "ok" (but not great). Per the comment above the CHECK

    // adding CHECK(!IsInObserverList()) to the ~CheckedObserver() (destructor)
    // override. However, note that this is not always a bug: a destroyed
    // observer can exist in an ObserverList so long as nothing iterates over
    // the ObserverList before the list itself is destroyed.

i.e. this is why we don't simply submit https://chromium-review.googlesource.com/c/chromium/src/+/1226472 . There are unfortunate (yet valid) use-cases where the ObserverList still has entries when the Widget is destroyed. These use-cases also need to guarantee there is no codepath that _iterates_ over the observer list between deleting the observer and deleting the observer list.

Let me reopen this, as the test added by #c15 is failing on the CI.

Disabling the test by https://crrev.com/c/1235264.

The failing logs of MediaRouterDialogControllerViewsTest.OpenCloseMediaRouterDialog are:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/mac-cocoa-rel/1321
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8935002203177419984/+/steps/browser_tests_on__none__GPU_on_Mac/0/logs/MediaRouterDialogControllerViewsTest.OpenCloseMediaRouterDialog/0

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ec6f906dde3e27ce2bcc172dde5cd9d9b300313

commit 0ec6f906dde3e27ce2bcc172dde5cd9d9b300313
Author: tzik <tzik@chromium.org>
Date: Thu Sep 20 06:16:26 2018

Disable MediaRouterDialogControllerViewsTest.OpenCloseMediaRouterDialog on Mac

MediaRouterDialogControllerViewsTest.OpenCloseMediaRouterDialog is failing
on the mac bot. The error logs are:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/mac-cocoa-rel/1321
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8935002203177419984/+/steps/browser_tests_on__none__GPU_on_Mac/0/logs/MediaRouterDialogControllerViewsTest.OpenCloseMediaRouterDialog/0

Tbr: mfoltz@chromium.org
Bug:  883976 
Change-Id: I9b7e42b65ec44a97a74d236c179e1698ef0c895e
Reviewed-on: https://chromium-review.googlesource.com/1235264
Reviewed-by: Taiju Tsuiki <tzik@chromium.org>
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592695}
[modify] https://crrev.com/0ec6f906dde3e27ce2bcc172dde5cd9d9b300313/chrome/browser/ui/views/media_router/media_router_dialog_controller_views_browsertest.cc

It looks like there is an unimplemented codepath for Viz that, for some reason, is being hit only on Macs.  Maybe this is because the Mac bots are disabling Views by command-line flag?

void DelegatedFrameHost::SetNeedsBeginFrames(bool needs_begin_frames) {
  if (enable_viz_) {
    NOTIMPLEMENTED();
    return;
  }
  needs_begin_frame_ = needs_begin_frames;
  support_->SetNeedsBeginFrame(needs_begin_frames);
}

kylechar@, you added this check in 2017.  Do you know if there a straightforward way to skip views browsertests on Mac that might hit this code?

That gets hit because OOP-D (the VizDisplayCompositor feature) is enabled on Mac now. Whatever is calling DelegatedFrameHost::SetNeedsBeginFrames() shouldn't be. The way the browser compositor embeds a renderer is different on mac, which is probably why this only occurs there, but I'm not 100% on all the details.

ccameron do you have any ideas?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8dfb056b2a6bf6402efd0e9aeb05d1a0c8e06519

commit 8dfb056b2a6bf6402efd0e9aeb05d1a0c8e06519
Author: mark a. foltz <mfoltz@chromium.org>
Date: Thu Sep 20 21:03:12 2018

[Cast Harmony] Re-enable MRDCV browsertest.

You can't run Views browsertests on Mac when views browser windows are disabled.

TBR=imcheng@chromium.org

Bug:  883976 
Change-Id: I4d318db3f58743f543e278f744e3926d6e9076e4
Reviewed-on: https://chromium-review.googlesource.com/1237151
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Commit-Queue: mark a. foltz <mfoltz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592941}
[modify] https://crrev.com/8dfb056b2a6bf6402efd0e9aeb05d1a0c8e06519/chrome/browser/ui/views/media_router/media_router_dialog_controller_views_browsertest.cc
[modify] https://crrev.com/8dfb056b2a6bf6402efd0e9aeb05d1a0c8e06519/chrome/test/BUILD.gn

(This is fixed now? I'm still Owner -- is there more work to do?)

For some reason, the test is still running on the mac_cocoa_rel bot.  But a bunch of other tests are failing too.  I will try a different way to clean this up.

The mac-cocoa-rel bot has --disable-features=ViewsBrowserWindows but VizDisplayCompositor would still be enabled by fieldtrial testing config. That's not supported configuration. Does --disable-features="VizDisplayCompositor,ViewsBrowserWindows" fix things?

FYI mac-cocoa-rel is now gone ( issue 887808 ).

Was the bot failure the only remaining issue? Can we close this bug?

Yes.  This can be closed.