195355 - listBoxObject.getRowCount() returns wrong value

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Shane Caraveo]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030210
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030210

I discovered this while looking into keyboard navigation handling, I was getting
some exceptions in listboxes:

Komodo: Debug: (0): uncaught exception: [Exception... "Component returned failur
e code: 0x80004005 (NS_ERROR_FAILURE) [nsIListBoxObject.getItemAtIndex]"  nsresu
lt: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/cont
ent/bindings/listbox.xml#listbox.getItemAtIndex() :: getItemAtIndex :: line 1"
data: no]

This is reproducable with a simple listbox, just select the first item, the
press the 'z' key to get the exception.

<listbox>
      <listitem label="Normal"/>
      <listitem label="Selected" selected="true" />
      <listitem label="Disabled" disabled="true" />
      <listitem label="Checkbox" class="listitem-iconic" type="checkbox" 
        image="images/betty_boop.xbm"/>
      <listitem label="Checked"  type="checkbox" checked="true" />
</listbox>

I tracked the exception down to the keypress handler in the listbox xbl,
rowCount is always higher than the actual rows in the listbox, then the call to
getItemAtIndex would of course fail.  This is with a build from moz latest
downloaded a few hours ago.

It's not causing me any major problems, at least for now :)


Reproducible: Always

Steps to Reproduce:

Comment 1

[John Morrison]

-> varga

Comment 2

[[e-mail]]

I've seen a similar problem, the row count returned by getRowCount() is one less
than the actual number of rows in the listbox. In my case I think the error is
caused by line 1328 in nsListBoxBodyFrame.cpp which doesn't ensure that the row
count is accurate before modifying it.

Comment 3

[Jan Varga [:janv]]

WFM, can you still reproduce it ?

Comment 4

[[e-mail]]

It's probably dependent on the order things happen, but I always see it on the
mailnews attachment list, if the very first message with attachments has three
or more, then the list thinks it has one fewer, possibly because it adds the 3
to the default value of -1, which means "count will be computed later".

Comment 5

[Shane Caraveo]

Hmm, no I'm not able to easily reproduce it now (trunk as of last week).

Comment 6

[Jan Varga [:janv]]

Attachment: possible patch

Neil, I guess you already have something similar in your tree, but could you
try to reproduce the problem with this patch ?

Comment 7

[Nigel McFarlane]

Attachment: Test case of too-large return values

getRowCount() returns 5 for me.

Comment 8

[Nigel McFarlane]

getRowCount() can also return a value greater than that required.

See the boiled-down test case that shows this. Apologies,
I only tested this on 1.3 final so far. View with -chrome.
This file may assist in confirming your fix works.

I wonder if the listbox "tallest row" calculations are getting knotted
up by the layout imposed by the listbox's sibling tag. That would
account for both getRowCount() problems and occassional odd layout
problems, and would be a more general issue than just getRowCount().

hope this helps. - N.

Comment 9

[[e-mail]]

Attachment: Test case of too-small return values

Jan, actually the way I fixed this variant was to use
if (mRowCount >= 0)
  ++mRowCount;
(similarly for the -- version)
But your patch works too...

Comment 10

[Mark Kaplun]

FWIW, the too-large test case works for me under firebird 0.6.1 and win xp.

My own problem is that when using a template in a listbox (sorry don't have an
available test case) the numbers returns by this function vary if this is the
first time I open the window (in which I get the currect number) or its not (in
which I get one more then there is), since my base code is 1.2 I'm not sure that
it is relevent but from inspecting the code it seems that the code didn't change.

Comment 11

[David :Bienvenu]

see also bug 122180

Comment 12

[Mike Cowperthwaite]

I'm going to request blocking here since not only is there a patch here, it 
addresses the longstanding and much-duped bug 122180.

> Jan, actually the way I fixed this variant was to use
> if (mRowCount >= 0)
>   ++mRowCount;
> (similarly for the -- version)

That implies that mRowCount will never be incremented if it's negative -- is 
there a later call to ComputeRowCount(), as seen in Jan's patch, that takes care 
of that issue?

Comment 13

[[e-mail]]

(In reply to comment #12)
>is there a later call to ComputeRowCount(), as seen in Jan's patch, that
>takes care of that issue?
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsListBoxBodyFrame.cpp
631 PRInt32
632 nsListBoxBodyFrame::GetRowCount()
633 {
634   if (mRowCount < 0)
635     ComputeTotalRowCount();
636   return mRowCount;
637 }

Comment 14

[Jan Varga [:janv]]

I think Neil's approach is better. We might need to add this check for
mRowCount-- too.

Comment 15

[[e-mail]]

Attachment: Proposed patch

Comment 16

[[e-mail]]

Comment on attachment 146703 [details] [diff] [review]
Proposed patch

Low-risk fix for annoying attachment display issue.

Comment 17

[[e-mail]]

Fix checked in to the trunk.

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 146703 [details] [diff] [review]
Proposed patch

a=asa (on behalf of drivers) for checkin to 1.7

Comment 19

[[e-mail]]

Fix checked into the 1.7 branch.

Comment 20

[David :Bienvenu]

*** Bug 214685 has been marked as a duplicate of this bug. ***