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

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P2)

Description

[Jesse Ruderman]

Attachment: 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".

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Where are those comments? 

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsPresShell.cpp&rev=3.1081&mark=4459-4460#4458

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Here you go!

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 298409 [details] [diff] [review]
fix

Remove the implementations of that copy ctor and operator= (so leave them private and unimplemented), and looks great.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in, with a reftest 

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Carsten Book [:Tomcat]]

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