New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 653325 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

MacViews: Check failed: 0u == [[window_ childWindows] count] (0 vs. 1)

Project Member Reported by karandeepb@chromium.org, Oct 5 2016

Issue description

Version: 55.0.2875.0
OS: Mac

What steps will reproduce the problem?
(1) Build chrome with DCHECKs enabled.
(2) Enable chrome://flags/#mac-views-native-dialogs
(3) Open bookmark bubble.
(4) Click inside the Name textfield and enter some text.
(5) Hover inside the Name textfield. Press Ctrl+Command+D. This should launch Mac Dictionary popup.
(6) Click outside the bookmark bubble.

What is the expected output?
The Bookmark bubble should close.

What do you see instead?
Browser crashes.


Stack trace-

[74035:1295:1006/104639:FATAL:bridged_native_widget.mm(1194)] Check failed: 0u == [[window_ childWindows] count] (0 vs. 1)
0   libbase.dylib                       0x00000001205d918e _ZN4base5debug10StackTraceC2Ev + 30
1   libbase.dylib                       0x00000001205d91f5 _ZN4base5debug10StackTraceC1Ev + 21
2   libbase.dylib                       0x000000012066fb80 _ZN7logging10LogMessageD2Ev + 80
3   libbase.dylib                       0x000000012066d735 _ZN7logging10LogMessageD1Ev + 21
4   libviews.dylib                      0x00000001346fdccd _ZN5views19BridgedNativeWidget26NotifyVisibilityChangeDownEv + 1405
5   libviews.dylib                      0x00000001346fd6ce _ZN5views19BridgedNativeWidget19OnVisibilityChangedEv + 526
6   libviews.dylib                      0x0000000134714708 -[ViewsNSWindowDelegate onWindowOrderChanged:] + 40
7   libviews.dylib                      0x0000000134713247 -[NativeWidgetMacNSWindow orderWindow:relativeTo:] + 135
8   libviews.dylib                      0x000000013485291d _ZN5views15NativeWidgetMac5CloseEv + 349
9   libviews.dylib                      0x000000013486456a _ZN5views6Widget5CloseEv + 362
10  libviews.dylib                      0x00000001346e4277 _ZN5views24BubbleDialogDelegateView25OnWidgetActivationChangedEPNS_6WidgetEb + 135
11  libviews.dylib                      0x00000001348666e8 _ZN5views6Widget31OnNativeWidgetActivationChangedEb + 184
12  libviews.dylib                      0x00000001346fe367 _ZN5views19BridgedNativeWidget26OnWindowKeyStatusChangedToEb + 87
13  libviews.dylib                      0x0000000134714c3c -[ViewsNSWindowDelegate windowDidResignKey:] + 44
14  CoreFoundation                      0x00007fff93a4fbbc __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
15  CoreFoundation                      0x00007fff93a4fb4f ___CFXRegistrationPost_block_invoke + 63
16  CoreFoundation                      0x00007fff93a4fac7 _CFXRegistrationPost + 407
17  CoreFoundation                      0x00007fff93a4f832 ___CFXNotificationPost_block_invoke + 50
18  CoreFoundation                      0x00007fff93a0c5e2 -[_CFXNotificationRegistrar find:object:observer:enumerator:] + 1922
19  CoreFoundation                      0x00007fff93a0b835 _CFXNotificationPost + 693
20  Foundation                          0x00007fff8754317a -[NSNotificationCenter postNotificationName:object:userInfo:] + 66
21  AppKit                              0x00007fff8ae2755c -[NSWindow resignKeyWindow] + 748
22  AppKit                              0x00007fff8ad4f971 _NXSendWindowNotification + 252
23  AppKit                              0x00007fff8ae271d7 endKeyAndMain + 84
24  AppKit                              0x00007fff8ada5b9b -[NSApplication sendEvent:] + 6991
25  libchrome_dll.dylib                 0x000000010dea0e23 __34-[BrowserCrApplication sendEvent:]_block_invoke + 259
26  libbase.dylib                       0x0000000120673b6a _ZN4base3mac15CallWithEHFrameEU13block_pointerFvvE + 10
27  libchrome_dll.dylib                 0x000000010dea0cfd -[BrowserCrApplication sendEvent:] + 109
28  AppKit                              0x00007fff8ac0bdf2 -[NSApplication run] + 796
29  libbase.dylib                       0x00000001206d4c28 _ZN4base24MessagePumpNSApplication5DoRunEPNS_11MessagePump8DelegateE + 312
30  libbase.dylib                       0x00000001206d3b9a _ZN4base24MessagePumpCFRunLoopBase3RunEPNS_11MessagePump8DelegateE + 122
31  libbase.dylib                       0x00000001206c18d1 _ZN4base11MessageLoop10RunHandlerEv + 289
32  libbase.dylib                       0x000000012078bea5 _ZN4base7RunLoop3RunEv + 85
33  libchrome_dll.dylib                 0x000000010deb4060 _ZN22ChromeBrowserMainParts18MainMessageLoopRunEPi + 400
34  libcontent.dylib                    0x0000000126b9fbf1 _ZN7content15BrowserMainLoop23RunMainMessageLoopPartsEv + 417
35  libcontent.dylib                    0x0000000126baa091 _ZN7content21BrowserMainRunnerImpl3RunEv + 481
36  libcontent.dylib                    0x0000000126b93b95 _ZN7content11BrowserMainERKNS_18MainFunctionParamsE + 421
37  libcontent.dylib                    0x0000000129580787 _ZN7content23RunNamedProcessTypeMainERKNSt3__112basic_stringIcNS0_11char_traitsIcEENS0_9allocatorIcEEEERKNS_18MainFunctionParamsEPNS_19ContentMainDelegateE + 599
38  libcontent.dylib                    0x0000000129582676 _ZN7content21ContentMainRunnerImpl3RunEv + 1462
39  libcontent.dylib                    0x000000012957ffed _ZN7content11ContentMainERKNS_17ContentMainParamsE + 349
40  libchrome_dll.dylib                 0x000000010c61dca9 ChromeMain + 105
41  Chromium                            0x000000010c3b1d6c main + 780
42  Chromium                            0x000000010c3b1a54 start + 52
43  ???                                 0x0000000000000001 0x0 + 1
 
Cc: spqc...@chromium.org
Sarah: Can you confirm whether this is the same crash you were experiencing? This crashes already and so may not probably be related to your change. Also, let me know if you are taking this up.
Yes, same crash. I can take it up, but if I do it'll be a long time before I'll actually get to it. Think you can take it up instead? Thanks!
Owner: karandeepb@chromium.org
Status: Assigned (was: Available)
Sure will take it up. And you were right, this is related to dictionary lookups. Also, please update the bug report if you can find other ways in which this can be reproduced.
hunh, I guess we can't stop AppKit toying with our child window list.

It would be nice to keep the DCHECK..

Perhaps instead of
  DCHECK_EQ(visible_children, [[window_ childWindows] count]);

we can do
  DCHECK_EQ(visible_children, CountBridgedWindows([window_ childWindows]);


with

#if DCEHCK_IS_ON()
// AppKit may add its own child windows, so only count the child
// windows that are managed by BridgedNativeWidget.
NSUinteger CountBridgedWindows(NSArray* child_windows) {
  NSUInteger count = 0;
  for (NSWindow* child in child_windows) {
    if ([child isKindOfClass:NativeWidgetMacNSWindow])
      ++count;
  }
  return count;
}
#endif


Project Member

Comment 5 by bugdroid1@chromium.org, Oct 6 2016

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

commit 93d211ca178ee90dc77d3825cdd1c7fd3ce5f021
Author: karandeepb <karandeepb@chromium.org>
Date: Thu Oct 06 06:23:22 2016

MacViews: Fix crash due to failed DCHECK in BridgedNativeWidget.

BridgedNativeWidget::NotifyVisibilityChangeDown() currently has DCHECKs to
ensure that
- When a window is hidden, all its child windows are hidden and removed from its
  child window list.
- When a window is made visible, all its child windows which want to become
  visible are made visible and added to its child window list.

However, this does not account for windows which may be added by AppKit, for
example, the Mac Dictionary popup. This CL modifies the DCHECKs to correctly
account for windows added by AppKit.

BUG= 653325 

Review-Url: https://codereview.chromium.org/2396883004
Cr-Commit-Position: refs/heads/master@{#423450}

[modify] https://crrev.com/93d211ca178ee90dc77d3825cdd1c7fd3ce5f021/ui/views/cocoa/bridged_native_widget.mm

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 6 2016

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

commit 0e73c3942b2b2b8a623a7c42dd32bc6fabd86ceb
Author: karandeepb <karandeepb@chromium.org>
Date: Thu Oct 06 06:38:47 2016

Revert of MacViews: Fix crash due to failed DCHECK in BridgedNativeWidget. (patchset #1 id:20001 of https://codereview.chromium.org/2396883004/ )

Reason for revert:
Breaks compile. Probably because the CountBridgedWindows function is only compiled in when DCHECK are ON.

Original issue's description:
> MacViews: Fix crash due to failed DCHECK in BridgedNativeWidget.
>
> BridgedNativeWidget::NotifyVisibilityChangeDown() currently has DCHECKs to
> ensure that
> - When a window is hidden, all its child windows are hidden and removed from its
>   child window list.
> - When a window is made visible, all its child windows which want to become
>   visible are made visible and added to its child window list.
>
> However, this does not account for windows which may be added by AppKit, for
> example, the Mac Dictionary popup. This CL modifies the DCHECKs to correctly
> account for windows added by AppKit.
>
> BUG= 653325 
>
> Committed: https://crrev.com/93d211ca178ee90dc77d3825cdd1c7fd3ce5f021
> Cr-Commit-Position: refs/heads/master@{#423450}

TBR=tapted@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 653325 

Review-Url: https://codereview.chromium.org/2399783002
Cr-Commit-Position: refs/heads/master@{#423453}

[modify] https://crrev.com/0e73c3942b2b2b8a623a7c42dd32bc6fabd86ceb/ui/views/cocoa/bridged_native_widget.mm

Status: Fixed (was: Assigned)
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/93d211ca178ee90dc77d3825cdd1c7fd3ce5f021

commit 93d211ca178ee90dc77d3825cdd1c7fd3ce5f021
Author: karandeepb <karandeepb@chromium.org>
Date: Thu Oct 06 06:23:22 2016

MacViews: Fix crash due to failed DCHECK in BridgedNativeWidget.

BridgedNativeWidget::NotifyVisibilityChangeDown() currently has DCHECKs to
ensure that
- When a window is hidden, all its child windows are hidden and removed from its
  child window list.
- When a window is made visible, all its child windows which want to become
  visible are made visible and added to its child window list.

However, this does not account for windows which may be added by AppKit, for
example, the Mac Dictionary popup. This CL modifies the DCHECKs to correctly
account for windows added by AppKit.

BUG= 653325 

Review-Url: https://codereview.chromium.org/2396883004
Cr-Commit-Position: refs/heads/master@{#423450}

[modify] https://crrev.com/93d211ca178ee90dc77d3825cdd1c7fd3ce5f021/ui/views/cocoa/bridged_native_widget.mm

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 27 2016

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

commit 0e73c3942b2b2b8a623a7c42dd32bc6fabd86ceb
Author: karandeepb <karandeepb@chromium.org>
Date: Thu Oct 06 06:38:47 2016

Revert of MacViews: Fix crash due to failed DCHECK in BridgedNativeWidget. (patchset #1 id:20001 of https://codereview.chromium.org/2396883004/ )

Reason for revert:
Breaks compile. Probably because the CountBridgedWindows function is only compiled in when DCHECK are ON.

Original issue's description:
> MacViews: Fix crash due to failed DCHECK in BridgedNativeWidget.
>
> BridgedNativeWidget::NotifyVisibilityChangeDown() currently has DCHECKs to
> ensure that
> - When a window is hidden, all its child windows are hidden and removed from its
>   child window list.
> - When a window is made visible, all its child windows which want to become
>   visible are made visible and added to its child window list.
>
> However, this does not account for windows which may be added by AppKit, for
> example, the Mac Dictionary popup. This CL modifies the DCHECKs to correctly
> account for windows added by AppKit.
>
> BUG= 653325 
>
> Committed: https://crrev.com/93d211ca178ee90dc77d3825cdd1c7fd3ce5f021
> Cr-Commit-Position: refs/heads/master@{#423450}

TBR=tapted@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 653325 

Review-Url: https://codereview.chromium.org/2399783002
Cr-Commit-Position: refs/heads/master@{#423453}

[modify] https://crrev.com/0e73c3942b2b2b8a623a7c42dd32bc6fabd86ceb/ui/views/cocoa/bridged_native_widget.mm

Comment 10 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840

Sign in to add a comment