399940 - "ASSERTION: Invalid offset" with table and MathML

Status: VERIFIED FIXED

(Core :: MathML, defect, P2)

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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.

Comment 2

[Boris Zbarsky [: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?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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.

Comment 4

[David Baron :dbaron:]

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can try that. Boris, does that make sense to you?

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 285033 [details] [diff] [review]
fix

a=drivers for after M9 freeze

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Er... Ignore the list at the end of comment 7.  It shouldn't be there.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix #2

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?

Comment 10

[Boris Zbarsky [:bzbarsky]]

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...

Comment 11

[Reed Loden [:reed]]

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

Comment 12

[Jesse Ruderman]

I checked in the testcase as a crashtest.

Comment 13

[Carsten Book [:Tomcat]]

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