366203 - "ASSERTION: row count changed unexpectedly" with multiple <treechildren> and removing one

Status: RESOLVED WORKSFORME

(Core :: XUL, defect)

Description

[Jesse Ruderman]

Loading this testcase triggers:

###!!! ASSERTION: row count changed unexpectedly: 'mRowCount == rowCount', file /Users/admin/trunk/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp, line 2633

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[[e-mail]]

What seems to be happening here is that each <treechildren> element has its own tree body frame, but only the first one can receive row change notifications.

Comment 3

[[e-mail]]

If we change the style rule in xul.css to
tree > treechildren:first-child
would that resolve the problem?

Comment 4

[Jesse Ruderman]

Now loading the assertion gives me lots of stuff like:

###!!! ASSERTION: bad index: 'aIndex >= 0 && aIndex < mRows.Count()', file /Users/jruderman/trunk/mozilla/layout/xul/base/src/tree/src/nsTreeContentView.cpp, line 284

###!!! ASSERTION: bad row: 'aRow >= 0 && aRow < mRows.Count()', file /Users/jruderman/trunk/mozilla/layout/xul/base/src/tree/src/nsTreeContentView.cpp, line 244

Comment 5

[Jesse Ruderman]

How bad are these assertions?

Comment 6

[[e-mail]]

(In reply to comment #5)
> How bad are these assertions?
The #0 one warns that the tree may not paint what you're expecting it to.
The #4 ones aren't really bad; they're usually just a programming error, like trying to access the (n+1)th row of an n-row tree, so maybe they should be converted from NS_PRECONDITION + if to NS_ENSURE_TRUE instead. However this is often linked with #0 because if the frame's count is out of sync with the view then it may access an invalid row while trying to paint it.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Just do what Neil suggested in comment 3. Well, actually first-of-type, not first-child. This fixes the assertions and makes the tree do what you would expect: it uses the next treechildren when the first is removed.

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 407199 [details] [diff] [review]
patch

I just happened now to see the message at the top of xul.css about needing review from the other Neil.

Comment 9

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/58fd8a926bf5

Comment 10

[Timothy Nikkel (:tnikkel)]

Backed out due to bug 524293.

Comment 11

[Timothy Nikkel (:tnikkel)]

I think autocomplete was failing (bug 524293) because it attaches the treechildren element using a binding, and the first-of-type selector doesn't work on anonymous nodes.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

The assertion this was filed for was downgraded to a warning in bug 326033. On Android, the testcase still hits the warning and also asserts:

WARNING: row count changed unexpectedly: 'mRowCount == rowCount', file /builds/slave/try-and-api-11-d-0000000000000/build/src/layout/xul/tree/nsTreeBodyFrame.cpp, line 2878
[731] ###!!! ASSERTION: bad index: 'aIndex >= 0 && aIndex < int32_t(mRows.Length())', file /builds/slave/try-and-api-11-d-0000000000000/build/src/layout/xul/tree/nsTreeContentView.cpp, line 224
[731] ###!!! ASSERTION: bad row: 'aRow >= 0 && aRow < int32_t(mRows.Length())', file /builds/slave/try-and-api-11-d-0000000000000/build/src/layout/xul/tree/nsTreeContentView.cpp, line 193

It's otherwise green on desktop platforms (and I'm told that an NS_WARN_IF_FALSE would show up in a desktop test log if hit).

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment 12 spun off to bug 1217984 at Jesse's request. I'll close this out as WFM when I push the crashtest.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8c5b8c435e