A bunch of reftests trigger: ###!!! ASSERTION: ResolveBidi called on non-first continuation: '!GetPrevInFlow()', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 6602 For example, reftests/bugs/379349-2b.xhtml triggers it.

This seems to lead to other assertions, so I've nominated it for blocking1.9.

This moves the assertion back before testing the state bit (where I originally had it before roc's review comment in bug 382422 comment 17). This is the right place for it because on non-first continuations the state bit shouldn't have been set so we should be bailing before asserting. Alternatively, we could make the callers of ResolveBidi() test the state bit.

Hmm, so the fix for this is a debug-only change? In that case, I'll make reduced testcases for the other assertions I was seeing along with this one sometimes and file new bugs.

The "other assertions" I mentioned in comment 1 are probably just consequences of bug 393956 and bug 394237 rather than other regressions caused by bug 382422.

Comment on attachment 279633 [details] [diff] [review] Patch OK, but who's calling ResolveBidi on non-first-continuations, and should we fix them? I mean, we could have those callers just do a GetPrevInFlow check, but maybe there's something deeper going on, like they should find the first continuation and call ResolveBidi on it?

Yeah, you're right. Doing it this way should be better.

Comment on attachment 279717 [details] [diff] [review] Better patch I'll change NS_STATIC_CAST to static_cast before checkin to keep jwalden happy :)

This makes reflow of a block with N continuations take O(N^2). Can we rearrange things so we only call GetFirstContinuation inside #ifdef DEBUG code? I think so...

That's tricky. In the testcase a continuation is being reflowed before its first continuation, and the first continuation has the NEEDS_BIDI_RESOLUTION set, so we do need the call to GetFirstContinuation() in non-debug code. Maybe what we should be doing is setting the bit on the frame that calls MarkIntrinsicWidthsDirty itself, not on its first continuation, and have ResolveBidi do GetFirstContinuation and clear the bit on all continuations? That assumes that we wouldn't get a situation where one continuation does MarkIntrinsicWidthsDirty and then a different continuation is reflowed.

Why is a continuation being reflowed before its first continuation?

"Before" may be misleading -- it's possible that only the continuation is being reflowed. For example if character data changes in a continuation, its first continuation wouldn't need to reflow, but we would still need to do bidi resolution on the whole sequence of continuations. (In theory, that is. We don't do that correctly yet, but I have a patch to make us do so)

okay, can we arrange to have the NEEDS_BIDI_RESOLUTION bit set on *all* continuations whenever it's set on any continuation?

Won't that make things like changing page direction O(N^2)?

I'm suggesting the invariant should be "either NEEDS_BIDI_RESOLUTION is set on all blocks in a continuation chain, or it is set on none of them". Then if something changes in one block that requires re-resolution, we set NEEDS_BIDI_RESOLUTION on all blocks. If something then changes in another block, we notice that NEEDS_BIDI_RESOLUTION is already set on that block and we don't have to do anything more. So changing something in each of N blocks is still O(N).

Comment on attachment 279717 [details] [diff] [review] Better patch This line didn't compile for me: >+ NS_STATIC_CAST(nsBlockFrame*, GetFirstContinuation()); I think you meant this: >+ static_cast<nsBlockFrame*>(GetFirstContinuation()); right?

D'oh, my bad -- didn't read the comments back far enough to see that this was already covered. Ignore my last comment. :)

Comment on attachment 279717 [details] [diff] [review] Better patch cancelling request pending comments being addressed

This is blocking us being able to run reftests with fatal assertions (which I would really like us to do sometime soonish, if possible).

If we're going to do what I said in comment #14, it should be documented, preferably alongside the definition of NS_BLOCK_NEEDS_BIDI_RESOLUTION. nsBlockFrame::Init: + AddStateBits(NS_BLOCK_NEEDS_BIDI_RESOLUTION); To preserve the "all or none" invariant, we should check the state bit on aPrevInFlow, and if it's not set, set the state bit on the entire continuation chain. Although actually, if it's not set on aPrevInFlow, do we need to set it on this new frame? I don't think we do. + static_cast<nsBlockFrame*>(GetFirstContinuation())->ResolveBidi(); Shouldn't we be guarding this with a check of NS_BLOCK_NEEDS_BIDI_RESOLUTION? otherwise we're looking at O(N^2) behaviour when we reflow a long chain of blocks. Actually, why not have ResolveBidi do GetFirstContinuation itself after it's checked NS_BLOCK_NEEDS_BIDI_RESOLUTION?

Comment on attachment 281510 [details] [diff] [review] what comment 14 said cancelling request pending new patch

Comment on attachment 284334 [details] [diff] [review] New patch cool

Fix checked in.