Closed Bug 492037 Opened 12 years ago Closed 12 years ago

EndUpdateBatch not being called for tree builders in some cases

Categories

(Core :: XUL, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: enndeakin, Assigned: enndeakin)

References

Details

(Keywords: fixed1.9.0.12, fixed1.9.1, regression)

Attachments

(5 files)

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
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+
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)
Depends on: 433014
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;
};
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?
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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 :/
Attached 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
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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.
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.
Attached patch possible patchSplinter Review
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-
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.
Attachment #378936 - Flags: review?(kairo)
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
Attachment #378936 - Flags: review?(kairo) → review+
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
Closed: 12 years ago12 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?
Assignee: Olli.Pettay → enndeakin
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.