I just took a realtime jprof profile of a complete run of the ibench HTML load speed test, and 1.7% of the time was within SinkContext::DemoteContainer. (It was in 512 stacks out of 30,300 non-polling stacks in the profile.) Of those 512 stacks: * 334 are calling nsGenericHTMLContainerElement::AppendChild * 110 are calling nsGenericHTMLContainerElement::RemoveChild * 51 are calling SinkContext::FlushTags I was wondering why we actually need to move content around. Would it be possible to come up with a solution where all the form controls are all parented to the correct parent (i.e., none, or a different form other than one incorrectly terminated within a table) and we didn't have to rearrange the content model as we were going?

A correct removeChild() if a form's child should make all controls in that child's subtree drop pointers to the form. The current form control handling in DemoteContainer would break in those circumstances. So this is blocking bug 98487.

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 (you can query for this string to delete spam or retrieve the list of bugs I've moved)

It seems like this should be possible, but I have heard it has been tried before. Note that DemoteContainer() is now DemoteForm(), since it only works on forms anyway.

This patch removes DemoteForm entirely, per conversations with Vidur and Harish. In fact I modified it a little from what I discussed with you two, specifically instead of adding that "close form immediately after X tag" feature, I just let the content sink handle automatic closing of the form tag. Here is how forms work with this patch: 1. When <form> is found, mCurrentForm is set and the container is opened. 2. When </form> is found, mCurrentForm is nulled and IF we are balanced (if we would not be closing any other tags inadvertently), we close the container. Otherwise, we wait for the sink to close the form container automatically, which it will do as soon as it finds the close tag for the form's parent (same as any other unbalanced container). Big Monkey Testcase and screenshots shortly.

This testcase shows the three possible cases: one balanced form and two unbalanced forms.

Screenshot on build with fix applied

Screenshot WITHOUT fix

Reassigning to me.

Going through smoketests I noticed a crash with iframes and realized I had exposed issues with iframe that I had not addressed in other iframe bugs (the big one being that you should *never* destroy a the frameloader while the frame is still around). Iframe issues fixed. TESTING ON THIS PATCH: - smoketest - went to every smoketest site and did searches on each one - testcases on this bug - bugzilla.mozilla.org (all updates to this bug done with the patched build) - tinderbox (including verifying that the iframe popup works) - bug 98487 - bug 52334 I think we are clear. Harish? Vidur? Reviews? :)

Comment on attachment 82178 [details] [diff] [review] Patch v1.0.1 sr=vidur on all but the iframe/frameloader stuff. jkeiser says he'll get jst to look at it.

Comment on attachment 82178 [details] [diff] [review] Patch v1.0.1 sr=vidur on all but the iframe/frameloader stuff. jkeiser says he'll get jst to look at it.

Comment on attachment 82178 [details] [diff] [review] Patch v1.0.1 r=harishd

Patch to fix an issue JST brought up, that the change of nsGenericHTML(Container|Leaf)FormElement::mForm to an nsCOMPtr creates a circular dependency. This should not create a leak, as it turns out, because we have code in to break the dependency anyway, but a circular dependency is still a bad idea. This is UNTESTED but compiles. This is mainly for JST to verify and hopefully sr= alongside vidur; I will test later on Bugzilla, my form tester and on these testcases to verify that everything still works. (It will. :)

OK, so one should use ->Release() instead of NS_RELEASE if one does not wish the pointer to be cleared :) This has been tested specifically against Bugzilla, the testcase in this bug, and a form at my website. Uploading with patched build of course.

Comment on attachment 82820 [details] [diff] [review] Patch v1.0.3 (Brown Bag Release) sr=jst, but please loose the changes in neGenericHTMLElement.h, you don't need those any more.

Comment on attachment 82820 [details] [diff] [review] Patch v1.0.3 (Brown Bag Release) Carrying r= (small changes, I let Harish know what changes before the first r=)

Fixed on trunk, and gave us 10ms pageload time out of 1200 there. I have not run iBench, either, but it is literally impossible for DemoteForm to take 1.7% of the time on it because DemoteForm no longer exists :)

Nominating in case adt wants the 10ms pageload improvement (between 0.5% and 1%).

Just wanted to notate that the removal of DemoteForm() fixes a problem seen with some sites like: http://www.google.com/advanced_group_search?hl=en where a textwidget is programatically given focus, via JS, during the document load, and then the caret disappears. (First reported in bug 51897 comment 11) What was happening was that DemoteForm() was triggering frame destruction and reconstruction, after a textwidget frame was given focus. After the frames were recreated the textwidget was totally unaware that it had focus, since no focus event was dispatched during it's lifetime, so it didn't know it should display it's caret.

The patch in bug 143917 must be checked in if the patch in this bug is checked in.

adding adt1.0.0-. Let's get this in the next release.

Verified fixed with trunk build ID 20020529 using winxp

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

Acting on Duplicate resolution from bug 64450 comment 29: *Moving 'Blocks: bug 64451' from bug 64450 to bug 90756. (Take further action as needed.)