1517415 - Mozilla Firefox Nightly 66.0a1 (2019-01-02) crashes in [@ mozilla::dom::ToJSValue ] after landing patches from bug #1353867

Status: VERIFIED FIXED

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

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

STR:
1. Start browsing website pages
and in some time enjoy crash in [@ mozilla::dom::ToJSValue ]

Crashlog reports:
https://crash-stats.mozilla.org/report/index/a9ae0eba-74e3-47c2-91d6-dbf8d0190103
https://crash-stats.mozilla.org/signature/?product=Firefox&signature=mozilla%3A%3Adom%3A%3AToJSValue

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Regression probably caused by:
bug #1353867

Comment 2

[Jorg K (CEST = GMT+2)]

My follow-up didn't cause the crash, but you might want to ask Boris.

Comment 3

[Peter Van der Beken [:peterv]]

Hmm, the docshell shouldn't be null but looks like its outer window might be. This is annoying, because of course we used to just get the right window even if the docshell had lost its reference.

Comment 4

[Peter Van der Beken [:peterv]]

Seems like nika tried to make that reference not go away in bug 1401379, but had to back it out.

Comment 5

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

I don't think there's any way to make sure that these objects have the same lifetime - nulling out the references between the two objects has some pretty serious impacts here unfortunately.

A solution here might be to either:
a) Make BrowsingContext hold a strong reference to the outer window which isn't nulled out when the window is closed. There's a chance this could cause leaks, but I'm not sure.
b) Create a dummy closed WindowProxy object which just acts like a closed window if we can't grab the inner window (as the window will have been closed at that point).

I don't know if we have the dummy window functionality yet, so I'm not sure how easy it would be to use that strategy. I can look into doing a quick test for the extra pointer strategy if you'd be interested.

ni? bz for any insight as to whether the dummy window functionality exists already.

Comment 6

[Marcia Knous [:marcia]]

721 crashes/393 installs in 2 days. Can we back something out to get back to a good working state?

Comment 8

[Boris Zbarsky [:bzbarsky]]

The stacks linked in bug 1517518 sure look like bc->GetDOMWindow() is returning null.

GetDOMWindow does:

  return mDocShell ? mDocShell->GetWindow() : nullptr;

and we've already tested that mDocShell is not null, so mDocShell->GetWindow() is returning null.

The old code for nsGenericHTMLFrameElement::GetContentWindow() looked like this:


  nsCOMPtr<nsIDocShell> doc_shell = mFrameLoader->GetDocShell(IgnoreErrors());
  if (!doc_shell) {
    return nullptr;
  }

  nsCOMPtr<nsPIDOMWindowOuter> win = doc_shell->GetWindow();

  if (!win) {
    return nullptr;
  }

so it sure looks to me like we used to get null here in the situation when the docshell returns null from GetWindow.  Patch coming up to just go back to the old behavior of returning null in that case.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, and as far as comment 5 goes, I don't think we have any dummy window stuff...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1517415. We might be trying to JS-wrap a BrowsingContext with a torn-down-enough docshell that we have no window. r=nika

The old code did this check in GetContentWindow, basically.  We _could_ just put
the null-check there, but this seems safer.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I guess comment 3 might be correct for other binding things that returned WindowProxy, though.  We might want to audit them and see whether any of them might have switched from non-null to null.  I'll leave Peter to do that.  But the specific .contentWindow consumer used to return null in this situation already.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2abc6090d9f4
We might be trying to JS-wrap a BrowsingContext with a torn-down-enough docshell that we have no window.  r=nika

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/2abc6090d9f4

Comment 15

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 66.0a1 (2019-01-04), so I'm marking this bug as VERIFIED. Thank you very much! \o/