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 9 users

Issue metadata

Status: Verified
Owner:
Mostly out sick until Fri, travelin...
Closed: Mar 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Crashes below OmniboxEditModel::OpenMatch / AutocompleteEditModel::OpenMatch

Project Member Reported by thakis@chromium.org, Aug 15 2012

Issue description

Used to be AutocompleteEditModel::OpenMatch in <= 21, renamed to  OmniboxEditModel::OpenMatch in >= 22

E.g. https://crash.corp.google.com/reportdetail?reportid=201bc36f59ff4662

0x00bafb06	 [Google Chrome Framework]	 - ../base/memory/scoped_ptr.h:204]	TabContents::web_contents
0x00b88b8f	 [Google Chrome Framework]	 - omnibox_edit_model.cc:526]	OmniboxEditModel::OpenMatch
0x00b35e87	 [Google Chrome Framework]	 - omnibox_view_mac.mm:265]	OmniboxViewMac::OpenMatch
0x00b88abc	 [Google Chrome Framework]	 - omnibox_edit_model.cc:509]	OmniboxEditModel::AcceptInput
0x00b37ad2	 [Google Chrome Framework]	 - omnibox_view_mac.mm:823]	OmniboxViewMac::OnDoCommandBySelector
0x00b37cea	 [Google Chrome Framework]	 + 0x00adfcea]	non-virtual thunk to OmniboxViewMac::OnDoCommandBySelector(objc_selector*)
0x00b26146	 [Google Chrome Framework]	 - autocomplete_text_field_editor.mm:436]	-[AutocompleteTextFieldEditor doCommandBySelector:]
0x938a7b80	 [AppKit]	 + 0x0086fb80]	-[NSTextInputContext doCommandBySelector:]
0x938a3c71	 [AppKit]	 + 0x0086bc71]	-[NSTextInputContext _handleCommand:]
0x934e1f66	 [AppKit]	 + 0x004a9f66]	-[NSKeyBindingManager(NSKeyBindingManager_MultiClients) interpretEventAsCommand:forClient:]
0x938a8619	 [AppKit]	 + 0x00870619]	-[NSTextInputContext handleEvent:]
0x9374c635	 [AppKit]	 + 0x00714635]	-[NSView interpretKeyEvents:]
0x00b25f81	 [Google Chrome Framework]	 - autocomplete_text_field_editor.mm:395]	-[AutocompleteTextFieldEditor interpretKeyEvents:]
0x936b97b9	 [AppKit]	 + 0x006817b9]	-[NSTextView keyDown:]
0x931149c2	 [AppKit]	 + 0x000dc9c2]	-[NSWindow sendEvent:]
 

Comment 1 by thakis@chromium.org, Aug 15 2012

Labels: Feature-Omnibox

Comment 2 by k...@google.com, Aug 16 2012

Owner: pkasting@chromium.org
Status: Assigned
Peter can you take a look at this?
Cc: skuhne@chromium.org nhuyhl@chromium.org rohi...@chromium.org krisr@chromium.org kaznacheev@chromium.org
Issue 157286 has been merged into this issue.

Comment 4 by a...@chromium.org, Dec 30 2012

Issue 167878 has been merged into this issue.
Peter, any updates on this? thanks!
Cc: -pkasting@chromium.org
No, I basically am not doing much omnibox work anymore.  Is this a significant crash?

Comment 7 by rsesek@chromium.org, Feb 12 2013

 Issue 175727  has been merged into this issue.
Owner: mpear...@chromium.org
Status: Started
I have a simple change that may fix this.
https://codereview.chromium.org/12250014/

I think we're trying to call OpenMatch() when there's no currently active tab (WebContents is NULL); this protects the call to verify there's an active tab.

I'll leave this bug open so it still shows up in searches, etc.

If the crash reports stop, then we'll follow-up by auditing the code to see if any other places assume non-NULL WebContents.  If the crash reports continue, then I'll revert this change.


A root question is how can we call OpenMatch() when there's no active tab.  While comparing the omnibox logic between Mac and other platforms (this bug appears to be Mac-only), I spotted the following:
---
OmniboxPopupViewGtk::~OmniboxPopupViewGtk() {
  // Stop listening to our signals before we destroy the model. I suspect that
  // we can race window destruction, otherwise.
  signal_registrar_.reset();

  // Explicitly destroy our model here, before we destroy our GTK widgets.
  // This is because the model destructor can call back into us, and we need
  // to make sure everything is still valid when it does.
  model_.reset();
  g_object_unref(layout_);
  gtk_widget_destroy(window_);
}
---
yet on the Mac side, I see:
---
OmniboxPopupViewMac::~OmniboxPopupViewMac() {
  // Destroy the popup model before this object is destroyed, because
  // it can call back to us in the destructor.
  model_.reset();

  // Break references to |this| because the popup may not be
  // deallocated immediately.
  AutocompleteMatrix* matrix = [popup_ contentView];
  DCHECK(matrix == nil || [matrix isKindOfClass:[AutocompleteMatrix class]]);
  [matrix setPopupView:NULL];
}
---

That is, on the Mac side I don't see anything about unregistering a signal handler before deletion.  I wonder if this is why we can receive an OpenMatch() with no active tab.  Does anyone who knows Mac UI code know whether we can/need to do something like that?

Cc: sh...@chromium.org
Explicitly CCing shess as I'm not sure there's anyone else who can answer that (maybe rohitrao?).
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 13 2013

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

------------------------------------------------------------------------
r182244 | mpearson@chromium.org | 2013-02-13T16:39:43.005132Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/omnibox/omnibox_edit_model.cc?r1=182244&r2=182243&pathrev=182244

Omnibox: Possibly Fix Rare OpenMatch Crash

The associated bug seems to imply a crash on this line
ClassifyPage(controller_->GetWebContents()->GetURL())
in OmniboxEditModel::OpenMatch().

This change corrects the part of the test that may fail
(if there's no active tab for some reason).
I think controller_ is always valid (the rest of the edit
model code use it without testing it for non-NULL).
But I see at least two places in OmniboxEditModel that
test for the non-NULLness of controller_->GetWebContents().
Hence, I think it might be able to be NULL, so this
change adds a test for it here too.

TEST=new code compiles
BUG= 142931 


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

Comment 12 by sh...@chromium.org, Feb 16 2013

signal_registrar_ is a GTK thing, not a Chrome thing, so doesn't even exist on the OSX omnibox code.

My bet is that there was a scripted tab close while the user was doing something in the omnibox.  The stack implies there is a window with an omnibox around, though I can't think of where the race would be that allowed the key event through when the close happens.  It looks to me like ~OmniboxViewMac() clears the field's observer, and, anyhow, the controller_ is still around, so possibly this is at root an ordering problem in window shutdown.

Without a repro, it's hard to think of better suggestions to fix the problem, though.

Comment 13 by sh...@chromium.org, Feb 16 2013

Issue 176654 seemed reasonable to dupe to 175341, but the reportid was for this crash.  The described activity does NOT seem like a weirdo tab-closing edge case, but like someone just typing along.  I wonder if there is anything like instant or something going on which is confusing the web contents setting.

Comment 14 by sh...@chromium.org, Feb 16 2013

Hmm, also, there's a key set for rwhvm_window = Missing window.  This is a diagnostic from  issue 148882  , where it seemed like rendering was being attempted to a RenderWidgetHostViewMac which wasn't in a window at all.  That also seems consistent with something being confused, though it's impossible to tell if it was set immediately before the crash or long ago.
Is there anything I can do to help track this down? Any logging I can run or something like that? I am getting crashes about every 10-15 minutes now, and while it seems to be random it's definitely omnibox related and it's starting to drive me wacky.
chrisvanpatten: What version of Chrome are you running?  I ask so I can figure out if you have the purported fix in comment #11.

Also, can you go to chrome://crashes/ and post the crash IDs here?

thanks,
mark

I have two crash IDs in crome://crashes, but it shuts down much more often than that. Perhaps they aren't registering as crashes?

Here are the two crash IDs:

49704e0aa79c6b9e
9ccd30cd1f782cd0

I'm running 25.0.1364.84 beta (I filed a separate bug at 175727 that was merged here that describes my experience more completely).
chrisvanpatten: In that bug marked as a duplicate, you say that chrome crashes sometimes when you "go to open a new tab".  How do you go open a new tab?

FYI: that change that may fix the problem is only in canary.  It'll probably be in tomorrow's _dev_ channel update if you want to switch your channel to see if the problem goes away.
http://www.chromium.org/getting-involved/dev-channel#TOC-What-should-I-do-before-I-change-my-channel-

It seems to happen only when I use Cmd+T. Once I saw someone reference the omnibox, it made total sense; I seem to be able to open new tabs from the Most Visited screen, Apps page or "Open in new tab" without problem, but the minute my cursor hits the omnibox it's a crapshoot.
Labels: OS-Chrome

Comment 21 by sh...@chromium.org, Feb 19 2013

Was there a reason to add ChromeOS?  Per my comments #13 and #14, I suspect that this might be pretty platform-specific.
any updates on this bug?
dharani:

I don't see this crash on recent canaries.  Not sure yet if my change has fixed it or whether it's rare enough that it hasn't been triggered.


Labels: Merge-Approved
looks like the fix isn't part of M26. Shall we merge your fix?
Okay, let's merge it.

I'll drover it this afternoon.

While looking for this crash, I found a cluster with a similar signature.  See for instance
https://crash.corp.google.com/reportdetail?reportid=c3096aca7dc84a0f#crashing_thread

Thread 0 *CRASHED* ( EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000000 )

0x5ff5d127	 [chrome.dll]	 - omnibox_edit_model.cc:837]	OmniboxEditModel::OnEscapeKeyPressed()
0x5ffae1b0	 [chrome.dll]	 - omnibox_view_win.cc:1139]	OmniboxViewWin::SkipDefaultKeyEventProcessing(ui::KeyEvent const &)
0x5ff8696a	 [chrome.dll]	 - location_bar_view.cc:1226]	LocationBarView::SkipDefaultKeyEventProcessing(ui::KeyEvent const &)
0x5fae4252	 [chrome.dll]	 - focus_manager.cc:66]	views::FocusManager::OnKeyEvent(ui::KeyEvent const &)
0x5eecab5e	 [chrome.dll]	 - accelerator_handler_win.cc:27]	views::AcceleratorHandler::Dispatch(tagMSG const &)
0x5ebcf09c	 [chrome.dll]	 - message_pump_win.cc:395]	base::MessagePumpForUI::ProcessMessageHelper(tagMSG const &)
0x5ebcf1b8	 [chrome.dll]	 - message_pump_win.cc:447]	base::MessagePumpForUI::ProcessPumpReplacementMessage()
0x5ebcf048	 [chrome.dll]	 - message_pump_win.cc:386]	base::MessagePumpForUI::ProcessMessageHelper(tagMSG const &)
0x5ebcec95	 [chrome.dll]	 - message_pump_win.cc:237]	base::MessagePumpForUI::DoRunLoop()
0x5e9f4b84	 [chrome.dll]	 - message_loop.cc:428]	MessageLoop::RunInternal()
0x5e9f4aef	 [chrome.dll]	 - run_loop.cc:45]	base::RunLoop::Run()
0x5eeca750	 [chrome.dll]	 - chrome_browser_main.cc:1642]	ChromeBrowserMainParts::MainMessageLoopRun(int *)
0x5eeca769	 [chrome.dll]	 - chrome_browser_main.cc:1646]	ChromeBrowserMainParts::MainMessageLoopRun(int *)
...

There's a pointer use here in OnEscapeKeyPressed that uses controller_->GetWebContents() without checking that it's non-NULL.  This is exactly what I fixed in the OpenMatch() change that seems to have worked.
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/omnibox/omnibox_edit_model.cc&exact_package=chromium&l=824

I'll prep a similar fix to OnEscapeKeyPressed().

Project Member

Comment 27 by bugdroid1@chromium.org, Feb 26 2013

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

------------------------------------------------------------------------
r184741 | mpearson@chromium.org | 2013-02-26T21:48:44.375539Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/browser/ui/omnibox/omnibox_edit_model.cc?r1=184741&r2=184740&pathrev=184741

Merge 182244
> Omnibox: Possibly Fix Rare OpenMatch Crash
> 
> The associated bug seems to imply a crash on this line
> ClassifyPage(controller_->GetWebContents()->GetURL())
> in OmniboxEditModel::OpenMatch().
> 
> This change corrects the part of the test that may fail
> (if there's no active tab for some reason).
> I think controller_ is always valid (the rest of the edit
> model code use it without testing it for non-NULL).
> But I see at least two places in OmniboxEditModel that
> test for the non-NULLness of controller_->GetWebContents().
> Hence, I think it might be able to be NULL, so this
> change adds a test for it here too.
> 
> TEST=new code compiles
> BUG= 142931 
> 
> 
> Review URL: https://chromiumcodereview.appspot.com/12250014

TBR=mpearson@chromium.org
Review URL: https://codereview.chromium.org/12330154
------------------------------------------------------------------------
For what it's worth, I haven't seen this persist in 27.0.1423.0 canary, so as far as I'm concern it seems fixed! Thanks for the quick attention.
I'm still seeing this crash even after the fix was merged to M26.

https://crash.corp.google.com/reportdetail?reportid=fae736a46725adb4#crashing_thread
There's one more instance of in OpenMatch() that could (theoretically) cause problems.  I had thought fixing it wasn't necessary after the last patch landed and I didn't see any more crashes in canary, but I guess the population wasn't big enough.  I'll fix it.
Project Member

Comment 31 by bugdroid1@chromium.org, Mar 8 2013

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

------------------------------------------------------------------------
r186931 | mpearson@chromium.org | 2013-03-08T09:39:27.519605Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/omnibox/omnibox_edit_model.cc?r1=186931&r2=186930&pathrev=186931

Fix crash in OmniboxEditModel::OpenMatch()


BUG= 142931 


Review URL: https://chromiumcodereview.appspot.com/12510006
------------------------------------------------------------------------
Labels: Mstone-26 Merge-Requested
Labels: -Merge-Requested Merge-Approved
Project Member

Comment 34 by bugdroid1@chromium.org, Mar 8 2013

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

------------------------------------------------------------------------
r187012 | mpearson@chromium.org | 2013-03-08T19:29:36.198253Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/browser/ui/omnibox/omnibox_edit_model.cc?r1=187012&r2=187011&pathrev=187012

Merge 186931
> Fix crash in OmniboxEditModel::OpenMatch()
> 
> 
> BUG= 142931 
> 
> 
> Review URL: https://chromiumcodereview.appspot.com/12510006

TBR=mpearson@chromium.org
Review URL: https://codereview.chromium.org/12529007
------------------------------------------------------------------------
Project Member

Comment 35 by bugdroid1@chromium.org, Mar 8 2013

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

------------------------------------------------------------------------
r187029 | mpearson@chromium.org | 2013-03-08T20:22:20.510387Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/browser/ui/omnibox/omnibox_edit_model.cc?r1=187029&r2=187028&pathrev=187029

Revert 187012
> Merge 186931
> > Fix crash in OmniboxEditModel::OpenMatch()
> > 
> > 
> > BUG= 142931 
> > 
> > 
> > Review URL: https://chromiumcodereview.appspot.com/12510006
> 
> TBR=mpearson@chromium.org
> Review URL: https://codereview.chromium.org/12529007

TBR=mpearson@chromium.org
------------------------------------------------------------------------
Project Member

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

Labels: -Area-UI -Feature-Omnibox -Mstone-26 Cr-UI M-26 Cr-UI-Browser-Omnibox
Owner: ----
Status: Available
The patch fix doesn't apply cleanly to the M-26 release branch.  (The code has changed since then.)  I wrote a patch that applies to M-26 as the code is in the release branch.  However, I'm having trouble convincing my checkout to upload and submit the patch directly to the branch, and I'm about leaving for vacation tomorrow so I'm posting instructions here in hopes someone else can submit the patch.

In the M-26 release branch, there's a section of code in
chrome/browser/ui/omnibox/omnibox_edit_model.cc
in the function
OmniboxEditModel::OpenMatch()
that looks like:
---
    if (disposition == CURRENT_TAB) {
      // If we know the destination is being opened in the current tab,
      // we can easily get the tab ID.  (If it's being opened in a new
      // tab, we don't know the tab ID yet.)
      log.tab_id = SessionTabHelper::FromWebContents(
          controller_->GetWebContents())->session_id().id();
    }
---

Change that code to look like:
---
    if ((disposition == CURRENT_TAB) && web_contents) {
      // If we know the destination is being opened in the current tab,
      // we can easily get the tab ID.  (If it's being opened in a new
      // tab, we don't know the tab ID yet.)
      log.tab_id =
          SessionTabHelper::FromWebContents(web_contents)->session_id().id();
    }
---

That's it!

switch flags to show this is available

Owner: thakis@chromium.org
I'll try to merge the patch described in comment 37.
Status: Fixed
Merged to 1410 in r187362.
Project Member

Comment 40 by bugdroid1@chromium.org, Mar 11 2013

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

------------------------------------------------------------------------
r187362 | thakis@chromium.org | 2013-03-11T20:27:23.545683Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/browser/ui/omnibox/omnibox_edit_model.cc?r1=187362&r2=187361&pathrev=187362

Merge 186931 "Fix crash in OmniboxEditModel::OpenMatch()"

> Fix crash in OmniboxEditModel::OpenMatch()
> 
> 
> BUG= 142931 
> 
> 
> Review URL: https://chromiumcodereview.appspot.com/12510006

TBR=mpearson@chromium.org
Review URL: https://codereview.chromium.org/12478006
------------------------------------------------------------------------

Comment 41 by krisr@chromium.org, Mar 20 2013

Status: Verified

Sign in to add a comment