660416 - "ASSERTION: Invalid offset" with bidi, -moz-column

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (hangs Firefox with infinite loop of assertions)

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

###!!! ASSERTION: Text run does not map enough text for our reflow: 'gfxSkipCharsIterator(iter).ConvertOriginalToSkipped(offset + length) <= mTextRun->GetLength()', file layout/generic/nsTextFrameThebes.cpp, line 6792

###!!! ASSERTION: redo line on totally empty line with non-empty band...: 'aFloatAvailableSpace.mHasFloats', file layout/generic/nsBlockFrame.cpp, line 3710

Comment 1

[Jesse Ruderman]

Attachment: stack trace for the first assertion

Comment 2

[Simon Montagu :smontagu]

Regression from bug 566066.

Comment 3

[Simon Montagu :smontagu]

Attachment: Patch

Comment 4

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

Can you explain in more detail why this is needed?

Comment 5

[Simon Montagu :smontagu]

On initial reflow the <span id="a">x&#x202E;</span> is split into two frames by bidi resolution. The onload handler then changes the text of this span to "", so both frames are now empty, and then changes the text to "z". The loop at the top of nsTextFrame::CharacterDataChanged skips over the first empty frame, so the new text goes into the second frame. The result is that we now have a frame with contentOffset=0 and contentEnd=0 which has a fixed continuation with contentOffset=0 and contentEnd=1.

So the correct results of GetInFlowContentLength are 0 for the first frame (the offset of its fixed continuation) and 1 for the second (the content length).

However, when we call GetInFlowContentLength on the first frame after caching the result from the second frame, flowLength->mStartOffset is 0 (== mContentOffset) and flowLength->mEndFlowOffset is 1 (> mContentOffset), so we end up using the cached result for the first frame.

Comment 6

[Simon Montagu :smontagu]

Having said all that, maybe a better solution would be for bidi resolution to make the continuation of the empty frame into a fluid continuation. Let me try that and get back to you :)

Comment 7

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

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

Review of attachment 535940 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrameThebes.cpp
@@ +634,5 @@
> +  if (flowLength) {
> +    /**
> +     * Bidi resolution combined with dynamic changes to content can leave empty
> +     * text frames in the frame tree. If this frame is empty, don't use the
> +     * cached in-flow length

Let's say
  * This frame must start inside the cached flow. If the flow starts at mContentOffset but this frame is empty, logically it might be before the start of the cached flow.

@@ +639,5 @@
> +     */
> +    GetOffsets(start, end);
> +    if (flowLength->mStartOffset <= mContentOffset &&
> +        flowLength->mEndFlowOffset > mContentOffset &&
> +        end > start) {

So this should probably be
  if ((flowLength->mStartOffset < mContentOffset ||
       (flowLength->mStartOffset == mContentOffset && GetContendEnd() > mContentOffset)) &&
      flowLength->mEndFlowOffset > mContentOffset)

I'd prefer explicit calls to GetContentEnd instead of using GetOffsets, which uses horrible out-parameters :-)

Comment 8

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

(In reply to comment #6)
> Having said all that, maybe a better solution would be for bidi resolution
> to make the continuation of the empty frame into a fluid continuation. Let
> me try that and get back to you :)

That might make sense but I still think this patch is a good change.

Comment 9

[Simon Montagu :smontagu]

Attachment: Patch v.2

I'd like you to check this version over as well, if you wouldn't mind. I removed the other calls to GetOffsets and tightened the code up a bit.

Comment 10

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

Isn't this the same as the previous version?

Comment 11

[Simon Montagu :smontagu]

Attachment: Patch v.2

remembered to qrefresh this time around

Comment 12

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

Comment on attachment 536022 [details] [diff] [review]
Patch v.2

Review of attachment 536022 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 13

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/188979b60337

Comment 14

[Daniel Veditz [:dveditz]]

Is this exploitable or just a DoS?

Comment 15

[Johnny Stenback (:jst)]

This is a regression from 5 so tracking this for 6. If this patch applies to aurora, please request approval for it.

Comment 16

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

Comment on attachment 536022 [details] [diff] [review]
Patch v.2

Should be good for Aurora.

Comment 17

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

But we could also back out bug 566066 on Aurora --- drivers' call really.

Comment 18

[Asa Dotzler [:asa]]

ROC, what's the risk of taking the patch? The regressing change has been in the code for a while so I'm concerned about risk of backing out as well.

Comment 19

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

The risk is very low. The patch disables an optimization in one particular edge case.

Comment 20

[Simon Montagu :smontagu]

http://hg.mozilla.org/releases/mozilla-aurora/rev/ca882c8f7be4

Comment 21

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

(In reply to Daniel Veditz from comment #14)
> Is this exploitable or just a DoS?

It's unclear. Given we have a fix, we may as well treat it conservatively as exploitable.

Comment 22

[Daniel Veditz [:dveditz]]

According to comment 13 this was checked into mozilla-central in the Firefox7 time frame.

Comment 23

[u279076]

qa+ for verification in Firefox 7 using attached testcase.

Comment 24

[u279076]

Changing to qa- as this is an assertion.