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•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•