1233607 - "ASSERTION: Invalid offset" with <input type="image" style="display: contents;">

Status: RESOLVED WORKSFORME

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

Description

[Jesse Ruderman]

Attachment: testcase

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

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

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Mats Palmgren (inactive)]

Attachment: frame dumps

This is a bug in restyling frames with style contexts that are
children of a display:contents style context (SC).

First, let's look at the correct frame / style context tree -
the one at the top.  Both ::before frames share the same SC
which links has a display:contents SC (pink) as the parent
which in turn is a child of the HTMLScroll frame's SC (yellow).

Then we restyle the first ::before, which works correctly,
it creates a new SC with the new display:contents SC as
parent (magenta).

Restyling the second ::before results in a new SC with
the HTMLScroll frame's SC as parent.  This is a bug,
it should also have the display:contents SC as parent
(and share the SC with its prev-in-flow).

Comment 3

[Mats Palmgren (inactive)]

Attachment: wip

The problem is that the (!nsCSSAnonBoxes::IsAnonBox(pseudo) &&
mContent->GetPrimaryFrame() == this) condition is too strict.
It's true for the first ::before (which happens to be the primary
frame) but not the second ::before.

The "GetPrimaryFrame() == this" condition was added in bug 1153757.

Comment 4

[Mats Palmgren (inactive)]

Attachment: frame tree with "wip" patch applied

Comment 5

[Mats Palmgren (inactive)]

So how does all that connect with the nsTextFrame assertion?

At the top in nsTextFrame::ReflowText we calculate the max
length of text this frame can map (7+5 in this case):
http://hg.mozilla.org/mozilla-central/annotate/cb66ffeb6725/layout/generic/nsTextFrame.cpp#l8505
and initialize this to 'length' here:
http://hg.mozilla.org/mozilla-central/annotate/cb66ffeb6725/layout/generic/nsTextFrame.cpp#l8533
Then nothing much happens until we hit EnsureTextRun here:
http://hg.mozilla.org/mozilla-central/annotate/cb66ffeb6725/layout/generic/nsTextFrame.cpp#l8648
which decides to recreate the textrun.  If you look at frame
dumps up above you'll see that the text nodes share the same
textrun.  The problem is the frames now have different styles,
a different font size to be exact, which makes EnsureTextRun
only map the text in the first ::before (i.e. 7 characters).
Then we fail the assertion at line 8672 because 12 != 7.

Comment 6

[Mats Palmgren (inactive)]

I'm not really sure why this assertion is correct to be honest.
It seems to me that this is something that could happen legitimately
(though it wasn't in this case) and this code needs to deal with.
Perhaps just by setting length = mTextRun->GetLength().

Comment 7

[Mats Palmgren (inactive)]

After stepping through nsTextFrame::ReflowText and looking the resulting
frame tree, I don't see any potential security issue or crash.
We hit this line, which makes transformedLength = 7:
http://hg.mozilla.org/mozilla-central/annotate/cb66ffeb6725/layout/generic/nsTextFrame.cpp#l8717
so I think the rest of the method treats this a text transformation
or something.

Marking it sec-low for now.

Comment 8

[Mats Palmgren (inactive)]

Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c017351fdf1f6d80a1ee860b4c218e2003d1b06c

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c017351fdf1f