Bugzilla@Mozilla – Bug 576649
"ASSERTION: Invalid offset" and more with -moz-column, abs pos, huge letter-spacing
Last modified: 2011-06-20 18:09:17 PDT
Summon comment box
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
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.
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.
is the sg: severity right?
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.
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.
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.
Created attachment 461741 [details] [review] wip2 (diff -w) for 1.9.2
Comment on attachment 455922 [details] [review] wip1 (diff -w) Seems like more of a Boris thing --- style system.
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()?
(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 on attachment 455922 [details] [review] wip1 (diff -w) OK, r=me with the changes from comment 10.
Created attachment 463035 [details] [review] wip2 (diff -w) Updated according to review comments.
Comment on attachment 463035 [details] [review] wip2 (diff -w) r=me
I guess this is ready to land.
Created attachment 464583 [details] [review] [checked-in] wip2 (for trunk checkin)
Comment on attachment 464583 [details] [review] [checked-in] wip2 (for trunk checkin) http://hg.mozilla.org/mozilla-central/rev/735ff84db7b1
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?
Created attachment 465949 [details] [review] fix for 1.9.2
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.
Reopening to investigate the code in light of the last comment.
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.
(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).
Mats, progress here?
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).
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.
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.
Created attachment 493787 [details] [review] additional check I think we should remove now
See comment 26 for an explanation of the patch in comment 28.
Comment on attachment 493787 [details] [review] additional check I think we should remove now I think this needs review from roc or fantasai...
Comment on attachment 493787 [details] [review] additional check I think we should remove now I agree.
Did attachment 493787 [details] [review] ever land? Do we need a separate bug to track that?
(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.
Followup landed: http://hg.mozilla.org/mozilla-central/rev/d0e5fb03bae2
Comment on attachment 465948 [details] [review] fix for 1.9.2 (diff -w) r=dbaron
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3e38f463af2a http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7ee6d0744ad9
Crashtest: http://hg.mozilla.org/mozilla-central/rev/83185e96e3c3