Last Comment Bug 576649 - "ASSERTION: Invalid offset" and more with -moz-column, abs pos, huge letter-spacing
: "ASSERTION: Invalid offset" and more with -moz-column, abs pos, huge letter-s...
Status: RESOLVED FIXED
: [sg:critical?][critsmash:patch]
: assertion, testcase
Product: Core
Classification: Components
Component: Layout
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b4
Assigned To: Mats Palmgren [:mats]
: layout
:
: 587484
: 306939
  Show dependency treegraph
 
Reported: 2010-07-02 15:09 PDT by Jesse Ruderman
Modified: 2011-06-20 18:09 PDT (History)
11 users (show)
jruderman: in‑testsuite+
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  final+
  ---
  .14+
  .14-fixed
  .17+
  .17-fixed


Attachments
testcase (257 bytes, text/html)
2010-07-02 15:09 PDT, Jesse Ruderman
no flags Details
framedump + stack (19.67 KB, text/html)
2010-07-03 21:21 PDT, Mats Palmgren [:mats]
no flags Details
wip1 (diff -w) (2.63 KB, patch)
2010-07-03 21:27 PDT, Mats Palmgren [:mats]
bzbarsky: review+
Details | Diff | Splinter Review
wip2 (diff -w) for 1.9.2 (2.54 KB, patch)
2010-07-30 19:48 PDT, Mats Palmgren [:mats]
no flags Details | Diff | Splinter Review
wip2 (diff -w) (5.30 KB, patch)
2010-08-04 19:27 PDT, Mats Palmgren [:mats]
bzbarsky: review+
Details | Diff | Splinter Review
[checked-in] wip2 (for trunk checkin) (5.69 KB, patch)
2010-08-10 14:38 PDT, Mats Palmgren [:mats]
no flags Details | Diff | Splinter Review
fix for 1.9.2 (diff -w) (5.12 KB, patch)
2010-08-13 22:13 PDT, Mats Palmgren [:mats]
fantasai.bugs: review-
dbaron: review+
Details | Diff | Splinter Review
fix for 1.9.2 (5.39 KB, patch)
2010-08-13 22:14 PDT, Mats Palmgren [:mats]
no flags Details | Diff | Splinter Review
additional check I think we should remove now (1.14 KB, patch)
2010-11-29 13:56 PST, David Baron [:dbaron]
roc: review+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2010-07-02 15:09:00 PDT
Created attachment 455778 [details]
testcase

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /builds/slave/mozilla-central-macosx-debug/build/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 /builds/slave/mozilla-central-macosx-debug/build/layout/generic/nsTextFrameThebes.cpp, line 6278
Comment 1 Mats Palmgren [:mats] 2010-07-03 21:21:31 PDT
Created attachment 455921 [details]
framedump + stack

There's something wrong with the style contexts in the
abs.pos. continuations (and their descendants).
BuildTextRunsScanner::ContinueTextRunAcrossFrames returns false
due to a difference in the font style so we end the text run
prematurely - which leads up to the assertions in Reflow.
Comment 2 Mats Palmgren [:mats] 2010-07-03 21:27:36 PDT
Created attachment 455922 [details] [review]
wip1 (diff -w)

I think we need to do something like this to take care of
the continuations in overflow containers.
Comment 3 Robert Sayre 2010-07-13 13:22:40 PDT
is the sg: severity right?
Comment 4 Mats Palmgren [:mats] 2010-07-30 18:55:44 PDT
I think I've seen a bug in the past where the assertions in comment 0
led to an out-of-bounds error of some sort.
I don't see anything like that for this particular testcase, and valgrind
is silent too, but that doesn't prove anything of course.
Comment 5 Mats Palmgren [:mats] 2010-07-30 19:17:51 PDT
I tracked down the changeset where these assertions first started:
http://hg.mozilla.org/mozilla-central/rev/eb19d94d35ef
That change is not on 1.9.2 (despite bug 508325 having milestone 1.9.2a1).
Applying it makes the assertions appear also on 1.9.2, so I suspect the
underlying bug is present also on 1.9.2 with a slightly different testcase.
Applying the "wip1" patch on top of that makes the nasty assertions go away.
Comment 6 Mats Palmgren [:mats] 2010-07-30 19:45:12 PDT
For 1.9.2 we might need a slightly different patch since we still have
placeholder continuations there, which should take care of the out-of-flow
next-in-flows except for overflow containers.
Comment 7 Mats Palmgren [:mats] 2010-07-30 19:48:32 PDT
Created attachment 461741 [details] [review]
wip2 (diff -w) for 1.9.2
Comment 8 Robert O'Callahan (:roc) (Mozilla Corporation) 2010-08-03 12:44:44 PDT
Comment on attachment 455922 [details] [review]
wip1 (diff -w)

Seems like more of a Boris thing --- style system.
Comment 9 Boris Zbarsky (:bz) 2010-08-03 12:52:36 PDT
The reparenting part looks fine.

The reresolving part, I'm not sure about in terms of dynamic changes being handled properly; in particular, if we get a repaint hint, will we apply it correctly?  Or does ApplyRenderingChangeToTree also need to walk the continuations of the out-of-flow?

Speaking of which, do we want GetNextInFlow(), or GetNextContinuation()?
Comment 10 Robert O'Callahan (:roc) (Mozilla Corporation) 2010-08-03 13:28:17 PDT
(In reply to comment #9)
> The reresolving part, I'm not sure about in terms of dynamic changes being
> handled properly; in particular, if we get a repaint hint, will we apply it
> correctly?  Or does ApplyRenderingChangeToTree also need to walk the
> continuations of the out-of-flow?

I think it does.

> Speaking of which, do we want GetNextInFlow(), or GetNextContinuation()?

GetNextContinuation probably, although it doesn't matter since fixed-continuations are never out of flow.
Comment 11 Boris Zbarsky (:bz) 2010-08-03 13:29:57 PDT
Comment on attachment 455922 [details] [review]
wip1 (diff -w)

OK, r=me with the changes from comment 10.
Comment 12 Mats Palmgren [:mats] 2010-08-04 19:27:54 PDT
Created attachment 463035 [details] [review]
wip2 (diff -w)

Updated according to review comments.
Comment 13 Boris Zbarsky (:bz) 2010-08-04 19:46:08 PDT
Comment on attachment 463035 [details] [review]
wip2 (diff -w)

r=me
Comment 14 Robert O'Callahan (:roc) (Mozilla Corporation) 2010-08-10 12:57:25 PDT
I guess this is ready to land.
Comment 15 Mats Palmgren [:mats] 2010-08-10 14:38:15 PDT
Created attachment 464583 [details] [review]
[checked-in] wip2 (for trunk checkin)
Comment 16 Rob Campbell [:rc] (robcee) 2010-08-10 15:12:21 PDT
Comment on attachment 464583 [details] [review]
[checked-in] wip2 (for trunk checkin)

http://hg.mozilla.org/mozilla-central/rev/735ff84db7b1
Comment 17 Rob Campbell [:rc] (robcee) 2010-08-10 15:12:48 PDT
Comment on attachment 464583 [details] [review]
[checked-in] wip2 (for trunk checkin)

http://hg.mozilla.org/mozilla-central/rev/735ff84db7b1
Comment 18 Mats Palmgren [:mats] 2010-08-13 22:13:47 PDT
Created attachment 465948 [details] [review]
fix for 1.9.2 (diff -w)

Same as the trunk patch, with the following additional loop condition:
  (outOfFlowFrame->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER)
since on branches we have placeholder continuations for out-of-flow
continuations, except for overflow containers, right?
Comment 19 Mats Palmgren [:mats] 2010-08-13 22:14:24 PDT
Created attachment 465949 [details] [review]
fix for 1.9.2
Comment 20 fantasai 2010-08-17 12:25:37 PDT
Comment on attachment 465948 [details] [review]
fix for 1.9.2 (diff -w)

Ok, I don't understand this patch at all.

1. UpdateViewsForTree

Why is this grabbing out-of-flow frames at all? If I have a placeholder for a fixedpos frame (or simply an abspos frame whose containing block is outside this tree), am I really wanting to pop out and DoApplyRenderingChangeToTree to the fixedpos frame?

2. ReparentStyleContext

AFAICT, both your code and the existing code looks wrong. It seems to me that
  a) In 1.9.2, the top level (non-recursive) call to ReparentStyleContext
     should be skipping placeholder continuations.
  b) In 1.9.2, whenever ReparentStyleContext is called on a placeholder, it
     should reparent all its continuations as well.
  b) In both trees, whenever ReparentStyleContext is called on a placeholder,
     it should reparent the out-of-flow and all its continuations.
  c) In both trees, the top level (non-recursive) call to ReparentStylecontet
     should be skipping all out-of-flow continuations.
  d) No other continuations should be reparented by ReparentStyleContext.
     Specifically, there should be no exceptions for true overflow containers.

So for a) and c), maybe have an aIsTopLevelCall argument that defaults to True, and pass False from all calls within  ReparentStyleContext. Then return immediately if isTopLevelCall is true and aFrame is either {a placeholder or an out-of-flow} with a prev-continuation.

3. I don't really understand how ReResolveStyleContext is supposed to work.

I think you might need someone else to review this--probably either dbaron or bzbarsky.
Comment 21 Mats Palmgren [:mats] 2010-08-24 18:57:26 PDT
Reopening to investigate the code in light of the last comment.
Comment 22 Daniel Veditz 2010-09-21 13:13:48 PDT
Does the checked-in patch fix the testcase? if so maybe this bug should stay fixed and comment 20 should be a new bug. The status of this bug is unclear.
Comment 23 David Baron [:dbaron] 2010-09-28 14:37:51 PDT
(In reply to comment #20)
> 1. UpdateViewsForTree
> 
> Why is this grabbing out-of-flow frames at all? If I have a placeholder for a
> fixedpos frame (or simply an abspos frame whose containing block is outside
> this tree), am I really wanting to pop out and DoApplyRenderingChangeToTree to
> the fixedpos frame?

Going into out-of-flow frames is correct here.

UpdateViewsForTree and DoApplyRenderingChangeToTree are mutually recursive functions that are both called only from ApplyRenderingChangeToTree, whose purpose is handling "repaint" style changes.  (Its only caller is nsCSSFrameConstructor::ProcessRestyledFrames .)  The way we handle style changes optimizes away style changes of the same type on descendants:  if we have a reflow style change on A, we don't report a reflow style change on any descendants of A, etc.

This isn't quite sufficient for out-of-flows, so we need to do a little extra work for out-of-flows for both reflow and repaint.  For repaint, that extra work is in these mutually recursive functions.  For reflow, the extra work happens in PresShell::FrameNeedsReflow.

An example of a style change where we need this code is this markup:
  <span id="a" style="color:red"><span style="float:left">text</span></span>
and this script:
  document.getElementById("a").style.color="green"
When we change the style on the span, we report a style change only on the span, not on its descendants.  Then ApplyRenderingChangeToTree goes through and looks for appropriate descendants.

Mats's change to this function seems correct to me, although I don't see how it actually fixes any bugs that are currently present.  (I see plenty of easily observable bugs that would be present if we re-enabled splitting of floats in columns, though; just put the example above in a situation where the float splits (and, preferably, the span doesn't).)


That said, i still need to understand what the overflow containers checks are doing (and whey they're different between the m-c patch and the 1.9.2 patch).
Comment 24 Damon Sicore (:damons) 2010-10-12 13:28:54 PDT
Mats, progress here?
Comment 25 Mats Palmgren [:mats] 2010-10-12 13:53:41 PDT
David's comment seems to suggest that the trunk patch (still in the tree)
is correct, although redundant since we don't split frames in a way that
creates the crash condition anymore.  If so, we should decide if we want
to leave it in or back it out.  (I haven't looked at the details regarding
fantasai's comment myself yet.  I'm working on bug 571995 now, then I'll
get to this bug.)

David, regarding the difference between m-c and 1.9.2 patches:
on 1.9.2 we have placeholder continuations to the out-of-flow
continuations so they should be reachable that way, except for frames
on the overflow-container lists which don't have a placeholder so we
need to process those (they should be the tail end of that continuation
chain).
Comment 26 David Baron [:dbaron] 2010-11-09 13:56:11 PST
So write after writing comment 23 I started writing this additional review, but then was distracted by other work; I'll try to get back to it, but this information may be useful:

nsCSSFrameConstructor.cpp:

So, looking at the original patch
( http://hg.mozilla.org/mozilla-central/rev/735ff84db7b1 ), I don't
understand why the NS_FRAME_IS_OVERFLOW_CONTAINER check is present
(where it's ||ed with the ! NS_FRAME_OUT_OF_FLOW check), right above the
code you modified in nsCSSFrameConstructor.cpp.  It was added in
http://hg.mozilla.org/mozilla-central/rev/854d0d479141 , but it seems
like it might be a workaround for part of the bug that you fixed (i.e.,
not looking at next-in-flows).  I don't know if fantasai remembers why
that was added; it seems to have first appeared in the patch in bug
154892 comment 205, which I think suggests that may be correct, but it's
not clear.

I think that check should have been removed on mozilla-central.  I'm not
yet sure about 1.9.2.


As to the differences between the m-c patch and the 1.9.2 patch, I'm
still trying to understand and verify comment 18.
Comment 27 Daniel Veditz 2010-11-12 10:10:23 PST
FIXED seems a better resolution -- things got checked in, remain checked in, and made the assertions go away. If there's more investigation to do it can be done as part of the branch back-port, or maybe a new bug for it that's make the remaining work more clear.

As long as things are taking it seems unrealistic to resolve by next Thursday's branch code-freeze: punting to next release.
Comment 28 David Baron [:dbaron] 2010-11-29 13:56:58 PST
Created attachment 493787 [details] [review]
additional check I think we should remove now
Comment 29 David Baron [:dbaron] 2010-11-29 13:57:51 PST
See comment 26 for an explanation of the patch in comment 28.
Comment 30 Boris Zbarsky (:bz) 2010-11-29 19:38:23 PST
Comment on attachment 493787 [details] [review]
additional check I think we should remove now

I think this needs review from roc or fantasai...
Comment 31 Robert O'Callahan (:roc) (Mozilla Corporation) 2010-11-30 16:56:03 PST
Comment on attachment 493787 [details] [review]
additional check I think we should remove now

I agree.
Comment 32 Daniel Veditz 2010-12-10 10:43:15 PST
Did attachment 493787 [details] [review] ever land? Do we need a separate bug to track that?
Comment 33 Mats Palmgren [:mats] 2010-12-13 16:41:54 PST
(In reply to comment #28)
> additional check I think we should remove now

I agree, we should reach those traversing next-continuation...
Also, since we test !OUT_OF_FLOW, we can skip lists that only
contains OOFs.  I think we could probably skip this test in
nsFrameManager too in a couple of places where we traverse
next-continuations...

(In reply to comment #32)
> Did attachment 493787 [details] [review] ever land?

Not yet

> Do we need a separate bug to track that?

I spawned it off as bug 618949 to get separate tracking.
Comment 34 David Baron [:dbaron] 2010-12-16 08:58:36 PST
Followup landed:
http://hg.mozilla.org/mozilla-central/rev/d0e5fb03bae2
Comment 35 David Baron [:dbaron] 2011-01-18 14:05:50 PST
Comment on attachment 465948 [details] [review]
fix for 1.9.2 (diff -w)

r=dbaron
Comment 37 Jesse Ruderman 2011-06-20 18:09:17 PDT
Crashtest: http://hg.mozilla.org/mozilla-central/rev/83185e96e3c3

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