384649 - [FIXr]Memory corruption with MathML table, position: fixed, position: inherit

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Attachment: testcase (causes memory corruption / crashes)

Loading the testcase causes several of each of these assertions:

###!!! ASSERTION: invalid array index: 'i < Length()', file ../../dist/include/xpcom/nsTArray.h, line 318

###!!! ASSERTION: wrong display type on rowframe: 'NS_STYLE_DISPLAY_TABLE_ROW == childFrame->GetStyleDisplay()->mDisplay', file /Users/jruderman/trunk/mozilla/layout/tables/nsTableRowFrame.cpp, line 1287

It often leads to a random crash, either immediately or later.  I'm guessing this means the bug causes Firefox to corrupt a random part of its heap.

The symptoms are very similar to those in bug 372376.

Comment 1

[Jesse Ruderman]

Crashes in random places, with random addresses on top --> [sg:critical]

Comment 2

[Jesse Ruderman]

Attachment: stack traces for the assertions

Comment 3

[Boris Zbarsky [:bzbarsky]]

We have a nsMathMLmtrFrame which has a display:block mContent...  Specifically, it's the one for the <mtr>.

I assume that testcase is pretty minimal?

Comment 4

[Jesse Ruderman]

> I assume that testcase is pretty minimal?

I think so, but I've been proven wrong before.

Comment 5

[Bernd]

>ASSERTION: wrong display type on rowframe: 'NS_STYLE_DISPLAY_TABLE_ROW
This is a very serious assert, once you see this crashing is the rule not the exception. The assert usually indicates a frame construction problem which typically is the result of style resolution issue as one can see in bug 372376.
>It often leads to a random crash, either immediately or later.  

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, the frame tree is pretty busted.  See comment 3.  The question is how we got there...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Looks like a possible regression from bug 323656.  Hard to tell.  In any case, patch coming up.  The basic problem is using a different style parent for the initial style resolution and the style reresolve...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Getting the right parent style context up front helps

Comment 9

[Boris Zbarsky [:bzbarsky]]

This might not be as much of an issue on branch because of the style rules we have there... but I think we still want this fix on branch.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 271244 [details] [diff] [review]
Getting the right parent style context up front helps

r+sr=dbaron.  Sorry for the delay.

Do you think any other similar calls should be fixed?  I'm particularly suspicious of the ones in CreateFloatingLetterFrame, CreateLetterFrame, RemoveFloatingFirstLetterFrames, and RemoveFirstLetterFrames.  And maybe also the ones in PropagateScrollToViewport.

Comment 11

[Boris Zbarsky [:bzbarsky]]

CreateFloatingLetterFrame resolves the following style contexts:
  - Style context for the text inside the first-letter, parented to the 
    first-letter.  This is correct.
  - Style context for the text outside the first-letter, parented to the
    first-letter's parent context.  This is also correct.

CreateLetterFrame just does the first of those two, again correctly.

RemoveFloatingFirstLetterFrames resolves style for the text using the style parent of the first-letter.  Again, correct.

Same for RemoveFirstLetterFrames.

PropagateScrollToViewport uses the style resolved for the root element as the parent for the style resolved for a child of the root element.  That's correct as well.

In general, the issues arise when an anon box is incorrectly used as the style parent for an element or pseudo-element or text.  In all the cases above, that's not happening.

I just checked all the callers of ResolveStyleFor(), and the following look fishy:

* nsSplitterFrame::Init: this should be getting the right style parent
                         directly off its style context.
* nsMathMLmactionFrame::Init: same
* ReResolveStyleContext when reresolving the undisplayed content.  I'm just
  not sure how that works, given that it's keyed off the content, and various
  stacked anonymous boxes can share the same content pointer...  Do you happen
  to know?

For ResolveStyleForNonElement, the only issue I see is the ReResolveStyleContext one.

For ResolvePseudoStyleFor resolving style for pseudo-elements (which is where we might need to adjust the style context):
* nsBlockFrame::SetInitialChildList is covered by another patch you just
  reviewed.
* I think the tree style cache is ok.
* ReResolveStyleContext... I can't tell offhand.  :(
* Frameset should be ok
* Frame constructor is ok.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 271244 [details] [diff] [review]
Getting the right parent style context up front helps

Fixes a bug in MathML frame construction which leads to frametree corruption.  Fairly low risk, and we need this.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 271244 [details] [diff] [review]
Getting the right parent style context up front helps

Er, nevermind.  This is blocking+, so I'm set.  ;)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed.  Filed bug 390689 on comment 11.

Comment 15

[Jesse Ruderman]

I don't see any assertions or crashes on branch, so I'm making this bug report public.

Comment 16

[Jesse Ruderman]

Crashtest checked in.