It looks like a case slipped through our focus change. It only occurs on the NTP. Even when I erase the URL on a non-NTP page, it doesn't occur. If I enter some text on the NTP, the cursor is shifted by that amount on the ^U page.

I looked into this and it appears that the problem is actually in the browser focusing code, not the Omnibox. (The Omnibox recently relaxed it's policies, which exposed the issue, but we do not wish to revert and wish to fix it correctly.)

When the user presses ^U, it does indeed open a new tab, but the focus is incorrectly in the Omnibox. When the user presses ^!T (open previously closed tab), it also opens a new tab, but the focus is correctly in the content.

You can see it more clearly in the stack traces. The split is within 'SetInitialFocus'. The good case focuses the contents; the bad case focuses the location bar.

I don't know enough about the FocusManager to know why it's behaving differently in these cases, but it appears that the Omnibox isn't doing anything wrong since, for the same set-up (save state), the focus logic is producing different behavior depending on keystroke.

Could someone who understands it better look at this?

This trace works:

#0 0x7f913605fd5d base::debug::StackTrace::StackTrace()
#1 0x7f9129263f00 views::FocusManager::SetFocusedView()
#2 0x7f91292949de views::View::RequestFocus()

Note, "Contents"
#3 0x7f91289d15e0 views::WebView::OnWebContentsFocused()
#4 0x7f91310f5467 content::WebContentsImpl::NotifyWebContentsFocused()
#5 0x7f9131122671 content::WebContentsViewAura::GotFocus()
#6 0x7f9130deed7c content::RenderViewHostImpl::RenderWidgetGotFocus()
#7 0x7f9130e01dfa content::RenderWidgetHostImpl::GotFocus()
#8 0x7f9130e30264 content::RenderWidgetHostViewAura::OnWindowFocused()
#9 0x7f91283d6b63 wm::FocusController::SetFocusedWindow()

#10 0x7f81dd75f329 wm::FocusController::FocusAndActivateWindow()
#11 0x7f81dd75eed1 wm::FocusController::FocusWindow()
#12 0x7f81e0fd7adb aura::Window::Focus()
#13 0x7f81e61b3312 content::RenderWidgetHostViewAura::Focus()
#14 0x7f81e64a93a3 content::WebContentsViewAura::Focus()
#15 0x7f81e6487460 content::WebContentsImpl::Focus()

(it's different here. it simply calls 'Focus()')
(it looks like this is a different call to 'SetInitialFocus' within
'RestoreFocus' than the one in the "reopen tab" trace)
(this is the first call in 'RestoreFocus')
#16 0x00ee33be87e4 ChromeWebContentsViewDelegateViews::SetInitialFocus()
#17 0x00ee33be86d3 ChromeWebContentsViewDelegateViews::RestoreFocus()
#18 0x7f81e64a9501 content::WebContentsViewAura::RestoreFocus()
#19 0x7f81e6487550 content::WebContentsImpl::RestoreFocus()

#20 0x0086d3f900e1 BrowserView::OnActiveTabChanged()
#21 0x0086d3b89689 Browser::ActiveTabChanged()
#22 0x0086d3c02c82 TabStripModel::NotifyIfActiveTabChanged()
#23 0x0086d3c0be21 TabStripModel::NotifyIfActiveOrSelectionChanged()
#24 0x0086d3c01f60 TabStripModel::SetSelection()
#25 0x0086d3c01923 TabStripModel::InsertWebContentsAt()
#26 0x0086d3bbaeb3 chrome::AddRestoredTab()
#27 0x0086d3bb3665 BrowserLiveTabContext::AddRestoredTab()
#28 0x7f7e75117b3f sessions::TabRestoreServiceHelper::RestoreTab()
#29 0x7f7e7511514d sessions::TabRestoreServiceHelper::RestoreEntryById()

This trace doesn't:

#0 0x7f913605fd5d base::debug::StackTrace::StackTrace()
#1 0x7f9129263f00 views::FocusManager::SetFocusedView()
#2 0x7f91292949de views::View::RequestFocus()
#3 0x00cb69f99f32 OmniboxViewViews::SetFocus()
#4 0x00cb69f5d1ef LocationBarView::FocusLocation()
#5 0x00cb69ec5e5d BrowserView::SetFocusToLocationBar()
#6 0x00cb69abf3e3 Browser::SetFocusToLocationBar()

(it looks like this is the difference. why is it focusing the location bar?)
(it claims that it's the same call location within 'RestoreFocus'
as in the "reopen" trace)
#7 0x00cb6a08d7cb ChromeWebContentsViewDelegateViews::SetInitialFocus()
#8 0x00cb6a08d6d3 ChromeWebContentsViewDelegateViews::RestoreFocus()
#9 0x7f9131120501 content::WebContentsViewAura::RestoreFocus()

#10 0x7f81e6487550 content::WebContentsImpl::RestoreFocus()
#11 0x00ee33a1f0e1 BrowserView::OnActiveTabChanged()
#12 0x00ee33618689 Browser::ActiveTabChanged()
#13 0x00ee33691c82 TabStripModel::NotifyIfActiveTabChanged()
#14 0x00ee3369ae21 TabStripModel::NotifyIfActiveOrSelectionChanged()
#15 0x00ee33690f60 TabStripModel::SetSelection()
#16 0x00ee33690923 TabStripModel::InsertWebContentsAt()

#17 0x00ee33632337 chrome::ViewSource()
#18 0x00ee336319e7 chrome::ViewSource()
#19 0x00ee33632554 chrome::ViewSelectedSource()

Nice investigation.  Because this bug only appears on the new tab page, I think I know where to look.  I think the bug is likely here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/search/search_tab_helper.cc?type=cs&q=fakebox+file:cc$&sq=package:chromium&l=280-319
in particular line 297
  omnibox_view->model()->SetCaretVisibility(true);
My guess is that we're keeping that status when creating a new tab and shouldn't be.

Thanks for looking, Mark. But either I'm not following, or I should point out that the issue isn't that the cursor is visible, but instead that the Omnibox is focused at all.

btw, gdb is telling me that that function is never called.

Ah, I got the impression from the original report video that the problem is that the omnibox with focused with the cursor at the beginning.  I guess that's not the real issue; the real issue is that the focus is in the omnibox.

It's interesting this bug only reproduces when doing view source on the NTP.  It doesn't reproduce (at least for me, on Mac) on any other page.  On other pages, the view source page never ends up with focus regardless of whether the omnibox had focus beforehand.

This leads me to think the bug is here in
Browser::ShouldFocusLocationBarByDefault(WebContents* source)
where we say yes on the NTP.
  return search::NavEntryIsInstantNTP(source, entry);
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser.cc?l=1635

We probably shouldn't say yes on "view source" NTPs...

Thanks for finding this. I think you're close, but I wonder if there's something wrong going on upstream. The first problem that I ran into was, the URL in this case has the original site, not 'view-source:...', but maybe I'm looking at the wrong field.

But this got me thinking about the whole stack trace: why are we looking at a URL to decide what to do? It produces incorrect behavior. For example, if you navigate to 'about:blank', then close it, then hit ^!T, it focuses the Omnibox (because it thinks it's on an NTP), even though it wasn't when closed.

Shouldn't the caller just tell the library what to do? "We just started up.", "The user hit ^T.", "We're restoring this tab.", etc. I'm wondering if maybe it took the wrong branch upstream, gets here and throws up its hands.

Good point.  I agree the current focus handling logic seems like a mess to me, and that the actor creating the tab should best be able to make the decision about what should be focussed.  Maybe there's some complexity that made the current design what it is that I'm missing.

 Issue 860121  has been merged into this issue.

Given that this only happens in very specific circumstances and the effect is not harmful, I'm lowering the priority.