1263330 - The focus ring initialization in the nsGlobalWindow ctor appears to be broken (WARNING: No inner window available: nsGlobalWindow.cpp)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonathan Watt [:jwatt]]

During startup I see several errors like this:

[8187] WARNING: No inner window available!: file /Users/jonathan/moz/trees/i/dom/base/nsGlobalWindow.cpp, line 9767

That warning happens under the stack:

  nsGlobalWindow::GetKeyboardIndicators
  nsGlobalWindow::InitializeShowFocusRings
  nsGlobalWindow::nsGlobalWindow
  nsGlobalChromeWindow::nsGlobalChromeWindow
  nsGlobalChromeWindow::nsGlobalChromeWindow
  nsGlobalChromeWindow::Create
  nsGlobalWindow::SetNewDocument
  nsDocumentViewer::InitInternal
  nsDocumentViewer::Init
  nsDocShell::SetupNewViewer
  nsDocShell::Embed
  nsDocShell::CreateAboutBlankContentViewer
  nsDocShell::CreateAboutBlankContentViewer
  nsWebShellWindow::Initialize
  nsAppShellService::JustCreateTopWindow
  nsAppShellService::CreateHiddenWindowHelper
  nsAppShellService::CreateHiddenWindow
  nsAppStartup::CreateHiddenWindow
  XREMain::XRE_mainRun

Basically the nsGlobalWindow ctor calls InitializeShowFocusRings which calls GetKeyboardIndicators, but that forwards to the inner Window which doesn't exist yet because it isn't created and set until we return all the way back up to nsGlobalWindow::SetNewDocument. Because of that the InitializeShowFocusRings call in the nsGlobalWindow ctor is a useless no-op.

This code was added in http://hg.mozilla.org/mozilla-central/rev/becc2eec1601 (bug 1214293).

Comment 1

[Neil Deakin]

The code also seems to have gotten messed up and is missing part of it. I'll investigate a proper fix here.

Comment 2

[Andrew Overholt [:overholt]]

Assigning to Neil given comment 1.

Comment 3

[Jonathan Watt [:jwatt]]

Neil, if we aren't going to get to this soon can we either back out the patch that caused this or else comment out the InitializeShowFocusRings call in the nsGlobalWindow ctor for now to silence the console noise?

Comment 4

[Neil Deakin]

Attachment: Only initialize the focus ring state for non-root windows

This silences the assertion. I'll clean this up as part of 1174798.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonathan Watt [:jwatt] from comment #3)
> Neil, if we aren't going to get to this soon can we either back out the
> patch that caused this or else comment out the InitializeShowFocusRings call
> in the nsGlobalWindow ctor for now to silence the console noise?
Tiny bit noise in terminal really shouldn't be reason to back out patches.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8747281 [details] [diff] [review]
Only initialize the focus ring state for non-root windows

I have no idea why we'd like to do this, especially for child process case.
We'd end up not calling GetKeyboardIndicators for top level content windows, only for iframes.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8747281 [details] [diff] [review]
Only initialize the focus ring state for non-root windows

Oh, now I see, we don't need the code for top level case, since we just end up
re-set mShowFocusRings to the existing value. 
Please add a comment why != root check.

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #5)
> Tiny bit noise in terminal really shouldn't be reason to back out patches.

As I wrote above: "Because of that the InitializeShowFocusRings call in the nsGlobalWindow ctor is a useless no-op." In other words the patch is broken, so it's not just about console noise.

(In reply to Olli Pettay [:smaug] from comment #6)
> We'd end up not calling GetKeyboardIndicators for top level content windows,
> only for iframes.

Since the InitializeShowFocusRings call does nothing, I'm guessing that's currently the case!

Comment 9

[Neil Deakin]

> Since the InitializeShowFocusRings call does nothing, I'm guessing that's
> currently the case!

That analysis isn't actually correct.

InitializeShowFocusRings does the following pseudocode:

thisWindow->getTopLevelWindow()->GetKeyboardIndicators()

For top level windows, InitializeShowFocusRings does nothing (and doesn't need to) and causes an assertion. This patch adds a conditional check for this.

For child windows, it initializes the state from the root window. This should already have an inner window, so initializes properly.

We don't handle child processes yet; that is bug 1174798.

Comment 10

[Jonathan Watt [:jwatt]]

Oh fun, so in digging into this some more it seems that at the point in time when the assertion triggers, both IsOuterWindow() and IsInnerWindow() return true...

Comment 11

[Jonathan Watt [:jwatt]]

Scrap that, I'm confused.

Comment 12

[Jonathan Watt [:jwatt]]

So in fact the analysis in comment 0 is incorrect, but nobody challenged that initial inalysis so I was proceeding on the basis that it was correct. :(

Specifically the nsGlobalWindow for the nsGlobalWindow::GetKeyboardIndicators call is the root window, and the nsGlobalWindow for the nsGlobalWindow::InitializeShowFocusRings call is some other sub-window.

Strange that the root window doesn't have its mInnerWindow at the time we're setting up a sub-window though... (the root outer window not having its mInnerWindow being the reason that the assertion fires).

Comment 13

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #12)
> Strange that the root window doesn't have its mInnerWindow at the time we're
> setting up a sub-window though...

Meh, except that this is not a sub-window, it's the inner window, and GetPrivateRoot just gets the inner's outer in this case. So comment 0 is correct. I'm going to stop making rushed drive-by comments now. Sorry for the noise.

(In reply to Olli Pettay [:smaug] from comment #7)
> Oh, now I see, we don't need the code for top level case, since we just end
> up re-set mShowFocusRings to the existing value. 
> Please add a comment why != root check.

I'd suggest also mentioning the assert in that comment, something like the following maybe?

    // Initialize mShowFocusRings, except if we're the inner window of a
    // root window. (In that case there's no point because <blah, blah>,
    // and besides, we haven't been set as the inner window of our outer
    // window yet so calling GetKeyboardIndicators would assert.)

I'd suggest that while you're here you also add a MOZ_ASSERT(IsInnerWindow()) at the top of InitializeShowFocusRings to document the expectations of that function call.

Comment 14

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4def9166eb5b7f17d382a1e9a3cd5f34c8074aa1
Bug 1263330, only initialize focus ring state for non-top-level windows, r=smaug

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4def9166eb5b