478504 - [FIX]"ASSERTION: Bad offset looking for glyphrun" with ireflow, bidi

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

Steps to reproduce:
1. Apply the ireflow patch (bug 67752 comment 56).
2. Set the following environment variables:
  export GECKO_REFLOW_INTERRUPT_MODE=counter
  export GECKO_REFLOW_INTERRUPT_CHECKS_TO_SKIP=0
  export GECKO_REFLOW_INTERRUPT_FREQUENCY=0
3. Load the testcase
4. Wait a few seconds

Result:

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /Users/jruderman/central/gfx/thebes/src/gfxSkipChars.cpp, line 92

###!!! ASSERTION: Substring out of range: 'aStart + aLength <= mCharacterCount', file /Users/jruderman/central/gfx/thebes/src/gfxFont.cpp, line 1781

###!!! ASSERTION: Bad offset looking for glyphrun: 'aOffset <= mCharacterCount', file /Users/jruderman/central/gfx/thebes/src/gfxFont.cpp, line 2160

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the issue is that we call gfxSkipCharsIterator::SetOriginalOffset with aOriginalStringOffset == 1 while its mOriginalStringToSkipCharsOffset == -2.

This causes us to pass unsigned(-1) as the first arg to gfxSkipCharsIterator::SetOffsets, which is bad.

Higher up the stack, the problematic call is this (in nsTextFrame::EnsureTextRun):

2003  gfxSkipCharsIterator iter(mTextRun->GetSkipChars(),
2004                            flow->mDOMOffsetToBeforeTransformOffset,
                                mContentOffset);

  flow->mDOMOffsetToBeforeTransformOffset == -2
  mContentOffset == 1

At this point, mContent->mText is "d ".

It seems that the issue is that the |flow| is still reflecting the old text...  For what it's worth, this is all happening during painting, so presumably the issue is that we interrupted reflow before reflowing this textframe, hence never updated the mDOMOffsetToBeforeTransformOffset in the flow... and now we're painting it.

roc, any ideas on what to do about this?  ;)

Comment 2

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

Perhaps just refuse to paint dirty text frames?

Comment 3

[Boris Zbarsky [:bzbarsky]]

That sorta works (in fact, I made dirty textframes not put an nsDisplayText in the display list at all, so they won't get hit by clicks if they're invisible), until I try to select across the flickering text in the testcase. Then I get the same assert, with this stack to the EnsureTextRun call:

#8  0x1195f92b in nsTextFrame::EnsureTextRun
#9  0x11962336 in nsTextFrame::CalcContentOffsetsFromFramePoint
#10 0x118df31c in nsIFrame::GetContentOffsetsFromPoint
#11 0x11949c69 in nsFrameSelection::HandleDrag

I suppose I could make CalcContentOffsetsFromFramePoint return something like 0 for dirty frames...  Thoughts?

Comment 4

[Boris Zbarsky [:bzbarsky]]

In general, I should probably audit all callers of EnsureTextRun and make sure they don't happen for dirty frames unless they're under reflow...

Comment 5

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

You could just have EnsureTextRun check that dynamically. Callers should be checking for a null result.

Comment 6

[Boris Zbarsky [:bzbarsky]]

But during reflow, don't we have to EnsureTextRun on dirty frames?  Or does reflow construct textruns via some totally different codepath?

Comment 7

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

EnsureTextRun can check whether we're in reflow and allow it then.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

The other option is to just call ClearTextRun() in that clause in EnsureTextRun().  If that's not likely to lead to performance problems, it might be a simpler approach...

Comment 9

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

Actually I think the correct fix here is just to change the GetNextInFlow to GetNextContinuation in ClearTextRunsInFlowChain. When the CharacterDataChanged arrives we are trying to wipe out all text runs associated with the data, which would prevent this kind of problem, but we fail because we're doing it wrong.

The patch in bug 504524 probably fixes this too.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> just to change the GetNextInFlow to GetNextContinuation in
> ClearTextRunsInFlowChain

I can confirm that this fixes this bug, yeah.  Explains why bidi had to be involved!

I can trigger this assert in GetTrimmedOffsets by double-clicking like mad on the testcase:

NS_ASSERTION(!(GetStateBits() & NS_FRAME_FIRST_REFLOW),
             "Can only call this on frames that have been reflowed"); 

but that seems like a separate issue.

> The patch in bug 504524 probably fixes this too.

Seems likely.  Marking dependent; will do the one-liner if that doesn't land soonish.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Since bug 504524 seems to be held up, let's just do this for now.

Comment 12

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

Comment on attachment 397625 [details] [diff] [review]
Fix

sorry

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/3467daadc268

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 397625 [details] [diff] [review]
Fix

Need this on 1.9.2 as well.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/70487907ab6b

Comment 16

[Jonathan Griffin (:jgriffin)]

Fix verified on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b1pre) Gecko/20090929 Namoroka/3.6b1pre (.NET CLR 3.5.30729)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e14825bf17d0
Add crashtest.  r=mats

Comment 18

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/e14825bf17d0