370255 - ASSERTION: frame not dirty: 'aFrame->GetStateBits() & (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN)' on changing page direction

Status: RESOLVED FIXED

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

Description

[Simon Montagu :smontagu]

Attachment: Patch

I get this assertion sometimes when changing page direction from the context menu.
Caller is nsContainerFrame::InsertFrames():

#ifdef IBMBIDI
    if (nsnull == aListName)
#endif
    // Ask the parent frame to reflow me.
    AddStateBits(NS_FRAME_IS_DIRTY);
    GetPresContext()->PresShell()->
      FrameNeedsReflow(this, nsIPresShell::eTreeChange);

The #ifdef IBMBIDI condition should apply to both lines, like the parallel code in nsContainerFrame::AppendFrames

Comment 1

[Uri Bernstein]

Comment on attachment 254922 [details] [diff] [review]
Patch

Yeah. This is actually a regression from bug 300030 (reflow branch):
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/generic&command=DIFF_FRAMESET&file=nsInlineFrame.cpp&rev2=3.265&rev1=3.264

Comment 2

[Simon Montagu :smontagu]

Checked in.

Comment 3

[Jeff Walden [:Waldo]]

This can indeed be tested -- it requires setting the appropriate environment variable to make assertions cause a crash.  We need a test harness which runs in this manner and is graphical (xpcshell tests run with assertions fatal already, but you don't get any graphical capabilities) to properly write a testcase to ensure the assertion isn't violated in the future in this manner (bug 368573).

Comment 4

[Robert Sayre]

(In reply to comment #3)
> This can indeed be tested -- it requires setting the appropriate environment
> variable to make assertions cause a crash.

Or we could scrape the logs for assertions.

Comment 5

[Simon Montagu :smontagu]

Attachment: testcase

In the meanwhile, here is a testcase that asserts on load in builds without the patch.