Closed Bug 332528 Opened 19 years ago Closed 19 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: 19 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.

Attachment

General

Created:
Updated:
Size: