604314 - "ASSERTION: unexpected disconnected nodes"

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: unexpected disconnected nodes: 'aDisconnected', file content/base/src/nsContentUtils.cpp, line 1789

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[(no longer active)]

Jesse, is this something which is worth blocking on?

Comment 3

[Jesse Ruderman]

It doesn't seem especially dangerous to me, but I'm not familiar with the code.

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Comment 5

[(no longer active)]

This is exactly like bug 586662, BTW.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why not use the aDisconnected parameter of ComparePoints and just check that the nodes aren't disconnected?

Comment 7

[(no longer active)]

(In reply to comment #6)
> Why not use the aDisconnected parameter of ComparePoints and just check that
> the nodes aren't disconnected?

That won't solve the assertion.  Please note that this bug is only an assertion which we can prevent against, and nothing bad really happens at runtime as far as I can see.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It would solve the assertion, since you'd be passing non-null for aDisconnected and therefore indicating to ComparePoints that you're prepared to handle the disconnected situation.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(and FWIW I don't think we should be working on harmless non-blocker assertions right now)

Comment 10

[(no longer active)]

(In reply to comment #8)
> It would solve the assertion, since you'd be passing non-null for aDisconnected
> and therefore indicating to ComparePoints that you're prepared to handle the
> disconnected situation.

The current semantics of ComparePoints is that it does use aDisconnected to report unrelated nodes, but it still asserts whether or not aDisconnected is passed or not <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#1785>.

Of course this can change (and I don't see why it would assert when the caller indicates that it's interested in the disconnected case).

(In reply to comment #9)
> (and FWIW I don't think we should be working on harmless non-blocker assertions
> right now)

True.  I was merely trying to understand whether this is an indication of a serious issue somewhere, and from the moment that I realized that it isn't it took me 2 more minutes to write the patch.  But sure, this can wait till post-2.0.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #10)
> The current semantics of ComparePoints is that it does use aDisconnected to
> report unrelated nodes, but it still asserts whether or not aDisconnected is
> passed or not
> <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#1785>.

   NS_ASSERTION(aDisconnected, "unexpected disconnected nodes");

This assertion will not fire if aDisconnected is non-null.

> (In reply to comment #9)
> > (and FWIW I don't think we should be working on harmless non-blocker assertions
> > right now)
> 
> True.  I was merely trying to understand whether this is an indication of a
> serious issue somewhere, and from the moment that I realized that it isn't it
> took me 2 more minutes to write the patch.  But sure, this can wait till
> post-2.0.

Actually, picking off low-hanging fruit like that is efficient and OK by me. Just want to make sure that's all we're doing here.

Comment 12

[(no longer active)]

Attachment: Patch (v2)

Comment 13

[(no longer active)]

(In reply to comment #11)
> (In reply to comment #10)
> > The current semantics of ComparePoints is that it does use aDisconnected to
> > report unrelated nodes, but it still asserts whether or not aDisconnected is
> > passed or not
> > <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#1785>.
> 
>    NS_ASSERTION(aDisconnected, "unexpected disconnected nodes");
> 
> This assertion will not fire if aDisconnected is non-null.

I don't know what's wrong with my eyes today!  :-)

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 484228 [details] [diff] [review]
Patch (v2)

nsContentUtils says
   If the two nodes aren't in the same connected subtree, the result is undefined

but you're relying on it being 1. So please change nsContentUtils to say that if the nodes are disconnected, the result is 1.

Comment 15

[(no longer active)]

Attachment: For check-in

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/62e95bdbae61