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

Issue 821312 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 1
Type: Bug-Regression

Blocking:
issue 826248



Sign in to add a comment

Regression:Overlay of 'keep dangerous file' in chrome://downloads/ closes after crashing the page manually.

Reported by vku...@etouch.net, Mar 13 2018

Issue description

Chrome Version: 67.0.3368.0 (Official Build) Revision 0f36d3901569535a63173a1835f8dfbcf7b66d60-refs/heads/master@{#542340}(32/64-bit)
OS:Windows (7,8,8.1,10),Linux (14.04 LTS)

What steps will reproduce the problem?
(1)Launch chrome and navigate to http://www.provos.org/tmp/content.exe
(2)Press Ctrl+J to open downloads > click 'keep dangerous file' such that overlay appears
(3)Now crash the page manually via chrome://kill and observe overlay.

Actual: Overlay of 'keep dangerous file' closes after crashing the page manually via chrome://kill.

Expected: Overlay of 'keep dangerous file' should not close after crashing the page manually via chrome://kill. 

This is a regression issue broken in 'M66' and below is the manual bisect info
Good Build: 66.0.3344.0(Revision:535593)
Bad Build:  66.0.3345.0(Revision:536026)

Note: Issue not seen on Mac(10.13.1, 10.12.6, 10.13.4) OS 


 
Actual_download.mp4
398 KB View Download
Expected_download.mp4
321 KB View Download

Comment 1 by vku...@etouch.net, Mar 13 2018

Labels: Target-67 RegressedIn-66 Target-66 FoundIn-66 FoundIn-67 hasbisect
Owner: dmazz...@chromium.org
Status: Assigned (was: Unconfirmed)
You are probably looking for a change made after 535731 (known good), but no later than 535737 (first known bad).
CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/b418ba9fb4875c53ef63b684e283699f2cfcd4e1..a60e495ea2084539e350f51125843dc207868d58?pretty=fuller&n=50
(Unable to narrow down the range using per-revision bisect as chrome build crashes after launch)

Suspecting: r535735 ?

@dmazzoni:  Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.
Blocking: 826248
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 13 2018

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

commit 2837c5891e07d0cd2afe61537b08706d2fef71e1
Author: Dominic Mazzoni <dmazzoni@chromium.org>
Date: Fri Apr 13 20:18:58 2018

WebView should not hide its NativeViewHost when showing a crash overlay.

In r535735 we made WebView responsible for showing an overlay (like a
sad tab) when the WebContents has crashed. When the overlay was showing
we hid the NativeViewHost to be safe (using SetVisible(false)).
This had the unintended consequence of closing modal dialogs such
as a cookie dialog or dangerous download warning dialog.

In testing, it seems safe to not actually hide |holder_|, the
NativeViewHost. The overlay is absolute-positioned on top of it,
and it's not possible to focus the WebContents. This was more
or less true in the previous Sad Tab implementation where it
was displayed in a Widget on top of the WebView instead of as a
child View.

Bug:  826248 ,  821312 
Change-Id: I748afeff1ac8d7e1fa18ccafe709706f803ae6cc
Reviewed-on: https://chromium-review.googlesource.com/1008621
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550742}
[modify] https://crrev.com/2837c5891e07d0cd2afe61537b08706d2fef71e1/ui/views/controls/webview/webview.cc

Status: Fixed (was: Assigned)
Labels: Merge-Request-66
Project Member

Comment 7 by sheriffbot@chromium.org, Apr 16 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: We are only 0 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
M66 Stable release is tomorrow. Why is this critical to land for 66 vs waiting until 67? 
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2837c5891e07d0cd2afe61537b08706d2fef71e1

commit 2837c5891e07d0cd2afe61537b08706d2fef71e1
Author: Dominic Mazzoni <dmazzoni@chromium.org>
Date: Fri Apr 13 20:18:58 2018

WebView should not hide its NativeViewHost when showing a crash overlay.

In r535735 we made WebView responsible for showing an overlay (like a
sad tab) when the WebContents has crashed. When the overlay was showing
we hid the NativeViewHost to be safe (using SetVisible(false)).
This had the unintended consequence of closing modal dialogs such
as a cookie dialog or dangerous download warning dialog.

In testing, it seems safe to not actually hide |holder_|, the
NativeViewHost. The overlay is absolute-positioned on top of it,
and it's not possible to focus the WebContents. This was more
or less true in the previous Sad Tab implementation where it
was displayed in a Widget on top of the WebView instead of as a
child View.

Bug:  826248 ,  821312 
Change-Id: I748afeff1ac8d7e1fa18ccafe709706f803ae6cc
Reviewed-on: https://chromium-review.googlesource.com/1008621
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550742}
[modify] https://crrev.com/2837c5891e07d0cd2afe61537b08706d2fef71e1/ui/views/controls/webview/webview.cc

Cc: manoranj...@chromium.org
Labels: -Merge-Review-66 Merge-Rejected-66
This doesn't appear to be critical for stable. Let's target this for M67.
Dmazzoni@ - can you please confirm?

Sign in to add a comment