Closed
Bug 332528
Opened 19 years ago
Closed 19 years ago
[FIX]nsNodeInfoManager destructor can crash
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: crash)
Attachments
(2 files)
1.22 KB,
patch
|
peterv
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
1.47 KB,
patch
|
peterv
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•19 years ago
|
||
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)
![]() |
Assignee | |
Updated•19 years ago
|
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 2•19 years ago
|
||
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+
![]() |
Assignee | |
Comment 3•19 years ago
|
||
Releasing the remaining nodeinfo manager calls ~nsNodeInfoManager, which calls nsNodeInfo::ClearCache, which deletes sCachedNodeInfo. See stack in comment 0. ;)
![]() |
Assignee | |
Comment 4•19 years ago
|
||
Fixed.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 5•19 years ago
|
||
(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.
![]() |
Assignee | |
Comment 6•19 years ago
|
||
Oh, I see what you mean. I'd misunderstood your question.
Hmm.... Yeah, layout shutdown should call nsNodeInfo::ClearCache.
![]() |
Assignee | |
Comment 7•19 years ago
|
||
Attachment #217044 -
Flags: superreview?(peterv)
Attachment #217044 -
Flags: review?(peterv)
Updated•19 years ago
|
Attachment #217044 -
Flags: superreview?(peterv)
Attachment #217044 -
Flags: superreview+
Attachment #217044 -
Flags: review?(peterv)
Attachment #217044 -
Flags: review+
![]() |
Assignee | |
Comment 8•19 years ago
|
||
Checked in the followup
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•