404118 - Crash [@ nsPropertyTable::PropertyList::Equals] with <input type=file>, position: fixed

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded or closed)

The testcase makes Firefox (Mac trunk debug) crash 
[@ nsPropertyTable::PropertyList::Equals] dereferencing 0xddddde09.

Sometimes the crash is when loading the page, and sometimes the crash is when closing the page.

Comment 1

[Jesse Ruderman]

Before the crash, I see:

###!!! ASSERTION: reflow roots should never split: 'status == NS_FRAME_COMPLETE', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 6165

Comment 2

[Martijn Wargers (dead)]

See also bug 399994.

Comment 3

[Matthew Gregan [:kinetik]]

With the testcase attached, the button child frame of the file control ends up on the overflow list when we perform the second nsAreaFrame::Reflow in nsFileControlFrame::Reflow.  This causes a reflow status of NS_FRAME_NOT_COMPLETE|NS_FRAME_REFLOW_NEXTINFLOW to propagate up to PresShell::DoReflow, where the assertion fires.  The second reflow is being performed to deal with sizing the text frame in the case where the file control has been styled with a size.

The button only ends up on the overflow list with the style white-space:normal in the test case--without this, the default white-space style for the form control is nowrap (set in layout/style/forms.css).

The patch proposed in bug #363696 avoids this crash by removing the second nsAreaFrame::Reflow call from nsFielControlFrame::Reflow, meaning the button never ends up on the overflow list.

Comment 4

[Daniel Holbert [:dholbert]]

Be careful about clearing other flags when you commit changes to a bug page.  (Unless you intended to make those changes -- it didn't sound like it from your comment, so I'm assuming it was accidental)

I am:
 --> Resetting priority to P2
 --> Clearing 'assigned' flag (bug is currently 'assigned' to [e-mail].  You can reassign it to your own email address if you want to take the bug)

Can someone more powerful than I set the blocking1.9+ flag again?  (was cleared from blocking1.9+ to blocking1.9?)

Comment 5

[Daniel Holbert [:dholbert]]

Also:
 --> restoring OS to 'All'.

Comment 6

[Matthew Gregan [:kinetik]]

Except for assigning it to myself, touching the other flags was very much
accidental.  Sorry about that--I'll be more careful in future.

Setting assignee back to myself.

Comment 7

[Matthew Gregan [:kinetik]]

Trying to set assignee to myself again.

Comment 8

[Jesse Ruderman]

The testcase no longer crashes for me.  I guess Matthew's patch in bug 363696 fixed it.

Matthew, were there other issues here?  I can't tell from comment 3, probably because I don't know layout code at all ;)

Comment 9

[Matthew Gregan [:kinetik]]

The patch from bug 363696 fixes the problem by changing the behaviour such that the button frame doesn't end up on the overflow list after reflow.  If there are other ways to get into the same state that we used to with this testcase, we can still crash.

I'm not sure if these other ways exist yet.  I've been looking at this off and on trying to work it out, but haven't made much useful progress yet.

Comment 10

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

Sorry I should have looked closer at this before.

The code that we removed in bug 363696 had this:

-        nsSize txtAvailSize(aReflowState.availableWidth, aDesiredSize.height);
-        nsHTMLReflowState   txtKidReflowState(aPresContext,
-                                              *aReflowState.parentReflowState,
-                                              this, txtAvailSize);

In particular it was setting available height to aDesiredSize.height. That was really bogus, the available height should only be set in column or paginated situations where we can break vertically and move content to a new column or page. Setting a height constraint here was leading to the button being placed on the overflow list. Now that this bogus code has been removed, that can't happen anymore and this bug can't happen.

Comment 11

[Carsten Book [:Tomcat]]

verified fixed using the testcase and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008020601 Firefox/3.0b4pre - no crash on testcase

-> Verified fixed

Comment 12

[Jesse Ruderman]

No crash (or even assertions) on the 1.8 branch.

Comment 13

[Jesse Ruderman]

Crashtest checked in on trunk.