New issue
Advanced search Search tips

Issue 134263 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Crash on fullscreen, exit, CTRL+L, input with --enable-views-textfield

Project Member Reported by msw@chromium.org, Jun 22 2012

Issue description

Taken from innaterebel's awesome comment at:
https://code.google.com/p/chromium/issues/detail?id=131660#c12

What steps will reproduce the problem?
0. Run Chrome with "Enable Views Textfield" / --enable-views-textfield.
1. Press F11 to enter full screen.
2. Pressing F11 again to exit full screen.
3. Press Ctrl+L to move focus to omnibox.
4. Type any letter to Crash...

Partial callstack:
chrome.dll!ToolbarModel::GetNavigationController()  Line 177 + 0xa bytes
chrome.dll!ToolbarModel::GetText()  Line 48 + 0x8 bytes
chrome.dll!OmniboxViewViews::Update(const content::WebContents * contents=0x00000000)  Line 441 + 0xf bytes
chrome.dll!LocationBarView::Update(const content::WebContents * tab_for_state_restoring=0x00000000)  Line 376 + 0x24 bytes
chrome.dll!ChromeToMobileView::EnabledStateChangedForCommand(int id=35008, bool enabled=false)  Line 44
chrome.dll!CommandUpdater::UpdateCommandEnabled(int id=35008, bool enabled=false)  Line 63 + 0x49 bytes
chrome.dll!LocationBarView::Update(const content::WebContents * tab_for_state_restoring=0x00000000)  Line 371
chrome.dll!ToolbarView::OnInputInProgress(bool in_progress=true)  Line 417
chrome.dll!LocationBarView::OnInputInProgress(bool in_progress=true)  Line 950 + 0x1a bytes
chrome.dll!AutocompleteEditModel::SetInputInProgress(bool in_progress=true)  Line 411 + 0x1a bytes
chrome.dll!OmniboxViewViews::UpdatePopup()  Line 570
chrome.dll!AutocompleteEditModel::OnAfterPossibleChange(const std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > & old_text="chrome://flags", const std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > & new_text="a", unsigned int selection_start=1, unsigned int selection_end=1, bool selection_differs=true, bool text_differs=true, bool just_deleted_text=false, bool allow_keyword_ui_change=true)  Line 886 + 0x15 bytes
chrome.dll!OmniboxViewViews::OnAfterPossibleChange()  Line 650 + 0x55 bytes
chrome.dll!OmniboxViewViews::OnAfterUserAction(views::Textfield * sender=0x0782e000)  Line 756 + 0x13 bytes
chrome.dll!views::NativeTextfieldViews::OnAfterUserAction()  Line 1089 + 0x19 bytes
chrome.dll!views::NativeTextfieldViews::InsertChar(wchar_t ch=L'a', int flags=0)  Line 715
chrome.dll!views::InputMethodWin::OnChar(unsigned int message=258, unsigned int wparam=97, long lparam=1966081, int * handled=0x0035ecd8)  Line 264 + 0x33 bytes
chrome.dll!views::InputMethodWin::OnImeMessages(unsigned int message=258, unsigned int w_param=97, long l_param=1966081, int * handled=0x0035ecd8)  Line 129 + 0x18 bytes
chrome.dll!views::NativeWidgetWin::OnImeMessages(unsigned int message=258, unsigned int w_param=97, long l_param=1966081)  Line 1525 + 0x18 bytes
chrome.dll!views::NativeWidgetWin::_ProcessWindowMessage(HWND__ * hWnd=0x00210442, unsigned int uMsg=258, unsigned int wParam=97, long lParam=1966081, long & lResult=0, unsigned long dwMsgMapID=0)  Line 325 + 0x2e bytes
chrome.dll!views::NativeWidgetWin::ProcessWindowMessage(HWND__ * hWnd=0x00210442, unsigned int uMsg=258, unsigned int wParam=97, long lParam=1966081, long & lResult=0, unsigned long dwMsgMapID=0)  Line 289 + 0x47 bytes
chrome.dll!views::NativeWidgetWin::OnWndProc(unsigned int message=258, unsigned int w_param=97, long l_param=1966081)  Line 1253 + 0x24 bytes
chrome.dll!BrowserFrameWin::OnWndProc(unsigned int message=258, unsigned int w_param=97, long l_param=1966081)  Line 334
chrome.dll!ui::WindowImpl::WndProc(HWND__ * hwnd=0x00210442, unsigned int message=258, unsigned int w_param=97, long l_param=1966081)  Line 239 + 0x1b bytes
chrome.dll!base::win::WrappedWindowProc<&ui::WindowImpl::WndProc>(HWND__ * hwnd=0x00210442, unsigned int message=258, unsigned int wparam=97, long lparam=1966081)  Line 77 + 0x15 bytes
 

Comment 1 by msw@chromium.org, Jun 23 2012

Slightly tricky bug/investigation! Triggering fullscreen would somehow mysteriously change the address of |OmniboxViewViews::toolbar_model_| by a small amount, making it invalid and causing a read AV on the next access. toolbar_model_ had no accessors, and the hierarchy of owners wasn't changing, so I was pretty baffled.

Luckily VS data breakpoints came to the rescue and pointed me right to the offending pointer changes in BrowserView::ProcessFullscreen, triggered by an invalid static_cast to OmniboxViewWin (the object was an OmniboxViewViews) and calling "omnibox_win->set_force_hidden(true);".

http://codereview.chromium.org/10636023
Visual Studio data breakpoints: 1
Incorrect static_casts: 0
:)
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 25 2012

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

------------------------------------------------------------------------
r144001 | msw@chromium.org | Mon Jun 25 13:49:47 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/location_bar/location_bar_view.cc?r1=144001&r2=144000&pathrev=144001
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/omnibox/omnibox_views.h?r1=144001&r2=144000&pathrev=144001
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/omnibox/omnibox_view_views.cc?r1=144001&r2=144000&pathrev=144001
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/frame/browser_view.cc?r1=144001&r2=144000&pathrev=144001
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/omnibox/omnibox_views.cc?r1=144001&r2=144000&pathrev=144001

Update BrowserView::ProcessFullscreen OmniboxViewWin use; etc.

Make OmniboxViewWin code in ProcessFullscreen conditional.
Move OmniboxView util functions to omnibox_views.[h|cc].
Minor refactoring; init OmniboxViewViews::textfield_ to NULL.

BUG= 131660 , 134263 
TEST=Omnibox doesn't trigger crashes after exiting fullscreen.

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

Comment 3 by msw@chromium.org, Jun 25 2012

Status: Fixed

Comment 4 by msw@chromium.org, Jun 26 2012

innaterebel, can you help verify this fix on Canary?
Sure. Just received 22.0.1187.0 (r144126) canary.
Confirmed with the flag, neither clean profile nor my regular profile encounters crashes when using omnibox after exited manually/site triggered fullscreen.

No idea about  issue 134381  though. We don't know the repro steps of it, and the latest Dev released yesterday doesn't have this fix merged yet.

Comment 6 by msw@chromium.org, Jun 26 2012

Status: Started
Thanks for the verification on Canary, innaterebel!
I'll request merge for  Issue 134381  if this still looks good on Friday or so.

Comment 7 by msw@chromium.org, Jun 28 2012

Cc: sky@chromium.org
Labels: -Pri-2 Pri-1 Mstone-21 Stability-Crash ReleaseBlock-Stable Merge-Requested
Requesting merge of http://crrev.com/144001 to fix this on M21 (see related/dup top crasher  Issue 134381 ).
These crashes only occur with an experimental flag on Windows, but it would be good and safe to fix IMO.

My fix looks to have stopped corresponding crashes on the canary channel, see both:
http://crash.corp.google.com/search?query=OmniboxViewViews%3A%3AUpdate+custom_name%3A%22channel%22+custom_value%3A%22canary%22
and (with OmniboxViewViews::Update on the callstack):
http://crash.corp.google.com/search?query=ToolbarModel%3A%3AGetNavigationController+custom_name%3A%22channel%22+custom_value%3A%22canary%22+-v8
All of these crashes have --enable-views-textfield or >15 num-switches (the max shown here) and:
They start on 21.0.1177.0 (r142583), just after my omnibox views change http://crrev.com/142507
They last occur on 22.0.1186.0 (r143854), which is just before my above fix http://crrev.com/144001

FYI, there are simple conflicts merging back to M21, I'll have my merge CL reviewed as needed.
See "Sync and merge" Patch Sets 2&3 at http://codereview.chromium.org/10636023

Comment 8 by msw@chromium.org, Jun 28 2012

Cc: osh...@chromium.org
 Issue 134381  has been merged into this issue.

Comment 9 by kareng@google.com, Jun 29 2012

Labels: -Merge-Requested Merge-Approved
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 29 2012

Labels: -Merge-Approved merge-merged-1180
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=144974

------------------------------------------------------------------------
r144974 | msw@chromium.org | Fri Jun 29 13:20:54 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/ui/views/location_bar/location_bar_view.cc?r1=144974&r2=144973&pathrev=144974
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/ui/views/omnibox/omnibox_views.cc?r1=144974&r2=144973&pathrev=144974
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/ui/views/omnibox/omnibox_views.h?r1=144974&r2=144973&pathrev=144974
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/ui/views/omnibox/omnibox_view_views.cc?r1=144974&r2=144973&pathrev=144974
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/ui/views/frame/browser_view.cc?r1=144974&r2=144973&pathrev=144974

Merge 144001 - Update BrowserView::ProcessFullscreen OmniboxViewWin use; etc.

Make OmniboxViewWin code in ProcessFullscreen conditional.
Move OmniboxView util functions to omnibox_views.[h|cc].
Minor refactoring; init OmniboxViewViews::textfield_ to NULL.

BUG= 131660 , 134263 
TEST=Omnibox doesn't trigger crashes after exiting fullscreen.

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

TBR=msw@chromium.org,sky@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10702044
------------------------------------------------------------------------

Comment 11 by msw@chromium.org, Jun 29 2012

Status: Fixed
Fixed, please help verify this on the next M21 Beta update; thanks!
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 13 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

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

Labels: -Area-UI -Feature-TextInput -Feature-Omnibox -Mstone-21 Cr-UI M-21 Cr-UI-Browser-Omnibox Cr-UI-Input-Text-IME
Project Member

Comment 14 by bugdroid1@chromium.org, Mar 14 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue

Sign in to add a comment