removeChild of form's child does not clear form pointer of descendant controls

RESOLVED FIXED in mozilla0.9.7

Status

()

Core
DOM: Core & HTML
P2
normal
RESOLVED FIXED
17 years ago
10 years ago

People

(Reporter: Wim Roffel, Assigned: bz)

Tracking

Trunk
mozilla0.9.7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

17 years ago
If you add and remove inputs to a form and these inputs are packed in a <div>
then it will leave a phantom input field. This field is not displayed but it is
part of the forms elements array[]. You can assign values to it but if you try
to retrieve them it will return an empty string.

The following code demonstrates the bug. It contains a "run demo" button to get
a fast impression.

Some notes:
 - this happens only when the inputs are part of a div
 - if you have more then one field in the div one may remember its input
 - if you have more then one field in the div you will have the same number of
ghostfields
 - it does not make a difference whether you remove one or several children
right after each other. You get only one set of ghostfields.
 - this code is surrounded by a lot of instability. During testing I was
regularly thrown back to my homepage. Sometimes inserting an alert repaired this.

<Head>
<SCRIPT Language=JAVASCRIPT>
var fieldcount=0;

function createfield()
{
  var field1Elt = document.createElement("input");
  field1Elt.size = 20;
  var divElt = document.createElement("div");
  divElt.appendChild(field1Elt);
  divElt.appendChild(document.createElement("br"));
  document.MainForm.appendChild(divElt);
  fieldcount++;
}

function removefield()
{ if(fieldcount==0) return;
  MainFormElt =  document.getElementById("MainFormId");
  endchild = MainFormElt.lastChild;
  MainFormElt.removeChild(endchild);
  fieldcount--;
}

function fillfields()
{ for(j=1; j<=fieldcount; j++)
  { temp = "field"+j;
    document.MainForm.elements[j+3].value = temp;
  }
}

function rundemo()
{ for(i=1; i<=3; i++)
    createfield();
  fillfields();
  alert("This is how it should look: each field filled with its name");
  removefield();
  createfield();
  createfield();
  fillfields();
  alert("Note that 'field3' is now missing. It is assigned to a ghostelement
that remained after we removed one field and added two others.");
}

</SCRIPT>
</head>
<body>
<form name=MainForm id=MainFormId>
<input type=button onclick="rundemo()" value="Run Demo"><br>
<input type=button onclick="createfield()" value="Create Field"><br>
<input type=button onclick="removefield()" value="Remove Field"><br>
<input type=button onclick="fillfields()" value="Fill Fields"><br>
</form>
</body>

Comment 1

17 years ago
Created attachment 48408 [details]
testcase provided by the reporter as an attachment to make using it easier
alright... here is the problem.  removeChild() ends up calling
nsGenericHTMLContainerElement::RemoveChildAt()

This calls oldKid->SetParent(nsnull);

Here things get messy.  If the child being removed is a form element, all is
good.  The SetParent that gets called then is
nsGenericHTMLLeafFormElement::SetParent() which removes the form control from
the form's list of controls.

In the div case, however, we call nsGenericElement::SetParent() which just sets
the parent for the div and does not look at the contents of the div for form
controls...

So, what needs to happen:

We need something like  nsGenericHTMLContainerElement::ClearForm() which will
get called and do the right thing.  That is, it needs to pass on down to all
it's children that are containers calls to ClearForm() and all the children that
are form controls should get SetForm(nsnull) called on them.

Oh, and I'm seeing this with current linux debug builds, of course (where else
would I get all this juicy data?)
Status: UNCONFIRMED → NEW
Component: DOM Other → DOM Core
Ever confirmed: true
QA Contact: gerardok → stummala
Alright.  Throught the wonders of nsIContentIterator, this has a fairly simple
solution.  Attaching patch and taking bug.
Assignee: jst → bzbarsky
OS: Windows 98 → All
Hardware: PC → All
reviews?
Status: NEW → ASSIGNED
Keywords: patch, review
jst says we call SetParent(nsnull) on every element on tear-down.  That would
mean that this patch absolutely _kills_ performance.  Thoughts:

1)  Only iterate if we have a form as an ancestor.
2)  Have a bool arg to SetParent() that's true only in teardown and is false
    normally (this would be combined with #1 anyway).
3)  Come up with something clever and more complicated to solve this problem
    without iterating over all the descendants.
Comment on attachment 48493 [details] [diff] [review]
Patch

We need to do #1 anyway for correctness reasons.  Consider removing a <div> that contains a <form> and all its children....
Attachment #48493 - Flags: needs-work+
Created attachment 48544 [details] [diff] [review]
Faster, better, simpler, more correct patch.  Thanks for the idea, jst
Attachment #48493 - Attachment is obsolete: true
Attachment #48493 - Flags: needs-work+
Created attachment 48550 [details] [diff] [review]
Last patch still broke if a parent of the form was removed... This one should be better.
Comment on attachment 48550 [details] [diff] [review]
Last patch still broke if a parent of the form was removed... This one should be better.

Oh, fun.  With that last patch I can't submit comments to bugzilla.  "Product" is not defined.  I added a breakpoint in nsGenericHTMLContainerFormElement::SetDocument() and got this stack:

#0  nsGenericHTMLContainerFormElement::SetDocument (this=0x887fbf8, aDocument=0x0, 
    aDeep=1, aCompileEventHandlers=1) at nsGenericHTMLElement.cpp:4001
#1  0x4133755b in nsGenericElement::SetDocumentInChildrenOf (aContent=0x88852e0, 
    aDocument=0x0, aCompileEventHandlers=1) at nsGenericElement.cpp:1477
#2  0x413377d6 in nsGenericElement::SetDocument (this=0x88852e0, aDocument=0x0, aDeep=1, 
    aCompileEventHandlers=1) at nsGenericElement.cpp:1515
#3  0x411223c4 in nsGenericHTMLElement::SetDocument (this=0x88852e0, aDocument=0x0, 
    aDeep=1, aCompileEventHandlers=1) at nsGenericHTMLElement.cpp:1158
#4  0x4133755b in nsGenericElement::SetDocumentInChildrenOf (aContent=0x87d3088, 
    aDocument=0x0, aCompileEventHandlers=1) at nsGenericElement.cpp:1477
#5  0x413377d6 in nsGenericElement::SetDocument (this=0x87d3088, aDocument=0x0, aDeep=1, 
    aCompileEventHandlers=1) at nsGenericElement.cpp:1515
#6  0x411223c4 in nsGenericHTMLElement::SetDocument (this=0x87d3088, aDocument=0x0, 
    aDeep=1, aCompileEventHandlers=1) at nsGenericHTMLElement.cpp:1158
#7  0x4133755b in nsGenericElement::SetDocumentInChildrenOf (aContent=0x8900680, 
    aDocument=0x0, aCompileEventHandlers=1) at nsGenericElement.cpp:1477
#8  0x413377d6 in nsGenericElement::SetDocument (this=0x8900680, aDocument=0x0, aDeep=1, 
    aCompileEventHandlers=1) at nsGenericElement.cpp:1515
#9  0x411223c4 in nsGenericHTMLElement::SetDocument (this=0x8900680, aDocument=0x0, 
    aDeep=1, aCompileEventHandlers=1) at nsGenericHTMLElement.cpp:1158
#10 0x4133755b in nsGenericElement::SetDocumentInChildrenOf (aContent=0x87fafa0, 
    aDocument=0x0, aCompileEventHandlers=1) at nsGenericElement.cpp:1477
#11 0x413377d6 in nsGenericElement::SetDocument (this=0x87fafa0, aDocument=0x0, aDeep=1, 
    aCompileEventHandlers=1) at nsGenericElement.cpp:1515
#12 0x411223c4 in nsGenericHTMLElement::SetDocument (this=0x87fafa0, aDocument=0x0, 
    aDeep=1, aCompileEventHandlers=1) at nsGenericHTMLElement.cpp:1158
#13 0x4112b2b8 in nsGenericHTMLContainerElement::RemoveChildAt (this=0x889eb38, 
    aIndex=0, aNotify=1) at nsGenericHTMLElement.cpp:3861
#14 0x4119fb66 in SinkContext::DemoteContainer (this=0x87074f8, aNode=@0xbfffef38)
    at nsHTMLContentSink.cpp:1800
#15 0x411a52ad in HTMLContentSink::CloseForm (this=0x872ce00, aNode=@0xbfffef38)
    at nsHTMLContentSink.cpp:3259
#16 0x40c0e3a7 in CNavDTD::CloseForm (this=0x88eb338, aNode=0xbfffef38)
    at CNavDTD.cpp:3197
#17 0x40c0ec3e in CNavDTD::CloseContainer (this=0x88eb338, aNode=0xbfffef38, 
    aTarget=eHTMLTag_form, aClosedByStartTag=0) at CNavDTD.cpp:3491
#18 0x40c0bbb2 in CNavDTD::HandleEndToken (this=0x88eb338, aToken=0x8979708)
    at CNavDTD.cpp:1896

Worse yet, when the element is readded to the document it's not added as a child of the form, so it can't find the form...  As a result it does not get submitted properly.

Basically, DemoteContainer() removes a parent of the <select> from the doc and reinserts it outside the form.  In the process, it keeps the form element (if any) of the element it's demoting but not of that element's children.

This looks like it may take some work....
Attachment #48550 - Flags: needs-work+
Great.  From comments before DemoteContainer:

  // This method is called when a container is determined to be
  // non well-formed in the source content. Currently this can only
  // happen for forms, since the parser doesn't do fixup of forms.
  // The method makes the container a leaf and moves all the container's
  // children up a level to the container's parent.

So this method will take a malformed form and move all its children to its
parent.  It'll restore mForm for children of the form that are form controls but
any form controls which are indirect descendants of the form get no special
treatment.  Needless to say with my patch this breaks the form controls.  In
fact, any approach to fixing this should involve modifications to
DemoteContainer().

I'm getting tempted to add a aDemoting arg to SetDocument and have it be false
by default.  If it's true (which would only happen for calls from
DemoteContainer) we don't forget our form.
while that last patch works, it seems very kludgey.  A better method is probably
needed...
resummarizing to reflect what's really going on
Summary: appendChild/removeChild of form inputs with <div> leaves ghostfield → removeChild of form's child does not clear form pointer of descendant controls
Priority: -- → P3
Target Milestone: --- → mozilla1.0
*** Bug 111502 has been marked as a duplicate of this bug. ***
*** Bug 111502 has been marked as a duplicate of this bug. ***
Created attachment 59582 [details] [diff] [review]
Patch to really fix this
Attachment #48550 - Attachment is obsolete: true
Attachment #48588 - Attachment is obsolete: true
jst, jkeiser, would you review?  That last patch makes us pass the testcase in
this bug, I can submit to bugzilla fine, and it does not regress bug 109854.  :)
Priority: P3 → P2
Target Milestone: mozilla1.0 → mozilla0.9.7
Comment on attachment 59582 [details] [diff] [review]
Patch to really fix this

I'm a little queasy about using NS_ENSURE_SUCCESS() instead of if
(formContent).	Both NS_ENSURE_SUCCESS() checks seem redundant to me, if you
add if (formContent) ... functionally it looks good though.
good point.  The first NS_ENSURE_SUCCESS should just be an |if (formContent)|. 
Changed.

The second one is _not_ equivalent to the check for |doc|.  It's rather a check
that the doc value is null because the form is not in the document as opposed to
being null because some error occured.
Sounds good.  r=jkeiser for such a beast then :)
Comment on attachment 59582 [details] [diff] [review]
Patch to really fix this

sr=jst
Attachment #59582 - Flags: superreview+
Attachment #59582 - Flags: review+
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.