Closed Bug 604314 Opened 9 years ago Closed 9 years ago

"ASSERTION: unexpected disconnected nodes"

Categories

(Core :: Selection, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: jruderman, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 2 obsolete files)

Attached file testcase
###!!! ASSERTION: unexpected disconnected nodes: 'aDisconnected', file content/base/src/nsContentUtils.cpp, line 1789
Attached file stack trace
Jesse, is this something which is worth blocking on?
It doesn't seem especially dangerous to me, but I'm not familiar with the code.
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #484122 - Flags: review?(roc)
Attachment #484122 - Flags: approval2.0?
This is exactly like bug 586662, BTW.
Why not use the aDisconnected parameter of ComparePoints and just check that the nodes aren't disconnected?
(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.
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.
(and FWIW I don't think we should be working on harmless non-blocker assertions right now)
(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.
(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.
Attached patch Patch (v2) (obsolete) — Splinter Review
Attachment #484122 - Attachment is obsolete: true
Attachment #484228 - Flags: review?(roc)
Attachment #484228 - Flags: approval2.0?
Attachment #484122 - Flags: review?(roc)
Attachment #484122 - Flags: approval2.0?
(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 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.
Attachment #484228 - Flags: review?(roc)
Attachment #484228 - Flags: review+
Attachment #484228 - Flags: approval2.0?
Attachment #484228 - Flags: approval2.0+
Attached patch For check-inSplinter Review
Attachment #484228 - Attachment is obsolete: true
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/62e95bdbae61
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.