Closed
Bug 398083
Opened 17 years ago
Closed 17 years ago
[FIX]XBL unlinking triggered hash-table recursion assertion
Categories
(Core :: XBL, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: bzbarsky)
References
Details
(Keywords: assertion, crash)
Attachments
(2 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
sicking
:
review+
sicking
:
superreview+
sicking
:
approval1.9+
|
Details | Diff | Splinter Review |
###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file pldhash.c, line 585
pure virtual method called
terminate called without an active exception
I can't reproduce at will, but I did get a stack trace for the assertion.
Regression from bug 372769?
Comment 1•17 years ago
|
||
Possibly fixed by the patch (once I found the final fix) for the insertion
parent loop. At least sounds similar to what I saw when making the current
patch and why I added the following to the patch:
+++ content/xbl/src/nsBindingManager.cpp
@@ -292,18 +292,27 @@ SetOrRemoveObject(PLDHashTable& table, n
table.ops = nsnull;
return NS_ERROR_OUT_OF_MEMORY;
}
aKey->SetFlags(NODE_MAY_BE_IN_BINDING_MNGR);
return AddObjectEntry(table, aKey, aValue);
}
// no value, so remove the key from the table
- if (table.ops)
- RemoveObjectEntry(table, aKey);
+ if (table.ops) {
+ ObjectEntry* entry =
+ static_cast<ObjectEntry*>
+ (PL_DHashTableOperate(&table, aKey, PL_DHASH_LOOKUP));
+ if (entry && PL_DHASH_ENTRY_IS_BUSY(entry)) {
+ // Keep key and value alive while removing the entry.
+ nsCOMPtr<nsISupports> key = entry->GetKey();
+ nsCOMPtr<nsISupports> value = entry->GetValue();
+ RemoveObjectEntry(table, aKey);
+ }
+ }
return NS_OK;
}
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → bzbarsky
Flags: blocking1.9?
Summary: XBL unlinking triggered hash-table recursion assertion → [FIX]XBL unlinking triggered hash-table recursion assertion
Assignee | ||
Comment 2•17 years ago
|
||
There's no need to nuke the hashtables to unlink. Just have to nuke the precompiled functions that are holding the JS scope alive. Since we unlink event listeners off of nsGenericElement, holding all those refs to the content nodes from here is ok.
Attachment #282949 -
Flags: superreview?(jonas)
Attachment #282949 -
Flags: review?(jonas)
Don't you at least want to traverse the hashes and remove all values? Or will unlinking the elements do that?
Assignee | ||
Comment 4•17 years ago
|
||
The hashes will get destroyed in ~nsXBLPrototypeBinding, as long as we unlink enough stuff. Which we still do with this patch.
But if we don't unlink enough stuff we won't destroy anything and no destructors will run. Or are you sure that we'll never end up with a cycle where this is the only breakable link?
Assignee | ||
Comment 6•17 years ago
|
||
I'm pretty sure, yes. The hashtables hold refs to things entirely within the XBL document, and the unlink methods of those content nodes will drop refs out into JS. So at that point we just have refs into the XBL document DOM (but not out of it), and we should be good.
Comment on attachment 282949 [details] [diff] [review]
Possible fix
Ok, sounds good.
I've actually come to realize that we need to be a little careful with what we put in Unlink for performance reasons. We're already calling unlink a lot, and it's only going to happen more.
Attachment #282949 -
Flags: superreview?(jonas)
Attachment #282949 -
Flags: superreview+
Attachment #282949 -
Flags: review?(jonas)
Attachment #282949 -
Flags: review+
Assignee | ||
Comment 8•17 years ago
|
||
Comment on attachment 282949 [details] [diff] [review]
Possible fix
Requesting approval for crash regression fix.
Attachment #282949 -
Flags: approval1.9?
Attachment #282949 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 9•17 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: blocking1.9? → in-testsuite?
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•