Closed Bug 98487 Opened 23 years ago Closed 23 years ago

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

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.7

People

(Reporter: wimroffel, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 4 obsolete files)

testcase provided by the reporter as an attachment to make using it easier
1.41 KB, text/html
Details
Patch to really fix this
1.74 KB, patch
jst
: review+
jst
: superreview+
Details | Diff | Splinter Review
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>
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
Attached patch Patch (obsolete) — — Splinter Review
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+
Attachment #48493 - Attachment is obsolete: true
Attachment #48493 - Flags: needs-work+
Attachment #48544 - Attachment is obsolete: true
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...
Depends on: 90756
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. ***
Depends on: 109854
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
Closed: 23 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: