Last Comment Bug 433758 - Crash [@ nsContentList::Item] with null this
: Crash [@ nsContentList::Item] with null this
Status: RESOLVED FIXED
: [sg:critical?] post 1.8-branch
: crash, verified1.9.0.2
Product: Core
Classification: Components
Component: DOM: Core & HTML
: Trunk
: x86 All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: general
: http://v.788111.com/Video.html
:
: 259332
  Show dependency treegraph
 
Reported: 2008-05-14 13:31 PDT by Bob Clary [:bc:]
Modified: 2008-09-23 21:33 PDT (History)
12 users (show)
samuel.sidler+old: blocking1.9.0.2+
samuel.sidler+old: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
asac: wanted1.8.0.x-
samuel.sidler+old: in‑testsuite?
bclary: in‑litmus-
See Also:
Crash Signature:
[@ nsContentList::Item]


Attachments
Keep strong ref (1.38 KB, patch)
2008-05-21 07:11 PDT, Olli Pettay [:smaug]
jst: review+
jst: superreview+
samuel.sidler+old: approval1.9.0.2+
Details | Diff | Splinter Review

Summon comment box

Description Bob Clary [:bc:] 2008-05-14 13:31:14 PDT
reproduced on fedora 6 32bit but not centos5 64bit.

#5  0x00002aaab3a3eae0 in nsContentList::Item (this=0x0, aIndex=0, aDoFlush=1)
    at /work/mozilla/builds/1.9.0/mozilla/content/base/src/nsContentList.cpp:378
#6  0x00002aaab3bbb4d7 in nsHTMLDocument::GetDocumentAllResult (
    this=0x21253660, aID=@0x7fff98372c10, aResult=0x7fff98372c40)
    at /work/mozilla/builds/1.9.0/mozilla/content/html/document/src/nsHTMLDocument.cpp:3892
#7  0x00002aaab3d21360 in nsHTMLDocumentSH::DocumentAllGetProperty (
    cx=0x20c4ae10, obj=0x216ae340, id=560133188, vp=0x7fff98372d20)
    at /work/mozilla/builds/1.9.0/mozilla/dom/src/base/nsDOMClassInfo.cpp:8010
#8  0x00002aaab3d21786 in nsHTMLDocumentSH::DocumentAllNewResolve (
    cx=0x20c4ae10, obj=0x216ae340, id=560133188, flags=1, objp=0x7fff98372de0)
    at /work/mozilla/builds/1.9.0/mozilla/dom/src/base/nsDOMClassInfo.cpp:8098
#9  0x00002aaaaadc8c4c in js_LookupPropertyWithFlags (cx=0x20c4ae10, 
    obj=0x216ae340, id=560133188, flags=1, objp=0x7fff98372ee0, 
    propp=0x7fff98372ed8)
    at /work/mozilla/builds/1.9.0/mozilla/js/src/jsobj.c:3334
Comment 1 Bob Clary [:bc:] 2008-05-14 13:41:57 PDT
(In reply to comment #0)
> reproduced on fedora 6 32bit but not centos5 64bit.

ignore that. found on centos5 32 and 64bit. /me has too many windows open.
Comment 2 timeless 2008-05-15 01:51:51 PDT
that's the optimizer confusing the debugger

nsHTMLDocument::GetDocumentAllResult(const nsAString& aID, nsISupports** aResult)
  if (!entry->mDocAllList) {
    entry->mDocAllList = new nsContentList(root, DocAllResultMatch,
                                           nsnull, nsnull, PR_TRUE, id);
    NS_ENSURE_TRUE(entry->mDocAllList, NS_ERROR_OUT_OF_MEMORY);
  nsIContent* cont = entry->mDocAllList->Item(1, PR_TRUE);
  if (cont) {
    return NS_OK;
you are here:
  NS_IF_ADDREF(*aResult = entry->mDocAllList->Item(0, PR_TRUE));
Comment 3 Olli Pettay [:smaug] 2008-05-21 07:11:12 PDT
Created attachment 321944 [details] [review]
Keep strong ref

contentList->Item(..., PR_TRUE) flushes, so better to keep contentList alive, 
even when hashtable entry goes away.

Would be great to find some small testcase, but at least on Linux I couldn't
reproduce the crash with the patch, but without patch crash happens ~50% of time
when loading the url.
Comment 4 Samuel Sidler (old account; do not CC) 2008-08-17 15:53:49 PDT
Olli, do you still want this on the branch? If so, does the current patch apply and can we get a test for it?
Comment 5 Olli Pettay [:smaug] 2008-08-17 16:55:16 PDT
Oh yes, we want this. I'll try to write some testcase tomorrow.
Comment 6 Daniel Veditz 2008-08-17 20:16:22 PDT
Reproduced on Windows (as expected from the patch), reliable crasher in a debug build (not always at the same spot, depending on what random junk was in the content list object).

Could not reproduce on the 1.8 branch, is this due to trunk deCOMtamination or similar changes?
Comment 7 Olli Pettay [:smaug] 2008-08-18 10:39:06 PDT
The code was added in Bug 259332, so 1.9 only.
Comment 8 Olli Pettay [:smaug] 2008-08-18 12:32:20 PDT
Jonas, can you think of any reasonable small test case for this?
Comment 9 Olli Pettay [:smaug] 2008-08-18 12:37:19 PDT
Comment on attachment 321944 [details] [review]
Keep strong ref

The patch does apply cleanly to 1.9.0.x
Comment 10 Jonas Sicking (:sicking) 2008-08-18 15:44:56 PDT
Hmm.. not really sure exactly what needs to happen in order to trigger the badness here.

What *probably* was going wrong here wasn't that the list was going away, but rather that the hash table changed and so the |entry| variable was stale.

These steps might do it:

1. Call getElementById some 100 times with different IDs. That will make us make the hash fully live.
2. document.write a bunch of markup which contains lots of elements with ids. Probably in the order of 64 such elements. Let at least 1 element have the id "foo".
3. Call document.all.foo

The tricky part is finding the right number of elements in 2 I think. You want enough number that it forces the hash to resize, but you also don't want us to force a flush during the document.write, though flushing might never happen during document.write. If flushing is messy during document.write you could end the current script and put normal markup in the doc instead, before step 3.
Comment 11 Samuel Sidler (old account; do not CC) 2008-08-19 16:48:02 PDT
Comment on attachment 321944 [details] [review]
Keep strong ref

Approved for 1.9.0.2. Please land in CVS. a=ss

It'd be good to get a test for this landed as well, if possible.
Comment 12 Olli Pettay [:smaug] 2008-08-24 03:30:51 PDT
Forgot to add fixed1.9.0.2
CVS Log:
3.787	Olli.Pettay%helsinki.fi	2008-08-20 08:24 Bug 433758, r+sr=jst, a=ss
Comment 13 Tony Chung [:tchung] 2008-09-08 15:02:49 PDT
hi, did we ever get a testcase landed for this?   Without one, i am having a hard time verifying this fix on 1.9.0.2 builds.    Thanks.
Comment 14 Olli Pettay [:smaug] 2008-09-08 15:39:07 PDT
Unfortunately I haven't figured out the testcase yet. Does the URL still work as a test?
Comment 15 Al Billings [:abillings] 2008-09-09 12:44:31 PDT
The url crashes 3.01 on OS X for me. 3.02 does not crash. I reproduced this multiple times.

Verified for 1.9.0.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2) Gecko/2008090212 Firefox/3.0.2.

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