Last Comment Bug 604843 - "ASSERTION: No text for IsSpace" with wrapping inside table cell
: "ASSERTION: No text for IsSpace" with wrapping inside table cell
Status: RESOLVED FIXED
[sg:critical?]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b7
Assigned To: Mats Palmgren (:mats)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: stirdom 571995 605340
  Show dependency treegraph
 
Reported: 2010-10-15 19:43 PDT by Jesse Ruderman
Modified: 2011-01-27 17:29 PST (History)
5 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.13+
.13-fixed
.16+
.16-fixed


Attachments
testcase (702 bytes, text/html)
2010-10-15 19:43 PDT, Jesse Ruderman
no flags Details
stack traces (25.77 KB, text/plain)
2010-10-15 19:44 PDT, Jesse Ruderman
no flags Details
Trace results (38.67 KB, text/html)
2010-10-18 07:53 PDT, Mats Palmgren (:mats)
no flags Details
wip1 (4.72 KB, patch)
2010-10-18 08:00 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
wip2 (11.56 KB, patch)
2010-10-18 12:03 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 3 (8.84 KB, patch)
2010-10-19 09:53 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 4 (7.97 KB, patch)
2010-10-20 13:03 PDT, Mats Palmgren (:mats)
roc: review+
roc: approval2.0+
Details | Diff | Splinter Review

Description Jesse Ruderman 2010-10-15 19:43:06 PDT
Created attachment 483694 [details]
testcase

###!!! ASSERTION: No text for IsSpace!: 'aPos < aFrag->GetLength()', 
file layout/generic/nsTextFrameThebes.cpp, line 627

###!!! ASSERTION: bad index: 'PRUint32(aIndex) < mState.mLength', 
file nsTextFragment.h, line 204
Comment 1 Jesse Ruderman 2010-10-15 19:44:14 PDT
Created attachment 483695 [details]
stack traces
Comment 2 Mats Palmgren (:mats) 2010-10-16 04:10:15 PDT
This is a regression from bug 571995.
Comment 3 Mats Palmgren (:mats) 2010-10-18 07:53:23 PDT
Created attachment 483967 [details]
Trace results

We have a text run (0x7fffdfd943e0 in red) that spans multiple frames,
then we call AssignTextRun for a frame (0x7fffdfdc5c48 green) in the
middle of that continuation chain, this is the case were we keep the
old text run for the prev-continuations (per bug 571995) and assign
the new text run to the tail of continuations.

The problem is that old text run still has an unmodified
'mCharacterCount' which leads to the assertions here.
The old text run has 'mCharacterCount' = 80, the new (aTextRun) is 41.
(so there's an overlap after AssignTextRun)
Comment 4 Mats Palmgren (:mats) 2010-10-18 08:00:51 PDT
Created attachment 483973 [details] [diff] [review]
wip1

We have a text run (0x7fffdfd943e0 in red) that spans multiple frames,
then we call AssignTextRun for a frame (0x7fffdfdc5c48 green) in the
middle of that continuation chain, this is the case were we keep the
old text run for the prev-continuations (per bug 571995) and assign
the new text run to the tail of continuations.

The problem is that old text run still has an unmodified
'mCharacterCount' which leads to the assertions here.
The old text run has 'mCharacterCount' = 80, the new (aTextRun) is 41.

Introducing a brute force gfxTextRun::SetLength() to set the old
text run length to 39 fixes the assertions, but leads to a new one:
  ASSERTION: Textrun was not completely removed from the cache!:
    'aTextRun->mCachedWords == 0', gfxTextRunWordCache.cpp, line 869
so I added "gfxTextRunWordCache::RemoveTextRun(oldTextRun)" which
removes ALL cached words for the old text run.
(fwiw, the patch passed TryServer unit tests)

It fixes the assertion above, but it doesn't feel right, we should
try to keep the words for 0..38 I think.  The problem is that the key
for the hash table includes the string length in the hash, so if I keep
the [0..38] words and then try to find them later with a different
text run length I won't find the last word since it now is shorter
than when we put it into the table so we'll use the wrong key.

So, for a text run [0..N] that is "split" at M, for the words 0..M
we should keep them as is, except the last one which should be
re-entered with length M instead of N. Words M..N should be removed
(since they belong to aTextRun).

Does that sound right? is it necessary? (or is this word cache data
re-created as needed?)  Do we have other data structures that will
fail if I shorten the text run length?

(I'll take a stab at fixing the word cache and see what falls out)
Comment 5 Mats Palmgren (:mats) 2010-10-18 08:51:01 PDT
correction: the length itself isn't part of the hash key,
but length characters from the text is.
Comment 6 Mats Palmgren (:mats) 2010-10-18 12:03:02 PDT
Created attachment 484041 [details] [diff] [review]
wip2

Here's a patch that updates the word cache per comment 4.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-10-18 15:28:58 PDT
(In reply to comment #4)
> Created attachment 483973 [details] [diff] [review]
> wip1
> 
> We have a text run (0x7fffdfd943e0 in red) that spans multiple frames,
> then we call AssignTextRun for a frame (0x7fffdfdc5c48 green) in the
> middle of that continuation chain, this is the case were we keep the
> old text run for the prev-continuations (per bug 571995) and assign
> the new text run to the tail of continuations.

Hmm, I must have got the previous reviews wrong. I thought we were only going to keep the old textrun when the frame continuations from the assignment point forward were all empty. I don't want to start chopping textruns up like this.
Comment 8 Mats Palmgren (:mats) 2010-10-19 09:53:56 PDT
Created attachment 484348 [details] [diff] [review]
Patch rev. 3

Ok, point taken.  So, this looks at continuations from the assignment point
forward while they refer to the same text run, stopping at the first frame
where GetContentLength() != 0, and if so destroys the old text run.
Is there an easier way to check this?  Perhaps in the multi flow
case it's enough to check TextRunMappedFlow.mContentLength != 0
and avoid looking at each frame?
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-10-19 12:29:44 PDT
Actually I think all you need to do is get the content-offset of f, map it into the textrun text offsets using gfxSkipCharsIterator, and check that the resulting offset is at the end of the textrun!
Comment 10 Damon Sicore (:damons) 2010-10-19 14:25:38 PDT
Guys, can we get a security rating for this?
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-10-19 16:15:23 PDT
critical I guess.
Comment 12 Mats Palmgren (:mats) 2010-10-20 13:03:38 PDT
Created attachment 484790 [details] [diff] [review]
Patch rev. 4

Simplify the emptiness check.  Also, I found that ~98% of the checks
can be optimized away by first checking if f == firstFrame, in which
case 'ClearTextRun' will remove all references and destroy the text
run anyway.  (FindFlowForContent was just moved earlier in the file,
it's not changed in any way)
Comment 14 Daniel Veditz [:dveditz] 2010-11-10 10:43:58 PST
Marking this regression a branch blocker for qa verification because bug 571995 is a blocker.

Note You need to log in before you can comment on or make changes to this bug.