495648 - Incorrect itemCount and scrolling of listboxes with XUL templates for RDF datasources

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[firefox]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; de-DE; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; de-DE; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4 (.NET CLR 3.5.30729)

When dynamically adding a RDF datasource to a listbox and rebuilding the XUL templates, the listbox shows the right entries, but itemCount is always 1. When the listbox has scrollbars, those are displayed incorrectly (claiming there is only one item below), and scrolling always jumps back to the top.

Reproducible: Always

Steps to Reproduce:
1. Get the testcase from http://schierla.de/tmp/testcase/testcase.zip (requires chrome privileges)
2. open testcase.xul
3. click the "count" button below
4. try to scroll down the list (if scrollbars are present)
Actual Results:  
The count button shows "1" as an item count, though 40 items are shown.

Scrolling down does not work.

Expected Results:  
The count button shows "40".

Scrolling down the list works.

This behaviour is new in FF 3.5b4, it was not yet present in 3.1b3.

This behaviour occurs 3.0.12pre, but does not occur in 3.0.10.

Comment 1

[firefox]

Attachment: Testcase (requires chrome privileges)

Comment 2

[Ria Klaassen (not reading all bugmail)]

Confirmed with latest trunk on Windows XP and Vista

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=54c37f642c5c&tochange=615c57912892
 
I don't know why, but I get not always 1 but also 3 as a result.
On Windows XP always 1, and on Vista 3.

Comment 3

[Ria Klaassen (not reading all bugmail)]

None of the other bugs in the range are checked in into 1.9.0 so it must be bug 484031.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Verified, via local backout, that this is a regression from bug 484031.  Looking into it.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, this is ridiculous...  What happens is that notifying the listbox of the very first listitem calls CreateRows, which happily creates _all_ the listitem frames (well, all the ones that fit in the listbox).  So we don't notify it on any more items, which makes it confused about its count.

I think creating those frames for rows that it hasn't counted yet is competely daft, but then again all this code is completely daft.  We really need to remove it.  :(

Unless someone has a bright idea, I will likely hack around this in the check added for bug 484031.  It'll slow down things generated via the template builder, but I'm sort of failing to care at this point.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Boris: do you think this needs to block the release, especially since it's new functionality that nobody's likely depending on, or can we mop it up in 3.5.1?

Comment 8

[Boris Zbarsky [:bzbarsky]]

It's not new functionality: see comment 0.  It's a regression from a security fix we landed.  We need to fix this on all branches where we introduced it, including m-c, 1.9.1, and 1.9.0.

If review comes in fast, I'd prefer to fix this in 1.9.1.0, but I wouldn't necessarily hold the release for it (esp. since we've already broken this on 1.9.0 and need to fix it there too); this would certainly be something that can go in a security update...

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

I hate myself for not blocking on this. If & when review comes, this has my pre-approval to land on trunk for baking and should be nominated for 191 once it goes green there. Someone should also poke me or shaver directly to get the flag set for a191.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/fe91973cc783

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 380769 [details] [diff] [review]
Proposed fix

This is looking green on trunk.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 380769 [details] [diff] [review]
Proposed fix

a191=beltzner; roc, please do let us know if this looks wrong, but so far it's looking really really right!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2256f32c2866

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.0 merge

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2f33f928640f to fix resulting 1.9.1 crashes.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated 1.9.0 merge

Comment 17

[Daniel Veditz [:dveditz]]

qawanted: This may be breaking addons, and therefore be a stop-ship re-spin 3.0.11 rather than ship a security update that makes people want to downgrade.

A quick add-on grep for AddDataSource() gets hits in at least 150 different files (partial collection of addons). Some might be listboxes, some are definitely trees or menus -- do those have problems too? Here are a few to check out:
 DOM Inspector (addons 1806, 1837, 2334)
 Torbutton  https://addons.mozilla.org/en-US/firefox/addon/2275
 Subtile    https://addons.mozilla.org/en-US/firefox/addon/4906
 SiteDelta  https://addons.mozilla.org/en-US/firefox/addon/4337 (sidebar)
 FoxyTunes (maybe? in the included jslib, not sure they use it)
            https://addons.mozilla.org/en-US/firefox/addon/219
 Mnenhy     https://addons.mozilla.org/en-US/firefox/addon/2516
 Hermes     https://addons.mozilla.org/en-US/firefox/addon/1460
 Music Player Minion https://addons.mozilla.org/en-US/firefox/addon/6324
 Tab Mix (not TMPlus) https://addons.mozilla.org/en-US/firefox/addon/625
 Delicious Complete  https://addons.mozilla.org/en-US/firefox/addon/2354
  ... and more ...

Comment 18

[Boris Zbarsky [:bzbarsky]]

This issue is listbox-specific.

Comment 19

[Daniel Veditz [:dveditz]]

Maybe the Music Player Minion playlist then, or yoono https://addons.mozilla.org/en-US/firefox/addon/1833

Comment 20

[Daniel Veditz [:dveditz]]

Can't repro the bug with yoono. The variable name/element ID in the script said "listbox" but it's really a <tree>

Comment 21

[Daniel Veditz [:dveditz]]

The bug reporter's own SiteDelta exhibits the problem. Guess that one isn't in the dataset I was scanning, or it was an older version that didn't use AddDataSource.
https://addons.mozilla.org/en-US/firefox/addon/4337

STR/verify:
 1. Install SiteDelta
 2. Surf around and click the delta in the statusbar to add sites to list
 3. open sitedelta sidebar if not already open
 4. shrink page so page list gets a scrollbar
 5. try to scroll list

Comment 22

[Daniel Veditz [:dveditz]]

Another Add-on that shows the problem is Download Sort https://addons.mozilla.org/en-US/firefox/addon/25

Open the pref dialog (from the addon dialog "preferences" button, dunno if there's another way) and add more than 12 extension rules so that the slider appears on the listbox. In 3.0.11 it won't scroll.

Download Sort has 10 times the daily users of SiteDelta, although the broken behavior would be less obvious than SiteDelta's to existing users since they have probably already set up their preferences.

Between the two that's probably enough to justify a stop-ship on 3.0.11, and there are likely other addons affected.

Comment 23

[Samuel Sidler (old account; do not CC)]

Comment on attachment 380934 [details] [diff] [review]
Updated 1.9.0 merge

Alright, let's get this on 1.9.0 and on the GECKO190_20090519_RELBRANCH.

Approved for 1.9.0.11 and 1.9.0.12. a=ss

Comment 24

[Daniel Veditz [:dveditz]]

Checked into cvs trunk and relbranch

Checking in nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1483; previous revision: 1.1482
done
Checking in tests/Makefile.in;
/cvsroot/mozilla/layout/base/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.64; previous revision: 1.63
done
RCS file: /cvsroot/mozilla/layout/base/tests/bug495648.rdf,v
done
Checking in tests/bug495648.rdf;
/cvsroot/mozilla/layout/base/tests/bug495648.rdf,v  <--  bug495648.rdf
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/base/tests/test_bug495648.xul,v
done
Checking in tests/test_bug495648.xul;
/cvsroot/mozilla/layout/base/tests/test_bug495648.xul,v  <--  test_bug495648.xul
initial revision: 1.1

Checking in nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1482.2.1; previous revision: 1.1482
done
Checking in tests/Makefile.in;
/cvsroot/mozilla/layout/base/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.63.24.1; previous revision: 1.63
done
Checking in tests/bug495648.rdf;
/cvsroot/mozilla/layout/base/tests/bug495648.rdf,v  <--  bug495648.rdf
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in tests/test_bug495648.xul;
/cvsroot/mozilla/layout/base/tests/test_bug495648.xul,v  <--  test_bug495648.xul
new revision: 1.1.2.2; previous revision: 1.1.2.1
done

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.11 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11) Gecko/2009060214 Firefox/3.0.11.

Comment 26

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8 patch

Comment 27

[Joshua Mitchell (Inactive)]

Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task