1031303 - mContext can be null when nsGlobalWindow::SetNewDocument is called.

Status: RESOLVED FIXED

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

Description

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

Testcase is in bug 1031145

http://mxr.mozilla.org/mozilla-aurora/source/dom/base/nsGlobalWindow.cpp?rev=042efa075740&mark=2406-2406#2400

Comment 1

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

Looks like we need this fix on Aurora too.

Comment 2

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

...since it looks like we get these crashes now there.

peterv, bholley, you may have played with this code recently.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #2)
> peterv, bholley, you may have played with this code recently.

I didn't. Peter did, but he's on PTO.

I investigated this a little bit. The basic issue is that we end up invoking ReallyCloseWindow (which invokes CleanUp, but without detaching ourselves from the docshell), and _then_ invoking SetNewDocument from nsDocumentViewer::InitInternal. That code grabs the window via getInterface from the docshell. So we end up invoking nsDocShell::EnsureScriptEnvironment there, but that bails out early because mScriptGlobal is non-null.

It seems like nsGlobalWindow needs do a better job of telling the docshell about what's going on in nsGlobalWindow::ReallyCloseWindow. I think you (smaug) are probably in an equal-or-better position to sort that out than I am, though.

Comment 4

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

Thanks.
I'll fix the crash in some way.

Comment 5

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

Attachment: simple

https://tbpl.mozilla.org/?tree=Try&rev=d4d7427fc179

Simple fix for the crash.

Comment 6

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

Attachment: propagate error, non-simple-approach

https://tbpl.mozilla.org/?tree=Try&rev=877cb35c3c6d

Comment 7

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

Comment on attachment 8462774 [details] [diff] [review]
propagate error, non-simple-approach

Mike, want go though the ancient code this is changing :)

I prefer this larger patch so that we don't end up having content viewers
which are sort of initialized, but really aren't.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8462774 [details] [diff] [review]
propagate error, non-simple-approach

Review of attachment 8462774 [details] [diff] [review]:
-----------------------------------------------------------------

From my still somewhat limited understanding of what's going on, this looks reasonable.

::: docshell/base/nsDocShell.cpp
@@ +7357,5 @@
>    NS_ASSERTION(!mCreatingDocument, "infinite(?) loop creating document averted");
>    if (mCreatingDocument)
>      return NS_ERROR_FAILURE;
>  
> +  AutoRestore<bool> creatingDocument(mCreatingDocument);

TIL about AutoRestore. Neat.

::: dom/base/nsGlobalWindow.cpp
@@ +2331,5 @@
>    }
>  
>    NS_PRECONDITION(IsOuterWindow(), "Must only be called on outer windows");
>  
> +  NS_ENSURE_STATE(!mCleanedUp);

It'd be good to maybe document what the consequences of not ensuring this are.

::: layout/base/nsDocumentViewer.cpp
@@ +1802,4 @@
>  
>      // Clear the list of old child docshells. Child docshells for the new
>      // document will be constructed as frames are created.
>      if (!aDocument->IsStaticDocument()) {

This can probably be "else'd" after the conditional in 1798-1801, and maybe chuck the comment inside the else block.

Comment 9

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

> ::: layout/base/nsDocumentViewer.cpp  
> >      // Clear the list of old child docshells. Child docshells for the new
> >      // document will be constructed as frames are created.
> >      if (!aDocument->IsStaticDocument()) {
> 
> This can probably be "else'd" after the conditional in 1798-1801, and maybe
> chuck the comment inside the else block.
Indeed.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 8462774 [details] [diff] [review]
propagate error, non-simple-approach

r=jst, and I agree with mconley's feedback here.

Comment 11

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

Actually, that else doesn't work.
The first 'if' is about mDocument, the latter aDocument.

(The whole docshell tree item removal is crazy left over code from early
bfcache patches.)

Comment 12

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

Attachment: +added comment

Comment 13

[Mike Conley (:mconley) (:⚙️)]

(In reply to Olli Pettay [:smaug] from comment #11)
> Actually, that else doesn't work.
> The first 'if' is about mDocument, the latter aDocument.
> 
> (The whole docshell tree item removal is crazy left over code from early
> bfcache patches.)

Ah, my mistake.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/12b4cad8e0bc

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/12b4cad8e0bc

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/12b4cad8e0bc

Comment 18

[Ed Lee :Mardak]

(In reply to Olli Pettay [:smaug] from comment #1)
> Looks like we need this fix on Aurora too.
Did this make it to aurora?

Comment 19

[Ed Lee :Mardak]

For some more context, this bug was triggering a tart crash that was preventing bug 1039881 from landing on beta. I didn't know about this bug when I added the following line to prevent the tart crash:

> NS_ENSURE_TRUE(GetContextInternal(), NS_ERROR_NOT_INITIALIZED);

bholley suggested getting the fix into nightly and aurora.

Comment 20

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

Ed, if this fixes a crasher, I think we should certainly take this to Aurora.
Could you ask for approval.

Comment 21

[Ed Lee :Mardak]

Comment on attachment 8465718 [details] [diff] [review]
+added comment

Approval Request Comment
[Feature/regressing bug #]: bug 997440
[User impact if declined]: potential crash when doing a newtab background thumbnail; unable to land enhanced tiles on aurora
[Describe test coverage new/current, TBPL]: bug 1026561 landed on nightly without test failures because this bug was fixed, and it'll be uplifted to aurora so it requires this fix
[Risks and why]: smaug?
[String/UUID change made/needed]: none

Comment 22

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-aurora/rev/71a901b41f4c