###!!! 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

Regression from bug 566066.

Can you explain in more detail why this is needed?

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.

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

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

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.

Isn't this the same as the previous version?

remembered to qrefresh this time around

Comment on attachment 536022 [details] [diff] [review] Patch v.2 Review of attachment 536022 [details] [diff] [review]: -----------------------------------------------------------------

Is this exploitable or just a DoS?

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

Comment on attachment 536022 [details] [diff] [review] Patch v.2 Should be good for Aurora.

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

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.

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

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

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

qa+ for verification in Firefox 7 using attached testcase.

Changing to qa- as this is an assertion.