70648 - Fieldsets reparent legend incorrectly

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[ckritzer (gone)]

Overview: It seems that using JS, DOM, & CSS to change a LEGEND tag's style
attribute hangs the application.

Steps to Reproduce:
1) Load testcase (see attachment)
2) Click on 'Help' button

Actual Results: Application hangs

Expected Results: The style info gets swapped smoothly and there is an audible
sigh of joy when the page changes the LEGEND text from 'Info' to 'Help' and back.



Okay, I was just kidding about the 'audible sigh' bit.  But not the smooth swapping.

Occurs on:
- Win98  2001-03-01-10-Commercial
- MacOS  2001-03-01-09-Commercial

Need to test: Linux bits.

Comment 1

[ckritzer (gone)]

Attachment: testcase

Comment 2

[ckritzer (gone)]

Also, this appears to be related to the recent split of layout.  I have tried
this test with pre-layout-split versions of tree builds, and the bug doesn't exist.

Comment 3

[karnaze (gone)]

Reassigning to jst.

Comment 4

[Johnny Stenback (:jst)]

This triggers an infinite loop in nsBlockFrame::DoRemoveFrame(), I know next to
nothing about that code, Mark, could you have a look?

Comment 5

[Marc Attinasi]

I'll take a look...

Comment 6

[karnaze (gone)]

Moving to m0.9.1.

Comment 7

[Andreas Franke (gone)]

I think this is a duplicate of bug 62049 "toggling display block/none on tables
causes browser to freeze". The infinite loop in nsBlockFrame::DoRemoveFrame() is
mentioned there, too.

Comment 8

[Marc Attinasi]

This is a dup of bug 62049 as Andreas suggested. This one is actually the more
general bug, however they are the same problem. The same simple code change
fixes the crash for this testcase and the one in bug 62049, but not the
underlying problem.

Marking bug 62049 as a dup of this bug: the testcase in 62049 is very useful too.

Comment 9

[Marc Attinasi]

*** Bug 62049 has been marked as a duplicate of this bug. ***

Comment 10

[Marc Attinasi]

--> P1

Comment 11

[Brian Duff]

*** Bug 77121 has been marked as a duplicate of this bug. ***

Comment 12

[Marc Attinasi]

*** Bug 73587 has been marked as a duplicate of this bug. ***

Comment 13

[Marc Attinasi]

Attachment: More reduced testcase: legend in a fieldset being set to display:none hangs app

Comment 14

[Marc Attinasi]

CC'ing RodS since fieldsets are his domain (I think). I tried to repro this with 
a simpler set of tags, but setting a span inside of a div to display:none does 
not cause this. Probably special to fieldsets.

Comment 15

[Marc Attinasi]

OK, nsCSSFrameConstructor::ConstructFieldSetFrame clearly does some strange 
things with the legend frame. It looks like the legend is take out of the normal 
 childlist and set as a child of the FieldsetFrame. The RemoveFrame method on 
the FieldSet probably needs to check for the legendFrame and treat is special.

Comment 16

[Marc Attinasi]

Attachment: Work In PRogress: patch that fixes fieldsetframe::RemoveFrame to handle legend frame

Comment 17

[Marc Attinasi]

I attached a patch that handles the RemoveFrame for a legend frame, but it is 
not clear to me who owns the legend frame, and if my change will cause a leak. 
Needs more investigation, but I'm tired.

Comment 18

[rods (gone)]

I think this HTML is invalid, there are two legend tags and a field set can only 
have one. 

Comment 19

[Jacob Kjome]

try this testcase attached to bug 62049 which was marked a duplicate to this 
bug...I believe that is valid html....xhtml actually.

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=20241

Jake

Comment 20

[Marc Attinasi]

The patch I attached fixes the hang, but there is still an assertion that
happens afterwards (subsequent clicking of the Help/Info button in the testcase
causes it). I'd like to checkin the patch I have, and then let Rod fix the
remainder of the problem with the dubious parenting of the legend frame. Plan?

Oh, here is the assertion I get:

###!!! ASSERTION: messed up delete code: 'flow == parent', file
nsBlockFrame.cpp, line 5613

Comment 21

[Marc Attinasi]

Attachment: Updated patch: fixes hang but leaves lingering assertion

Comment 22

[Marc Attinasi]

Could Rod (or someone else) please review the attached patch (id=33439)? Thanks, 
I'd like to get it in while Rod figures out the parenting issues since it 
prevents the hang.

Comment 23

[Marc Attinasi]

--> patch

Comment 24

[rods (gone)]

Please add some comments at the top and indicate the Bug # and that it is a 
temporary fix. Thanks r=rods

Comment 25

[Marc Attinasi]

Will do, thanks Rod.

Comment 26

[Chris Waterson]

what rod said, sr=waterson

Comment 27

[Marc Attinasi]

Temporary fix checked in - the rest if for you, Rod (you might want to update 
the summary?).

Comment 28

[Jacob Kjome]

The current patch does not fix the hang in bug 62049.  Mozilla still freezes up 
and doesn't respond to anything.  Have to kill it after trying the following 
testcase (already attached to mentioned bug):

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=20241

1. Try the top one and click where it says "Click Me".
2. Click it two more times
3. Mozilla should now be hung

If bug 62049 is truly a duplicate of this bug, then the current patch should 
fix the hang in the case described in 62049.  Since it doesn't, either the 
patch needs to be updated to handle that case or bug 62049 needs to have its 
status change to something other than duplicate.

Jake

Comment 29

[rods (gone)]

Moving to Future

Comment 30

[Jacob Kjome]

Attachment: testcase showing toggling of legend display value

This corrects a number of html validation issues in the first test and shows
that the initial display value for a legend is "" where, I would think, it
should be "inline".  It also seems that toggling the display value of the
legend tag multiple times at a rapid rate can crash Mozilla.

Talkback ID#:  TB5613460M

Jake

Comment 31

[Jacob Kjome]

It seems that the testcase mentioned in comment 28 no longer freezes Mozilla.  
There is other weird behavior, but that is a different bug.

I seem to me that this bug is keeping us from doing any sort of dhtml with 
fieldsets because for one, it can crash the browser, and secondly it makes all 
sorts of weird painting problems.  See the testcase I attached and play with 
the buttons to see what I mean.  Also, if new elements are added within a 
fieldset by the DOM, the Fieldset gets messed up.  For instance, if a textfield 
is added by the DOM within a fieldset and a focus() is called on the added 
textfield, the cursor will sit blinking on top of the Legend.

Can this bug get some attention soon?  Fieldsets are really nice to be able to 
use, but they are worthless when using DHTML in Mozilla right now.

Jake

Comment 32

[Christopher Hoess (gone)]

This WFM with a fresh Linux CVS trunk build, even with vigorous thrashing: no assertion, hang, or painting problems. Could someone please confirm this works? I'm pretty sure jkeiser changed a lot of fieldset/legend stuff since the last comment.

Comment 33

[John Keiser (jkeiser)]

attachment 81030 [details] does not crash for me, but it does display weird painting
behaviors when I toggle back and forth with the Help button.

Comment 34

[Mats Palmgren (inactive)]

The remaining problems was fixed by 276104