58256 - Crash [@ PresShell::~PresShell][@ 0x00000000]

Status: VERIFIED FIXED

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

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

This is a topcrash according to Talkback. There are 4 instances of this
according to the talkback sidebar. Crashes have been reported on Win2k and Win
98. Uptime between 18 secs and 2 min 22 secs. I cannot reproduce this.

I am assigning this to DOM first although I am not sure which is the right
component.

This looks suspicuously similar to bug 53763, bug 54233 and bug 54868. All those
bugs were crashers that happened in ~PresShell because the destructor was trying
to access pointers that were already deleted. The fix for those bugs was to hold
a ref to view manager so that presshell was released before view manager.
Unfortunately I do not see where (if it is even possible here) in this case we
could fix using the same principle.

Stack Trace:
PresShell::~PresShell
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp line 1324]
PresShell::`scalar deleting destructor'
nsTextInputListener::Release
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsGfxTextControlFrame2.cpp
line 225]
nsCOMPtr_base::~nsCOMPtr_base
[d:\builds\seamonkey\mozilla\xpcom\base\nsCOMPtr.cpp line 50]
nsComputedDOMStyle::~nsComputedDOMStyle
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsComputedDOMStyle.cpp line 121]
nsGeneratedContentIterator::Release
[d:\builds\seamonkey\mozilla\layout\base\src\nsGeneratedIterator.cpp line 195]
nsJSUtils::nsGenericFinalize
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSUtils.cpp line 438]
FinalizeNSHTMLOptionCollection
[d:\builds\seamonkey\mozilla\dom\src\html\nsJSNSHTMLOptionCollection.cpp line 244]
js_FinalizeObject [d:\builds\seamonkey\mozilla\js\src\jsobj.c line 1603]
gc_finalize_phase [d:\builds\seamonkey\mozilla\js\src\jsgc.c line 915]
js_GC [d:\builds\seamonkey\mozilla\js\src\jsgc.c line 1156]
js_ForceGC [d:\builds\seamonkey\mozilla\js\src\jsgc.c line 872]
JS_GC [d:\builds\seamonkey\mozilla\js\src\jsapi.c line 1543]
nsJSContext::GC [d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp
line 1296]
PL_HashTableRawLookup [plhash.c line 190]
PL_HashTableLookup [plhash.c line 373]
PR_AtomicIncrement [pratom.c line 286]
nsHTMLDocument::AddRef
[d:\builds\seamonkey\mozilla\layout\html\document\src\nsHTMLDocument.cpp line 381]
nsDocument::QueryInterface
[d:\builds\seamonkey\mozilla\layout\base\src\nsDocument.cpp line 815]

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The same stack trace is also in Talkback stack signature of 0x00000000 with 9
instances. So this counts as 13 instances total.

Comment 2

[Johnny Stenback (:jst)]

Attachment: Proposed fix that removes the strong nsPresShell reference from nsComputedDOMStyle

Comment 3

[Johnny Stenback (:jst)]

*** Bug 58147 has been marked as a duplicate of this bug. ***

Comment 4

[Johnny Stenback (:jst)]

The fix I just attached fixes the crash by removing the strong nsPressShell
reference from nsComputedDOMStyle so that nsComputedDOMStyle objects in the GC
don't interfere with the presshell teardown process. With this patch there's no
strong reference from nsComputedDOMStyle to the presshell, in stead the
presshell is gotten from the document that is gotten from the content object
(nsComputedDOMStyle has a strong reference to the content).

The patch is not trivial at first look but what it does is really not
complicated at all, and it should be safe. The nsComputedDOMStyle's are used by
XBL and that's why we're seeing this crasher, I guess, since I doubt there's a
lot of those used out on the web yet.

Oh, and Heikki says r=heikki.

Comment 5

[Nisheeth Ranjan]

Marking rtm need info.  We should get this super reviewed from Vidur.

Comment 6

[Nisheeth Ranjan]

r=nisheeth.

Comment 7

[vidur (gone)]

Is getting the first docshell the right thing to do? The computed style is
supposed to be view specific i.e. it might originate from a view/window that
isn't the one represented by the first docshell.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

It is actually presshell (if you are referring to the GetPresShell() method). I
guestioned this as well, but Johnny's opinion is that nobody actually uses it/it
does not even work (meaning you could have same content in multiple presshells).
Searching in LXR will show you that 90% of all the callers of GetShellAt() call
it with argument 0. If it works for them, it is fairly safe bet it works for us.
This usage pattern should probably be fixed eventually.

Comment 9

[Johnny Stenback (:jst)]

Getting the first shell is arguably wrong but it's IMO ok to do in here to fix
the crash. The bottom line is, as you can guess from Heikkis comment, that we're
soooo broken in sooo many places even without this change if we ever try
supporting multiple presshells...

Talked to Vidur again about this and he says sr=vidur.

Comment 10

[Jason Eager]

This has a r= and sr=, and should go to rtm+

Comment 11

[Nisheeth Ranjan]

Marking rtm+.  This is a crash that will be seen any time somebody uses the DOM 
2 getComputedStyle() method.  Admittedly, there is very little existing 
content on the web that uses this call right now.  But, since getComputedStyle() 
is something that we say we support, we should not crash on the most trivial 
usage of it.  The fix looks relatively large but is conceptually straightforward 
and low risk.  Please allow this in for rtm.

Comment 12

[Phil Peterson]

rtm++. Please check into the branch ASAP

Comment 13

[Johnny Stenback (:jst)]

The fix is checked into the branch.

Comment 14

[Nisheeth Ranjan]

Marking fixed.  We are going to fix this the "right way" on the trunk.  See bug
58830 for details.

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Verified fixed on NT & Linux branch builds 2000-11-03-09. Trunk fix will need to
wait for bug 58830.