Closed Bug 332528 Opened 14 years ago Closed 14 years ago

[FIX]nsNodeInfoManager destructor can crash

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: crash)

Attachments

(2 files)

The stack is something like:

#0  0xdbdbdbdb in ?? ()
#1  0xb7d34c2d in PL_HashTableRemove (ht=0x83cb140, key=0x83cb16c)
    at ../../../../mozilla/nsprpub/lib/ds/plhash.c:374
#2  0xb5afda4e in nsNodeInfoManager::RemoveNodeInfo (this=0x82c8b98, aNodeInfo=0x83cb168)
    at ../../../../mozilla/content/base/src/nsNodeInfoManager.cpp:324
#3  0xb5afc0b7 in nsNodeInfo::Clear (this=0x83cb168)
    at ../../../../mozilla/content/base/src/nsNodeInfo.cpp:85
#4  0xb5afc05a in ~nsNodeInfo (this=0x83cb168)
    at ../../../../mozilla/content/base/src/nsNodeInfo.cpp:78
#5  0xb5afca12 in nsNodeInfo::ClearCache ()
    at ../../../../mozilla/content/base/src/nsNodeInfo.cpp:278
#6  0xb5afd025 in ~nsNodeInfoManager (this=0x82c8b98)
    at ../../../../mozilla/content/base/src/nsNodeInfoManager.cpp:114

because we destroy our hashtable and then get an attempt to access it.  My patch for bug 198595 triggers this, when combined with the safe-browsing extension.
Attached patch FixSplinter Review
So the setup here is that we have only one nodeinfo manager and only one nodeinfo and the one-slot cache is empty.  With the current code, when the nodeinfo is released we do:

1)  Put it in cache
2)  Clear
3)  Clear removes the nodeinfo from the nodeinfo manager
4)  Clear releases the nodeinfo manager (but does NOT null it yet!)
5)  ~nsNodeInfoManager sees this is the last one and clears the nodeinfo cache
6)  We delete the nodeinfo (which is in the cache per step 1)
7)  ~nsNodeInfo calls Clear()
8)  Since mOwnerManager is not null yet, we release it

So we get a double-release, which calls ~nsNodeInfoManager _again_ on the same nodeinfo manager, which crashes.

The solution, it seems to me, is to make sure that the cached nodeinfo has already been cleared before caching it.  I put the Clear() before the check of sCachedNodeInfo because Clear() can cause sCachedNodeInfo to be filled in, in theory, since it deletes XPCOM objects.
Attachment #216990 - Flags: superreview?(peterv)
Attachment #216990 - Flags: review?(peterv)
Keywords: crash
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Summary: nsNodeInfoManager destructor can crash → [FIX]nsNodeInfoManager destructor can crash
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 216990 [details] [diff] [review]
Fix

This seems right. On a sidenote: I don't understand how we're deleting the sCachedNodeInfo when the one remaining nodeinfomanager has already been released. Should we make layout shutdown call nsNodeInfo::ClearCache?
Attachment #216990 - Flags: superreview?(peterv)
Attachment #216990 - Flags: superreview+
Attachment #216990 - Flags: review?(peterv)
Attachment #216990 - Flags: review+
Releasing the remaining nodeinfo manager calls ~nsNodeInfoManager, which calls nsNodeInfo::ClearCache, which deletes sCachedNodeInfo.  See stack in comment 0.  ;)
Fixed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
(In reply to comment #3)
> Releasing the remaining nodeinfo manager calls ~nsNodeInfoManager, which calls
> nsNodeInfo::ClearCache, which deletes sCachedNodeInfo.  See stack in comment 0.
>  ;)

Ok, I might be missing something, but with your patch: if it's only the nodeinfo holding on the nodeinfo manager, nsNodeInfo::LastRelease calls Clear which releases the nodeinfo manager which calls ~nsNodeInfoManager which sets sCachedNodeInfo to nsnull through nsNodeInfo::ClearCache, and then nsNodeInfo::LastRelease goes on and does |sCachedNodeInfo = this;|. It can't be the nodeinfo manager which deletes that, it's already gone.
Oh, I see what you mean.  I'd misunderstood your question.

Hmm....  Yeah, layout shutdown should call nsNodeInfo::ClearCache.
Attached patch Like soSplinter Review
Attachment #217044 - Flags: superreview?(peterv)
Attachment #217044 - Flags: review?(peterv)
Attachment #217044 - Flags: superreview?(peterv)
Attachment #217044 - Flags: superreview+
Attachment #217044 - Flags: review?(peterv)
Attachment #217044 - Flags: review+
Checked in the followup
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.