332528 - [FIX]nsNodeInfoManager destructor can crash

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

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.

Comment 2

[Peter Van der Beken [:peterv]]

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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Releasing the remaining nodeinfo manager calls ~nsNodeInfoManager, which calls nsNodeInfo::ClearCache, which deletes sCachedNodeInfo.  See stack in comment 0.  ;)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 5

[Peter Van der Beken [:peterv]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, I see what you mean.  I'd misunderstood your question.

Hmm....  Yeah, layout shutdown should call nsNodeInfo::ClearCache.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 8

[Boris Zbarsky [:bzbarsky]]

Checked in the followup