Closed Bug 399940 Opened 13 years ago Closed 13 years ago

"ASSERTION: Invalid offset" with table and MathML

Categories

(Core :: MathML, defect, P2)

x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: jruderman, Assigned: roc)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase, Whiteboard: [dbaron-1.9:RsCs])

Attachments

(2 files, 1 obsolete file)

Attached file testcase
Loading the testcase triggers:

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /Users/jruderman/trunk/mozilla/gfx/thebes/src/gfxSkipChars.cpp, line 92

###!!! ASSERTION: Substring out of range: 'aStart + aLength <= mCharacterCount', file /Users/jruderman/trunk/mozilla/gfx/thebes/src/gfxFont.cpp, line 1435
Flags: blocking1.9?
Attached patch fix (obsolete) — Splinter Review
Boris, the problem you speculated about where we would reconstruct frames on ContentRemoved and then construct new frames on ContentInserted, resulting in frame doubling, came true in this bug.

This patch should prevent such things by having ContentRemoved tell us when it reconstructed, in which case we do not need to call ContentInserted at all.
Assignee: rbs → roc
Status: NEW → ASSIGNED
Attachment #285033 - Flags: superreview?(bzbarsky)
Attachment #285033 - Flags: review?(bzbarsky)
Comment on attachment 285033 [details] [diff] [review]
fix

Let's get a reftest in for this?

And I'm not sure what I think of the null default arg, but I guess the only other caller is presshell, right?
Attachment #285033 - Flags: superreview?(bzbarsky)
Attachment #285033 - Flags: superreview+
Attachment #285033 - Flags: review?(bzbarsky)
Attachment #285033 - Flags: review+
Blocks: 399946
Blocks: 397518
> Let's get a reftest in for this?

I'll make it part of the new crashtest framework when that's set up.

> And I'm not sure what I think of the null default arg, but I guess the only
> other caller is presshell, right?

I think so. I guess it'd be slightly simpler to make that arg mandatory so we don't need null checks; I'll do that.
Flags: blocking1.9? → blocking1.9+
Whiteboard: [needs approval/landing]
Blocks: 400904
Merging with bug 321402 by adding one more = PR_TRUE would also fix bug 401176.

But I wonder why you shouldn't remove the aInReinsertContent argument and also add an assignment to PR_TRUE in the one case where aInReinsertContent is checked.  (It's safe as-is because RecreateFramesForContent already called MaybeRecreateContainerForIBSplitterFrame, but it seems like the ReinsertContent codepath there could be buggy.)  Though I didn't investigate why that was originally added as a boolean rather than using this approach.
I can try that. Boris, does that make sense to you?
Comment on attachment 285033 [details] [diff] [review]
fix

a=drivers for after M9 freeze
Attachment #285033 - Flags: approval1.9? → approval1.9+
Whiteboard: [needs approval/landing] → [needs approval/landing][dbaron-1.9:RsCs]
Assignee: roc → rbs
Status: ASSIGNED → NEW
Assignee: rbs → roc
Blocks: 400445
Blocks: 324857
Flags: in-testsuite?
> I can try that. Boris, does that make sense to you?

So here's the current situation as I understand it:

We call ReinsertContent from WipeContainingBlock and ReframeContainingBlock.  [Note that the former should really be calling GetIBContainingBlockFor towards the end there instead of what it does, by the way.  We need a separate bug on that.]

WipeContainingBlock doesn't do that for the XUL things, though.  It calls RecreateFramesForContent.  Not sure what's up with that.

To remove aInReinsertContent, we basically need to make sure that we don't get into an infinite recursion and don't double-create content.

Setting the out param here to true when MaybeRecreateContainerForIBSplitterFrame returns true would handle the latter.

The former can only happen if we were to pass a special frame to ReinsertContent.  We shouldn't be doing that (and aren't now); adding an assert to this effect is a good idea.

1)  Not be special
2)  Not be a block child of a special frame
3)  Not be the only inline child of the last part of an {ib} split.
Er... Ignore the list at the end of comment 7.  It shouldn't be there.
Attached patch fix #2Splinter Review
Updated patch to all comments, except for asserting in ReinsertContent that the frame is not special. The problem is that we don't have a frame pointer here. I'm reluctant to call GetPrimaryFrameFor since that could modify the primary frame map and cause bugs to not show up in debug builds. Boris, any thoughts on that?
Attachment #285033 - Attachment is obsolete: true
Attachment #287641 - Flags: superreview?(bzbarsky)
Attachment #287641 - Flags: review?(bzbarsky)
Whiteboard: [needs approval/landing][dbaron-1.9:RsCs] → [needs review][dbaron-1.9:RsCs]
Comment on attachment 287641 [details] [diff] [review]
fix #2

Looks good.  I agree that we don't want to call GPFF here; I just forgot that we don't have a frame there...
Attachment #287641 - Flags: superreview?(bzbarsky)
Attachment #287641 - Flags: superreview+
Attachment #287641 - Flags: review?(bzbarsky)
Attachment #287641 - Flags: review+
Whiteboard: [needs review][dbaron-1.9:RsCs] → [needs landing][dbaron-1.9:RsCs]
Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1422; previous revision: 1.1421
done
Checking in layout/base/nsCSSFrameConstructor.h;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.h,v  <--  nsCSSFrameConstructor.h
new revision: 1.245; previous revision: 1.244
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.1067; previous revision: 3.1066
done
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing][dbaron-1.9:RsCs] → [dbaron-1.9:RsCs]
Target Milestone: --- → mozilla1.9 M10
I checked in the testcase as a crashtest.
Flags: in-testsuite? → in-testsuite+
verified fixed using the testcase from jesse and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre - no assertion--> Verified fixed
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.