EndUpdateBatch not being called for tree builders in some cases

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
a year ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

({fixed1.9.0.12, fixed1.9.1, regression})

Trunk
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.12 +
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Assignee

Description

10 years ago
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
Assignee

Comment 1

10 years ago
This regression causes tree templates that set the datasource dynamically to fail.
Flags: blocking1.9.1?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.12+
Assignee

Comment 2

10 years ago
Hmm. Seems like the attachment that was supposed to be here got lost. Here it is.
Attachment #376563 - Flags: superreview?(Olli.Pettay)
Attachment #376563 - Flags: review?(Olli.Pettay)
Assignee

Updated

10 years ago
Depends on: 433014

Updated

10 years ago
Attachment #376563 - Flags: superreview?(Olli.Pettay)
Attachment #376563 - Flags: superreview+
Attachment #376563 - Flags: review?(Olli.Pettay)
Attachment #376563 - Flags: review+
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.
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;
};
Assignee

Comment 5

10 years ago
The test should only be checked in once bug 433014 is.
Keywords: checkin-needed
Blocking as it's blocking 1.9.0.12
Flags: blocking1.9.1? → blocking1.9.1+
Flags: blocking1.9.1+ → blocking1.9.1?
Assignee

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Assignee

Updated

10 years ago
Keywords: checkin-needed
Could this have caused crashtest 441785-1.xul to have started to fail?
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 ...
Flags: blocking1.9.1? → blocking1.9.1+
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).
Ok, I've managed to reproduce the problem once :/
Posted patch fix the testSplinter Review
I think this should do it. Use nsIXULBuilderListener and not a
timer to trigger next test.
http://hg.mozilla.org/mozilla-central/rev/815856a7144b
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 15

10 years ago
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 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?
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.
(In reply to comment #17)
> Not sure if this is happening on trunk.

It is, I was seeing it on trunk.
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

10 years ago
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.
So, one other regression is that RebuildAll assumes that Uninit calls BeginUpdateBatch but not EndUpdateBatch.
This moves BeginUpdateBatch to happen before Uninit.
Would be great if someone who can reproduce this could test the patch.
Conveniently it also seems to fix Mossop/KaiRo's bug. I don't know why though.
Attachment #378940 - Flags: review?(Olli.Pettay)
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.
Attachment #378940 - Flags: review?(Olli.Pettay) → review-

Updated

10 years ago
Attachment #378936 - Flags: review?(neil)
Comment on attachment 378936 [details] [diff] [review]
possible patch

Neil, does this fix the problem?
(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 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).
Attachment #378936 - Flags: review?(neil) → review+
(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.

Updated

10 years ago
Attachment #378936 - Flags: review?(kairo)

Updated

10 years ago
Attachment #378936 - Flags: review?(dtownsend)
Depends on: 494185
Just to be sure, what remains here is review from kairo and dtownsend?
(reassigning to olli since he seems to be driving, and since Enn's afk)
Assignee: enndeakin → Olli.Pettay

Updated

10 years ago
Attachment #378936 - Flags: review?(kairo) → review+

Comment 32

10 years ago
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.
(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 on attachment 378936 [details] [diff] [review]
possible patch

Yeah this seems to work fine for me
Attachment #378936 - Flags: review?(dtownsend) → review+
http://hg.mozilla.org/mozilla-central/rev/897e626d8d8b
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/41c389eaa495
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Flags: in-testsuite?
Thanks a bunch for getting this done.
Enn, will you take care of 1.9.0.12?

Updated

10 years ago
Assignee: Olli.Pettay → enndeakin
Assignee

Updated

10 years ago
Keywords: fixed1.9.0.12
This landed without approval...
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.