Last Comment Bug 453406 - [FIX]"ASSERTION: Child not in controls" and other badness
: [FIX]"ASSERTION: Child not in controls" and other badness
Status: VERIFIED FIXED
: [sg:critical]
: assertion, crash, testcase, verified1.9.0.4, verified1.9.1
Product: Core
Classification: Components
Component: DOM: Core & HTML
: Trunk
: x86 Mac OS X
: P1 critical (vote)
: ---
Assigned To: Boris Zbarsky (:bz)
: general
:
:
: 306663 336383 453496 469513
  Show dependency treegraph
 
Reported: 2008-09-02 23:53 PDT by Jesse Ruderman
Modified: 2009-03-05 13:48 PST (History)
11 users (show)
benjamin: blocking1.9.1+
samuel.sidler+old: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
bzbarsky: in‑testsuite+
See Also:
Crash Signature:


Attachments
testcase (can crash Firefox when closed or later) (835 bytes, text/html)
2008-09-02 23:53 PDT, Jesse Ruderman
no flags Details
Testcase that asserts immediately instead of sometime later (896 bytes, text/html)
2008-09-03 11:58 PDT, Boris Zbarsky (:bz)
no flags Details
Attempt at a mochitest not using the designMode stuff (2.87 KB, patch)
2008-09-03 12:00 PDT, Boris Zbarsky (:bz)
no flags Details | Diff | Splinter Review
Fix (3.66 KB, patch)
2008-09-03 12:04 PDT, Boris Zbarsky (:bz)
jst: review+
jst: superreview+
dveditz: approval1.9.0.4+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2008-09-02 23:53:56 PDT
Created attachment 336620 [details]
testcase (can crash Firefox when closed or later)

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.
Comment 1 Jesse Ruderman 2008-09-03 00:14:48 PDT
Should that nsTArray.h assertion be an NS_ABORT_IF_FALSE?
Comment 2 Jonas Sicking (:sicking) 2008-09-03 02:10:10 PDT
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.
Comment 3 Boris Zbarsky (:bz) 2008-09-03 11:58:40 PDT
Created attachment 336683 [details]
Testcase that asserts immediately instead of sometime later
Comment 4 Boris Zbarsky (:bz) 2008-09-03 12:00:18 PDT
Created attachment 336685 [details] [review]
Attempt at a mochitest not using the designMode stuff

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?  :(
Comment 5 Boris Zbarsky (:bz) 2008-09-03 12:04:58 PDT
Created attachment 336687 [details] [review]
Fix

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.
Comment 6 Boris Zbarsky (:bz) 2008-09-08 08:11:26 PDT
Pushed changeset 8b309b9a6ba7.
Comment 7 Daniel Veditz 2008-09-24 15:31:25 PDT
Does this happen on the 1.8 branch?
Comment 8 Daniel Veditz 2008-09-24 15:32:03 PDT
Comment on attachment 336687 [details] [review]
Fix

Approved for 1.9.0.4, a=dveditz for release-drivers
Comment 9 Boris Zbarsky (:bz) 2008-09-24 17:40:05 PDT
I would assume this isn't an issue on branch, since this code was changed pretty heavily from 1.8 to 1.9....
Comment 10 Daniel Veditz 2008-09-26 00:22:04 PDT
Assertion does not happen on the 1.8 branch
Comment 11 Boris Zbarsky (:bz) 2008-10-07 08:33:46 PDT
Fixed on 1.9 branch.
Comment 12 Carsten Book [:Tomcat] 2008-10-24 09:02:12 PDT
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
Comment 13 Benjamin Smedberg [:bsmedberg] 2009-01-27 11:28:25 PST
This was actually fixed in http://hg.mozilla.org/mozilla-central/rev/10dfd117b6c9

Which landed before 1.9.1 branched
Comment 14 Tony Chung [:tchung] 2009-03-05 13:48:21 PST
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

Note You need to log in before you can comment on or make changes to this bug.