Closed Bug 478504 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: Layout, defect, P2)

x86
macOS
defect

Tracking

()

VERIFIED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 1 obsolete file)

Attached file 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
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?  ;)
Perhaps just refuse to paint dirty text frames?
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?
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...
You could just have EnsureTextRun check that dynamically. Callers should be checking for a null result.
But during reflow, don't we have to EnsureTextRun on dirty frames?  Or does reflow construct textruns via some totally different codepath?
EnsureTextRun can check whether we're in reflow and allow it then.
Blocks: 492522
Attached patch Proposed fix (obsolete) — Splinter Review
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...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #389710 - Flags: review?(roc)
Summary: "ASSERTION: Bad offset looking for glyphrun" with ireflow, bidi → [FIX]"ASSERTION: Bad offset looking for glyphrun" with ireflow, bidi
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.
> 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.
Depends on: 504524
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Blocks: 507566
Attached patch FixSplinter Review
Since bug 504524 seems to be held up, let's just do this for now.
Attachment #389710 - Attachment is obsolete: true
Attachment #397625 - Flags: review?(roc)
Attachment #389710 - Flags: review?(roc)
Pushed http://hg.mozilla.org/mozilla-central/rev/3467daadc268
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 397625 [details] [diff] [review]
Fix

Need this on 1.9.2 as well.
Attachment #397625 - Flags: approval1.9.2?
Attachment #397625 - Flags: approval1.9.2? → approval1.9.2+
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)
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.