Closed Bug 576649 Opened 9 years ago Closed 9 years ago

"ASSERTION: Invalid offset" and more with -moz-column, abs pos, huge letter-spacing

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b4
Tracking Status
blocking2.0 --- final+
blocking1.9.2 --- .14+
status1.9.2 --- .14-fixed
blocking1.9.1 --- .17+
status1.9.1 --- .17-fixed

People

(Reporter: jruderman, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [sg:critical?][critsmash:patch])

Attachments

(7 files, 2 obsolete files)

Attached file 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
Attached file 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.
Attached patch wip1 (diff -w) (obsolete) — Splinter Review
I think we need to do something like this to take care of
the continuations in overflow containers.
is the sg: severity right?
Assignee: nobody → matspal
Whiteboard: [sg:critical?]
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:patch]
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.
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #455922 - Flags: review?(fantasai.bugs)
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.
Attached patch wip2 (diff -w) for 1.9.2 (obsolete) — Splinter Review
Attachment #461741 - Flags: review?(fantasai.bugs)
Comment on attachment 455922 [details] [diff] [review]
wip1 (diff -w)

Seems like more of a Boris thing --- style system.
Attachment #455922 - Flags: review?(fantasai.bugs) → review?(bzbarsky)
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.
blocking2.0: --- → final+
Comment on attachment 455922 [details] [diff] [review]
wip1 (diff -w)

OK, r=me with the changes from comment 10.
Attachment #455922 - Flags: review?(bzbarsky) → review+
Attachment #461741 - Attachment is obsolete: true
Attachment #461741 - Flags: review?(fantasai.bugs)
Attached patch wip2 (diff -w)Splinter Review
Updated according to review comments.
Attachment #455922 - Attachment is obsolete: true
Attachment #463035 - Flags: review?(bzbarsky)
Comment on attachment 463035 [details] [diff] [review]
wip2 (diff -w)

r=me
Attachment #463035 - Flags: review?(bzbarsky) → review+
I guess this is ready to land.
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?][critsmash:patch][needs landing]
Keywords: checkin-needed
Comment on attachment 464583 [details] [diff] [review]
[checked-in] wip2 (for trunk checkin)

http://hg.mozilla.org/mozilla-central/rev/735ff84db7b1
Attachment #464583 - Attachment description: wip2 (for trunk checkin) → [checked-in] wip2 (for trunk checkin)
Attachment #464583 - Attachment is private: true
Comment on attachment 464583 [details] [diff] [review]
[checked-in] wip2 (for trunk checkin)

http://hg.mozilla.org/mozilla-central/rev/735ff84db7b1
Attachment #464583 - Attachment is private: false
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][critsmash:patch][needs landing] → [sg:critical?][critsmash:patch][landed on trunk, needs updated branch patch]
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?
Attachment #465948 - Flags: review?(fantasai.bugs)
Attached patch fix for 1.9.2Splinter Review
status1.9.1: --- → ?
status1.9.2: --- → ?
Whiteboard: [sg:critical?][critsmash:patch][landed on trunk, needs updated branch patch] → [sg:critical?][critsmash:patch][landed on trunk]
Target Milestone: --- → mozilla2.0b4
Depends on: 587484
Comment on attachment 465948 [details] [diff] [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.
Attachment #465948 - Flags: review?(fantasai.bugs) → review-
Reopening to investigate the code in light of the last comment.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Whiteboard: [sg:critical?][critsmash:patch][landed on trunk] → [sg:critical?][critsmash:patch][pushed to trunk]
Attachment #465948 - Flags: review?(dbaron)
(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).
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
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.
Status: REOPENED → RESOLVED
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
See comment 26 for an explanation of the patch in comment 28.
Comment on attachment 493787 [details] [diff] [review]
additional check I think we should remove now

I think this needs review from roc or fantasai...
Attachment #493787 - Flags: review?(bzbarsky) → review?(roc)
Comment on attachment 493787 [details] [diff] [review]
additional check I think we should remove now

I agree.
Attachment #493787 - Flags: review?(roc) → review+
Did attachment 493787 [details] [diff] [review] ever land? Do we need a separate bug to track that?
blocking1.9.1: needed → .17+
blocking1.9.2: needed → .14+
(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] [diff] [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.
Whiteboard: [sg:critical?][critsmash:patch][pushed to trunk] → [sg:critical?][critsmash:patch][pushed to trunk][branch patch needs review dbaron]
Comment on attachment 465948 [details] [diff] [review]
fix for 1.9.2 (diff -w)

r=dbaron
Attachment #465948 - Flags: review?(dbaron) → review+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3e38f463af2a
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7ee6d0744ad9
Flags: in-testsuite?
Whiteboard: [sg:critical?][critsmash:patch][pushed to trunk][branch patch needs review dbaron] → [sg:critical?][critsmash:patch]
Group: core-security
Crashtest: http://hg.mozilla.org/mozilla-central/rev/83185e96e3c3
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.