433758 - Crash [@ nsContentList::Item] with null this

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bob Clary [:bc] (inactive)]

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] (inactive)]

(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]

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][[e-mail]]]

Attachment: 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)]

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][[e-mail]]]

Oh yes, we want this. I'll try to write some testcase tomorrow.

Comment 6

[Daniel Veditz [:dveditz]]

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][[e-mail]]]

The code was added in Bug 259332, so 1.9 only.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Jonas, can you think of any reasonable small test case for this?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 321944 [details] [diff] [review]
Keep strong ref

The patch does apply cleanly to 1.9.0.x

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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)]

Comment on attachment 321944 [details] [diff] [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][[e-mail]]]

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]]

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][[e-mail]]]

Unfortunately I haven't figured out the testcase yet. Does the URL still work as a test?

Comment 15

[Al Billings [:abillings - ex-MoCo]]

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.