395623 - (CVE-2008-5501) [FIX]"ASSERTION: not in child list" and crash with position:inherit, counters, :first-line

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers three assertions:

###!!! ASSERTION: not in child list: 'found', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 1767

###!!! ASSERTION: didn't find frame to delete: 'result', file /Users/jruderman/trunk/mozilla/layout/generic/nsAbsoluteContainingBlock.cpp, line 128

###!!! ASSERTION: no placeholder frame for out-of-flow frame: 'Not Reached', file /Users/jruderman/trunk/mozilla/layout/generic/nsFrame.cpp, line 5447

If I try to resize the Firefox window at this point, it often crashes 
[@ nsSpaceManager::CreateFrameInfo] [@ nsIFrame::GetStyleDisplay] dereferencing 0xddddddf5.  This happens most reliably if I launch Firefox with the testcase filename on the command line.

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK. This is the first-line chicken coming home to roost.

The problem is that we reparent the body's style context to the first-line, which means that since its position is "inherit" it would suddenly change position.... except that ReParentStyleContext assumes that no restyling will be needed as a result of the reparent.  To make a long story short, we end up with the body having a POSITION_ABSOLUTE and FLOAT_NONE, but it's on the float list.  Which is just all bad.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I tried hacking around this by making the first-line rule set position/float to inherit, but then the first-line ends up display:block, which causes no end of grief (blockframe tries to reflow it as a block, reflow state tries to treat it as a block, etc).

I really don't see a sane way to fix this in our arch.  :(  I would strongly suggest that we just back out first-line support until we can do it without security holes.  If ever.

Comment 3

[Jesse Ruderman]

I'd be fine with removing first-line support.  It's not worth the memory safety bugs (looks like we've had 5 others involving first-line before this one), and the use cases are less clear than for first-letter.

Comment 4

[David Baron :dbaron:]

I'd be fine with either:

 * removing :first-line, or

 * switching to the model in early drafts of css3-selectors, which the working group agreed was better, but which then wasn't implemented, where the :first-line and :first-letter pseudo-elements end up innermost (and potentially repeated if there are multiple elements in the line).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Another option

This basically implements hixie's proposal: reset props do not inherit from ::first-line.  The ReParentStyleContext changes are nice, but not strictly necessary.

The tests include tests for this bug, bug 380012, bug 367489, bug 322348, bug 360599, bug 350370, bug 364220, bug 390417, bug 368568, bug 368863, and bug 343293.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

Comment 7

[David Baron :dbaron:]

Comment on attachment 283305 [details] [diff] [review]
Merged to tip

r+sr=dbaron.

But you might want to comment a little more about the idea that ReParentStyleContext shouldn't need change hints (which was always my understanding, anyway), since it should never produce anything requiring frame-change (though we should enforce that better) and whenever it's used we're already going to reflow and repaint.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 283305 [details] [diff] [review]
Merged to tip

Requesting approval.  It would be great to get testing for this in beta, and to land bug 398803 too.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Boris, is this essential for M9?  Not sure what the risks/upsides are.

Comment 10

[Boris Zbarsky [:bzbarsky]]

It's probably not essential except insofar as we would like to get testing of the changes sooner than later.

The risk is that rendering of first-line will break somehow or that we'll introduce crashes.

The upside is that this fixes exploitable crashes.

At this stage in the game, I'd probably put this off for M10.

Comment 11

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

Comment on attachment 283305 [details] [diff] [review]
Merged to tip

a=drivers for after M9 freeze

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in, with the documentation changes dbaron suggested in comment 7.

Comment 13

[Martijn Wargers (dead)]

Verified fixed with current debug trunk build.
Fwiw, it also doesn't seem to crash on branch.