394805 - "ASSERTION: ResolveBidi called on non-first continuation"

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jesse Ruderman]

Attachment: 379349-2b.xhtml

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.

Comment 1

[Jesse Ruderman]

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

Comment 2

[Simon Montagu :smontagu]

Attachment: Patch

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.

Comment 3

[Jesse Ruderman]

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.

Comment 4

[Jesse Ruderman]

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 5

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

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?

Comment 6

[Simon Montagu :smontagu]

Attachment: Better patch

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

Comment 7

[Simon Montagu :smontagu]

Comment on attachment 279717 [details] [diff] [review]
Better patch

I'll change NS_STATIC_CAST to static_cast before checkin to keep jwalden happy :)

Comment 8

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

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...

Comment 9

[Simon Montagu :smontagu]

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.

Comment 10

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

Why is a continuation being reflowed before its first continuation?

Comment 11

[Simon Montagu :smontagu]

"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)

Comment 12

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

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

Comment 13

[Simon Montagu :smontagu]

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

Comment 14

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

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 15

[Daniel Holbert [:dholbert]]

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?

Comment 16

[Daniel Holbert [:dholbert]]

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

Comment 17

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

Comment on attachment 279717 [details] [diff] [review]
Better patch

cancelling request pending comments being addressed

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[Simon Montagu :smontagu]

Attachment: what comment 14 said

Comment 20

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

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 21

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

Comment on attachment 281510 [details] [diff] [review]
what comment 14 said

cancelling request pending new patch

Comment 22

[Simon Montagu :smontagu]

Attachment: New patch

Comment 23

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

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

cool

Comment 24

[Simon Montagu :smontagu]

Fix checked in.