Closed
Bug 349069
Opened 18 years ago
Closed 18 years ago
Move more things from ~nsINode to nsNodeUtils::LastRelease
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(2 files, 3 obsolete files)
16.43 KB,
patch
|
sicking
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
20.11 KB,
patch
|
sicking
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
This may help with Bug 338595 too.
Assignee | ||
Comment 1•18 years ago
|
||
Assignee: general → Olli.Pettay
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•18 years ago
|
||
~nsINode deletes slots, but before that if slots have been extended, extra members of slots must be cleared. That is the reason for removing ~nsDOMSlots and adding those ->DropReference() calls to ~nsGenericElement. This way ~nsXULElement can first release slots->mControllers. LastRelease can't do this all because of the union in nsDOMSlots. Otherwise the patch should be quite clear. ~nsXULDocument had that extra mObservers.Clear() (it is also in ~nsDocument). ~nsDocument handled nsIDOMUserDataHandler::NODE_DELETED which is also handled in LastRelease (this is a leftover from the xul event handler change).
Attachment #234294 -
Attachment is obsolete: true
Attachment #234399 -
Flags: review?(bugmail)
Assignee | ||
Comment 3•18 years ago
|
||
This adds nsDocument::LastRelease. That is needed since properties must be deleted before even starting to deleted document. (Some TB stacktraces indicate this.)
Attachment #234399 -
Attachment is obsolete: true
Attachment #234750 -
Flags: review?(bugmail)
Attachment #234399 -
Flags: review?(bugmail)
Comment on attachment 234750 [details] [diff] [review] better handling for nsDocument >Index: content/base/src/nsDocument.cpp >@@ -663,29 +663,16 @@ nsDocument::~nsDocument() ... >- nsNodeUtils::NodeWillBeDestroyed(this); > // Clear mObservers to keep it in sync with the mutationobserver list > mObservers.Clear(); Could you move this call to the top of the dtor now that the mutationobserver list is destroyed much earlier. >+nsDocument::LastRelease() >+{ >+ nsNodeUtils::LastRelease(this, PR_FALSE); >+ // Delete properties before starting to destruct the document. >+ // Some of the properties are bound to nsINode objects and the destructor >+ // functions of the properties may want to use the owner document of the >+ // nsINode. >+ mPropertyTable.DeleteAllProperties(); >+ delete this; >+} Could you move the DeleteAllProperties call to before the nsNodeUtils::LastRelease call? That way it'd be ok for nsNodeUtils::LastRelease to delete the object and you could remove the aDelete argument. >Index: content/base/src/nsGenericElement.cpp > nsINode::~nsINode() > { ... >+ nsINode::nsSlots* slots = GetExistingSlots(); >+ delete slots; No need for the temporary here. >Index: content/base/src/nsGenericElement.h >@@ -891,17 +891,16 @@ protected: > * objects for each of these instance variables, we put them off > * in a side structure that's only allocated when the content is > * accessed through the DOM. > */ > class nsDOMSlots : public nsINode::nsSlots > { Add a _very_ visible comment to the top of this class stating that any properties in this class has to be cleared in the nsGenericElement dtor since the slots structure has a non-virtual dtor. >Index: content/base/src/nsNodeUtils.cpp >+nsNodeUtils::LastRelease(nsINode* aNode, PRBool aDelete) ... > if (aNode->HasProperties()) { ... >+ if (aNode->HasFlag(NODE_HAS_RANGELIST)) { ... >+ if (aNode->HasFlag(NODE_HAS_LISTENERMANAGER)) { ... Add code after these three if-statements that clear these flags so that they don't lie while the node dtor is running. Also, add a comment stating that we want to kill properties first since that may run external code, so we want to be in as complete state as possible at that time. >Index: content/base/src/nsNodeUtils.h >+ static void LastRelease(nsINode* aNode, PRBool aDelete = PR_TRUE); I'm not a big fan of default values and would really prefer to not use it here. Assuming we can't remove the argument entierly of course (see above). r/sr=sicking with that.
Attachment #234750 -
Flags: superreview+
Attachment #234750 -
Flags: review?(bugmail)
Attachment #234750 -
Flags: review+
Assignee | ||
Comment 5•18 years ago
|
||
(In reply to comment #4) > Could you move the DeleteAllProperties call to before the > nsNodeUtils::LastRelease call? That way it'd be ok for nsNodeUtils::LastRelease > to delete the object and you could remove the aDelete argument. Properties must be still alive when LastRelease is called so that nsIDOMUserDataHandler can be notified.
Assignee | ||
Comment 6•18 years ago
|
||
I'll check in this. Removed default parameter, added unsetFlags and comments etc.
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Is the last line here correct? 150 void 151 nsNodeUtils::LastRelease(nsINode* aNode, PRBool aDelete) 152 { 153 nsINode::nsSlots* slots = aNode->GetExistingSlots(); 154 if (slots && !slots->mMutationObservers.IsEmpty()) { 155 NS_OBSERVER_ARRAY_NOTIFY_OBSERVERS(slots->mMutationObservers, 156 nsIMutationObserver, 157 NodeWillBeDestroyed, (aNode)); Maybe 157 LastRelease, (aNode));
Assignee | ||
Comment 8•18 years ago
|
||
(In reply to comment #7) > 155 NS_OBSERVER_ARRAY_NOTIFY_OBSERVERS(slots->mMutationObservers, > 156 nsIMutationObserver, > 157 NodeWillBeDestroyed, (aNode)); > > Maybe > > 157 LastRelease, (aNode)); > nsIMutationObserver doesn't have method called LastRelease
The patch is correct as is
Comment 10•18 years ago
|
||
Could this have caused Bug 350158 ?
Assignee | ||
Comment 11•18 years ago
|
||
I backed out this to see if this caused xserve02 orange.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 12•18 years ago
|
||
This didn't cause the problems with xserve02. But I'll try to find the reason for bug 350158 and post a new patch asap.
Assignee | ||
Comment 13•18 years ago
|
||
The reason for bug 350158 was that nsHTMLIFrameElement used NS_IMPL_ADDREF/RELEASE, not the _INHERITED macros. Noticed also possible problem with attributes and XUL. Slots really need to be deleted before the element so that mAttributeMap->DropReference() in ~nsDOMSlots does the right thing (uses the right GetAttr). That is the reason why I added nsXULSlots, which is used only to get the right destruction for the slots.
Attachment #235063 -
Attachment is obsolete: true
Attachment #235631 -
Flags: superreview?(bugmail)
Attachment #235631 -
Flags: review?(bugmail)
Comment 14•18 years ago
|
||
(In reply to comment #13) > Created an attachment (id=235631) [edit] > Doesn't regress bug 350158 I ran for about a week with the patch that was temporarily checked in, and have run with this new patch since it was posted. I have not had an occurrence of the Pure Virtual Function call pop-up running a build with either of these patches. Previously I had been experiencing this issue at least once per day.
Comment on attachment 235631 [details] [diff] [review] Doesn't regress bug 350158 Please add something like NS_ASSERTION(!HasSlots(), "nsNodeUtils::LastRelease was not called"); to the nsINode dtor. That way you can make sure to avoid misstakes like the iframe element not calling LastRelease. Also add a comment to nsSlots stating that we could save 4 bytes by removing the virtual dtor and adding a private nsINode::DestroySlots() function. r/sr=sicking with that.
Attachment #235631 -
Flags: superreview?(bugmail)
Attachment #235631 -
Flags: superreview+
Attachment #235631 -
Flags: review?(bugmail)
Attachment #235631 -
Flags: review+
Assignee | ||
Comment 16•18 years ago
|
||
Because of http://support.microsoft.com/kb/122370/EN-US/ I had make nsDOMSlots public :(
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•