Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2375903002:
Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. (Closed)
DescriptionMacViews: Emit OnNativeWidgetDestroying() before clearing the
NSWindowDelegate.
Currently the NSWindowDelegate is cleared beforehand, which
means observers of OnWidgetDestroying() are unable to use
GetWidgetForNativeWindow(). This is a problem during closure of
the browser window because inactive tabs are reattached when they
become active due to the active tab being closed. This results in a
DCHECK in NativeViewHostMac::AttachNativeView().
To fix, clear the NSWindowDelegate in the method
BridgedNativeWidget calls on NativeWidgetMac which deletes the
BridgedNativeWidget instance (i.e. just after emitting
OnNativeWidgetDestroying()).
R=tapted@chromium.org
BUG=604628
Committed: https://crrev.com/f7941c454c74e229cedc6b3aed96bee9b3f22e68
Cr-Commit-Position: refs/heads/master@{#423676}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move reseting NSWindow delegate to NativeWidgetMac #
Total comments: 2
Patch Set 3 : Add comment #
Messages
Total messages: 31 (11 generated)
Can you file a BUG? it's not clear from the CL description where the crash is occurring, and whether this is the correct fix. But also we *must* clear the delegate - ViewsNSWindowDelegate has references to C++ objects that are being destroyed. We can't have refcounted Objective C objects floating around that could still reference them and cause UAF. If something is causing an invalid access by assuming the delegate is still set, then it needs to act earlier, before the delegate gets cleared, e.g. by listening for an event posted via the observer interface. Also, ideally we do not store an extra bool on the window that's only set when closing -- can we derive this from something else? Perhaps also take a look at some code I'm experimenting with in https://codereview.chromium.org/2061693003/ - this does something similar by setting a bool during close but I don't like it and I'm looking for a better solution.
On 2016/09/29 07:10:03, tapted wrote:
> Can you file a BUG? it's not clear from the CL description where the crash is
> occurring, and whether this is the correct fix.
For crash just open chromium, open second tab, switch to first tab and press red
"traffic light" button. (The BrowserViewTestMac.CloseWithTabsStartWithActive
does the same):
* thread #1: tid = 0x2c1226, 0x0000000115385d14
libbase.dylib`base::debug::BreakDebugger() + 20 at debugger_posix.cc:260, name =
'CrBrowserMain', queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT
(code=EXC_I386_BPT, subcode=0x0)
* frame #0: 0x0000000115385d14 libbase.dylib`base::debug::BreakDebugger() + 20
at debugger_posix.cc:260
frame #1: 0x0000000115430fee
libbase.dylib`logging::LogMessage::~LogMessage(this=0x00007fff5fbfada0) + 4302
at logging.cc:748
frame #2: 0x000000011542db25
libbase.dylib`logging::LogMessage::~LogMessage(this=0x00007fff5fbfada0) + 21 at
logging.cc:528
frame #3: 0x0000000128f1e242
libviews.dylib`views::NativeViewHostMac::AttachNativeView(this=0x000000010017def0)
+ 914 at native_view_host_mac.mm:51
frame #4: 0x0000000128f1cd2e
libviews.dylib`views::NativeViewHost::Attach(this=0x000000010017d6b0,
native_view=0x000000013a1c2ff0) + 510 at native_view_host.cc:37
frame #5: 0x000000012a871628
libwebview.dylib`views::WebView::AttachWebContents(this=0x000000010017d470) +
296 at webview.cc:334
frame #6: 0x000000012a870f38
libwebview.dylib`views::WebView::SetWebContents(this=0x000000010017d470,
replacement=0x0000000100851c00) + 1128 at webview.cc:80
frame #7: 0x0000000108f939f6
libchrome_dll.dylib`BrowserView::OnActiveTabChanged(this=0x000000013f852360,
old_contents=0x000000013c074000, new_contents=0x0000000100851c00, index=0,
reason=0) + 902 at browser_view.cc:861
frame #8: 0x0000000108a0ae53
libchrome_dll.dylib`Browser::ActiveTabChanged(this=0x000000013a30fb30,
old_contents=0x000000013c074000, new_contents=0x0000000100851c00, index=0,
reason=0) + 195 at browser.cc:1008
frame #9: 0x0000000108acca53
libchrome_dll.dylib`TabStripModel::NotifyIfActiveTabChanged(this=0x000000013a30fdf0,
old_contents=0x000000013c074000, notify_types=NOTIFY_DEFAULT) + 483 at
tab_strip_model.cc:1259
frame #10: 0x0000000108ac88e1
libchrome_dll.dylib`TabStripModel::DetachWebContentsAt(this=0x000000013a30fdf0,
index=0) + 2705 at tab_strip_model.cc:400
frame #11: 0x0000000108f9a71f
libchrome_dll.dylib`BrowserView::OnWidgetDestroying(this=0x000000013f852360,
widget=0x000000013f850820) + 111 at browser_view.cc:1810
frame #12: 0x0000000128ff9f40
libviews.dylib`views::Widget::OnNativeWidgetDestroying(this=0x000000013f850820)
+ 256 at widget.cc:1077
frame #13: 0x0000000128fe3571
libviews.dylib`views::NativeWidgetMac::OnWindowWillClose(this=0x000000013f851bf0)
+ 81 at native_widget_mac.mm:108
frame #14: 0x0000000128e8bdf1
libviews.dylib`views::BridgedNativeWidget::OnWindowWillClose(this=0x000000013f8509a0)
+ 193 at bridged_native_widget.mm:712
frame #15: 0x0000000128ea82c5 libviews.dylib`::-[ViewsNSWindowDelegate
windowWillClose:](self=0x000000013f84ed20, _cmd="windowWillClose:",
notification="NSWindowWillCloseNotification") + 373 at
views_nswindow_delegate.mm:95
frame #16: 0x00007fffc7cf063c
CoreFoundation`__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
frame #17: 0x00007fffc7cf053b CoreFoundation`_CFXRegistrationPost + 427
frame #18: 0x00007fffc7cf02a2
CoreFoundation`___CFXNotificationPost_block_invoke + 50
frame #19: 0x00007fffc7cad9a3 CoreFoundation`-[_CFXNotificationRegistrar
find:object:observer:enumerator:] + 1827
frame #20: 0x00007fffc7cac9dc CoreFoundation`_CFXNotificationPost + 604
frame #21: 0x00007fffc96bc0e3 Foundation`-[NSNotificationCenter
postNotificationName:object:userInfo:] + 66
frame #22: 0x00007fffc5c091ac AppKit`__18-[NSWindow _close]_block_invoke +
205
frame #23: 0x00007fffc5c09090 AppKit`-[NSWindow _close] + 365
frame #24: 0x00007fffc5c97984 AppKit`-[NSWindow __close] + 305
frame #25: 0x00007fffdcf4c03d libsystem_trace.dylib`_os_activity_initiate +
61
frame #26: 0x00007fffc60844e7 AppKit`-[NSApplication(NSResponder)
sendAction:to:from:] + 456
frame #27: 0x0000000102fe862d libchrome_dll.dylib`::-[BrowserCrApplication
sendAction:to:from:](self=0x0000000100114410, _cmd="sendAction:to:from:",
anAction="_close:", aTarget=0x000000014321ca20, sender=0x000000014321f110) +
1085 at chrome_browser_application_mac.mm:315
frame #28: 0x00007fffc5bd4245 AppKit`-[NSControl sendAction:to:] + 86
frame #29: 0x00007fffc5bd416d AppKit`__26-[NSCell
_sendActionFrom:]_block_invoke + 136
frame #30: 0x00007fffdcf4c03d libsystem_trace.dylib`_os_activity_initiate +
61
frame #31: 0x00007fffc5bd40c5 AppKit`-[NSCell _sendActionFrom:] + 128
frame #32: 0x00007fffc5c1692a AppKit`-[NSButtonCell _sendActionFrom:] + 98
frame #33: 0x00007fffdcf4c03d libsystem_trace.dylib`_os_activity_initiate +
61
frame #34: 0x00007fffc5bd2a58 AppKit`-[NSCell
trackMouse:inRect:ofView:untilMouseUp:] + 2481
frame #35: 0x00007fffc5c16667 AppKit`-[NSButtonCell
trackMouse:inRect:ofView:untilMouseUp:] + 785
frame #36: 0x00007fffc5bd14c8 AppKit`-[NSControl mouseDown:] + 832
frame #37: 0x00007fffc5c70e9e AppKit`-[_NSThemeWidget mouseDown:] + 87
frame #38: 0x00007fffc61e473d AppKit`-[NSWindow(NSEventRouting)
_handleMouseDownEvent:isDelayedEvent:] + 6341
frame #39: 0x00007fffc61e0f8c AppKit`-[NSWindow(NSEventRouting)
_reallySendEvent:isDelayedEvent:] + 1942
frame #40: 0x00007fffc61e042a AppKit`-[NSWindow(NSEventRouting) sendEvent:]
+ 541
frame #41: 0x0000000128ea6637 libviews.dylib`::-[NativeWidgetMacNSWindow
sendEvent:](self=0x000000014321ca20, _cmd="sendEvent:",
event=0x000000013a146240) + 199 at native_widget_mac_nswindow.mm:120
frame #42: 0x00007fffc6080bf5 AppKit`-[NSApplication(NSEvent) sendEvent:] +
1145
frame #43: 0x0000000102fe88d3
libchrome_dll.dylib`::__34-[BrowserCrApplication
sendEvent:]_block_invoke(.block_descriptor=<unavailable>) + 259 at
chrome_browser_application_mac.mm:343
frame #44: 0x0000000115433f0a libbase.dylib`base::mac::CallWithEHFrame(void
() block_pointer) + 10 at call_with_eh_frame_asm.S:36
frame #45: 0x0000000102fe87ad libchrome_dll.dylib`::-[BrowserCrApplication
sendEvent:](self=0x0000000100114410, _cmd="sendEvent:",
event=0x000000013a146240) + 109 at chrome_browser_application_mac.mm:327
frame #46: 0x00007fffc5967009 AppKit`-[NSApplication run] + 1002
frame #47: 0x0000000115491ac8
libbase.dylib`base::MessagePumpNSApplication::DoRun(this=0x000000013a15e270,
delegate=0x000000013a15df90) + 312 at message_pump_mac.mm:665
frame #48: 0x0000000115490a3a
libbase.dylib`base::MessagePumpCFRunLoopBase::Run(this=0x000000013a15e270,
delegate=0x000000013a15df90) + 122 at message_pump_mac.mm:238
frame #49: 0x000000011547e4ba
libbase.dylib`base::MessageLoop::RunHandler(this=0x000000013a15df90) + 298 at
message_loop.cc:432
frame #50: 0x0000000115549005
libbase.dylib`base::RunLoop::Run(this=0x00007fff5fbfe858) + 85 at run_loop.cc:35
frame #51: 0x0000000102ffbb10
libchrome_dll.dylib`ChromeBrowserMainParts::MainMessageLoopRun(this=0x000000013a106f30,
result_code=0x000000013a106da8) + 400 at chrome_browser_main.cc:2107
frame #52: 0x000000011b48d361
libcontent.dylib`content::BrowserMainLoop::RunMainMessageLoopParts(this=0x000000013a106d90)
+ 417 at browser_main_loop.cc:957
frame #53: 0x000000011b4977d1
libcontent.dylib`content::BrowserMainRunnerImpl::Run(this=0x000000013a1053c0) +
481 at browser_main_runner.cc:155
frame #54: 0x000000011b481305
libcontent.dylib`content::BrowserMain(parameters=0x00007fff5fbff330) + 421 at
browser_main.cc:46
frame #55: 0x000000011de514f7
libcontent.dylib`content::RunNamedProcessTypeMain(process_type="",
main_function_params=0x00007fff5fbff330, delegate=0x00007fff5fbff7c0) + 599 at
content_main_runner.cc:418
frame #56: 0x000000011de533e6
libcontent.dylib`content::ContentMainRunnerImpl::Run(this=0x000000013a20c660) +
1462 at content_main_runner.cc:786
frame #57: 0x000000011de50d5d
libcontent.dylib`content::ContentMain(params=0x00007fff5fbff7a0) + 349 at
content_main.cc:20
frame #58: 0x00000001018061c9 libchrome_dll.dylib`::ChromeMain(argc=1,
argv=0x00007fff5fbff910) + 105 at chrome_main.cc:97
frame #59: 0x0000000100000d6c Chromium`main(argc=1, argv=0x00007fff5fbff910)
+ 780 at chrome_exe_main_mac.c:85
frame #60: 0x0000000100000a54 Chromium`start + 52
So, I think remove ViewsNSWindowDelegate for NSWindow in
BridgedNativeWidget::OnWindowWillClose by call [window_ setDelegate:nil]; is a
bad idea.
Because the real reference for them will removed only in destructor, but we used
this fact to determine we will need call OnWindowWillClose in destructor or no.
It's not clear, and i think use the flag is more fairly, but not so beautiful.
Please file a bug. Ideally, the bug + CL description should be enough to convince me that this is the right fix without me having to diagnose the issue myself. Currently that's not the case, and you're asking me to reproduce the issue and look at crash stacks myself. I'm away from Internet for the next few days, so I'll need to pick this up again mid next week.
Description was changed from ========== MacViews: fix crash on press red "traffic light" button. This is a continued fix for CL https://codereview.chromium.org/2015223002/ In case [NSWindow close] we will crash on NativeViewHostMac::AttachNativeView: Because NativeWidgetMac::GetBridgeForNativeWindow get valid bridge only if NSWindow still has a delegate. R=tapted@chromium.org BUG= ========== to ========== MacViews: fix crash on press red "traffic light" button. This is a continued fix for CL https://codereview.chromium.org/2015223002/ In case [NSWindow close] we will crash on NativeViewHostMac::AttachNativeView: Because NativeWidgetMac::GetBridgeForNativeWindow get valid bridge only if NSWindow still has a delegate. R=tapted@chromium.org BUG=604628 ==========
On 2016/09/29 23:12:47, tapted wrote: > Please file a bug. > > Ideally, the bug + CL description should be enough to convince me that this is > the right fix without me having to diagnose the issue myself. Currently that's > not the case, and you're asking me to reproduce the issue and look at crash > stacks myself. I'm away from Internet for the next few days, so I'll need to > pick this up again mid next week. It is already created issue by you: https://bugs.chromium.org/p/chromium/issues/detail?id=604628 I attached it for this CL.
Make sure your CL description follows the guide at https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... You need to mention - current behavior (and, for bugs, why it occurs) - introduced differences - for bugs, how it fixes the problem https://codereview.chromium.org/2375903002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view_browsertest_mac.mm (right): https://codereview.chromium.org/2375903002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view_browsertest_mac.mm:23: } this isn't the right place for a test, and there are no expectations here, and no comments. The tests belong in widget_unittest.cc -- we need to ensure platforms are consistent. There is already a test there: // Test that the NativeWidget is still valid during OnNativeWidgetDestroying(), // and properties that depend on it are valid, when closed via Close(). TEST_F(WidgetTest, ValidDuringOnNativeWidgetDestroyingFromClose) If you change WidgetBoundsObserver::OnWidgetDestroying() to something like // WidgetObserver: void OnWidgetDestroying(Widget* widget) override { EXPECT_TRUE(widget->GetNativeWindow()); EXPECT_TRUE(Widget::GetWidgetForNativeWindow(widget->GetNativeWindow())); bounds_ = widget->GetWindowBoundsInScreen(); } I think this will give proper coverage for what we're interested in here. https://codereview.chromium.org/2375903002/diff/1/ui/views/cocoa/bridged_nati... File ui/views/cocoa/bridged_native_widget.mm (left): https://codereview.chromium.org/2375903002/diff/1/ui/views/cocoa/bridged_nati... ui/views/cocoa/bridged_native_widget.mm:712: native_widget_mac_->OnWindowWillClose(); Is the problem just that NativeWidgetMac::OnWindowWillClose() invokes OnWindowDestroying() too late? What about making this method something like void BridgedNativeWidget::OnWindowWillClose() { native_widget_mac_->OnWindowDestroying(); /* everything else */ native_widget_mac_->OnWindowDestroyed(); } (i.e. split NativeWidgetMac::OnWindowWillClose into two methods -- this will move the implementation closer to what NativeWidgetAura is already doing).
Maybe have a look at the experiments I'm doing in https://codereview.chromium.org/2393843002 The main point is that we need to properly understand these codepaths. (i.e. It's not good enough to just set a flag to ensure a particular method doesn't get called twice). I think we can make things more consistent by funnelling things through [NSWindow close] more consistently, but that requires some tests to be updated.
Description was changed from ========== MacViews: fix crash on press red "traffic light" button. This is a continued fix for CL https://codereview.chromium.org/2015223002/ In case [NSWindow close] we will crash on NativeViewHostMac::AttachNativeView: Because NativeWidgetMac::GetBridgeForNativeWindow get valid bridge only if NSWindow still has a delegate. R=tapted@chromium.org BUG=604628 ========== to ========== MacViews: fix crash on press red "traffic light" button. For crash just open chromium, open second tab, switch to first tab and press red "traffic light" button. This is a continued fix for CL https://codereview.chromium.org/2015223002/ In case [NSWindow close] we will crash on NativeViewHostMac::AttachNativeView: Because NativeWidgetMac::GetBridgeForNativeWindow get valid bridge only if NSWindow still has a delegate. So for case NSWindow closed by the user, we will reset NSWindow delegate inside NativeWidgetMac::OnWindowWillClose() after calling delegate_->OnNativeWidgetDestroying() (that might be want valid delegate for NSWindow), but before deleting the bridge. R=tapted@chromium.org BUG=604628 ==========
neat! I like it. But it's still difficult to grasp the core problem from the CL description. I suggest something like Subject: MacViews: Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. MacViews: Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. Currently the NSWindowDelegate is cleared beforehand, which means observers of OnWidgetDestroying() are unable to use GetWidgetForNativeWindow(). This is a problem during closure of the browser window because inactive tabs are reattached when they become active due to the active tab being closed. This results in a DCHECK in NativeViewHostMac::AttachNativeView(). To fix, clear the NSWindowDelegate in the method BridgedNativeWidget calls on NativeWidgetMac which deletes the BridgedNativeWidget instance (i.e. just after emitting OnNativeWidgetDestroying()). https://codereview.chromium.org/2375903002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2375903002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:711: native_widget_mac_->OnWindowWillClose(); Since it's convention to clear the delegate in -windowWillClose:, this needs a comment above like // Note this also clears the NSWindow delegate, after informing Widget // delegates about the closure. NativeWidgetMac then deletes |this| before // returning.
Description was changed from ========== MacViews: fix crash on press red "traffic light" button. For crash just open chromium, open second tab, switch to first tab and press red "traffic light" button. This is a continued fix for CL https://codereview.chromium.org/2015223002/ In case [NSWindow close] we will crash on NativeViewHostMac::AttachNativeView: Because NativeWidgetMac::GetBridgeForNativeWindow get valid bridge only if NSWindow still has a delegate. So for case NSWindow closed by the user, we will reset NSWindow delegate inside NativeWidgetMac::OnWindowWillClose() after calling delegate_->OnNativeWidgetDestroying() (that might be want valid delegate for NSWindow), but before deleting the bridge. R=tapted@chromium.org BUG=604628 ========== to ========== MacViews: Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. Currently the NSWindowDelegate is cleared beforehand, which means observers of OnWidgetDestroying() are unable to use GetWidgetForNativeWindow(). This is a problem during closure of the browser window because inactive tabs are reattached when they become active due to the active tab being closed. This results in a DCHECK in NativeViewHostMac::AttachNativeView(). To fix, clear the NSWindowDelegate in the method BridgedNativeWidget calls on NativeWidgetMac which deletes the BridgedNativeWidget instance (i.e. just after emitting OnNativeWidgetDestroying()). R=tapted@chromium.org BUG=604628 ==========
Description was changed from ========== MacViews: Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. Currently the NSWindowDelegate is cleared beforehand, which means observers of OnWidgetDestroying() are unable to use GetWidgetForNativeWindow(). This is a problem during closure of the browser window because inactive tabs are reattached when they become active due to the active tab being closed. This results in a DCHECK in NativeViewHostMac::AttachNativeView(). To fix, clear the NSWindowDelegate in the method BridgedNativeWidget calls on NativeWidgetMac which deletes the BridgedNativeWidget instance (i.e. just after emitting OnNativeWidgetDestroying()). R=tapted@chromium.org BUG=604628 ========== to ========== MacViews: Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. Currently the NSWindowDelegate is cleared beforehand, which means observers of OnWidgetDestroying() are unable to use GetWidgetForNativeWindow(). This is a problem during closure of the browser window because inactive tabs are reattached when they become active due to the active tab being closed. This results in a DCHECK in NativeViewHostMac::AttachNativeView(). To fix, clear the NSWindowDelegate in the method BridgedNativeWidget calls on NativeWidgetMac which deletes the BridgedNativeWidget instance (i.e. just after emitting OnNativeWidgetDestroying()). R=tapted@chromium.org BUG=604628 ==========
Description was changed from ========== MacViews: Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. Currently the NSWindowDelegate is cleared beforehand, which means observers of OnWidgetDestroying() are unable to use GetWidgetForNativeWindow(). This is a problem during closure of the browser window because inactive tabs are reattached when they become active due to the active tab being closed. This results in a DCHECK in NativeViewHostMac::AttachNativeView(). To fix, clear the NSWindowDelegate in the method BridgedNativeWidget calls on NativeWidgetMac which deletes the BridgedNativeWidget instance (i.e. just after emitting OnNativeWidgetDestroying()). R=tapted@chromium.org BUG=604628 ========== to ========== MacViews: Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. Currently the NSWindowDelegate is cleared beforehand, which means observers of OnWidgetDestroying() are unable to use GetWidgetForNativeWindow(). This is a problem during closure of the browser window because inactive tabs are reattached when they become active due to the active tab being closed. This results in a DCHECK in NativeViewHostMac::AttachNativeView(). To fix, clear the NSWindowDelegate in the method BridgedNativeWidget calls on NativeWidgetMac which deletes the BridgedNativeWidget instance (i.e. just after emitting OnNativeWidgetDestroying()). R=tapted@chromium.org BUG=604628 ==========
https://codereview.chromium.org/2375903002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2375903002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:711: native_widget_mac_->OnWindowWillClose(); On 2016/10/06 08:36:05, tapted wrote: > Since it's convention to clear the delegate in -windowWillClose:, this needs a > comment above like > > // Note this also clears the NSWindow delegate, after informing Widget > // delegates about the closure. NativeWidgetMac then deletes |this| before > // returning. done
lgtm
The CQ bit was checked by yamaxim@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
yamaxim@yandex-team.ru changed reviewers: + sky@chromium.org
LGTM
The CQ bit was checked by yamaxim@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MacViews: Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. Currently the NSWindowDelegate is cleared beforehand, which means observers of OnWidgetDestroying() are unable to use GetWidgetForNativeWindow(). This is a problem during closure of the browser window because inactive tabs are reattached when they become active due to the active tab being closed. This results in a DCHECK in NativeViewHostMac::AttachNativeView(). To fix, clear the NSWindowDelegate in the method BridgedNativeWidget calls on NativeWidgetMac which deletes the BridgedNativeWidget instance (i.e. just after emitting OnNativeWidgetDestroying()). R=tapted@chromium.org BUG=604628 ========== to ========== MacViews: Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. Currently the NSWindowDelegate is cleared beforehand, which means observers of OnWidgetDestroying() are unable to use GetWidgetForNativeWindow(). This is a problem during closure of the browser window because inactive tabs are reattached when they become active due to the active tab being closed. This results in a DCHECK in NativeViewHostMac::AttachNativeView(). To fix, clear the NSWindowDelegate in the method BridgedNativeWidget calls on NativeWidgetMac which deletes the BridgedNativeWidget instance (i.e. just after emitting OnNativeWidgetDestroying()). R=tapted@chromium.org BUG=604628 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. Currently the NSWindowDelegate is cleared beforehand, which means observers of OnWidgetDestroying() are unable to use GetWidgetForNativeWindow(). This is a problem during closure of the browser window because inactive tabs are reattached when they become active due to the active tab being closed. This results in a DCHECK in NativeViewHostMac::AttachNativeView(). To fix, clear the NSWindowDelegate in the method BridgedNativeWidget calls on NativeWidgetMac which deletes the BridgedNativeWidget instance (i.e. just after emitting OnNativeWidgetDestroying()). R=tapted@chromium.org BUG=604628 ========== to ========== MacViews: Emit OnNativeWidgetDestroying() before clearing the NSWindowDelegate. Currently the NSWindowDelegate is cleared beforehand, which means observers of OnWidgetDestroying() are unable to use GetWidgetForNativeWindow(). This is a problem during closure of the browser window because inactive tabs are reattached when they become active due to the active tab being closed. This results in a DCHECK in NativeViewHostMac::AttachNativeView(). To fix, clear the NSWindowDelegate in the method BridgedNativeWidget calls on NativeWidgetMac which deletes the BridgedNativeWidget instance (i.e. just after emitting OnNativeWidgetDestroying()). R=tapted@chromium.org BUG=604628 Committed: https://crrev.com/f7941c454c74e229cedc6b3aed96bee9b3f22e68 Cr-Commit-Position: refs/heads/master@{#423676} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f7941c454c74e229cedc6b3aed96bee9b3f22e68 Cr-Commit-Position: refs/heads/master@{#423676} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
