1599617 - WindowGlobalChild doesn't correctly handle a null mWindowGlobal

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

This can be triggered from a web page by creating a very deep callstack and then spinning the event loop so the machinery involved in ContentChild::RecvConstructBrowser runs.

Then we call WindowGlobalChild::WindowGlobalChild (under which sets mWindowGlobal to null. Then we call BrowserChild::Init, which calls, via nsWebBrowser::Create and nsDocShell::CreateContentViewerForActor, nsDocShell::CreateAboutBlankContentViewer. This tried to set up a new inner window, but when we land in nsGlobalWindowOuter::SetNewDocument we take the early return here:

  if (!js::CheckRecursionLimitConservativeDontReport(cx)) {
    NS_WARNING("Overrecursion in SetNewDocument");
    return NS_ERROR_FAILURE;
  }

and hence never set up the inner window. This means nsGlobalWindowInner::Create is never called, and hence WindowGlobalChild::InitWindowGlobal is never called.

The caller does not notice this, because in nsWebBrowser::Create we ignore the return value from nsDocShell::CreateContentViewerForActor.

There's another attempt to set up an inner via nsPIDOMWindowOuter::EnsureInnerWindow but that also fails, which throws an exception out of ContentFrameMessageManager_Binding::get_content.

Then we press on, land in WindowGlobalChild::RecvLoadURIInChild, dereference the null mWindowGlobal, and crash.

Kris, since Nika is out, do you know who could look into this?

Comment 2

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #0)

Kris, since Nika is out, do you know who could look into this?

I guess me. Nika has been working on refactoring the ways we create WindowGlobal actors so we run into fewer situations like this, but we probably shouldn't wait on that to fix a crash.

Comment 3

[Chris Peterson [:cpeterson]]

CreateContentViewerForActor() should to check if a WindowGlobal has been successfully bound. If not, then tear down the actor with WindowGlobalChild::Destroy()?

See bug 1599498 for a test case. Can we turn the test case into a new crashtest?

Assigning to smacleod.

Comment 4

[Chris Peterson [:cpeterson]]

P2 M5b because this is a possible crash that could affect dogfooders.

Comment 5

[Steven MacLeod [:smacleod]]

This was fixed by https://phabricator.services.mozilla.com/D56820 for Bug 1589123.

Writing a crashtest using the test case from Bug 1599498 wasn't really feasible.

:nika, do you think it's still worth adding the check for if we have an mWindowGlobal and calling Destroy() in CreatecontentViewerForActor()? If so, how would you suggest exposing if mWindowGlobal is null or not?

Comment 6

[Nika Layzell [:nika] (ni? for response)]

You can tell if mWindowGlobal is null by calling WindowGlobalChild::GetWindowGlobal(), which will return nullptr if it's null.

Perhaps we could have WindowGlobalChild::Init return a guard object which will call Destroy() if mWindowGlobal is not set by the time it is dropped?

Comment 7

[Steven MacLeod [:smacleod]]

(In reply to :Nika Layzell (ni? for response) from comment #6)

You can tell if mWindowGlobal is null by calling WindowGlobalChild::GetWindowGlobal(), which will return nullptr if it's null.

Gah, right, totally missed that it already had a GetWindowGlobal(), should have searched instead of just looking in the .cpp, heh.

Perhaps we could have WindowGlobalChild::Init return a guard object which will call Destroy() if mWindowGlobal is not set by the time it is dropped?

My initial though to this was to add something like the following to the end of WindowGlobalChild::Init:

  return MakeScopeExit([self{RefPtr(this)}] {
    if (!self->GetWindowGlobal()) {
      self->Destroy();
    }
  });

But I'm not if I can do this/the return typing... Were you thinking some custom guard object? Do you know if there is anything already in the codebase I could use as an example?

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Yeah, returning that from WindowGlobalChild::Init probably doesn't work super well.

There are only two callsites for WindowGlobalChild::Init which could possibly have a null WindowGlobal, so perhaps write the ScopeExits at those call-sites.