New issue
Advanced search Search tips

Issue 808318 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-04-25
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Chrome_Mac: Crash Report - views::Widget::OnNativeWidgetDestroying

Project Member Reported by cr...@system.gserviceaccount.com, Feb 2 2018

Issue description

reporter:tapted@google.com

Magic Signature: views::Widget::OnNativeWidgetDestroying

Updated Link: https://crash.corp.google.com/browse?q=product_name%3D%27Chrome_Mac%27+AND+EXISTS+%28SELECT+1+FROM+UNNEST%28CrashedStackTrace.StackFrame%29+WHERE+FunctionName%3D%27views%3A%3AWidget%3A%3AOnNativeWidgetDestroying%28%29%27%29+AND+expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27views%3A%3AWidget%3A%3AOnNativeWidgetDestroying%27&stbtiq=&reportid=&index=0

Crash link: https://crash.corp.google.com//browse?q=product.name%3D'Chrome_Mac'%20AND%20EXISTS%20(SELECT%201%20FROM%20UNNEST(CrashedStackTrace.StackFrame)%20WHERE%20FunctionName%3D'views%3A%3AWidget%3A%3AOnNativeWidgetDestroying()')%20AND%20ReportID%3D'fb0f46a358d74a8e'%20AND%20expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D'views%3A%3AWidget%3A%3AOnNativeWidgetDestroying'&sql_dialect=googlesql&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D#3

-------------------------------------------------------------------------------
Sample Report
-------------------------------------------------------------------------------
Product name: Chrome_Mac
Magic Signature : views::Widget::OnNativeWidgetDestroying
Product Version: 66.0.3336.5
Process type: browser
Report ID: fb0f46a358d74a8e
Report Url: https://crash.corp.google.com/fb0f46a358d74a8e
Report Time: 2018-02-01T17:48:38-08:00
Upload Time: 2018-02-01T17:48:41.593-08:00
Uptime: 165000 ms
CumulativeProductUptime: 0 ms
OS Name: Mac OS X
OS Version: 10.12.6 16G29
CPU Architecture: amd64
CPU Info: family 6 model 61 stepping 4

-------------------------------------------------------------------------------
Crashing thread: Thread index: 0. Stack Quality: 84%. Thread id: 506217.
-------------------------------------------------------------------------------
0x000000010d4f059e (Google Chrome Framework - widget.cc: 1086)	views::Widget::OnNativeWidgetDestroying()
0x000000010d48692d (Google Chrome Framework - bridged_native_widget.mm: 786)	views::BridgedNativeWidget::OnWindowWillClose()
0x000000010d48b12a (Google Chrome Framework - views_nswindow_delegate.mm: 126)	-[ViewsNSWindowDelegate windowWillClose:]
0x00007fff8881d45b (CoreFoundation + 0x0009d45b)	__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__
0x00007fff8881d35a (CoreFoundation + 0x0009d35a)	_CFXRegistrationPost
0x00007fff8881d0c1 (CoreFoundation + 0x0009d0c1)	___CFXNotificationPost_block_invoke
0x00007fff887da522 (CoreFoundation + 0x0005a522)	-[_CFXNotificationRegistrar find:object:observer:enumerator:]
0x00007fff887d955b (CoreFoundation + 0x0005955b)	_CFXNotificationPost
0x00007fff8a1fe676 (Foundation + 0x00006676)	-[NSNotificationCenter postNotificationName:object:userInfo:]
0x00007fff86597da3 (AppKit + 0x002ddda3)	__18-[NSWindow _close]_block_invoke
0x00007fff86597c87 (AppKit + 0x002ddc87)	-[NSWindow _close]
0x000000010d4ea514 (Google Chrome Framework - native_widget_mac.mm: 645)	views::Widget::CloseAllSecondaryWidgets()
0x000000010e217860 (Google Chrome Framework - browser_list.cc: 111)	BrowserList::RemoveBrowser(Browser*)
0x000000010e2094f4 (Google Chrome Framework - browser.cc: 497)	Browser::~Browser()
0x000000010e209c2d (Google Chrome Framework - browser.cc: 481)	<name omitted>
0x00007fff9d69cbb3 (libobjc.A.dylib + 0x00010bb3)	object_cxxDestructFromClass(objc_object*, objc_class*)
0x00007fff9d6955f5 (libobjc.A.dylib + 0x000095f5)	objc_destructInstance
0x000000010d2070e8 (Google Chrome Framework - objc_zombie.mm: 110)	(anonymous namespace)::ZombieDealloc(objc_object*, objc_selector*)
0x00007fff862f465a (AppKit + 0x0003a65a)	-[NSResponder dealloc]
0x000000010e42758c (Google Chrome Framework - tab_window_controller.mm: 262)	-[TabWindowController dealloc]
0x000000010e35eb8f (Google Chrome Framework - browser_window_controller.mm: 483)	-[BrowserWindowController dealloc]
0x00007fff8a261db3 (Foundation + 0x00069db3)	__delayedPerformCleanup
0x00007fff88810fe6 (CoreFoundation + 0x00090fe6)	CFRunLoopTimerInvalidate
0x00007fff88810915 (CoreFoundation + 0x00090915)	__CFRunLoopDoTimer
0x00007fff88810439 (CoreFoundation + 0x00090439)	__CFRunLoopDoTimers
0x00007fff88807b80 (CoreFoundation + 0x00087b80)	__CFRunLoopRun
0x00007fff88807113 (CoreFoundation + 0x00087113)	CFRunLoopRunSpecific
0x00007fff87d67ebb (HIToolbox + 0x00030ebb)	RunCurrentEventLoopInMode
0x00007fff87d67bf8 (HIToolbox + 0x00030bf8)	ReceiveNextEventCommon
0x00007fff87d67b25 (HIToolbox + 0x00030b25)	_BlockUntilNextEventMatchingListInModeWithFilter
0x00007fff86300a53 (AppKit + 0x00046a53)	_DPSNextEvent
0x00007fff86a7c7ed (AppKit + 0x007c27ed)	-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
0x000000010b79532f (Google Chrome Framework - chrome_browser_application_mac.mm: 174)	__71-[BrowserCrApplication nextEventMatchingMask:untilDate:inMode:dequeue:]_block_invoke
0x000000010bb518d9 (Google Chrome Framework + 0x01e4e8d9)	base::mac::CallWithEHFrame(void () block_pointer)
0x000000010b795273 (Google Chrome Framework - chrome_browser_application_mac.mm: 173)	-[BrowserCrApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
0x00007fff862f53da (AppKit + 0x0003b3da)	-[NSApplication run]
0x000000010bb606cb (Google Chrome Framework - message_pump_mac.mm: 815)	base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*)
0x000000010bb5f24d (Google Chrome Framework - message_pump_mac.mm: 189)	base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*)
0x000000010bb83534 (Google Chrome Framework - run_loop.cc: 133)	<name omitted>
0x000000010b79b1d7 (Google Chrome Framework - chrome_browser_main.cc: 1997)	ChromeBrowserMainParts::MainMessageLoopRun(int*)
0x000000010a510ca3 (Google Chrome Framework - browser_main_loop.cc: 1257)	content::BrowserMainLoop::RunMainMessageLoopParts()
0x000000010a513671 (Google Chrome Framework - browser_main_runner.cc: 145)	content::BrowserMainRunnerImpl::Run()
0x000000010a50d12b (Google Chrome Framework - browser_main.cc: 46)	content::BrowserMain(content::MainFunctionParams const&)
0x000000010b74d3df (Google Chrome Framework - content_main_runner.cc: 713)	content::ContentMainRunnerImpl::Run()
0x000000010d0d523a (Google Chrome Framework - main.cc: 456)	service_manager::Main(service_manager::MainParams const&)
0x000000010b74c923 (Google Chrome Framework - content_main.cc: 19)	content::ContentMain(content::ContentMainParams const&)
0x0000000109d07369 (Google Chrome Framework - chrome_main.cc: 144)	ChromeMain
0x0000000107494dd3 (Google Chrome Canary + 0x00000dd3)	
0x00007fff9df87234 (libdyld.dylib + 0x00005234)	start
0x00007fff9df87234 (libdyld.dylib + 0x00005234)	start

 
Labels: Proj-MacViews OS-Chrome OS-Linux OS-Windows
The crash link is 6 reports on mac

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Mac%27%20AND%20expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27views%3A%3AWidget%3A%3AOnNativeWidgetDestroying%27&sql_dialect=googlesql&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D&stbtiq=&reportid=&index=0

but 230 across all platforms, and a high proportion on ChromeOS.

https://crash.corp.google.com/browse?q=expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27views%3A%3AWidget%3A%3AOnNativeWidgetDestroying%27&sql_dialect=googlesql&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D&stbtiq=&reportid=&index=0


I expect some client code neglecting to remove itself as a WidgetObserver when destroying itself.

void Widget::OnNativeWidgetDestroyed() {
  for (WidgetObserver& observer : observers_)
    observer.OnWidgetDestroyed(this);
  widget_delegate_->DeleteDelegate();
  widget_delegate_ = NULL;
  native_widget_destroyed_ = true;
}

Components: Internals>Views
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 3 2018

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

commit 64a3626633f586dfd6241fe62502aa182457ffeb
Author: Trent Apted <tapted@chromium.org>
Date: Sat Feb 03 02:56:28 2018

Add crash keys inside Mac's Widget::CloseAllSecondaryWidgets()

crbug/788271 is a mystery, crashing in objc_msgSend without triggering
any zombie warnings.

crbug/808318 points to a leaked WidgetObserver affecting all platforms,
but has no indication of what the observer is, or what window it observes.

Record class names and the window title in crash keys. The window title
is usually set by the framework, even if it is not displayed anywhere.

Bug: 788271, 808318
Change-Id: I09f9697bfca4e91c8f3b0e8dba125c1eb39cde4b
Reviewed-on: https://chromium-review.googlesource.com/896774
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534240}
[modify] https://crrev.com/64a3626633f586dfd6241fe62502aa182457ffeb/ui/views/BUILD.gn
[modify] https://crrev.com/64a3626633f586dfd6241fe62502aa182457ffeb/ui/views/DEPS
[modify] https://crrev.com/64a3626633f586dfd6241fe62502aa182457ffeb/ui/views/widget/native_widget_mac.mm
[modify] https://crrev.com/64a3626633f586dfd6241fe62502aa182457ffeb/ui/views/widget/native_widget_mac_unittest.mm

Comment 4 by tapted@chromium.org, Feb 22 2018

Labels: -Pri-2 Pri-3
NextAction: 2018-03-22
Status: Available (was: Untriaged)
Query to track: https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Mac%27%20AND%20EXISTS%20(SELECT%201%20FROM%20UNNEST(productdata)%20WHERE%20key%3D%27windowInfo%27)&stbtiq=&reportid=&index=0

This is a low crash rate that affects all platforms.

Comment 5 by tapted@chromium.org, Feb 22 2018

Note that link has 2 reports, both of which are Issue 788271, now fixed. New ones could be this issue.
Project Member

Comment 6 by sheriffbot@chromium.org, Feb 26 2018

Labels: FoundIn-M-66 Fracas
Users experienced this crash on the following builds:

Mac Canary 66.0.3355.0 -  0.15 CPM, 1 reports, 1 clients (signature views::Widget::OnNativeWidgetDestroying)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas
Cc: rdevlin....@chromium.org est...@chromium.org
Components: Platform>Extensions UI>Browser>Bubbles
http://go/crash/2c6f04047f83d14a

The offending dialog title is "Your Internet connection is being controlled"

IDS_EXTENSIONS_PROXY_CONTROLLED_TITLE_HOME_PAGE_BUBBLE

I think that creates an instance of ToolbarActionsBarBubbleViews. That's just a BubbleDialogDelegateView, but it also has BrowserActionsContainer observing it. AddObserver added in BrowserActionsContainer::ShowToolbarActionBubble() here: 

https://cs.chromium.org/chromium/src/chrome/browser/ui/views/toolbar/browser_actions_container.cc?sq=package:chromium&dr=CSs&l=273

That observer is only removed in a call to

BrowserActionsContainer::ClearActiveBubble(views::Widget* widget) {
  DCHECK(active_bubble_);
  DCHECK_EQ(active_bubble_->GetWidget(), widget);
  widget->RemoveObserver(this);
  active_bubble_ = nullptr;
  toolbar_actions_bar_->OnBubbleClosed();
}

and the BrowserActionsContainer destructor only starts an *asynchronous* close. So it's quite possible for this Widget to outlive a BrowserActionsContainer, causing this use-after-free.

BrowserActionsContainer::~BrowserActionsContainer() {
  if (active_bubble_)
    active_bubble_->GetWidget()->Close();


I think either that Close() should be a CloseNow(), or ~BrowserActionsContainer() should do the RemoveObserver call early.
hm - there's more to this. This should clear it in the normal case:

void BrowserActionsContainer::OnWidgetClosing(views::Widget* widget) {
  ClearActiveBubble(widget);
}

but codepaths under Widget::CloseAllSecondaryWidgets() do not invoke OnWidgetClosing (on any platform).

they *do* invoke 

void BrowserActionsContainer::OnWidgetDestroying(views::Widget* widget) {
  ClearActiveBubble(widget);
}

but that's where this crash is happening -- for some reason (I suspect) BrowserActionsContainer is still observing at this point, and yet it has been destroyed, causing a crash when this gets invoked.


(there is another observer -- the BubbleDialogDelegateView itself -- so it's possible that's involved somehow..)
Cc: tapted@chromium.org
If the crash happens from BrowserWindow tear down, it could be possible that the BrowserActionsContainer is destroyed before the bubble begins the closing/destroying process, right?  In which case, the later call (when the bubble *is* closing/destroying as a later part of the BrowserWindow tear down) would notify the now-destroyed BrowserActionsContainer.

I gladly defer to your expertise here, since you're much more familiar with the views framework, but it seems like your suggestions from #7 should work.  Of the two, calling CloseNow() seems a bit safer, just to ensure the bubble doesn't inadvertently call back into the container for anything else (and there's no point in keeping the bubble around if everything's shutting down).
The NextAction date has arrived: 2018-03-22
NextAction: 2018-04-25
http://go/crash/2c6f04047f83d14a is still the only crash. m66 is in beta and these `ToolbarActionsBarBubbleViews` dialogs are 50/50 on Mac, about to go to 100%, and then through 100% to stable.. let's check in then.
Labels: -Pri-3 MacViews-Dialogs Target-67 Pri-2
Owner: ellyjo...@chromium.org
Status: Assigned (was: Available)
Cc: sky@chromium.org
+cc sky@ for the questions below, but I'm also interested in your thoughts, tapted@.

My reading of #7 and #8:

1) ~BrowserActionsContainer() can only happen when the window is already being destructed, in which case the attached secondary widget should already be closing or closed (and therefore it's fine to CloseNow() it)
2) The root issue is that Widget::CloseAllSecondaryWidgets() just unceremoniously closes the involved widgets, without going via Widget::Close() (and therefore without going via WidgetObserver::OnWidgetClosing().

To fix this at the root, could we either:

a) Make Widget::CloseAllSecondaryWidgets() use Widget::Close() via GetWidgetForNativeWindow(), or
b) Have Widget::CloseAllSecondaryWidgets() directly prod the observers of the widgets they are closing?
Project Member

Comment 14 by bugdroid1@chromium.org, Mar 26 2018

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

commit 22fa388609e18066449d4e124ba660594da87e6f
Author: Trent Apted <tapted@chromium.org>
Date: Mon Mar 26 21:14:08 2018

CHECK() if a WidgetDelegate would be deleted before the Widget it initialized is deleted.

WidgetDelegates must outlive their Widget.

Crashes suggest situations where they may not, but can't pinpoint the actual
client code. The crash is instead attributed to a UAF on the widget delegate
either in a layout call (during an animation), or when
Widget::OnNativeWidgetDestroyed() tries to delete the delegate (again).

This CL adds a CHECK() in ~WidgetDelegate which should track down the
misbehaving code.

Bug: 825091, 814821, 808318
Change-Id: I9490ff1682ee236436edf075f7c237ccf910c44c
Reviewed-on: https://chromium-review.googlesource.com/977244
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545873}
[modify] https://crrev.com/22fa388609e18066449d4e124ba660594da87e6f/ui/views/widget/widget.cc
[modify] https://crrev.com/22fa388609e18066449d4e124ba660594da87e6f/ui/views/widget/widget_delegate.cc
[modify] https://crrev.com/22fa388609e18066449d4e124ba660594da87e6f/ui/views/widget/widget_delegate.h

I think Widget::CloseAllSecondaryWidgets() is doing the right thing. Widget client code _always_ needs to handle the case where it is unceremoniously closed (i.e. with CloseNow() rather than Close()+PostTask).

The problem is that the OS can (effectively) invoke CloseNow on any window, any time it wants. Windows does this during the logoff procedure. Linux does it with `xkill`. Mac does it with -[NSWindow close] (although I think on Mac it is actually possible to refuse still).

So, Widget doesn't (and can't) guarantee that WidgetObserver::OnWidgetClosing() gets invoked. But it exists as an early opportunity for a dialog to do things with the NativeWidget that would be too late to attempt in OnWidgetDestroying().

Dialogs do not always cater for this, and it's a big source of bugs. With TestBrowserDialog, I'm hoping to actually exercise the different close codepaths for dialogs that have tests. The current default is to use CloseNow in TestBrowserDialog. One dialog did need TestBrowserDialog::AlwaysCloseAsynchronously() to get running, but the code seemed innocent.
Labels: M-67
tapted@, is there anything pending besided cl listed at #14 for this bug?

Comment 18 by sky@chromium.org, Mar 27 2018

I agree with what Trent says in comment 15.
Mergedinto: 827169
Status: Duplicate (was: Assigned)
The root cause for this is most likely issue 827169. I'm going to duplicate this issue into that one and mark that one for MacViews instead.
Status: Assigned (was: Duplicate)
 ellyjones@, per comment #19, this is dupe. Can status be change to Duplicate instead of Assigned?
No - I just unduped it because it's not actually a dupe of 827169, per 827169 c#7.
OK, got it. Sorry missed the previous update.
** Bulk Edit **

There are only two M67 dev releases left on 04/03 & 04/10 before M67 branch on 04/12. Please try to land the fix ASAP to trunk so we can move forward with 50%-50% experiment on M67 Canary/Dev (if possible at all). Thank you.
** Bulk Edit **

There is only one dev release left 04/10 before M67 branch on 04/12. Please try to land the fix ASAP to trunk so we can move forward with 50%-50% experiment on M67 Canary/Dev (if possible at all). Thank you.

FYI: Change/Fix has to be landed in trunk latest by 4:00 PM PT, Friday (04/06) so we can pick it up for next week dev release. 
We have four crashes for this, three from M66 and one from M67.

Two are "Closing Your Internet connection is being controlled (NativeWidgetMacNSWindow)".
One is "Closing Open (NSOpenPanel)".
One is "Closing overlay (TabWindowOverlayWindow)".

The former two are via:

0x0000000105938b0e	(Google Chrome Framework -widget.cc:1086 )	views::Widget::OnNativeWidgetDestroying()
0x00000001058ce3bd	(Google Chrome Framework -bridged_native_widget.mm:786 )	views::BridgedNativeWidget::OnWindowWillClose()
0x00000001058d2c0a	(Google Chrome Framework -views_nswindow_delegate.mm:126 )	-[ViewsNSWindowDelegate windowWillClose:]

The latter two are via:

0x00007fff89d5a4dd	(libobjc.A.dylib + 0x000014dd )	objc_msgSend
0x0000000111eab254	(Google Chrome Framework -native_widget_mac.mm:657 )	views::Widget::CloseAllSecondaryWidgets()
0x0000000112bcc0a0	(Google Chrome Framework -browser_list.cc:111 )	BrowserList::RemoveBrowser(Browser*)
0x0000000112bbdcf4	(Google Chrome Framework -browser.cc:497 )	Browser::~Browser()

And more specifically the third is a null+0x18 dereference and the fourth is a dereference of 0x0000656d6f726878, which is "chrome"+0x15 packed little-endian.

So I think there's probably two separate issues:

1) The "Your internet connection is being controlled" dialog has lifetime issues
2) Widget::CloseAllSecondaryWidgets() on Mac is closing invalid windows sometimes

I'll look into (1) today, but given the low crash rate here (and the cross-platform nature of it) this isn't super important.
Labels: -M-67 -Target-67 M-68 Target-68
I tried to reproduce (1) a bunch of different ways. The code doesn't "smell" right to me; in ToolbarActionsBarBubbleViews::ButtonPressed() there's a block comment like this:

  // Note that the Widget may or may not already be closed at this point,
  // depending on delegate_->ShouldCloseOnDeactivate(). Widget::Close() protects
  // against multiple calls (so long as they are not nested), and Widget
  // destruction is asynchronous, so it is safe to call Close() again.

This seems dubious to me - if the widget destruction actually *did* happen synchronously in the earlier close, the Widget::Close() would be a UAF. As it happens, the "your internet connection is being controlled" bubble (ProxyOverriddenBubbleDelegate) is one of very few bubbles that don't close when deactivated, so it's one of the few bubbles for which the GetWidget()->Close() in this function actually does cause the close, which otherwise happens during the earlier OnBubbleClosed() (I think, and the comment also suggests).

On the other hand, I can't concretely find anything *wrong* with this code, and I've still had no success reproducing the crash either. We might be better off waiting for more crashes and seeing where that leads us.

I'm going to kick this out of Target-67, though - this bug's been around for a bit now and the crash rates are low.
The NextAction date has arrived: 2018-04-25
Something similar is seen on other platforms, e.g. on Windows:

0x6b7568c1	(chrome.dll -widget.cc:1086 )	views::Widget::OnNativeWidgetDestroying()
0x6b760b6b	(chrome.dll -desktop_window_tree_host_win.cc:790 )	views::DesktopWindowTreeHostWin::HandleDestroying()
0x6b76ca5a	(chrome.dll -hwnd_message_handler.cc:1454 )	views::HWNDMessageHandler::OnDestroy()
0x6ab01d53	(chrome.dll -hwnd_message_handler.h:426 )	views::HWNDMessageHandler::_ProcessWindowMessage(HWND__ *,unsigned int,unsigned int,long,long &,unsigned long)
0x6ab0138d	(chrome.dll -hwnd_message_handler.cc:939 )	views::HWNDMessageHandler::OnWndProc(unsigned int,unsigned int,long)
0x6a954c5a	(chrome.dll -window_impl.cc:303 )	gfx::WindowImpl::WndProc(HWND__ *,unsigned int,unsigned int,long)
0x6a954be8	(chrome.dll -wrapped_window_proc.h:76 )	base::win::WrappedWindowProc<&gfx::WindowImpl::WndProc(HWND__ *,unsigned int,unsigned int,long)>(HWND__ *,unsigned int,unsigned int,long)
0x75ab86ee	(USER32.dll + 0x000186ee )	InternalCallWinProc
0x75ab8875	(USER32.dll + 0x00018875 )	UserCallWinProcCheckWow
0x75ab70f3	(USER32.dll + 0x000170f3 )	DispatchClientMessage
0x75ab738e	(USER32.dll + 0x0001738e )	__fnDWORD
0x7769642d	(ntdll.dll + 0x0004642d )	KiUserCallbackDispatcher
0x776963df	(ntdll.dll + 0x000463df )	KiUserApcDispatcher
0x6a96555e	(chrome.dll -task_annotator.cc:61 )	base::debug::TaskAnnotator::RunTask(char const *,base::PendingTask *)
0x6a968792	(chrome.dll -incoming_task_queue.cc:124 )	base::internal::IncomingTaskQueue::RunTask(base::PendingTask *)
0x6a967375	(chrome.dll -message_loop.cc:395 )	base::MessageLoop::RunTask(base::PendingTask *)
0x6a967192	(chrome.dll -message_loop.cc:407 )	base::MessageLoop::DeferOrRunPendingTask(base::PendingTask)
0x6a92bdc2	(chrome.dll -message_loop.cc:451 )	base::MessageLoop::DoWork()
0x6a9d6677	(chrome.dll -message_pump_win.cc:173 )	base::MessagePumpForUI::DoRunLoop()
0x6a92bb8d	(chrome.dll -message_pump_win.cc:56 )	base::MessagePumpWin::Run(base::MessagePump::Delegate *)
0x6a92bafe	(chrome.dll -message_loop.cc:346 )	base::MessageLoop::Run(bool)
0x6a92b82d	(chrome.dll -run_loop.cc:133 )	base::RunLoop::Run()
0x6ac19a7f	(chrome.dll -chrome_browser_main.cc:2190 )	ChromeBrowserMainParts::MainMessageLoopRun(int *)
0x6ac1990c	(chrome.dll -browser_main_loop.cc:1104 )	content::BrowserMainLoop::RunMainMessageLoopParts()
0x6ac198c7	(chrome.dll -browser_main_runner.cc:160 )	content::BrowserMainRunnerImpl::Run()
0x6a8e5813	(chrome.dll -browser_main.cc:46 )	content::BrowserMain(content::MainFunctionParams const &)
0x6a8e5707	(chrome.dll -content_main_runner.cc:423 )	content::RunNamedProcessTypeMain(std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,content::MainFunctionParams const &,content::ContentMainDelegate *)
0x6a8e55e1	(chrome.dll -content_main_runner.cc:703 )	content::ContentMainRunnerImpl::Run()
0x6a8d5a56	(chrome.dll -main.cc:453 )	service_manager::Main(service_manager::MainParams const &)
0x6a8d5737	(chrome.dll -content_main.cc:19 )	content::ContentMain(content::ContentMainParams const &)
0x6a8d2349	(chrome.dll -chrome_main.cc:101 )	ChromeMain
0x012c3001	(chrome.exe -main_dll_loader_win.cc:199 )	MainDllLoader::Launch(HINSTANCE__ *,base::TimeTicks)
0x012c145c	(chrome.exe -chrome_exe_main_win.cc:230 )	wWinMain
0x0137df67	(chrome.exe -exe_common.inl:283 )	__scrt_common_main_seh
0x75c91173	(kernel32.dll + 0x00051173 )	BaseThreadInitThunk
0x776ab3f4	(ntdll.dll + 0x0005b3f4 )	__RtlUserThreadStart
0x776ab3c7	(ntdll.dll + 0x0005b3c7 )	_RtlUserThreadStart

When I restrict the OS to Mac, we continue to see a very low level of this crash - about once a day.

Perhaps we can add some DCHECKs for the WidgetDelegate, observers, and NonClientView not being destroyed early. In particular WidgetObserver::~WidgetObserver() does **NOT** remove the observer from widgets it's observing, so that seems like fertile ground for lifetime bugs.
Pls mark the bug as fixed if CL is landed in trunk and nothing else is pending. Thank you.
Labels: -Target-68 Target-69
Labels: -M-68 Group-Stability
Labels: M-68
Description: Show this description
Labels: -M-68 M-69
Labels: -M-69 -Target-69 M-70 Target-70
The change that fixes this is likely unsafe to merge to M69
Project Member

Comment 37 by bugdroid1@chromium.org, Aug 21

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

commit 30f97fddc348e14c23499ffc0f638deff901641c
Author: Trent Apted <tapted@chromium.org>
Date: Tue Aug 21 09:03:47 2018

base::CheckedObserver - a common base class for observers.

Turns a possible UAF when trying to notify a deleted observer into a
CHECK(). This is achieved with minimal changes to ObserverList and an
adapter that gives WeakPtr-like semantics to an observer interface.

base::ObserverList<T>::Unchecked continues to use raw pointers that
are unchecked.

Bug: 842987, 808318
Change-Id: I8ab20845f4f6e1d2559490287731cea2dbf40d39
Reviewed-on: https://chromium-review.googlesource.com/1053338
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584694}
[modify] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/base/BUILD.gn
[modify] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/base/memory/weak_ptr.h
[modify] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/base/memory/weak_ptr_unittest.cc
[modify] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/base/observer_list.h
[add] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/base/observer_list_internal.cc
[add] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/base/observer_list_internal.h
[add] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/base/observer_list_types.cc
[add] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/base/observer_list_types.h
[modify] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/base/observer_list_unittest.cc
[add] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/base/observer_list_unittest.nc
[modify] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/base/test/gtest_util.h
[modify] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/ui/views/widget/widget.h
[modify] https://crrev.com/30f97fddc348e14c23499ffc0f638deff901641c/ui/views/widget/widget_observer.h

Labels: FoundIn-71
We have been consistently seeing this crash on M71 Beta when compared to previous Chrome Beta milestones. 

Please find the crash history here : https://goto.google.com/mjihf

Stack : 
Thread 0 (id: 0x4317) CRASHED [EXC_BREAKPOINT / EXC_I386_BPT @ 0x000000010bdbed06 ] MAGIC SIGNATURE THREAD
Stack Quality84%Show frame trust levels
0x000000010bdbed06	(Google Chrome Framework -observer_list_internal.h:73)	views::Widget::OnNativeWidgetDestroying()
0x000000010bdd28be	(Google Chrome Framework -bridged_native_widget_host_impl.mm:728)	views::BridgedNativeWidgetHostImpl::OnWindowWillClose()
0x000000010bd384be	(Google Chrome Framework -bridged_native_widget_impl.mm:702)	views::BridgedNativeWidgetImpl::OnWindowWillClose()
0x000000010bdce0d8	(Google Chrome Framework -views_nswindow_delegate.mm:143)	-[ViewsNSWindowDelegate windowWillClose:]
0x00007fff2e1e83f1	(CoreFoundation+ 0x0009f3f1)	__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__
0x00007fff2e1e836b	(CoreFoundation+ 0x0009f36b)	___CFXRegistrationPost_block_invoke
0x00007fff2e1e828c	(CoreFoundation+ 0x0009f28c)	_CFXRegistrationPost
0x00007fff2e1f06d8	(CoreFoundation+ 0x000a76d8)	___CFXNotificationPost_block_invoke
0x00007fff2e157e89	(CoreFoundation+ 0x0000ee89)	-[_CFXNotificationRegistrar find:object:observer:enumerator:]
0x00007fff2e15724c	(CoreFoundation+ 0x0000e24c)	_CFXNotificationPost
0x00007fff304df98a	(Foundation+ 0x0001198a)	-[NSNotificationCenter postNotificationName:object:userInfo:]
0x00007fff2bfeff14	(AppKit+ 0x00931f14)	-[NSWindow _finishClosingWindow]
0x00007fff2ba5f22b	(AppKit+ 0x003a122b)	-[NSWindow _close]
0x000000010bd39661	(Google Chrome Framework -bind_internal.h:498)	base::internal::Invoker<base::internal::BindState<base::mac::ScopedBlock<void () block_pointer> >, void ()>::RunOnce(base::internal::BindStateBase*)
0x000000010a144769	(Google Chrome Framework -callback.h:99)	base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*)
0x000000010a15ff1e	(Google Chrome Framework -message_loop.cc:434)	base::MessageLoop::RunTask(base::PendingTask*)
0x000000010a160272	(Google Chrome Framework -message_loop.cc:445)	base::MessageLoop::DoWork()
0x000000010a1624e9	(Google Chrome Framework -message_pump_mac.mm:455)	base::MessagePumpCFRunLoopBase::RunWork()
0x000000010a1543c9	(Google Chrome Framework+ 0x0274d3c9)	base::mac::CallWithEHFrame(void () block_pointer)
0x000000010a161e4e	(Google Chrome Framework -message_pump_mac.mm:431)	base::MessagePumpCFRunLoopBase::RunWorkSource(void*)
0x00007fff2e1a1154	(CoreFoundation+ 0x00058154)	__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
0x00007fff2e1a10fa	(CoreFoundation+ 0x000580fa)	__CFRunLoopDoSource0
0x00007fff2e184b94	(CoreFoundation+ 0x0003bb94)	__CFRunLoopDoSources0
0x00007fff2e18413d	(CoreFoundation+ 0x0003b13d)	__CFRunLoopRun
0x00007fff2e183a27	(CoreFoundation+ 0x0003aa27)	CFRunLoopRunSpecific
0x00007fff2d41cb34	(HIToolbox+ 0x0000ab34)	RunCurrentEventLoopInMode
0x00007fff2d41c86a	(HIToolbox+ 0x0000a86a)	ReceiveNextEventCommon
0x00007fff2d41c5e7	(HIToolbox+ 0x0000a5e7)	_BlockUntilNextEventMatchingListInModeWithFilter
0x00007fff2b6d8eb6	(AppKit+ 0x0001aeb6)	_DPSNextEvent
0x00007fff2b6d7c55	(AppKit+ 0x00019c55)	-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
0x0000000109d4250f	(Google Chrome Framework -chrome_browser_application_mac.mm:255)	__71-[BrowserCrApplication nextEventMatchingMask:untilDate:inMode:dequeue:]_block_invoke
0x000000010a1543c9	(Google Chrome Framework+ 0x0274d3c9)	base::mac::CallWithEHFrame(void () block_pointer)
0x0000000109d42443	(Google Chrome Framework -chrome_browser_application_mac.mm:254)	-[BrowserCrApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
0x00007fff2b6d1cb8	(AppKit+ 0x00013cb8)	-[NSApplication run]
0x000000010a162dab	(Google Chrome Framework -message_pump_mac.mm:808)	base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*)
0x000000010a16192d	(Google Chrome Framework -message_pump_mac.mm:184)	base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*)
0x000000010a1848e4	(Google Chrome Framework -run_loop.cc:102)	<name omitted>
0x0000000109d48c0c	(Google Chrome Framework -chrome_browser_main.cc:2028)	ChromeBrowserMainParts::MainMessageLoopRun(int*)
0x00000001086e4b03	(Google Chrome Framework -browser_main_loop.cc:998)	content::BrowserMainLoop::RunMainMessageLoopParts()
0x00000001086e7131	(Google Chrome Framework -browser_main_runner_impl.cc:165)	content::BrowserMainRunnerImpl::Run()
0x00000001086e187a	(Google Chrome Framework -browser_main.cc:47)	content::BrowserMain(content::MainFunctionParams const&)
0x0000000109cfe08a	(Google Chrome Framework -content_main_runner_impl.cc:541)	content::ContentMainRunnerImpl::Run(bool)
0x000000010ba11bdc	(Google Chrome Framework -main.cc:472)	service_manager::Main(service_manager::MainParams const&)
0x0000000109cfd273	(Google Chrome Framework -content_main.cc:19)	content::ContentMain(content::ContentMainParams const&)
0x0000000107a0ae5e	(Google Chrome Framework -chrome_main.cc:102)	ChromeMain
0x0000000104ee7dcd	(Google Chrome -chrome_exe_main_mac.cc:101)	main
0x00007fff5b36108c	(libdyld.dylib+ 0x0001708c)	start
0x00007fff5b36108c	(libdyld.dylib+ 0x0001708c)	start
Labels: -Restrict-View-EditIssue
Owner: robliao@chromium.org
We have an ongoing very low rate of this on Mac, and a higher rate on ChromeOS. I think it's pretty clear this is a cross-platform issue so I'm going to assign it to robliao@ for Views triage.

I'm also stripping R-V-EI since there's nothing sensitive in this bug.
Cc: mfo...@chromium.org
Note WidgetObserver inheriting from CheckedObserver flushed out one leaked observer in the Views Cast dialog -- that was  Issue 883976 . With a repro case, we were able to flush out the stack with a diagnostic CL --> https://chromium-review.googlesource.com/c/chromium/src/+/1226472

Crashes attributable to the known repro case for the cast dialog should have dried up after r592183 (71.0.3556.0). go/mjihf shows crashes in 71.0.3578.98, so another dialog (or another repro case) that we don't know of is likely still leaking a WidgetObserver.

Checking experiments, https://goto.google.com/ccivg shows 76.95% of crashes having ViewsCastDialog: Enabled (and none with it Disabled). It looks like it's currently at 50/50, so it's out of proportion, but maybe not a definite signal.
Actually, consider just 72.0.3593.0 for now (it ranks highly in the link in #c1)
 - 43 crashes [1]
 - 42 are ChromeOS [1] (1 mac)
 - 100% are ViewsCastDialog Enabled [1]
 - Only 39% of *all* ChromeOS 72.0.3593.0 crashes have ViewsCastDialog Enabled. [2]

branch 3593 is a bit old though. Let's focus on the branch going to beta (3626)
 - 12 crashes [3]
 - 1+6+5 (chromeOS,linux,mac) [3]
 - 100% are ViewsCastDialog Enabled [3]
 - (vs 20/20 split across all products/platforms) [4]

The numbers are low for now, but I think that's a clear signal of "something" involving the cast dialog. ChromeOS releases lag behind which might be why the numbers are low at the moment for 3626 (3626 only went to a ChromeOS beta 5 days ago, and many devices lag behind that still).

I'll make a new bug.


[1] dev - https://crash.corp.google.com/browse?q=expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27views%3A%3AWidget%3A%3AOnNativeWidgetDestroying%27+AND+product.Version%3D%2772.0.3593.0%27#-propertyselector,productname:1000,-magicsignature:50,-magicsignature2:50,-stablesignature:50,+experiments
[2] dev - https://crash.corp.google.com/browse?q=product_name%3D%27Chrome_ChromeOS%27+AND+product.Version%3D%2772.0.3593.0%27#-propertyselector,-productname:1000,-magicsignature:50,-magicsignature2:50,-stablesignature:50,+experiments:100,-magicsignaturesorted:50
[3] beta - https://crash.corp.google.com/browse?q=expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27views%3A%3AWidget%3A%3AOnNativeWidgetDestroying%27+AND+product.Version+like+%2772.0.3626.%25%27#-propertyselector,productname:1000,productversion:100,-magicsignature:50,-magicsignature2:50,-stablesignature:50,+experiments
[4] https://crash.corp.google.com/browse?q=product.Version+like+%2772.0.3626.%25%27#-propertyselector,productname:1000,productversion:100,-magicsignature:50,-magicsignature2:50,-stablesignature:50,+experiments:100
> I'll make a new bug

Filed Issue 922328 for m72/cast.

Also filed Issue 922327 which has OnNativeWidgetDestroying in the stack, but a different magic signature. And is definitely ChromeOS only and not related to cast.

Sign in to add a comment