Move more things from ~nsINode to nsNodeUtils::LastRelease

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

12 years ago
This may help with Bug 338595 too.
(Assignee)

Comment 1

12 years ago
Created attachment 234294 [details] [diff] [review]
Something like this
Assignee: general → Olli.Pettay
Status: NEW → ASSIGNED
(Assignee)

Comment 2

12 years ago
Created attachment 234399 [details] [diff] [review]
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).
Attachment #234294 - Attachment is obsolete: true
Attachment #234399 - Flags: review?(bugmail)
(Assignee)

Comment 3

12 years ago
Created attachment 234750 [details] [diff] [review]
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.)
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

12 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

12 years ago
Created attachment 235063 [details] [diff] [review]
final patch

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

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 7

12 years ago
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

12 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
Could this have caused Bug 350158 ?
(Assignee)

Updated

12 years ago
Depends on: 350158
(Assignee)

Comment 11

12 years ago
I backed out this to see if this caused xserve02 orange.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

12 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

12 years ago
Created attachment 235631 [details] [diff] [review]
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.
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+
(Assignee)

Comment 16

12 years ago
Because of http://support.microsoft.com/kb/122370/EN-US/ I had
make nsDOMSlots public :(
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.