New issue
Advanced search Search tips

Issue 676244 link

Starred by 1 user
Status: Fixed
Owner:
patricia...@chromium.org
Closed: Jan 2017
Cc:
tapted@chromium.org
ellyjo...@chromium.org
est...@chromium.org
mac-bugs-priority@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug

Blocking:
issue 603386
issue 657850



Sign in to add a comment

DCHECK failure at focus_ring.cc:39

Project Member Reported by patricia...@chromium.org, Dec 21 2016 
Chrome Version: Since r438433
OS: macOS 10.12.2

What steps will reproduce the problem?
(1) Make sure #secondary-ui-md chrome flag is turned on.
(2) Go to google.com.
(3) Click the "Secure" lock and open the collected cookies dialog.
(4) Tab through until getting to the ScrollView.

What is the expected result?
The ScrollView gets keyboard focus and a focus ring is drawn around it.

What happens instead?
DCHECK on line focus_ring.cc:39 is hit.

Stack trace:

[68181:775:1221/174022.691942:FATAL:focus_ring.cc(39)] Check failed: parent->HasFocus().
0   libbase.dylib                       0x000000010d6db503 _ZN4base5debug10StackTraceC1Ev + 19
1   libbase.dylib                       0x000000010d7029c7 _ZN7logging10LogMessageD2Ev + 71
2   libviews.dylib                      0x0000000113edcbb6 _ZN5views9FocusRing7InstallEPNS_4ViewEN2ui11NativeTheme7ColorIdE + 102
3   libviews.dylib                      0x0000000113f010bd _ZN5views10ScrollView15SetHasFocusRingEb + 45
4   libviews.dylib                      0x0000000113f2b0cc _ZN5views8TreeView7OnFocusEv + 380
5   libviews.dylib                      0x0000000113f2e979 _ZN5views12FocusManager24SetFocusedViewWithReasonEPNS_4ViewENS0_17FocusChangeReasonE + 313
6   libviews.dylib                      0x0000000113f2e537 _ZN5views12FocusManager10OnKeyEventERKN2ui8KeyEventE + 247
7   libviews.dylib                      0x0000000113f541d4 _ZN5views6Widget10OnKeyEventEPN2ui8KeyEventE + 116
8   libviews.dylib                      0x0000000113ec2ac8 _ZN5views19BridgedNativeWidget23DispatchKeyEventPostIMEEPN2ui8KeyEventE + 120
9   libui_base_ime.dylib                0x00000001138f4693 _ZNK2ui15InputMethodBase23DispatchKeyEventPostIMEEPNS_8KeyEventE + 19
10  libviews.dylib                      0x0000000113eba020 -[BridgedContentView handleKeyEvent:] + 176
11  libviews.dylib                      0x0000000113eba099 -[BridgedContentView handleUnhandledKeyDownAsKeyEvent] + 73
12  libviews.dylib                      0x0000000113ebb09e -[BridgedContentView keyDown:] + 110
13  AppKit                              0x00007fff97b21b34 -[NSWindow(NSEventRouting) _reallySendEvent:isDelayedEvent:] + 4086
14  AppKit                              0x00007fff97b20772 -[NSWindow(NSEventRouting) sendEvent:] + 541
15  libviews.dylib                      0x0000000113ec6f4b -[NativeWidgetMacNSWindow sendEvent:] + 283
16  AppKit                              0x00007fff979a9ed0 -[NSApplication(NSEvent) sendEvent:] + 4768
17  libchrome_dll.dylib                 0x00000001078d4b0c __34-[BrowserCrApplication sendEvent:]_block_invoke + 172
18  libbase.dylib                       0x000000010d703e4a _ZN4base3mac15CallWithEHFrameEU13block_pointerFvvE + 10
19  libchrome_dll.dylib                 0x00000001078d48e5 -[BrowserCrApplication sendEvent:] + 261
20  AppKit                              0x00007fff972254b1 -[NSApplication run] + 1002
21  libbase.dylib                       0x000000010d721ae7 _ZN4base24MessagePumpNSApplication5DoRunEPNS_11MessagePump8DelegateE + 359
22  libbase.dylib                       0x000000010d720ee7 _ZN4base24MessagePumpCFRunLoopBase3RunEPNS_11MessagePump8DelegateE + 119
23  libbase.dylib                       0x000000010d71c1d2 _ZN4base11MessageLoop10RunHandlerEv + 354
24  libbase.dylib                       0x000000010d753936 _ZN4base7RunLoop3RunEv + 134
25  libchrome_dll.dylib                 0x00000001078da15f _ZN22ChromeBrowserMainParts18MainMessageLoopRunEPi + 303
26  libcontent.dylib                    0x000000010fdc3884 _ZN7content15BrowserMainLoop23RunMainMessageLoopPartsEv +52
27  libcontent.dylib                    0x000000010fdc6bf6 _ZN7content21BrowserMainRunnerImpl3RunEv + 166
28  libcontent.dylib                    0x000000010fdbefac _ZN7content11BrowserMainERKNS_18MainFunctionParamsE + 124
29  libcontent.dylib                    0x000000011063d5d1 _ZN7content21ContentMainRunnerImpl3RunEv + 513
30  libcontent.dylib                    0x000000011063c6d6 _ZN7content11ContentMainERKNS_17ContentMainParamsE + 54
31  libchrome_dll.dylib                 0x000000010746d58b ChromeMain + 75
32  Chromium                            0x0000000107401daa main + 522
33  libdyld.dylib                       0x00007fffaecfd255 start + 1

[1]    68181 trace trap  ./out/Debug/Chromium.app/Contents/MacOS/Chromium

Comment 1 by tapted@chromium.org, Dec 21 2016

Blocking: 603386
Cc: tapted@chromium.org est...@chromium.org
Labels: -Pri-3 Phase3 M-58 Proj-MacViews Pri-1
Status: Available (was: Untriaged)
FocusManager::OnKeyEvent calls 
  SetFocusedViewWithReason(views[index], kReasonFocusTraversal);

i.e. multiple times, without changing |focused_view_| (which is the only thing that reports `true` from View::HasFocus()). But that's not it.

The DCHECK was added in r422921 -> https://codereview.chromium.org/2383243002

But I think this failure comes from stuff added in r425662 -> https://codereview.chromium.org/2411693002

I don't really see how it ever passed the DCHECK :/. The focus ring is added to the ScrollView, a grandparent view, which doesn't report true from HasFocus.

Comment 2 by tapted@chromium.org, Dec 21 2016 

(I think we can just remove the DCHECK btw)

Comment 3 by est...@chromium.org, Dec 21 2016

Owner: ellyjo...@chromium.org
Status: Assigned (was: Available)
> I don't really see how it ever passed the DCHECK

it may not have, I don't think any tests or bots are set to run with --secondary-ui-md on and most people probably don't build DCHECKs locally (debug or release w/DCHECK).

I agree we can probably just remove the DCHECK.

Comment 4 by patricia...@chromium.org, Dec 30 2016

Blocking: 657850

Comment 5 by patricia...@chromium.org, Dec 30 2016

Cc: ellyjo...@chromium.org
Owner: patricia...@chromium.org
Status: Started (was: Assigned)
Fixing  Issue 657850  would mean that this DCHECK would get hit every time the collected cookies dialog was opened, so I went ahead and deleted the DCHECK as suggested in #c2 and #c3 in the same CL (in progress at https://codereview.chromium.org/2604303002/).
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 3 2017 

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

commit 3c2faf580ef236142f2c1ed8a6491c3193becbfd
Author: patricialor <patricialor@chromium.org>
Date: Tue Jan 03 05:36:01 2017

Views: Remove DCHECK failing for ScrollViews, which can't be focused.

Focus rings can be installed on ScrollViews, but FocusRing::Install() DCHECKs
whether the view with a focus ring attached has focus before this happens. This
doesn't make sense since ScrollViews can't be focused - the focus ring is
installed when whatever is contained inside the ScrollView has focus to prevent
the ring from being scrolled.

In particular this affects the collected cookies dialog, which hits the DCHECK
whenever the TreeView inside the ScrollView receives focus. This change deletes
this DCHECK.

BUG= 676244 

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

[modify] https://crrev.com/3c2faf580ef236142f2c1ed8a6491c3193becbfd/ui/views/controls/focus_ring.cc

Comment 7 by patricia...@chromium.org, Jan 3 2017

Status: Fixed (was: Started)
►

Sign in to add a comment