Closed Bug 413292 Opened 15 years ago Closed 15 years ago

"ASSERTION: Invalid batch count!" and document stops painting (involves iframes, position:absolute)


(Core :: Web Painting, defect, P2)






(Reporter: jruderman, Assigned: roc)



(Keywords: assertion, testcase)


(2 files)

Attached file testcase
Steps to reproduce:
1. Load the testcase and wait a few seconds.

Result: The page appears blank and the following assertion failure occurs:

###!!! ASSERTION: Invalid batch count!: 'mUpdateBatchCnt >= 0', file /Users/jruderman/trunk/mozilla/view/src/nsViewManager.cpp, line 1875

Expected: The page should say "Done".
This happens because we tear down the document for a child view manager during a Begin/EndUpdateViewBatch on that view manager in DoFlushPendingNotifications. So in the BeginUpdateViewBatch call we forward the batch command up to the root view manger. In the EndUpdateViewBatch call, we no longer are connected to the same root view manager, in fact we don't have one so we decrement the batch count on this view manager, oops.
Yeah, we have XXX comments for this.

I've been thinking about this a bit recently, as it happens, and I think that we should change the signature of BeginUpdateViewBatch() to return an nsIViewManager that EndUpdateViewBatch() should be called on.  It'll return the root viewmanager.  Or something along those lines...  roc, what do you think?  To ease things, we can have a little helper class that does this.
Where are those comments? I have a patch that does exactly what you suggest :-).

The little helper class can't end the batch in its destructor, nsEditor::BeginUpdateViewBatch/EndUpdateViewBatch kind of prevents that without more refactoring than I want to do now (or ever, given compositor is what I'll be working on as soon as 1.9 wraps up). Also we need to pass flags to the End call. But it works out alright.
> Where are those comments?

We could use a helper at all the non-editor callsites, I think, but the complication that some callers need to hold a strong ref while others can hold a weak ref might make it too annoying.
Attached patch fixSplinter Review
Here you go!
Attachment #298409 - Flags: superreview?(bzbarsky)
Attachment #298409 - Flags: review?(bzbarsky)
Whiteboard: [needs review]
Comment on attachment 298409 [details] [diff] [review]

Remove the implementations of that copy ctor and operator= (so leave them private and unimplemented), and looks great.
Attachment #298409 - Flags: superreview?(bzbarsky)
Attachment #298409 - Flags: superreview+
Attachment #298409 - Flags: review?(bzbarsky)
Attachment #298409 - Flags: review+
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Checked in, with a reftest 
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
The test is based on Jesse's test. Comparing the two renderings might actually pass because offscreen rendering would not be blocked by the view manager update batch. However, the assertion(s) should still fire so this would be tested when reftest assertions are fatal.
Depends on: 432292
verified fixed using the testcase and  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre - no assertion on testcase

--> Verified fixed
Depends on: 465475
Blocks: 433538
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.