89536 - DOM test crashes mozilla

Status: VERIFIED FIXED

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

Description

[David Einstein]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.2+)
Gecko/20010704
BuildID:    2001070404

David Baron's test for bug 17390 now crashes mozilla.

Reproducible: Always
Steps to Reproduce:
1.Load the indicated page.
2.DON'T PRESS THE RED BUTTON. Press the grey one. There is no red button.
3.

Actual Results:  Kablooie

Expected Results:  I don't know, but not kablooie.

Looking at the description of bug 17390 mozilla used to leak webshells.  Now it
eats them.

Talkback ID's
TB32576907K     - w2k
TB32576982K     - w2k
TB32577624K     - Linux

I haven't looked at the javascript (I keep pressing the button) but I will
attempt to simplify if necessary.

Comment 1

[David Einstein]

Attachment: stacktrace

Comment 2

[David Einstein]

Attachment: Simplified testcase

Comment 3

[Pete Collins (MDG)]

This line is causing the crash.

http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.cpp#2772

  if (content.get() == mRootContent)
    mRootContent = nsnull;

If i comment it out, we don't crash.
I'm still investigating.

--pete

Comment 4

[Pete Collins (MDG)]

Attachment: this will stop the crash

Comment 5

[Pete Collins (MDG)]

If you apply patch #41524, you will stop the crash.
However, append child will not work in the test case.

Attaching slightly modified testcase.

--pete

Comment 6

[Pete Collins (MDG)]

Attachment: slightly modified test case

Comment 7

[Pete Collins (MDG)]

If i apply this patch my dump output is as follows.

Document file:///tmp/crash.html loaded successfully
Root: [object HTMLHtmlElement @ 0x8823c80]
document: [object HTMLDocument @ 0x8823c40]
JavaScript error: 
 line 0: uncaught exception: [Exception... "Component returned failure code:
0x80530003 [nsIDOMHTMLDocument.appendChild]"  nsresult: "0x80530003 (<unknown>)"
 location: "JS frame :: file:///tmp/crash.html :: testX :: line 11"  data: no]

The js exception happens on the "document.appendChild(root)" call.

--pete

Comment 8

[Pete Collins (MDG)]

OK, the creasher is right at this call:

http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLDocument.cpp#3400

We call the method:

 FindNamedItems(aName, mRootContent, *list);

mRootContent == nsnull here.

So if we look at method FindNamedItems()

We get to here: 

http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLDocument.cpp#3335

aContent->GetTag(*getter_AddRefs(tag));

Which is really: 

nsnull->GetTag(*getter_AddRefs(tag));

So what should we be doing here when mRootContent == nsnull???

--pete

Comment 9

[Pete Collins (MDG)]

Attachment: This patch fixes the crash but it introduces some asserts

Comment 10

[Pete Collins (MDG)]

New asserts:

###!!! ASSERTION: content doc != frame ctor doc: 'doc.get() == aDocument', file
nsCSSFrameConstructor.cpp, line 854
###!!! ASSERTION: content doc != frame ctor doc: 'doc.get() == aDocument', file
nsCSSFrameConstructor.cpp, line 854
###!!! ASSERTION: content doc != frame ctor doc: 'doc.get() == aDocument', file
nsCSSFrameConstructor.cpp, line 854


--pete

Comment 11

[Pete Collins (MDG)]

This patch is not complete. It fixes this particular situation however, if i use
other DOM calls such as getElementById() we core dump, BOOM!.

I just added a check to that call to test for mRootContent and fixed that as
well, but i will need to go and fix any other methods that may die.

I'll post a fully tested and complete patch when i'm finished.

jst, you can assign this bug to me if you want.

--pete

Comment 12

[Pete Collins (MDG)]

Attachment: proposed fix

Comment 13

[Pete Collins (MDG)]

Attachment: sorry, had an extra semicolon after "NS_OK"

Comment 14

[Pete Collins (MDG)]

Johnny can you look at this patch and see if everything is ok?

Thanks

--pete

Comment 15

[Pete Collins (MDG)]

As far as the assertion goes in nsCSSFrameConstructor.cpp, i think we are ok.
It is if def'd in there for debug builds only. If i undef DEBUG and test
everything works as expected.

So maybe we can review this patch now and get the fix in?

Thanks

--pete

Comment 16

[Johnny Stenback (:jst)]

Put the changes in nsHTMLDocument::GetElementById() *after*
NS_ENSURE_ARG_POINTER(), replace "if (nsnull==mRootContent)" with "if
(!mRootContent)" and "if (nsnull != mRootContent)" with "if (mRootContent)" and
add curly braces around the if expression in the later part of the changes and
you'll have my sr=jst.

Thanks alot for fixing this bug!

Comment 17

[Sivakiran Tummala]

confirmed on windows 2000

Comment 18

[Pete Collins (MDG)]

Attachment: updated patch as to jst's comments

Comment 19

[Pete Collins (MDG)]

sivakiran do i have an r=  from you?

If so, then i will check this sucker in.

--pete

Comment 20

[Eric Pollmann]

I've reviewed this fix, and it looks good to me. r=[e-mail]

Comment 21

[Pete Collins (MDG)]

Checked in this morning.

--pete

Comment 22

[Blake Ross]

jst, this looks like a harmless enough patch; is it worth taking on the branch?

Comment 23

[Blake Ross]

Attachment: minimized testcase

Comment 24

[Blake Ross]

Attachment: minimized testcase - now with the right mimetype

Comment 25

[Pete Collins (MDG)]

Maybe, this should move off of the "new" radar?

--pete

Comment 26

[selmer (gone)]

PDT+.  We *love* null check crash fixes!  Please check in before the morning's
builds if you're still around.

Comment 27

[jag (Peter Annema)]

post-checkin comment: ew on the difference in
space-between-if-and-opening-parenthesis with the surrounding code.

Checked in on the branch.

Comment 28

[Blake Ross]

And it was already checked in on the trunk, right?

Comment 29

[Johnny Stenback (:jst)]

Yup, this is already on the trunk, thanks jag for checking this in on the branch.

Comment 30

[Sivakiran Tummala]

does not crash anymore ----but hangs 

Comment 31

[Blake Ross]

Works for me...

Comment 32

[Pete Collins (MDG)]

Works fine for me off the trunk on unix. No crashes, hangs. Behaves exacly as it
should.

What platform ands build are you seeing this on?

--pete

Comment 33

[Sivakiran Tummala]

linux 07-18-04...

Comment 34

[Johnny Stenback (:jst)]

Sivakiran, please open a new bug on the hang you're seeing, markign this fixed
again.

Comment 35

[Sivakiran Tummala]

verified