Closed
Bug 604843
Opened 14 years ago
Closed 14 years ago
"ASSERTION: No text for IsSpace" with wrapping inside table cell
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
People
(Reporter: jruderman, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical?])
Attachments
(4 files, 3 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
roc
:
review+
roc
:
approval2.0+
|
Details | Diff | Splinter Review |
###!!! 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
Reporter | ||
Comment 1•14 years ago
|
||
Assignee | ||
Comment 2•14 years ago
|
||
This is a regression from bug 571995.
Blocks: 571995
blocking2.0: --- → ?
Component: Layout: Tables → Layout
Keywords: regression
OS: Mac OS X → All
QA Contact: layout.tables → layout
Hardware: x86 → All
Assignee | ||
Comment 3•14 years ago
|
||
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)
Assignee: nobody → matspal
Assignee | ||
Comment 4•14 years ago
|
||
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)
Assignee | ||
Comment 5•14 years ago
|
||
correction: the length itself isn't part of the hash key, but length characters from the text is.
(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.
Assignee | ||
Comment 8•14 years ago
|
||
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?
Attachment #483973 -
Attachment is obsolete: true
Attachment #484041 -
Attachment is obsolete: true
Attachment #484348 -
Flags: review?(roc)
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•14 years ago
|
||
Guys, can we get a security rating for this?
critical I guess.
Assignee | ||
Comment 12•14 years ago
|
||
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)
Attachment #484348 -
Attachment is obsolete: true
Attachment #484790 -
Flags: review?(roc)
Attachment #484348 -
Flags: review?(roc)
Attachment #484790 -
Flags: review?(roc) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #484790 -
Flags: approval2.0?
blocking2.0: ? → final+
Attachment #484790 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 13•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/4c01c008b5c7 http://hg.mozilla.org/mozilla-central/rev/847ed2b55fe0
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Updated•14 years ago
|
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Comment 14•14 years ago
|
||
Marking this regression a branch blocker for qa verification because bug 571995 is a blocker.
blocking1.9.1: --- → .16+
blocking1.9.2: --- → .13+
status1.9.1:
--- → wanted
status1.9.2:
--- → wanted
Assignee | ||
Comment 15•14 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/650e7b0d6d34 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bd03fbc2b2fc http://hg.mozilla.org/releases/mozilla-1.9.1/rev/19e3f097936a http://hg.mozilla.org/releases/mozilla-1.9.1/rev/91bf1ee0a8ac http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5cb35072f373 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6444dcb39b02 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/64e803656a1f http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ee365bf30311 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0ac503e14b53 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1d2fa8e53cc3 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/57ce1356af46 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3956db8d0373
Updated•14 years ago
|
Whiteboard: [sg:critical?]
Updated•14 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•