472260 - Crash [@ nsDocShell::EnsureContentViewer] with XBL, iframe

Status: VERIFIED FIXED

(Core :: XBL, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Null dereference [@ nsDocShell::EnsureContentViewer] inside nsJSChannel::AsyncOpen.

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK.  So we're inside CreateAboutBlankContentViewer.  We Embed() the new viewer (sets mContentViewer), then we call SetDOMDocument on it.  This destroys the docshell, with the following stack:

#3  0x01d7a3e1 in nsDocShell::Destroy (this=0xf6a6590) at /Users/bzbarsky/mozilla/debug/mozilla/docshell/base/nsDocShell.cpp:3762
#4  0x0372d3f0 in nsFrameLoader::Finalize (this=0xecbe380) at /Users/bzbarsky/mozilla/debug/mozilla/content/base/src/nsFrameLoader.cpp:287
#5  0x03712766 in nsDocument::InitializeFinalizeFrameLoaders (this=0x1566000) at /Users/bzbarsky/mozilla/debug/mozilla/content/base/src/nsDocument.cpp:5190
#6  0x037213dc in nsFrameLoaderRunner::Run (this=0xf69bfa0) at /Users/bzbarsky/mozilla/debug/mozilla/content/base/src/nsDocument.cpp:5123
#7  0x036df5ea in nsContentUtils::RemoveScriptBlocker () at /Users/bzbarsky/mozilla/debug/mozilla/content/base/src/nsContentUtils.cpp:4249
#8  0x034187b7 in nsAutoScriptBlocker::~nsAutoScriptBlocker (this=0xbfffb12b) at nsContentUtils.h:1514
#9  0x03482ffe in PresShell::DoFlushPendingNotifications (this=0x1577c00, aType=Flush_Layout, aInterruptibleReflow=1) at /Users/bzbarsky/mozilla/debug/mozilla/layout/base/nsPresShell.cpp:4458
#10 0x0348324f in PresShell::WillPaint (this=0x1577c00) at /Users/bzbarsky/mozilla/debug/mozilla/layout/base/nsPresShell.cpp:5957
#11 0x03991777 in nsViewManager::FlushPendingInvalidates (this=0xeccb670) at /Users/bzbarsky/mozilla/debug/mozilla/view/src/nsViewManager.cpp:2211
#12 0x03991d7e in nsViewManager::EnableRefresh (this=0xeccb670, aUpdateFlags=2) at /Users/bzbarsky/mozilla/debug/mozilla/view/src/nsViewManager.cpp:1940
#13 0x03991d48 in nsViewManager::EnableRefresh (this=0xd6c65c0, aUpdateFlags=2) at /Users/bzbarsky/mozilla/debug/mozilla/view/src/nsViewManager.cpp:1926
#14 0x034485aa in DocumentViewerImpl::InitPresentationStuff (this=0xd828220, aDoInitialReflow=0, aReenableRefresh=1) at /Users/bzbarsky/mozilla/debug/mozilla/layout/base/nsDocumentViewer.cpp:747
#15 0x0344982b in DocumentViewerImpl::SetDOMDocument (this=0xd828220, aDocument=0xf8a4494) at /Users/bzbarsky/mozilla/debug/mozilla/layout/base/nsDocumentViewer.cpp:1664
#16 0x01d6a4fe in nsDocShell::CreateAboutBlankContentViewer (this=0xf6a6590, aPrincipal=0xf694a30) at /Users/bzbarsky/mozilla/debug/mozilla/docshell/base/nsDocShell.cpp:5353

That, of course, nulls out mContentViewer, and then we return success from CreateAboutBlankContentViewer but the viewer is null.

Seems to me we shouldn't allow all this random stuff (loads, etc) to be happening in frameloaders that are pending finalization...  At the very least, EnsureContentViewer in such a loader should throw.  We could also just check for this particular case (null viewer after SetDOMDocument, throw if that happens), I guess, but the architecture seems fundamentally fragile where you have this live object but any random unrelated thing will cause it to die.

What particularly confuses me is that the frameloader is in the finalization list so there was already a live script blocker, right?  Why are we finalizing under the presshell stuff, then?

Comment 2

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

The iframe is appended to an element in the first
binding and that causes eventually a flush which removes the first binding, which
causes the iframe to be unbound from doc.

The crash is gone if layout is flushed before ReallyStartLoading.

Comment 3

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

Attachment: wip

Have to think this a bit. But something like this might be needed: don't destroy while starting to load, use asynchronous destroy.
(Fixes the crash)

Comment 4

[Boris Zbarsky [:bzbarsky]]

That last makes some sense to me...

Comment 5

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

Attachment: wip2

This contains something which might fix bug 472312 too.
(if I just could reproduce that crash)

Comment 6

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

Attachment: patch

Comment 7

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

This was backed out to see if this caused some crashtest orange on linux.
I haven't managed to reproduce that locally and the orange was there just before
commiting. So once the tree is open again, I'll re-commit.

Comment 8

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

relanded
http://hg.mozilla.org/mozilla-central/rev/3b5664b4a1b1

Comment 9

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dcfed22e981b

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090201 Minefield/3.2a1pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090201 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090201020520