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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

defect
P1
critical
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: jruderman, Assigned: smaug)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
mozilla1.9.2a1
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

11 years ago
Null dereference [@ nsDocShell::EnsureContentViewer] inside nsJSChannel::AsyncOpen.
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?
Flags: blocking1.9.1?
Keywords: regression
Assignee

Comment 2

11 years ago
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.
Assignee

Comment 3

11 years ago
Posted patch wip (obsolete) — Splinter Review
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)
That last makes some sense to me...
Assignee

Comment 5

11 years ago
Posted patch wip2 (obsolete) — Splinter Review
This contains something which might fix bug 472312 too.
(if I just could reproduce that crash)
Assignee

Updated

11 years ago
Blocks: 472312
Assignee

Updated

11 years ago
Attachment #356006 - Attachment is obsolete: true
Assignee

Comment 6

11 years ago
Posted patch patchSplinter Review
Assignee: nobody → Olli.Pettay
Attachment #355740 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #356015 - Flags: superreview?(bzbarsky)
Attachment #356015 - Flags: review?(bzbarsky)
Attachment #356015 - Flags: superreview?(bzbarsky)
Attachment #356015 - Flags: superreview+
Attachment #356015 - Flags: review?(bzbarsky)
Attachment #356015 - Flags: review+
Assignee

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

11 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
Assignee

Comment 7

11 years ago
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.
Assignee

Comment 9

11 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dcfed22e981b
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Reporter

Updated

11 years ago
Flags: in-testsuite+
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
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.2a1
Crash Signature: [@ nsDocShell::EnsureContentViewer]
You need to log in before you can comment on or make changes to this bug.