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)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files, 3 obsolete files)

This may help with Bug 338595 too.
Attached patch Something like this (obsolete) — Splinter Review
Assignee: general → Olli.Pettay
Status: NEW → ASSIGNED
Attached patch removed extra mObservers.Clear() (obsolete) — Splinter Review
~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)
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+
(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.
Attached patch final patch (obsolete) — Splinter Review
I'll check in this. Removed default parameter, added unsetFlags and comments etc.
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));
(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
Could this have caused Bug 350158 ?
Depends on: 350158
I backed out this to see if this caused xserve02 orange.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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)
(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+
Because of http://support.microsoft.com/kb/122370/EN-US/ I had make nsDOMSlots public :(
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
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: