Steps to reproduce: (1) File a new bug in Bugzilla. Stack trace: ntdll.dll!7c901230() [Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll] xpcom_core.dll!Break(const char * aMsg=0x0012e668) Line 471 C++ xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=0x00000001, const char * aStr=0x0174dd3c, const char * aExpr=0x0174dcfc, const char * aFile=0x0174dcb8, int aLine=0x0000090b) Line 350 + 0xc bytes C++ > gklayout.dll!nsFormControlList::GetSortedControls(nsTArray<nsIFormControl *> & aControls={...}) Line 2315 + 0x49 bytes C++ gklayout.dll!nsHTMLFormElement::WalkFormElements(nsIFormSubmission * aFormSubmission=0x051e53d8, nsIContent * aSubmitElement=0x05169c88) Line 1170 + 0x16 bytes C++ gklayout.dll!nsHTMLFormElement::BuildSubmission(nsCOMPtr<nsIFormSubmission> & aFormSubmission={...}, nsEvent * aEvent=0x0012ece8) Line 947 + 0x15 bytes C++ gklayout.dll!nsHTMLFormElement::DoSubmit(nsEvent * aEvent=0x0012ece8) Line 896 C++ gklayout.dll!nsHTMLFormElement::DoSubmitOrReset(nsEvent * aEvent=0x0012ece8, int aMessage=0x000004b0) Line 843 + 0xc bytes C++ gklayout.dll!nsHTMLFormElement::PostHandleEvent(nsEventChainPostVisitor & aVisitor={...}) Line 803 C++ gklayout.dll!nsEventTargetChainItem::PostHandleEvent(nsEventChainPostVisitor & aVisitor={...}) Line 369 + 0x15 bytes C++ gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=0x00000206, nsDispatchingCallback * aCallback=0x00000000) Line 438 C++ gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=0x00000006, nsDispatchingCallback * aCallback=0x00000000) Line 486 C++ gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x04ac7778, nsPresContext * aPresContext=0x05041f98, nsEvent * aEvent=0x0012ece8, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012ed14, nsDispatchingCallback * aCallback=0x00000000, int aTargetIsChromeHandler=0x00000000) Line 639 + 0x12 bytes C++ gklayout.dll!PresShell::HandleDOMEventWithTarget(nsIContent * aTargetContent=0x04ac7778, nsEvent * aEvent=0x0012ece8, nsEventStatus * aStatus=0x0012ed14) Line 6306 + 0x1e bytes C++ gklayout.dll!nsHTMLInputElement::PostHandleEvent(nsEventChainPostVisitor & aVisitor={...}) Line 1742 C++ gklayout.dll!nsEventTargetChainItem::PostHandleEvent(nsEventChainPostVisitor & aVisitor={...}) Line 369 + 0x15 bytes C++ gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=0x00000206, nsDispatchingCallback * aCallback=0x0012f02c) Line 438 C++ gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=0x00000006, nsDispatchingCallback * aCallback=0x0012f02c) Line 486 C++ gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x05169c88, nsPresContext * aPresContext=0x05041f98, nsEvent * aEvent=0x0012f0f8, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f56c, nsDispatchingCallback * aCallback=0x0012f02c, int aTargetIsChromeHandler=0x00000000) Line 639 + 0x12 bytes C++ gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0012f0f8, nsIView * aView=0x00000000, nsEventStatus * aStatus=0x0012f56c) Line 6263 + 0x2b bytes C++ gklayout.dll!PresShell::HandleEventWithTarget(nsEvent * aEvent=0x0012f0f8, nsIFrame * aFrame=0x05170110, nsIContent * aContent=0x05169c88, nsEventStatus * aStatus=0x0012f56c) Line 6155 + 0x12 bytes C++ gklayout.dll!nsEventStateManager::CheckForAndDispatchClick(nsPresContext * aPresContext=0x05041f98, nsMouseEvent * aEvent=0x0012f7a8, nsEventStatus * aStatus=0x0012f56c) Line 3230 + 0x45 bytes C++ gklayout.dll!nsEventStateManager::PostHandleEvent(nsPresContext * aPresContext=0x05041f98, nsEvent * aEvent=0x0012f7a8, nsIFrame * aTargetFrame=0x05170110, nsEventStatus * aStatus=0x0012f56c, nsIView * aView=0x050a8be0) Line 2194 + 0x1c bytes C++ gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0012f7a8, nsIView * aView=0x050a8be0, nsEventStatus * aStatus=0x0012f56c) Line 6281 + 0x36 bytes C++ gklayout.dll!PresShell::HandlePositionedEvent(nsIView * aView=0x050a8be0, nsIFrame * aTargetFrame=0x05170110, nsGUIEvent * aEvent=0x0012f7a8, nsEventStatus * aEventStatus=0x0012f56c) Line 6138 + 0x14 bytes C++ gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x050a8be0, nsGUIEvent * aEvent=0x0012f7a8, nsEventStatus * aEventStatus=0x0012f56c) Line 5966 + 0x1b bytes C++ gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x050a8be0, nsPoint aPoint={...}, nsGUIEvent * aEvent=0x0012f7a8, int aCaptured=0x00000000) Line 1668 C++ gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0012f7a8, nsEventStatus * aStatus=0x0012f694) Line 1621 + 0x22 bytes C++ gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012f7a8) Line 174 C++ gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012f7a8, nsEventStatus & aStatus=nsEventStatus_eIgnore) Line 1108 + 0xc bytes C++ gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0012f7a8) Line 1129 C++ gkwidget.dll!nsWindow::DispatchMouseEvent(unsigned int aEventType=0x0000012d, unsigned int wParam=0x00000000, long lParam=0x02f900a6) Line 6110 + 0x1a bytes C++ gkwidget.dll!ChildWindow::DispatchMouseEvent(unsigned int aEventType=0x0000012d, unsigned int wParam=0x00000000, long lParam=0x02f900a6) Line 6293 C++ gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=0x00000202, unsigned int wParam=0x00000000, long lParam=0x02f900a6, long * aRetValue=0x0012fc18) Line 4574 + 0x20 bytes C++ gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x00080708, unsigned int msg=0x00000202, unsigned int wParam=0x00000000, long lParam=0x02f900a6) Line 1297 + 0x1d bytes C++ user32.dll!77d48734() user32.dll!77d48816() user32.dll!77d489cd() user32.dll!77d49402() user32.dll!77d48a10() gkwidget.dll!nsAppShell::ProcessNextNativeEvent(int mayWait=0x00000001) Line 149 C++ gkwidget.dll!nsBaseAppShell::DoProcessNextNativeEvent(int mayWait=0x00000001) Line 136 + 0x11 bytes C++ gkwidget.dll!nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal * thr=0x00398e38, int mayWait=0x00000001, unsigned int recursionDepth=0x00000000) Line 231 + 0xf bytes C++ xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x0012fe04) Line 472 C++ xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00398e38, int mayWait=0x00000001) Line 225 + 0x16 bytes C++ gkwidget.dll!nsBaseAppShell::Run() Line 153 + 0xc bytes C++ appcomps.dll!nsAppStartup::Run() Line 219 C++ seamonkey.exe!main1(int argc=0x00000001, char * * argv=0x00394560, nsISupports * nativeApp=0x0095e930) Line 1239 + 0x22 bytes C++ seamonkey.exe!main(int argc=0x00000001, char * * argv=0x00394560) Line 1741 + 0x25 bytes C++ seamonkey.exe!__tmainCRTStartup() Line 586 + 0x19 bytes C seamonkey.exe!mainCRTStartup() Line 403 C kernel32.dll!7c816fd7() I hit this assertion on bugzilla.mozilla.org, but did not hit the assertion on http://landfill.bugzilla.org . Reproducible: I'm about to find out. :)

Yes, this bug is reproducible on bmo. No idea why I couldn't reproduce on landfill.

Okay, this bug is reproducible on the Bugzilla 2.20 branch. http://landfill.bugzilla.org/bugzilla-2.20-branch/

Thanks for figuring out an easy way to reproduce this Alex. I've got it up in the debugger, I'll try and get this figured out tonight.

Here's the offending control: <input type="hidden" name="bug_status" value="NEW"> It comes after a select: <select name="target_milestone">

Hm, very interesting. In DOM Inspector, the hidden input is a direct child node of the form element. In the source code, it's a direct child of a tr element...

I found the same offending control. I'll upload a minimal testcase in moment. I also found if the hidden control was wrapped with <td> tags the assertion went away.

So is the problem that the order of mInElements is not actually document order in this case or something? Do current builds submit the same thing as builds from before bug 347165 landed? Might want to use a GET action to test that.

The submission order is unchanged, just checked a build from 9/16 and confirmed with a beta 2 build. Both select and input type hidden should be in mElements, so we should just be doing a straight copy here to the submission list. I'll check if mElements isn't in document order.

mElements is not in document order. Here's how that happens: - During parsing, the select is parsed before the hidden control so it is added to the form first. - addElement is called for the hidden control. The parent of the hidden control is null, so we assume the controls are at equal positions and add it after the select. - The controls are parented, and the hidden control ends up as a direct child of the form, before the select. It seems like the only way to get this right would be to make sure the controls have parents before they are added to the form. bug 267511 stopped this case from crashing but didn't fix the lack of a parent.

So the problem here is that both Opera and IE (and current Gecko) submit the <select> before the hidden input. This is NOT DOM order in Gecko, but might be in the other browsers... So while we could switch mElements to be completely in DOM order, that would change our behavior, which is probably undesirable in this case. :( On the other hand, having mElements not be in DOM order means that a bunch of the nsHTMLFormElement code is wrong, since it pretty explicitly assumes DOM order there. The problem, of course, is that this is one of those cases when IE creates a non-tree DOM.... If you make the input a text input instead of a hidden input, you will see that the text input renders before the table, but submits after the <select> in IE, Gecko, Opera. :( sicking, peterv, jst, any ideas on what we can do here?

Ouch, this sucks. I bet that if we change the submission order then we will break a pile of sites. Would it be possible to make sure that we don't move form controls when we move stuff to before the table? Would that screw up rendering even if the un-moved control is a type=hidden?

Not moving type=hidden won't screw up the rendering, but using type=text will just cause the same issue...

Yes, but I wonder if that is as common. Though I guess there's no reason for it not to be. I could see two solutions here: A) We change the order in mInElements such that it matches the DOM order and hope it doesn't break too many pages. It should after all hopefully be rare both to insert stuff in the wrong place in tables and at the same time depend on the exact ordering of submission. B) We learn to deal with mInElements being out-of-order Of course, there's also C) Do fixup of missplaced content only during rendering rather than mucking around with the DOM However I suspect that would be a lot harder than any other option, though it would simplify the html sink and parser a bit.

I'd be kinda interested in trying #1, I think.... Trunk and all, right?

So the problem then is that we call SetForm before we BindToTree as comment 10 points out... Perhaps we should just move the AddElement call from SetForm to BindToTree or something?

Yeah, that seems like a good idea.

Hopefully nobody is relying on the exact order of form.elements for these cases either. This could increase parsing time slightly as before we were skipping the DOM position comparisons because the new input didn't have a parent. The code is already optimized for the new element being last though, so it shouldn't be too bad.

This just switches the order of adding the form control to its parent and calling setForm. Seems to work, I need to do a lot more testing though.

Also while I remember, it might be worth a follow up bug to fix SetForm to take a nsIForm directly. It gets called a lot, and its callers QI the nsIForm to nsIDOMHTMLFormElement and then SetForm QIs it back.

I think we want the MaybeSetForm before adding to the parent. Otherwise adding to the parent will start walking the DOM looking for the form, which is just wasted cycles. What we want to do here is to set mForm but not actually call AddElement. Then call AddElement in BindToTree. So basically refactor the SetForm/FindAndSetForm/BindToTree interactions a bit.

Maybe even just add another flag on the node that indicates whether it called AddElement and a boolean flag to SetForm to say not to call AddElement. Or something like that. This is based on like 10 seconds thought, so please take it with a grain of salt.

Ah, now I understand. I've got this working using bz's approach(w/o a flag yet), patch coming once I am comfortable that I have enough test cases, including perf.

This works, but causes a perf hit.

Parses a form containing a table with many controls.

trunk: Number of submit elements Time 10 1.2 100 6 1000 98.74 w/ patch applied Number of submit elements Time 10 1.6 100 6.02 1000 120.36 I'd guess this is simply due to the DOM position comparisons that are no longer skipped. I'm going to profile it and see what that shows.

Profiling confirmed the extra overhead is in nsLayoutUtils::DoCompareTreePosition

I figured out that I was not detecting the case where BindToTree was called multiple times early enough, and that was causing most of the performance hit. With this patch I'm not seeing much difference in times for the attached testcase.

Comment on attachment 240460 [details] [diff] [review] fix_order_v3 >? content/svg/content/src/nsSVGGenericStringValue.cpp What's this? :) (By the way, this isn't a review; I'm still learning the details of C++ hacking in Mozilla.) >+ return nsLayoutUtils::CompareTreePosition(content1, content2, >+ NS_CONST_CAST(nsIContent*, aForm)); Peanut gallery question, which I'm sure the DOM peers can answer: why is this function not using nsContentUtils::CompareDocumentPosition? (Efficiency, maybe, or something to do with the form?) > NS_IMETHODIMP > nsHTMLFormElement::AddElement(nsIFormControl* aChild, > PRBool aNotify) > { >+#ifdef DEBUG >+ { >+ nsCOMPtr<nsIContent> content = do_QueryInterface(aChild); >+ NS_ASSERTION(content->GetParent(), >+ "Form control should have a parent"); >+ } >+#endif Is there any need to check on aChild here, whether it's non-null or that the QI here succeeds? >+ NS_ASSERTION(CheckDocumentOrder(controls, this), >+ "Form controls out of order"); I worry about having the assertion outside CheckDocumentOrder for one reason. The old code at least gave a hint which control was out of order. May I suggest returning some value to indicate where the check failed, perhaps as an out param?

> Why is this function not using nsContentUtils::CompareDocumentPosition? This code predates the existence of nsContentUtils::ComparePosition (back then the nsContentUtils code was really slow and COMy and not usable here). That said, we should file a followup bug on only having one content tree position function, I think... Having two copies of that code is silly. > Is there any need to check on aChild here No. If desired, we can add an assert that aChild is non-null.

> The old code at least gave a hint which control was out of order. This is probably a good catch. I'd just name the function AssertDocumentOrder and assert right in the function before returning (make the function return void); that way a break on the assert would drop one into the debugger in the right spot... I'm out of town this weekend, and totally swamped with classes having just started, so I probably won't get to this review until a week from today. It's near the top of my list, though. And yeah, please file a followup bug on having SetForm take an nsIForm.

(In reply to comment #30) Extra review comments are always welcome! > (From update of attachment 240460 [details] [diff] [review] [edit]) > >? content/svg/content/src/nsSVGGenericStringValue.cpp > > What's this? :) Just a version control goof up. I'm not sure why that file is hanging around. > Is there any need to check on aChild here, whether it's non-null or that the QI > here succeeds? I often don't assert things aren't null because the dereference will lead to an obvious crash. I'm not opposed to it though. > That said, we should file a followup bug on only having one content tree > position function, I think... Having two copies of that code is silly. I ran into this function and was confused as well. I'll follow a file up bug. > This is probably a good catch. I'd just name the function AssertDocumentOrder Sure, I'll do this. It is very helpful to see which control is out of order. No need to hurry on the review, thanks for doing it! I'll come up with more test cases in the meantime.

Comment on attachment 240460 [details] [diff] [review] fix_order_v3 Canceling the review request, I wrote a form fuzz tester and it found a few problems with this patch. New patch once I sort those out.

We should probably add that fuzzer to some sort of regularly scheduled fuzz runs (and maybe check it into CVS).

Sure, I'm still working on improving it a bit but it does the job. In good news, I couldn't get it to crash the trunk at all. I did get at least one warning that I'll file a follow up bug on. I figured out that the assertions that were firing with the patch applied were actually when the form was being torn down and the elements are being unbound. It looks like the elements list can be out of order at that point, and children may not have a parent. The patch added assertions that caught those existing invariant violations. I'll look into it further to figure out exactly what's going on.

So it looks like a form control can legitimately have a null parent when the parent form is unbound from the tree. I think it's safe to remove the new error checking from removeElement, as long as SetForm is always called eventually with the new parent form, which it should be. I'll spin up a new patch with that change.

Comment on attachment 241404 [details] [diff] [review] fix_order_v3 >Index: content/html/content/src/nsGenericHTMLElement.cpp >@@ -3107,28 +3092,50 @@ nsGenericHTMLFormElement::BindToTree(nsI >+ if (mForm) { So consider the following scenario (all done via DOM APIs): 1) I create a form 2) I create an input 3) I append the input to the form 4) I append the form into a document At step 3 we would get into this code, find the form via FindForm, and set mForm. Then we would call AddElement on the form. At step 4, we would get into BindToTree, already have an mForm, and call AddElement on the form _again_. I realize you added code to AddElement to deal with this, but that code makes inserting a form with N kids into the DOM O(N^2), which seems undesirable. I think we should add a boolean member to nsGenericHTMLFormElement to indicate whether we've called AddElement since the last time SetForm() happened, more or less as I suggested in comment 23. You can probably use a bit in the node's flags for this, I'd think. Just need to be sure to take NODE_TYPE_SPECIFIC_BITS_OFFSET into account when deciding on the bit value. Might want to run that part of the patch by sicking. Everything else looks good!

Avoiding that O(n^2) behavior certainly is desirable. I'll fix that and attach another patch.

Test case for the situation bz described in comment 40. Performance is the same on the trunk, I'm building to test with patch applied now.

Patch using a flag as bz suggested to avoid nasty performance problem. I'll request review once I test this a bit more. I haven't run the regression tests yet as my linux box is waiting for a new hard drive to arrive.

Comment on attachment 244775 [details] [diff] [review] fix_order_v4 >+#ifdef DEBUG >+static void >+AssertDocumentOrder(const nsTArray<nsIFormControl*>& aControls, >+ const nsIContent* aForm) >+{ >+ if (!aControls.IsEmpty()) { >+ for (PRUint32 i = 0; i < aControls.Length() - 1; ++i) { >+ NS_ASSERTION(CompareFormControlPosition(aControls[i], aControls[i + 1], >+ aForm) < 0, "Form controls not ordered correctly"); >+ } >+ } >+ } >+#endif I actually just had another idea that might be more efficient (or less - I don't know). Suppose you had a tree walker with a root of aForm, accepting all form controls in the form (or just all elements, no node filter needed). Then, for each call to getNextNode(), you check which member of aControls it is. Since the tree walker goes in document order, you should get a nice sequence (0, 1, 2, 3...)

> I actually just had another idea that might be more efficient This is a debug-only assertion. The main consideration is that it be simple and correct. Let's not get treewalker involved here.

Wanted, but not a blocker.

Comment on attachment 244775 [details] [diff] [review] fix_order_v4 >Index: content/html/content/src/nsGenericHTMLElement.cpp >@@ -3102,28 +3092,51 @@ nsGenericHTMLFormElement::BindToTree(nsI >+ mForm->AddElement(this, PR_TRUE); We really don't want to notify here when coming from the sink. Luckily, we can detect that: that's the case when mForm was already non-null coming into this function, but the bit was not set. >Index: content/html/content/src/nsGenericHTMLElement.h >+#define PARENT_FORM_SET 1 << NODE_TYPE_SPECIFIC_BITS_OFFSET This should probably just be in the C++ file. Also, I think NEED_TO_ADD_TO_FORM is a better name for this flag, and it could use some documentation. >Index: content/html/content/src/nsHTMLFormElement.cpp >+ * Checks that all form elements are in document order. Throws an assertion "Asserts" >+static PRInt32 CompareFormControlPosition(nsIFormControl *aControl1, >+ nsIFormControl *aControl2, >+ const nsIContent* aForm) I think this should take an nsIContent* and push the const casts out to the few consumers that need them. r+sr=bzbarsky with those nits addressed.

I meant that the flag should be called ADDED_TO_FORM. Like I said, documentation!

Fix checked in. We could still use some tests for this stuff.

The start of my work to create a large number of form test cases. LOTS more are needed still.

Checked in those tests, with the following changes to make them pass: * content/html/content/test/test_bug353415-1.html - Add an explicit wait/finish pair to make sure we don't finish too early - Fix the expected value of field7 to be 2, not 1 * content/html/content/test/test_bug332893-6.html - Change the expected value of form1.elements["input"].length to be 3 - Fix the test for form1.elements["input"][0].id to expect the right value ("input", not "input2"). and for all tests the Windows newlines removed, <!DOCTYPE html> added, and the xmlns stuff removed.

I added a test for this bug itself (complete with table and hoisting out of it, etc) as well. It's called test_bug353415-2.html

(In reply to comment #53) > I added a test for this bug itself (complete with table and hoisting out of it, > etc) as well. It's called test_bug353415-2.html The test case test more than just type=hidden inside the table but outside a table cell. Hence, the test case fails with the HTML5 parser (bug 539687). HTML5 caters for the type=hidden case that was originally reported here. Is it OK to assume that the other form controls in test_bug353415-2.html don't test things that are strictly required for Web compat?

Not without checking what behavior HTML5 requires and other browsers have, sorry. In any case, the failure listed in that bug is in fact a failure of the thing the test is trying to test: the relative ordering of hidden inputs not in a cell and non-hidden inputs in a cell, no?

(In reply to comment #55) > Not without checking what behavior HTML5 requires and other browsers have, > sorry. > > In any case, the failure listed in that bug is in fact a failure of the thing > the test is trying to test: the relative ordering of hidden inputs not in a > cell and non-hidden inputs in a cell, no? Except the failing type=hidden is a child of a label element in the Mochitest but not in the minimized test case here. HTML5 deals with type=hidden as a child of tr. Is the label an essential Web compat aspect of the test?

Ah. There are just missing </label> tags in the testcase. Does adding them make the HTML5 parser happier? That said, the behavior of cutting off the shipping out on finding a hidden input (so closing any pending open parents, shipping them out, but not shipping the hidden input out) may in fact be needed for web compat. It's certainly sounding like something we've had issues with in the past. We probably won't know for sure until either someone goes back and writes regression tests specifically for past parser bugs or we ship a change to that behavior in a release and wait 6-12 months for feedback. :(

(In reply to comment #57) > Ah. There are just missing </label> tags in the testcase. Does adding them > make the HTML5 parser happier? Yes. > That said, the behavior of cutting off the shipping out on finding a hidden > input (so closing any pending open parents, shipping them out, but not shipping > the hidden input out) may in fact be needed for web compat. It's certainly > sounding like something we've had issues with in the past. We probably won't > know for sure until either someone goes back and writes regression tests > specifically for past parser bugs or we ship a change to that behavior in a > release and wait 6-12 months for feedback. :( I see. :-(