Last Comment Bug 766795 - "Assertion failure: IsElement()" with fragment-owned element in selection
: "Assertion failure: IsElement()" with fragment-owned element in selection
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla16
Assigned To: :Aryeh Gregor (working until September 2)
:
Mentors:
Depends on:
Blocks: 336383
  Show dependency treegraph
 
Reported: 2012-06-20 17:19 PDT by Jesse Ruderman
Modified: 2012-06-24 20:08 PDT (History)
4 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (asserts fatally when closed) (407 bytes, text/html)
2012-06-20 17:19 PDT, Jesse Ruderman
no flags Details
Patch v1 (3.18 KB, patch)
2012-06-22 05:47 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
roc: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-20 17:19:10 PDT
Created attachment 635144 [details]
testcase (asserts fatally when closed)

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
Comment 1 :Aryeh Gregor (working until September 2) 2012-06-22 05:31:07 PDT
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.
Comment 2 :Aryeh Gregor (working until September 2) 2012-06-22 05:47:58 PDT
Created attachment 635710 [details] [diff] [review]
Patch v1

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
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-22 05:52:03 PDT
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 4 :Ehsan Akhgari 2012-06-22 07:35:23 PDT
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.  :-)
Comment 5 :Aryeh Gregor (working until September 2) 2012-06-23 23:58:55 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/248f55ae4b23
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-06-24 20:08:33 PDT
https://hg.mozilla.org/mozilla-central/rev/248f55ae4b23

Note You need to log in before you can comment on or make changes to this bug.