397007 - "ASSERTION: no element to return" and memory corruption with :before, -moz-column

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (may crash Firefox or make Firefox unstable)

Loading the testcase triggers two assertions and corrupts random memory, often leading to a crash later.

###!!! ASSERTION: no element to return: '!empty()', file /Users/jruderman/trunk/mozilla/layout/generic/nsLineBox.h, line 1247

###!!! ASSERTION: running past end: 'mCurrent != mListLink', file /Users/jruderman/trunk/mozilla/layout/base/../generic/nsLineBox.h, line 620

Comment 1

[Mats Palmgren (inactive)]

I'm also seeing (on Linux):
###!!! ASSERTION: ResolveBidi called on non-first continuation: '!GetPrevInFlow()', file nsBlockFrame.cpp, line 6627

The crash is caused by setting a bit in arbitrary memory
when |line| is an overflow line and |mLines| is empty:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.866&root=/cvsroot&mark=5225-5226#5197

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

* fix the assertion in comment 1.
* fix the crash.  (I'm guessing we don't need to set this bit
  in case the frame is in an overflow line.)
* remove an unused variable

Comment 3

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

 #ifdef IBMBIDI
-  ResolveBidi();
+  if (!GetPrevInFlow())
+    ResolveBidi();
 #endif // IBMBIDI

This is not the correct fix for this. Simon is working on a better fix in bug 394805.

+  if (!searchingOverflowList && line != mLines.front()) {

We should still set the bit when the frame is an overflow line. We just need to check against the right first-line.

Comment 4

[Mats Palmgren (inactive)]

Ok, how about the case when it's the first overflow line - do we want to
set the bit on the last line (if any) in mLines in that case?

Comment 5

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

Yes, actually. Good call.

Comment 6

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

Attachment: patch rev. 2

This crash is annoying me, so here's my stab at a patch.

Comment 7

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

I need review here, Mats

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 282242 [details] [diff] [review]
patch rev. 2

Looks good so far, but you forgot the case in comment 4.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Your patch + an added else-block for the first overflow line case.

Comment 10

[Mats Palmgren (inactive)]

mozilla/layout/generic/nsBlockFrame.cpp 	3.874
mozilla/layout/generic/nsBlockFrame.h 	3.252 

-> FIXED

Comment 11

[Samuel Sidler (old account; do not CC)]

v. Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007110610 Minefield/3.0a9pre

Comment 12

[Jesse Ruderman]

The testcase doesn't trigger any assertions or crashes on branch.

Comment 13

[Jesse Ruderman]

Crashtest checked in.