Closed
Bug 576649
Opened 15 years ago
Closed 14 years ago
"ASSERTION: Invalid offset" and more with -moz-column, abs pos, huge letter-spacing
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla2.0b4
People
(Reporter: jruderman, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: assertion, testcase, Whiteboard: [sg:critical?][critsmash:patch])
Attachments
(7 files, 2 obsolete files)
|
257 bytes,
text/html
|
Details | |
|
19.67 KB,
text/html
|
Details | |
|
5.30 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
|
5.69 KB,
patch
|
Details | Diff | Splinter Review | |
|
5.12 KB,
patch
|
fantasai.bugs
:
review-
dbaron
:
review+
|
Details | Diff | Splinter Review |
|
5.39 KB,
patch
|
Details | Diff | Splinter Review | |
|
1.14 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
###!!! 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
| Assignee | ||
Comment 1•15 years ago
|
||
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.
| Assignee | ||
Comment 2•15 years ago
|
||
I think we need to do something like this to take care of
the continuations in overflow containers.
Comment 3•15 years ago
|
||
is the sg: severity right?
Assignee: nobody → matspal
Whiteboard: [sg:critical?]
Updated•15 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:patch]
| Assignee | ||
Comment 4•15 years ago
|
||
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
| Assignee | ||
Comment 5•15 years ago
|
||
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.
| Assignee | ||
Updated•15 years ago
|
Attachment #455922 -
Flags: review?(fantasai.bugs)
| Assignee | ||
Comment 6•15 years ago
|
||
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.
| Assignee | ||
Comment 7•15 years ago
|
||
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)
Comment 9•15 years ago
|
||
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.
Updated•15 years ago
|
blocking2.0: --- → final+
Comment 11•15 years ago
|
||
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+
| Assignee | ||
Updated•15 years ago
|
Attachment #461741 -
Attachment is obsolete: true
Attachment #461741 -
Flags: review?(fantasai.bugs)
| Assignee | ||
Comment 12•15 years ago
|
||
Updated according to review comments.
Attachment #455922 -
Attachment is obsolete: true
Attachment #463035 -
Flags: review?(bzbarsky)
Comment 13•15 years ago
|
||
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]
| Assignee | ||
Comment 15•15 years ago
|
||
| Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 16•15 years ago
|
||
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 17•15 years ago
|
||
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
Updated•15 years ago
|
Keywords: checkin-needed
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
| Assignee | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?][critsmash:patch][needs landing] → [sg:critical?][critsmash:patch][landed on trunk, needs updated branch patch]
| Assignee | ||
Comment 18•15 years ago
|
||
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)
| Assignee | ||
Comment 19•15 years ago
|
||
| Assignee | ||
Updated•15 years ago
|
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
Comment 20•15 years ago
|
||
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-
| Assignee | ||
Comment 21•15 years ago
|
||
Reopening to investigate the code in light of the last comment.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 22•15 years ago
|
||
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.
Updated•15 years ago
|
Whiteboard: [sg:critical?][critsmash:patch][landed on trunk] → [sg:critical?][critsmash:patch][pushed to trunk]
Updated•15 years ago
|
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).
Comment 24•15 years ago
|
||
Mats, progress here?
| Assignee | ||
Comment 25•15 years ago
|
||
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).
Updated•14 years ago
|
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•14 years ago
|
||
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: 15 years ago → 14 years ago
Resolution: --- → FIXED
Attachment #493787 -
Flags: review?(bzbarsky)
See comment 26 for an explanation of the patch in comment 28.
Comment 30•14 years ago
|
||
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+
Comment 32•14 years ago
|
||
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+
| Assignee | ||
Comment 33•14 years ago
|
||
(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.
Followup landed:
http://hg.mozilla.org/mozilla-central/rev/d0e5fb03bae2
| Assignee | ||
Updated•14 years ago
|
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+
| Assignee | ||
Comment 36•14 years ago
|
||
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]
Updated•14 years ago
|
Group: core-security
| Reporter | ||
Comment 37•14 years ago
|
||
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•