New issue
Advanced search Search tips

Issue 834926 link

Starred by 2 users
Status: Assigned
Owner:
ellyjo...@chromium.org
Cc:
tapted@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug
M-X



Sign in to add a comment

MacViews: tests exit with sheets open

Project Member Reported by ellyjo...@chromium.org, Apr 19 2018 
When tests exit with sheets open, they hit the DCHECK(![window_ attachedSheet]) in BridgedNativeWidget.

Some tests affected by this:

ChromePasswordProtectionServiceBrowserTest.OpenChromeSettingsViaPageInfo
ExtensionDisabledGlobalErrorTest.UninstallWhilePromptBeingShown
ExtensionInstallDialogViewTest.InstallButtonDelay
ExtensionUninstallDialogViewBrowserTest.TrackParentWindowDestructionAfterViewCreation
WebDialogBrowserTest.CloseParentWindow

Possible fixes:

1) Remove these DCHECKs, or disable them in test configs (by adding a command-line flag or something).
2) Have BrowserView call ::Hide() on any child widgets of the BrowserView's widget before trying to hide it, since hiding it is what triggers this DCHECK
3) Something else?

Comment 1 by ellyjo...@chromium.org, Apr 19 2018 

(2) won't work - instead what we need is to endSheet: on the browser's NSWindow when we're going to hide it, except that endSheet: is asynchronous (I think). Further study is required.

In the mean time, I'm going to disable these tests in MacViews mode.
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 20 2018 

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

commit 9bac37b48f9edbedef1856c77c0e47296579b5cd
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Fri Apr 20 17:36:41 2018

macviews: remove attachedSheet DCHECK

This DCHECK is testing for a real problem, but in a way that breaks a lot of
test cases. The problem does need to be addressed (see the linked bug) but this
DCHECK is not the right approach.

Bug: 834926,667602
Change-Id: I3735030562eecca32a9d48135285eb645e49e89b
Reviewed-on: https://chromium-review.googlesource.com/1021836
Reviewed-by: Sidney San Martín <sdy@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552371}
[modify] https://crrev.com/9bac37b48f9edbedef1856c77c0e47296579b5cd/ui/views/cocoa/bridged_native_widget.mm

Comment 3 by ellyjo...@chromium.org, Apr 23 2018

Labels: -M-68 -Target-68 -Sprint-1 M-X
The DCHECK is gone now, so the tests pass, but the underlying design problem still remains. Nonetheless, I'm kicking this down the road for now.
►

Sign in to add a comment