492037 - EndUpdateBatch not being called for tree builders in some cases

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

This causes the tree to not redraw properly. Testcase in bug 441785.

The test changes don't actually fail without this patch but they make the test check more things.

This bug blocks checking in 441785 on 1.9.0

Comment 1

[Neil Deakin]

This regression causes tree templates that set the datasource dynamically to fail.

Comment 2

[Neil Deakin]

Attachment: make sure to call EndUpdateBatch to update the tree

Hmm. Seems like the attachment that was supposed to be here got lost. Here it is.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 376563 [details] [diff] [review]
make sure to call EndUpdateBatch to update the tree

Please use -p and -U 8 when generating patches.

>diff --git a/content/xul/templates/src/nsXULTreeBuilder.cpp b/content/xul/templates/src/nsXULTreeBuilder.cpp
>--- a/content/xul/templates/src/nsXULTreeBuilder.cpp
>+++ b/content/xul/templates/src/nsXULTreeBuilder.cpp
>@@ -319,6 +319,7 @@
>     if (mBoxObject) {
>         mBoxObject->BeginUpdateBatch();
>         mBoxObject->RowCountChanged(0, -count);
>+        mBoxObject->EndUpdateBatch();
I think it is possible that mBoxObject is null after RowCountChanged.
So add a null check.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Another, and perhaps safer, option would be to add a simple stack object for
Begin/EndUpdateBatch and then use that when ever batching is needed.
Something like:

class nsTreeBatch
{
public:
  nsTreeBatch(nsITreeBoxObject* aBoxObject) : mBoxObject(aBoxObject)
  {
    if (mBoxObject)
      mBoxObject->BeginUpdateBatch();
  }
  ~nsTreeBatch()
  {
    if (mBoxObject)
      mBoxObject->EndUpdateBatch();
  }
private:
  nsCOMPtr<nsITreeBoxObject> mBoxObject;
};

Comment 5

[Neil Deakin]

The test should only be checked in once bug 433014 is.

Comment 6

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

Blocking as it's blocking 1.9.0.12

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could this have caused crashtest 441785-1.xul to have started to fail?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Backed this out since it caused linux unit test to fail.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242163597.1242171202.5863.gz&fulltext=1

Comment 9

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

Smaug: can you take a look at this and try to figure out what's causing that test to fail? This is a blocker (ss accidentally wiped out that flag :( ) but Enn's afk until after code freeze ...

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I can't reproduce the problem locally on linux.
I posted the patch to tryserver.

Note to myself, the patch does not contain the null check
(that was added when this was committed).

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Ok, I've managed to reproduce the problem once :/

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fix the test

I think this should do it. Use nsIXULBuilderListener and not a
timer to trigger next test.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/815856a7144b

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/40230a2e0c6c

Comment 15

[Robert Kaiser]

Hrm, looks like this patch actually made the folder tree in SeaMonkey not paint on scroll any more for me. Yesterday's local build worked fine, today's was broken, backing out this patch it works again.

Comment 16

[[e-mail]]

Comment on attachment 376563 [details] [diff] [review]
make sure to call EndUpdateBatch to update the tree

>     if (mBoxObject) {
>         mBoxObject->BeginUpdateBatch();
>         mBoxObject->RowCountChanged(0, -count);
>+        mBoxObject->EndUpdateBatch();
>     }
Batching one operation is kinda pointless, no?

Comment 17

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

People are asking me if we should back this out.

We should back this out, and remove fixed1.9.1:

15:31 < KaiRo> beltzner, smaug: the regression from this regression fix (sigh) is that some <tree> elements don't repaint when they should, e.g. on scrolling - I'm 
seeing this in SeaMonkey's folder pane, Mossop with an extension

Not sure if this is happening on trunk.

Comment 18

[Dave Townsend [:mossop]]

(In reply to comment #17)
> Not sure if this is happening on trunk.

It is, I was seeing it on trunk.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Backed out
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/11e0fd20fde9
http://hg.mozilla.org/mozilla-central/rev/897e626d8d8b

Comment 20

[Dave Townsend [:mossop]]

I was seeing this in an extension that used an rdf driven tree, but couldn't see it in the bookmarks organiser which I believe uses a js custom tree.

Comment 21

[Robert Kaiser]

Mossop says he is seeing the painting problem with the patch in an extension on trunk that probably has RDF-based trees, I'm seeing it in SeaMonkey on 1.9.1 in the still RDF-driven folder pane in mailnews, while Thunderbird people claim they are not seeing the problem and they have converted their folder pane to being driven by JS.
What happens is that scrolling or expanding/collapsing twisties doesn't repaint the tree. Clicking on the thread pane (different, also RDF-driven, tree in the same window) scrollbar can make the folder pane repaint though, and switching back and forth between windows does correctly repaint the tree as well. This is on i686 Linux for me, BTW.

Comment 22

[[e-mail]]

So, one other regression is that RebuildAll assumes that Uninit calls BeginUpdateBatch but not EndUpdateBatch.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

This moves BeginUpdateBatch to happen before Uninit.
Would be great if someone who can reproduce this could test the patch.

Comment 24

[[e-mail]]

Attachment: Supplementary patch that fixes my assertion

Conveniently it also seems to fix Mossop/KaiRo's bug. I don't know why though.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 378940 [details] [diff] [review]
Supplementary patch that fixes my assertion

Begin/EndUpdateBatch usage should be somehow sane, so they should, IMHO, always happen in the same method.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 378936 [details] [diff] [review]
possible patch

Neil, does this fix the problem?

Comment 27

[[e-mail]]

(In reply to comment #25)
> (From update of attachment 378940 [details] [diff] [review])
> Begin/EndUpdateBatch usage should be somehow sane, so they should, IMHO, always
> happen in the same method.
They do. I removed both Begin/EndUpdateBatch from Uninit which doesn't need them, and I disconnected the call to Uninit from the call to BeginUpdateBatch in RebuildAll. The call to EndUpdateBatch still exists, but out of context.

Comment 28

[[e-mail]]

Comment on attachment 378936 [details] [diff] [review]
possible patch

By visual inspection, this fixes the problem in much the same way as my patch did (sorry I didn't see this patch get attached because I'm not CC'd).

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #27)
> (In reply to comment #25)
> > (From update of attachment 378940 [details] [diff] [review])
> > Begin/EndUpdateBatch usage should be somehow sane, so they should, IMHO, always
> > happen in the same method.
> They do. I removed both Begin/EndUpdateBatch from Uninit which doesn't need
> them, and I disconnected the call to Uninit from the call to BeginUpdateBatch
> in RebuildAll. The call to EndUpdateBatch still exists, but out of context.
Ah, sorry, I misread the patch. But still, I'd like to keep the changes at
minimum at this point.

Comment 30

[Damon Sicore (:damons)]

Just to be sure, what remains here is review from kairo and dtownsend?

Comment 31

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

(reassigning to olli since he seems to be driving, and since Enn's afk)

Comment 32

[Robert Kaiser]

Comment on attachment 378936 [details] [diff] [review]
possible patch

r=me from a functionality POV as this patch fixes the regression from the previously checked in fix.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #30)
> Just to be sure, what remains here is review from kairo and dtownsend?
And a testcase for the regression would be great.

Comment 34

[Dave Townsend [:mossop]]

Comment on attachment 378936 [details] [diff] [review]
possible patch

Yeah this seems to work fine for me

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/897e626d8d8b
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/41c389eaa495

Comment 36

[Damon Sicore (:damons)]

Thanks a bunch for getting this done.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Enn, will you take care of 1.9.0.12?

Comment 38

[Neil Deakin]

Attachment: Patch for 1.9.0.x, includes the changes for bug 441785, but without the test

Comment 39

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

This landed without approval...

Comment 40

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336