[PATCH] crash in layout regression test

VERIFIED FIXED in mozilla0.9.9

Status

()

Core
Layout
--
critical
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Bernd, Assigned: karnaze (gone))

Tracking

({crash, regression})

Trunk
mozilla0.9.9
x86
Windows 98
crash, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

16 years ago
I dont understand how we can crash in a layout regression test, hmm may be I
understand it, but I don't think it is tolerable. Assigning the bug to waterson,
as he cares too about the regression tests and knows the blockreflow-state


before it crashes I see the following two assertions:

###!!! ASSERTION: SpaceManager should be set in nsBlockReflowState: 'mSpaceManag
er', file C:\MOZ_SOUR\MOZILLA\mozilla\layout\html\base\src\nsBlockReflowState.cp
p, line 87
###!!! ASSERTION: null pointer: 'aSpaceManager', file C:\MOZ_SOUR\MOZILLA\mozill
a\layout\html\base\src\nsBlockBandData.cpp, line 69 

I filed also two talkbacks:

TB2810493K
TB2810570Y
(Reporter)

Comment 1

16 years ago
Marc, this one is yours, as Chris is on sabbatical, didn't you write
documentation for this?? ;-)
Assignee: waterson → attinasi

Updated

16 years ago
Severity: major → critical
Keywords: crash
(Reporter)

Updated

16 years ago
Keywords: nsbeta1, regression
Marking nsbeta1+
Keywords: nsbeta1 → nsbeta1+
Target Milestone: --- → mozilla0.9.9

Updated

16 years ago
QA Contact: petersen → moied

Comment 3

16 years ago
I don't know what changed, but it happened sometime between 01/15 and 02/02

What I am seeing is that the <FORM> inside the <TABLE> is getting parented to
the TableOuterFrame in ConstructTableForeignFrame

  if (nsHTMLAtoms::form == tag.get()) {
    // A form doesn't get a psuedo frame parent, but it needs a frame.
    // if the parent is a table, put the form in the outer table frame
    // otherwise use aParentFrameIn as the parent
    nsCOMPtr<nsIAtom> frameType;
    aParentFrameIn->GetFrameType(getter_AddRefs(frameType));
    if (nsLayoutAtoms::tableFrame == frameType.get()) {
      aParentFrameIn->GetParent(&parentFrame);
    }
    else {
      parentFrame = aParentFrameIn;
    }
  }

but then it is also getting added to the child list aChildItems in
TableProcessChild and that ends up as the child list for the table frame.  So,
the form is getting put in the child list of both table frames.

If I prevent the formFrame from being added to the child list of the tableFrame
then the crash goes away.
Status: NEW → ASSIGNED

Comment 4

16 years ago
Created attachment 69159 [details] [diff] [review]
PATCH to prevent frame from being added to wrong parent's child list

A <FORM> in a <TABLE> gets a frame that is parented to the tableOuterFrame, not
the TableFrame, so a check is done to make sure that it is not added to the
child list of the TableFrame too.

Comment 5

16 years ago
I could not find the checkin(s) that caused the regression, but I put together a
patch to fix the problem anyway.  If anyone knows how the problem was introduced
that would be good to know, in any case, please review the patch too. Thanks.
Summary: crash in layout regression test → [PATCH] crash in layout regression test
Does this cause frames to be leaked?  (And if those frames have images in them,
that could leak the entire document...)
(Assignee)

Comment 7

16 years ago
Created attachment 69196 [details] [diff] [review]
patch to let the form keep its original parent

passed table regression tests
(Assignee)

Updated

16 years ago
Attachment #69159 - Attachment is obsolete: true

Comment 8

16 years ago
Chris, with your patch the form frame is never getting created. What exactly do
we _want_ to do with the form frame anyway?
(Assignee)

Comment 9

16 years ago
Oh, thanks for catching that. I should have done something like the code below 
in TableProcessChild. If the form frame is created by 
ConstructFrameByDisplayType, then I'll also need to make sure that an anonymous 
table cell doesn't get wrapped around it. So, the end result is that the form 
frame should get constructed with its natural parent and no anonymous frames in 
between. With the form frame having the table as its parent, there is some 
chance it will break the table code, but that should be fixed anyway. 

  nsCOMPtr<nsIAtom> tag;
  aChildContent->GetTag(*getter_AddRefs(tag));
  // A form doesn't get a psuedo frame parent, but it needs a frame, so just use 
     the current parent
  if (nsHTMLAtoms::form == tag.get()) {
    nsFrameItems items;
    rv = ConstructFrame(aPresShell, aPresContext, aState, aChildContent,         
                        aParentFrame, items);
    childFrame = items.childList;
  }
  else {
    rv = ConstructTableForeignFrame(aPresShell, aPresContext, aState,            
         aChildContent, aParentFrame, childStyleContext, aTableCreator,          
         aChildItems, childFrame, isPseudoParent);
  }    

Comment 10

16 years ago
I still do not understand why we want to change the parentage of the form frame
from the tableOuterFrame to the tableFrame.  Then again, I don't understand why
it was on the outer in the first place - but that is how it has been for a while
AFAICT.
(Assignee)

Comment 11

16 years ago
I don't think we should change the parent, so it has been wrong all along. I may 
have originally changed the parent to be the outer table to avoid an anonymous 
cell getting wrapped around it, but that can be prevented. Marc, I'm going to 
take the bug.
Assignee: attinasi → karnaze
Status: ASSIGNED → NEW
(Assignee)

Comment 12

16 years ago
By the way, the reason that I didn't catch the fact that my patch didn't create 
a form frame is because viewer's regression testing capability is broken.
Status: NEW → ASSIGNED
(Assignee)

Comment 13

16 years ago
Created attachment 69513 [details] [diff] [review]
revised patch in attachment #2 [details] [diff] [review]

The parser leaves the form inside the table. If there are form elements inside,
they get moved outside the table, so if the inner table frame doesn't reflow a
form frame child that is ok. The outer table only reflows captions and inner
tables, and now the innner table only reflows row groups and col groups.
(Assignee)

Updated

16 years ago
Attachment #69196 - Attachment is obsolete: true

Comment 14

16 years ago
Comment on attachment 69513 [details] [diff] [review]
revised patch in attachment #2 [details] [diff] [review]

r= alexsavulov
Attachment #69513 - Flags: review+

Comment 15

16 years ago
Comment on attachment 69513 [details] [diff] [review]
revised patch in attachment #2 [details] [diff] [review]

studly!  sr=attinasi
Attachment #69513 - Flags: superreview+
(Assignee)

Comment 16

16 years ago
The patch is in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 17

16 years ago
Verified fix checked in cvs (Rev 1.704)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.