Closed Bug 766795 Opened 12 years ago Closed 12 years ago

"Assertion failure: IsElement()" with fragment-owned element in selection

Categories

(Core :: DOM: Selection, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: jruderman, Assigned: ayg)

References

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

When loaded:

###!!! ASSERTION: aDocument must be current doc of aParent: '!aParent || aDocument == aParent->GetCurrentDoc()', file content/base/src/nsGenericElement.cpp, line 3037

When closed:

Assertion failure: IsElement(), at dist/include/mozilla/dom/Element.h:264
So there are two bugs here.  One is easy: PresShell::ContentRemoved has

  if (aContainer)
    mFrameConstructor->RestyleForRemove(aContainer->AsElement(), aChild,
                                        oldNextSibling);

and doesn't check that aContainer is actually an element.  This will blow up if aContainer is a fragment.  (Hi, bug 563659!)  Adding "&& aContainer->IsElement()" fixes that.

The other bug is that we call nsGenericElement::BindToTree with aParent equal to a DocumentFragment.  Obviously, it's not the descendant of a document, so GetCurrentDoc() is false, which isn't allowed.  The interesting part of the stack is:

#6  0xb4b07a5e in nsGenericElement::BindToTree (this=0x9df6d420, aDocument=0x9dae8800,
    aParent=0x9df6d330, aBindingParent=0x9df6d330, aCompileEventHandlers=true)
    at /mnt/extra/checkouts/mozilla-central/content/base/src/nsGenericElement.cpp:3036
#7  0xb4bf2518 in nsGenericHTMLElement::BindToTree (this=0x9df6d420, aDocument=0x9dae8800,
    aParent=0x9df6d330, aBindingParent=0x9df6d330, aCompileEventHandlers=true)
    at /mnt/extra/checkouts/mozilla-central/content/html/content/src/nsGenericHTMLElement.cpp:1700
#8  0xb4ef79bc in nsHTMLEditor::CreateAnonymousElement (this=0x9dad0e00, aTag=...,
    aParentNode=0x9df6d374, aAnonClass=..., aIsCreatedHidden=false, aReturn=0x9dad0f30)
    at /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLAnonymousUtils.cpp:154
#9  0xb4f4073a in nsHTMLEditor::CreateResizer (this=0x9dad0e00, aReturn=0x9dad0f30, aLocation=0,
    aParentNode=0x9df6d374)
    at /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLObjectResizer.cpp:138
#10 0xb4f41948 in nsHTMLEditor::ShowResizersInner (this=0x9dad0e00, aResizedElement=0x9df6d1e4)
    at /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLObjectResizer.cpp:307
#11 0xb4f41f87 in nsHTMLEditor::ShowResizers (this=0x9dad0e00, aResizedElement=0x9df6d1e4)
    at /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLObjectResizer.cpp:283
#12 0xb4ef71c8 in nsHTMLEditor::CheckSelectionStateForAnonymousButtons (this=0x9dad0e00,
    aSelection=0x9ae7a280)
    at /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLAnonymousUtils.cpp:342
#13 0xb4f4116a in NotifySelectionChanged (aSelection=0x9ae7a280, this=0xa06740c0,
    aReason=<optimized out>)
    at /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLObjectResizer.cpp:86
#14 ResizerSelectionListener::NotifySelectionChanged (this=0xa06740c0, aSelection=0x9ae7a280,
    aReason=16)
    at /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLObjectResizer.cpp:76
#15 0xb4952dee in mozilla::Selection::NotifySelectionListeners (this=0x9ae7a280)
    at /mnt/extra/checkouts/mozilla-central/layout/generic/nsSelection.cpp:5390
#16 0xb4956eb1 in Collapse (aOffset=0, aParentNode=0x9df6d1a0, this=0x9ae7a280)
    at /mnt/extra/checkouts/mozilla-central/layout/generic/nsSelection.cpp:4595
#17 mozilla::Selection::Collapse (this=0x9ae7a280, aParentNode=0x9df6d1a0, aOffset=0)
    at /mnt/extra/checkouts/mozilla-central/layout/generic/nsSelection.cpp:4550
#18 0xb4956f71 in mozilla::Selection::SelectAllChildren (this=0x9ae7a280, aParentNode=0x9df6d1e4)
    at /mnt/extra/checkouts/mozilla-central/layout/generic/nsSelection.cpp:5000

I'm sure that at some point between NotifySelectionChanged and CreateAnonymousElement, editor code should be bailing out if the parent isn't part of a document.  I'm just not sure where.  This latter bug occurs equally if the parent is an element rather than a document fragment.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attached patch Patch v1Splinter Review
Requesting review from roc on the layout change and Ehsan on the editor change.

The layout change looks sane to me, unless calling ContentRemoved with aContainer equal to a fragment is wrong to start with.  In that case we should probably add an assert -- or better yet, make nsDocumentFragment not nsIContent!

The editor change I'm less sure of.  It looks like the highest place on the call stack where it would make sense to check if the element is in a document.  I'm guessing it's correct to just bail out in that case, but maybe you'd prefer a different way of avoiding the issue.

Try: https://tbpl.mozilla.org/?tree=Try&rev=443412ebb6f4
Attachment #635710 - Flags: review?(roc)
Attachment #635710 - Flags: review?(ehsan)
I guess the idea is that if the content is removed from a fragment, then the content wasn't in a document and still isn't so nothing has really changed.
Comment on attachment 635710 [details] [diff] [review]
Patch v1

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

Yeah, this is the right place for this kind of check.

Also, Jesse, it would be great if you can fuzz more editor stuff in the face of fragments, and attempt to tumble down this house of cards.  :-)
Attachment #635710 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/248f55ae4b23
Flags: in-testsuite+
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/248f55ae4b23
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.