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

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Sep 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment
link

Issue 142422: Keyboard events (e.g. Ctrl+W) can crash full screen tabs

Reported by skuhne@chromium.org, Aug 13 2012 Project Member

Issue description

First revert CL from  issue 134465  in the main trunk. That fix was a hotfix and should get fixed the way as it is done in Windows: The window should get maximized instead of creating a new window.

That said - I don't know why this was done differently on ChromeOS and I "smell" a good reason for doing it so (and a bigger task).

What steps will reproduce the problem?
1. Goto YouTube
2. Press Full Screen
3. Press Ctrl+W

What is the expected output? What do you see instead?

Window closes. Instead the browser crashes.
 

Comment 1 by saintlou@chromium.org, Aug 14 2012

Owner: ihf@chromium.org
Status: Assigned

Comment 2 by ihf@chromium.org, Aug 18 2012

Cc: saintlou@chromium.org
I looked at the change mentioned but I don't understand how this is Flash related. Could anybody explain this?

Comment 3 by skuhne@chromium.org, Aug 19 2012

When opening a tab in ChromeOS in full screen, a new window gets created (and it covers the full screen).
Windows does apparently resize the existing window to cover the screen.

I dont know why this is - it could be because of flash, but it could also be because of any other reason.

My fix (as well as the fix from derat) were simply supressing keyboard commands sent to the window, because they cause problems with the full screen mode as it is implemented on ChromeOS.

Comment 4 by yusukes@chromium.org, Aug 20 2012

FYI: I'm working on a very similar issue ( crbug.com/143442 ) and I think I might have a rough idea of why the crash happens. I'm still not so sure what is the right fix for the crash though.

First, I think the crash only happens when a Pepper Flash window gets maximized. When a normal browser window is maximized, it seems that C-w/C-S-w work just fine. The difference between full-screen Pepper and full-screen browser would be:

- When a browser window is maximized, a new Aura window is created (as Stefan wrote above), but a new RenderWidgetHostViewAura is _not_ created. The current RWHVA is reused.
- When a Pepper window is maximized, a new Aura window is created, and a new RenderWidgetHostViewAura _is_ also created.

In both cases, the object which handles C-w/C-S-w is the Browser* object which started the full screen mode (please check http://src.chromium.org/viewvc/chrome?view=rev&revision=137436 for more details). For example, when a Pepper-backed YouTube video in a tab is maximized, all key event is forwarded to a Browser* which has the YT tab.

So I guess the crash happens like this:

- When C-S-w pressed for closing a maximized browser window, the Browser* handles the shortcut and deletes both the Browser and the RWHVA.
- When C-S-w pressed for closing a maximized Pepper window, the Browser* handles the shortcut and delete the Browser. However, the RWHVA object is still alive since the Browser (seemingly) doesn't know the new RWHVA object for Pepper. Chrome crashes by trying to forward a key event to the dangling Browser* object.

I can think of two ways to fix the crash. The first (and easier) one is to handle C-w/C-S-w in RenderWidgetHostViewAura::OnKeyEvent() which is called before a key event is forwarded to a Browser*. Please note that VKEY_ESCAPE is already handled in the method to close full screen Pepper Flash window correctly. The second way is to notify the new RWHVA object for Pepper when the Browser* executes IDC_CLOSE_TAB or IDC_CLOSE_WINDOW. The second one might be difficult since the Browser might not have a pointer of the RWHVA object.

Comment 5 by yusukes@chromium.org, Aug 20 2012

I think the crash was introduced by r137436. Until then, a key event was not forwarded to a Browser but was just ignored when Pepper was maximized. i.e. both C-w and C-S-w were no-op on full-screen Pepper.

Comment 6 by yusukes@chromium.org, Aug 20 2012

Cc: jochen@chromium.org

Comment 7 by bugdroid1@chromium.org, Aug 22 2012

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=152729

------------------------------------------------------------------------
r152729 | skuhne@chromium.org | 2012-08-22T04:10:25.417451Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser_command_controller.h?r1=152729&r2=152728&pathrev=152729
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser_command_controller.cc?r1=152729&r2=152728&pathrev=152729

Revert "Fixing crash Crash Report - Stack Signature: content::RenderWidgetHostImpl::ForwardKeybo"

ben asked to revert this in the main trunk. The real fix will be in once Issue
142422 gets fixed.

This reverts commit 54bac78404d9b2d3ddb475526da8211414ae36b2.

R=skuhne@chromium.org
BUG= 134465 


Review URL: https://chromiumcodereview.appspot.com/10866009
------------------------------------------------------------------------

Comment 8 by skuhne@chromium.org, Aug 22 2012

Just a heads up - Ben asked me to revert my fix now, so the keyboard crash is now back and this should get fixed for M23.

Comment 9 by bugdroid1@chromium.org, Aug 24 2012

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=153205

------------------------------------------------------------------------
r153205 | yusukes@chromium.org | 2012-08-24T14:38:36.713926Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser_command_controller.h?r1=153205&r2=153204&pathrev=153205
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser_command_controller.cc?r1=153205&r2=153204&pathrev=153205

Temporarily revert 152729 - Revert "Fixing crash Crash Report - Stack Signature: content::RenderWidgetHostImpl::ForwardKeybo"

ben asked to revert this in the main trunk. The real fix will be in once Issue
142422 gets fixed.

This reverts commit 54bac78404d9b2d3ddb475526da8211414ae36b2.

R=skuhne@chromium.org
BUG= 134465 


Review URL: https://chromiumcodereview.appspot.com/10866009

TBR=skuhne@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10876066
------------------------------------------------------------------------

Comment 10 by bugdroid1@chromium.org, Aug 24 2012

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=153206

------------------------------------------------------------------------
r153206 | yusukes@chromium.org | 2012-08-24T14:44:51.578099Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser_command_controller.cc?r1=153206&r2=153205&pathrev=153206

Re-enable Ctrl-w and Ctrl-Shift-w when a non-Pepper window is maximized.

This is a temporary fix for M22 for  crbug.com/143442 , and should be reverted on trunk as part of  crbug.com/142422 . This CL is basically a second trial of r151121 which was also a temporary fix for M22.  r151121 disabled Ctrl-w and Ctrl-Shift-w when any kind of full screen window was active, but it turned out that the limitation was too strict; we should not disable Ctrl-w and Ctrl-Shift-w when a non-Pepper window was maximized.

BUG= 143442 , 142422 
TEST=confirmed that C-w/C-S-w do not crash when a browser window is maximized.

Review URL: https://chromiumcodereview.appspot.com/10834421
------------------------------------------------------------------------

Comment 11 by bugdroid1@chromium.org, Aug 24 2012

Project Member
Labels: merge-merged-1229
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=153208

------------------------------------------------------------------------
r153208 | yusukes@chromium.org | 2012-08-24T14:47:00.087656Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1229/src/chrome/browser/ui/browser_command_controller.cc?r1=153208&r2=153207&pathrev=153208

Merge 153206 - Re-enable Ctrl-w and Ctrl-Shift-w when a non-Pepper window is maximized.

This is a temporary fix for M22 for  crbug.com/143442 , and should be reverted on trunk as part of  crbug.com/142422 . This CL is basically a second trial of r151121 which was also a temporary fix for M22.  r151121 disabled Ctrl-w and Ctrl-Shift-w when any kind of full screen window was active, but it turned out that the limitation was too strict; we should not disable Ctrl-w and Ctrl-Shift-w when a non-Pepper window was maximized.

BUG= 143442 , 142422 
TEST=confirmed that C-w/C-S-w do not crash when a browser window is maximized.

Review URL: https://chromiumcodereview.appspot.com/10834421

TBR=yusukes@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10879065
------------------------------------------------------------------------

Comment 12 by bugdroid1@chromium.org, Aug 24 2012

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=153210

------------------------------------------------------------------------
r153210 | yusukes@chromium.org | 2012-08-24T14:47:59.417290Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser_command_controller.cc?r1=153210&r2=153209&pathrev=153210

Revert 153206 - Re-enable Ctrl-w and Ctrl-Shift-w when a non-Pepper window is maximized.

This is a temporary fix for M22 for  crbug.com/143442 , and should be reverted on trunk as part of  crbug.com/142422 . This CL is basically a second trial of r151121 which was also a temporary fix for M22.  r151121 disabled Ctrl-w and Ctrl-Shift-w when any kind of full screen window was active, but it turned out that the limitation was too strict; we should not disable Ctrl-w and Ctrl-Shift-w when a non-Pepper window was maximized.

BUG= 143442 , 142422 
TEST=confirmed that C-w/C-S-w do not crash when a browser window is maximized.

Review URL: https://chromiumcodereview.appspot.com/10834421

TBR=yusukes@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10867053
------------------------------------------------------------------------

Comment 13 by bugdroid1@chromium.org, Aug 24 2012

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=153211

------------------------------------------------------------------------
r153211 | yusukes@chromium.org | 2012-08-24T14:49:09.568023Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser_command_controller.h?r1=153211&r2=153210&pathrev=153211
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser_command_controller.cc?r1=153211&r2=153210&pathrev=153211

Revert 153205 - Temporarily revert 152729 - Revert "Fixing crash Crash Report - Stack Signature: content::RenderWidgetHostImpl::ForwardKeybo"

ben asked to revert this in the main trunk. The real fix will be in once Issue
142422 gets fixed.

This reverts commit 54bac78404d9b2d3ddb475526da8211414ae36b2.

R=skuhne@chromium.org
BUG= 134465 


Review URL: https://chromiumcodereview.appspot.com/10866009

TBR=skuhne@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10876066

TBR=yusukes@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10879066
------------------------------------------------------------------------

Comment 14 by k...@google.com, Sep 13 2012

Cc: ihf@chromium.org yzshen@chromium.org mrvalla@chromium.org mbollu@chromium.org
Issue 145631 has been merged into this issue.

Comment 15 by k...@google.com, Sep 17 2012

Issue 149821 has been merged into this issue.

Comment 16 by k...@google.com, Sep 17 2012

Labels: -Mstone-23 Mstone-22 ReleaseBlock-Stable Feature-Flash Feature-FullScreen
Not sure this is fixed in 22.

Comment 17 by ihf@chromium.org, Sep 17 2012

Owner: yzshen@chromium.org
Assigning to yzshen.

Comment 18 by zelidrag@chromium.org, Sep 18 2012

issue 149821 in on Windows, not ChromeOS

Comment 19 by zelidrag@chromium.org, Sep 18 2012

Status: Fixed
I don't see this on ChromeOS anymore (chrome 23.0.1266.0, flash 11.4.31.102)

Comment 20 by krisr@chromium.org, Oct 18 2012

Labels: -Mstone-22 Mstone-23 bulkmove
Moved stragglers from 22 to 23 for verification.

Comment 21 by nhuyhl@chromium.org, Oct 18 2012

Status: Verified
Issue is no longer reproducible on:
- Alex with Chrome/OS: 23.0.1271.46/2913.113.0
- Lumpy with Chrome/OS: 23.0.1271.46/2913.110.0

Comment 22 by bugdroid1@chromium.org, Mar 10 2013

Project Member
Labels: -Area-UI -Feature-Flash -Feature-FullScreen -Mstone-23 Cr-UI Cr-Content-Plugins-Flash M-23 Cr-UI-Browser-FullScreen

Comment 23 by bugdroid1@chromium.org, Apr 6 2013

Project Member
Labels: Cr-Blink

Comment 24 by bugdroid1@chromium.org, Apr 6 2013

Project Member
Labels: -Cr-Content-Plugins-Flash Cr-Internals-Plugins-Flash

Sign in to add a comment