349069 - Move more things from ~nsINode to nsNodeUtils::LastRelease

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

This may help with Bug 338595 too.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Something like this

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: removed extra mObservers.Clear()

~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).

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better handling for nsDocument

This adds nsDocument::LastRelease. That is needed since
properties must be deleted before even starting to deleted document.
(Some TB stacktraces indicate this.)

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: final patch

I'll check in this.
Removed default parameter, added unsetFlags and comments etc.

Comment 7

[kitchin]

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));

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(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

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The patch is correct as is

Comment 10

[Ria Klaassen (not reading all bugmail)]

Could this have caused Bug 350158 ?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I backed out this to see if this caused xserve02 orange.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Doesn't regress bug 350158

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.

Comment 14

[Bill Gianopoulos [:WG9s]]

(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 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Because of http://support.microsoft.com/kb/122370/EN-US/ I had
make nsDOMSlots public :(