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

Issue 817124 link

Starred by 4 users

Issue metadata

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

Blocked on:
issue 815049



Sign in to add a comment

memory exhaust gives grey tab instead of normal sad tab

Project Member Reported by wfh@chromium.org, Feb 27 2018

Issue description

Chrome Version: 66.0.3356.0 (Official Build) canary (64-bit) (cohort: Clang-64)
OS: Windows 10

What steps will reproduce the problem?
(1) go to chrome://memory-exhaust
(2)
(3)

What is the expected result?

Sad tab saying that Chrome has run out of memory

What happens instead?

Grey tab.

Please use labels and text to provide additional information.

This is a regression.

For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.

 

Comment 1 by wfh@chromium.org, Feb 28 2018

Blockedon: 815049
bisecting is currently impossible due to issue 815049 - so can't make any progress on this.
Labels: FoundIn-66
Bug present in 66.3355 Dev
Bug not present in 65.0.3325.88 Beta
Labels: -Type-Bug Type-Bug-Regression
Components: UI>Accessibility
Labels: -Pri-2 -Needs-Bisect Triaged-ET M-66 Target-66 RegressedIn-66 Needs-Triage-M66 hasbisect Pri-1
Owner: dmazz...@chromium.org
Status: Assigned (was: Untriaged)
Able to reproduce the issue on reported chrome version 66.0.3356.0 using Windows-10  hence providing Bisect Info
Note: Issue is not seen on Mac 10.12.6 & Ubuntu 14.04
Bisect Info:
================
Good build: 66.0.3344.0
Bad build: 66.0.3345.0

You are probably looking for a change made after 535731 (known good), but no later than 535737 (first known bad).

https://chromium.googlesource.com/chromium/src/+log/b418ba9fb4875c53ef63b684e283699f2cfcd4e1..a60e495ea2084539e350f51125843dc207868d58
"Re-land: Improve sad tab accessibility on views platforms"

Reviewed-on: https://chromium-review.googlesource.com/910716

@Dominic Mazzoni: Please confirm the issue and help in re-assigning if it is not related to your change.
Adding ReleaseBlock-Stable as it is seems a receent break, feel free to remove it if not applicable.

Thanks!

Comment 5 by wfh@chromium.org, Mar 26 2018

Labels: ReleaseBlock-Stable
that rev is in 66.0.3345.0. I have verified the 'grey tab' also appears for a real OOM e.g. see attached oom repro, so this is certainly a RBS if not RBB.
oom.html
349 bytes View Download

Comment 6 by wfh@chromium.org, Mar 26 2018

Cc: wfh@chromium.org

Comment 7 by wfh@chromium.org, Mar 26 2018

Cc: sky@chromium.org tapted@chromium.org
I can confirm that reverting https://chromium-review.googlesource.com/910716 un-regresses this - a not-entirely-perfect (there were conflicts) revert CL is here -> https://chromium-review.googlesource.com/c/chromium/src/+/980579

dmazzoni - can you take a look at this release blocker bug? Shall I just go ahead and revert your change while you fix it?
Just wanted to touch base, do we have further update on the fix?
Just a heads up, M66 Stable cut is on April 12th, 10 days away. This issue is marked as RB-Stable for 66. Please make sure to address this issue prior to stable cut. Thanks! 
Status: Started (was: Assigned)
Sorry, the release block label was added while I was at a conference. Just catching up now, I'll update the bug shortly.

Fix here out for review:

https://chromium-review.googlesource.com/c/chromium/src/+/993992

This is a simple one-line fix so I'm hoping we can land and merge.

Project Member

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

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

commit ac00cdb55acf005d8aac4b862a3700187fb560b9
Author: Dominic Mazzoni <dmazzoni@chromium.org>
Date: Wed Apr 04 16:27:11 2018

Ensure sad tab is shown for all abnormal terminations.

WebView was calling WebContents::IsCrashed() to determine if the
crashed overlay view (sad tab) should be shown on top of the web
contents, but on Windows only, IsCrashed() was returning false for
a tab that was killed due to OOM.

It's not clear whether OOM should count as a crash or not or
whether there should be a platform difference. However, this bug
can be fixed safely either way. The real logic determining whether a
sad tab is shown or not happens in SadTabHelper. It's sufficient for
WebView to just check whether the WebContents has any abnormal
termination status, and show the crashed overlay view if so.

Bug:  817124 
Change-Id: I7ba5f5f40e8d4499ec509b38471bbc1a1f8b6225
Reviewed-on: https://chromium-review.googlesource.com/993992
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548086}
[modify] https://crrev.com/ac00cdb55acf005d8aac4b862a3700187fb560b9/ui/views/controls/webview/webview.cc

Labels: TE-Verified-M67 TE-Verified-67.0.3389.0
Able to reproduce the issue on chrome reported version 66.0.3356.0
Verified the fix on Windows-10 on Chrome version #67.0.3389.0 as per the comment#0
Attaching screenshot for reference.
Observed "Sad tab is seen"
Hence, the fix is working as expected.
Adding the verified label.

Thanks!
817124.PNG
52.6 KB View Download
Labels: Merge-Request-66
Thanks for verifying! Requesting merge to 66.

Project Member

Comment 15 by sheriffbot@chromium.org, Apr 5 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: We are only 11 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
Labels: -Merge-Review-66 Merge-Approved-66
Approving merge to M66. Branch:3359
Project Member

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

Cc: abdulsyed@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

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

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e46cf32c0f5dfaa8e0cfba8de61c3fedb169200

commit 5e46cf32c0f5dfaa8e0cfba8de61c3fedb169200
Author: Dominic Mazzoni <dmazzoni@chromium.org>
Date: Mon Apr 09 18:24:20 2018

Merge to M66: Ensure sad tab is shown for all abnormal terminations.

WebView was calling WebContents::IsCrashed() to determine if the
crashed overlay view (sad tab) should be shown on top of the web
contents, but on Windows only, IsCrashed() was returning false for
a tab that was killed due to OOM.

It's not clear whether OOM should count as a crash or not or
whether there should be a platform difference. However, this bug
can be fixed safely either way. The real logic determining whether a
sad tab is shown or not happens in SadTabHelper. It's sufficient for
WebView to just check whether the WebContents has any abnormal
termination status, and show the crashed overlay view if so.

Bug:  817124 
Change-Id: I7ba5f5f40e8d4499ec509b38471bbc1a1f8b6225
Reviewed-on: https://chromium-review.googlesource.com/993992
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#548086}(cherry picked from commit ac00cdb55acf005d8aac4b862a3700187fb560b9)
Reviewed-on: https://chromium-review.googlesource.com/1002858
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#623}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/5e46cf32c0f5dfaa8e0cfba8de61c3fedb169200/ui/views/controls/webview/webview.cc

Status: Fixed (was: Started)
Labels: TE-Verified-M66 TE-Verified-66.0.3359.106
Able to reproduce the issue on chrome reported version 66.0.3356.0
Verified the fix on Windows-10 on Chrome version #66.0.3359.106 as per the comment#0
Attaching screenshot for reference.
Observed "Sad tab is seen"
Hence, the fix is working as expected.
Adding the verified label.

Thanks!
817124.PNG
131 KB View Download
Project Member

Comment 21 by bugdroid1@chromium.org, May 25 2018

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

commit 73a48326422636f117318e27e1b8e09e54cc8cd2
Author: Dominic Mazzoni <dmazzoni@chromium.org>
Date: Fri May 25 18:14:07 2018

Handle all termination status enums in WebContentsImpl::IsCrashed.

Use a switch so that if a new enum value is added, a compile
error will force us to update this code.

Bug:  817124 
Change-Id: I4183b735c913532c47df97c0518853b7d26aa91d
Reviewed-on: https://chromium-review.googlesource.com/994414
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561947}
[modify] https://crrev.com/73a48326422636f117318e27e1b8e09e54cc8cd2/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/73a48326422636f117318e27e1b8e09e54cc8cd2/ui/views/controls/webview/webview.cc

Sign in to add a comment