473410 - Crash [@ GetContentRect] with -moz-column, fieldset

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (crashes Firefox when loaded)

This testcase crashes today's nightly mozilla-central build:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090113 Minefield/3.2a1pre

Note: This bug's testcase is exactly attachment 356643 [details] from bug 472919; however, it has a different regression range from that bug's original testcase, so I'm splitting it into its own bug, per bug 472919 comment 6.

See also attachment 356639 [details], a slightly-less-reduced testcase with the same regression range as this one.

Regression range for this testcase:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008021304
Minefield/3.0b4pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008021404
Minefield/3.0b4pre
Bonsai link: http://tinyurl.com/9ew7se  -- maybe bug 416018? (cairo upgrade)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: backtrace for crash

Here's a backtrace for the crash on testcase 1.
(Note: this is the same as the backtrace I previously posted on bug 472919, attachment 356735 [details])

Also, here's a crash report for the crash:
http://crash-stats.mozilla.com/report/index/7357f444-e4e7-4eff-8d64-9dbad2090113?p=1

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> Bonsai link: http://tinyurl.com/9ew7se  -- maybe bug 416018? (cairo upgrade)

Nope -- I tested a few CVS checkouts from the regression range, and it looks like this was caused by bug 400813.

I get a crash with a CVS checkout taken just after bug 400813's update landed (2008-02-14 02:10), but if I back out the patch per the commands at http://tinyurl.com/9hxxxe, I get no crash.

Comment 3

[Daniel Holbert [:dholbert]]

The following assertion accompanies the crash, both in a debug build from today and in a debug build from just after bug 400813 landed.  The assertion does not appear before bug 400813 landed, though.
###!!! ASSERTION: frame was not removed from primary frame map before destruction or was readded to map after being removed: 'Not Reached', file nsFrameManager.cpp, line 711

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: workaround

(Note: I'm not intending this as an actual fix -- at this point it's just a demo of what's going on.)

This patch works around the crash.  It reverts one line from bug 400813's patch -- the line that caused this issue.

The workaround is just this:
-      if (psd->mHasNonemptyContent || preMode || foundLI) {
+      if ((psd->mX != psd->mLeftEdge) || preMode || foundLI) {

(where the +'d line is the restored code from before bug 400813 landed)

On this bug's testcase, we hit this line with psd->mX == 0 and psd->mLeftEdge == 0, but with psd->mHasNonemptyContent == 1.

So in current mozilla-central, the condition passes (and we assert and crash later on), but with the workaround, the condition fails (and we don't assert or crash).  So, the assertion+crash are triggered in part by running the code immediately below that if-check.

Comment 5

[Jesse Ruderman]

Nominating for blocking1.9.1 -- calling 0xdddddddd is especially bad.

Is the workaround appropriate for checkin as a stopgap, or did you only attach it for testing purposes?

Comment 6

[Daniel Holbert [:dholbert]]

I attached the workaround for testing / demonstrating-what's-going-on purposes.  I'm not sure if landing it would break other stuff (or regress parts of bug 400813).

Comment 7

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

(In reply to comment #3)
> ###!!! ASSERTION: frame was not removed from primary frame map before
> destruction or was readded to map after being removed: 'Not Reached', file
> nsFrameManager.cpp, line 711

That suggests this may be the same problem as bug 463350, although I didn't check the stack.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> That suggests this may be the same problem as bug 463350, although I didn't
> check the stack.

That bug regressed a few months earlier than this one, FWIW.  So while the crashes may be related, there's at least something slightly different going on here.

Comment 9

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

Fixed by checkin for bug 463350