Jesse Ruderman Reporter
	Description • 16 years ago

Steps to reproduce:
1. Load the testcase.
2. Close the window (or for better reproducibility, Quit)

Result:

Crash [@ nsDefaultComparator] during js_GC, *or*

###!!! ASSERTION: Child not in controls: 'index != controls.NoIndex', file nsHTMLFormElement.cpp, line 1464
###!!! ASSERTION: Invalid start index: 'count == 0 || start < Length()', file nsTArray.h, line 597

often followed by various other assertions, malloc complaints such as "incorrect checksum for freed object", crashes, etc.

	Jesse Ruderman Reporter
	Comment 1 • 16 years ago

Should that nsTArray.h assertion be an NS_ABORT_IF_FALSE?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 2 • 16 years ago

I'm not super opinionated on if we should bounds check in various nsTArray methods or not. As long as we are consistent.

Though we should assert as there is for sure a bug somewhere if you try to remove elements out of bounds.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 16 years ago

Sadly, I couldn't get this to work because I couldn't get the form to become GCed.... I guess we should use the other testcase for this, eh?  :(

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 16 years ago

So the key part is that we have an input that is the kid of a form.  Someone's holding a ref to the input.  Then the form's destructor fires (in this case because designMode does some DOM rearrangement which includes removing all the forms from the document). This calls SetForm() on the input, but tells it to NOT remove itself from the form, which means it doesn't reset its ADDED_TO_FORM flag.  Then the input is added to a new form, but since ADDED_TO_FORM is set, doesn't add itself to the form.  Then when we go to remove it from the form later, we get the assertions, etc.  This patch fixes the flag-munging.  I'll file a followup bug on improving the API to make this sort of thing less likely to happen.

This patch applies as-is to 1.9.0, and we want it there.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 16 years ago

Pushed changeset 8b309b9a6ba7.

	Daniel Veditz [:dveditz]
	Comment 7 • 16 years ago

Does this happen on the 1.8 branch?

	Daniel Veditz [:dveditz]
	Comment 8 • 16 years ago

Comment on attachment 336687 [details] [diff] [review]
Fix

Approved for 1.9.0.4, a=dveditz for release-drivers

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 16 years ago

I would assume this isn't an issue on branch, since this code was changed pretty heavily from 1.8 to 1.9....

	Daniel Veditz [:dveditz]
	Comment 10 • 16 years ago

Assertion does not happen on the 1.8 branch

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 16 years ago

Fixed on 1.9 branch.

	Carsten Book [:Tomcat]
	Comment 12 • 16 years ago

verified fixed on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4pre) Gecko/2008102401 Firefox/3.0.4pre

i see not the assertion and not the crash on this build, when using the testcase. 
The only warning i see is WARNING: recurring into frame construction: 'mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0', file ../../dist/include/layout/nsPresContext.h, line 970

--> Verified 1.9.0.4

	Benjamin Smedberg
	Comment 13 • 16 years ago

This was actually fixed in http://hg.mozilla.org/mozilla-central/rev/10dfd117b6c9

Which landed before 1.9.1 branched

	Tony Chung [:tchung]
	Comment 14 • 16 years ago

Verified fix against first testcase in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090304 Firefox/3.1b3 
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090305 Minefield/3.2a1pre