Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Description • 10 years ago

Testcase is in bug 1031145

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 1 • 10 years ago

Looks like we need this fix on Aurora too.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 2 • 10 years ago

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

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

	Bobby Holley (:bholley)
	Comment 3 • 10 years ago

(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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 4 • 10 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 5 • 10 years ago

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

Simple fix for the crash.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 6 • 10 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 7 • 10 years ago

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.

	Mike Conley (:mconley) (:⚙️)
	Comment 8 • 10 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 9 • 10 years ago

> ::: 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.

	Johnny Stenback (:jst)
	Comment 10 • 10 years ago

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

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 11 • 10 years ago

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.)

	Mike Conley (:mconley) (:⚙️)
	Comment 13 • 10 years ago

(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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 14 • 10 years ago

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

	Ed Morley [:emorley]
	Comment 15 • 10 years ago

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

	Ed Morley [:emorley]
	Comment 16 • 10 years ago

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

	Ed Lee :Mardak
	Comment 18 • 10 years ago

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

	Ed Lee :Mardak
	Comment 19 • 10 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 20 • 10 years ago

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

	Ed Lee :Mardak
	Comment 21 • 10 years ago

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

	Ed Lee :Mardak
	Comment 22 • 10 years ago

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