UserData can cause reference cycles

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: sicking, Assigned: peterv)

Tracking

(Blocks: 1 bug, {mlk, testcase})

Trunk
mlk, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

If someone sticks something holding a reference to the document inside a nsIDOMNode UserData we will end up with a reference cycle that is never broken. We should fix this using dbarons gc-participant stuff.

Userdata is a DOM interface that lets you stick any type of data onto a node keyed on a string (in other words sort of like old-fashioned expandos). So to the user it looks like every node has a hash that the user can get and set data into.

Note that the ownership of the userdata is sort of weird. The user data lives in a hash owned by the document, so if the document goes away then so does the user data. However if the node that the user put the userdata on goes away then it kills the entries in the hash too.

But basically we can get two types of cycles:

userdata->document->node->userdata
     where node is in the main tree

userdata->document->userdata
     where the userdata is attached to the document

Updated

12 years ago
Keywords: mlk
So how does that work for C++ callers?  Should they just watch out for what they stick on nodes?  Or will they be unable to use userdata?
They just have to watch out for leak cycles, but that's less of a danger for C++ callers than JS callers (which always entrain the global object and often entrain content nodes).
I guess I'm not sure where we're breaking this reference cycle.  Since we can't control whether the UserData owns the document, we have to make the document not own the UserData?  But then how does one set UserData in C++?
Blocks: 326635
We're breaking the cycle by exposing underlying C++ reachability to the information to the JS GC instead of rooting.
This is the rooting in nsXPCWrappedJS, right?
Yes.

Comment 7

12 years ago
*** Bug 326635 has been marked as a duplicate of this bug. ***
No longer blocks: 326635

Updated

12 years ago
Blocks: 326633

Comment 8

12 years ago
Bug 326635 has a testcase, so adding testcase keyword.
Keywords: testcase
Blocks: 336791
There's also the possibility of leaking through the handler; that's something I definitely know how to fix.  I need to look a bit more closely at how we store the data.
This appears to be a bug that was created since the 1.8 branch, since bug 264308 landed after the branch; therefore maybe it can wait for graydon's cycle collector to fix the problem.  I'm not going to worry about it in my push for leaks for 1.8.1.

(Although I admit I'm a little disappointed that we took the patch at all given the complex memory management problems it poses; I think the description in comment 0 is only one of three problems I can see; the other two are comment 9 and bug 326635 comment 4.  And that list may not be exhaustive.  If graydon's cycle collector doesn't fix this, I'd rather back out the feature than try to spend my time fixing it.)

I'm nominating this bug as blocking1.9a2?, although I really mean blocking1.9.  And I do mean blocking -- I think if it's not fixed we should back out DOM user data support.
Blocks: 264308
No longer blocks: 336791
Flags: blocking1.9a2?
Actually, I think the first and the last are the same thing, so just two problems.
(Assignee)

Comment 12

12 years ago
Sure, we can back it out now if it makes you feel more comfortable.
I think there's a pretty good chance graydon's stuff will fix this, actually...
(Assignee)

Comment 14

12 years ago
Created attachment 223709 [details]
Testcase (leak through handler)

This shows the leak through the handler with a closure.
(Assignee)

Comment 15

12 years ago
Created attachment 223710 [details] [diff] [review]
Handler leak fix WIP

This fixes the leak through the handler. We do hit the assertion in nsMarkedJSFunctionHolder_base::Get (GC preservation didn't work) because we try to get the handler from the node destructor, and we only get there once the handler has been released. (Note that without or with the patch we never fire the handler, without the patch the document leaks and so is never destroyed, with the patch the handler is released before we reach the document's destructor)

Updated

11 years ago
Flags: blocking1.9a2? → blocking1.9+
Peter, now that graydon landed the cycle collector you could maybe hook it up to that instead.
Assignee: general → peterv
(Assignee)

Updated

11 years ago
Depends on: 367779
(Assignee)

Comment 17

11 years ago
Created attachment 255814 [details] [diff] [review]
v1 (using cycle collector)

This fixes the leaks and fixes bug 337541. However, NODE_DELETED is problematic: by the time we collect the cycle and the node is deleted the scope for the handler has been cleared and XPConnect barfs about "being called on a scope without a 'Components' property". Maybe we should just remove NODE_DELETED.
Attachment #223710 - Attachment is obsolete: true
I'm fine with removing NODE_DELETED. Seems like a source of confusion and hardship for developers anyway since it exposes GC details.
(Assignee)

Comment 19

11 years ago
Created attachment 259142 [details] [diff] [review]
v1.1
Attachment #255814 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #259142 - Flags: review?(jonas)
Comment on attachment 259142 [details] [diff] [review]
v1.1

>Index: content/base/src/nsNodeUtils.cpp
>@@ -216,21 +192,22 @@ nsNodeUtils::LastRelease(nsINode* aNode,
> 
>   // Kill properties first since that may run external code, so we want to
>   // be in as complete state as possible at that time.
>-  if (aNode->HasProperties()) {
>+  if (aNode->IsNodeOfType(nsINode::eDOCUMENT)) {
>+    // Deleting a document, delete all 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.
>+    NS_STATIC_CAST(nsIDocument*, aNode)->PropertyTable()->DeleteAllProperties();

Aren't all properties bound to nsINode objects? Also, mind chaning /starting to destruct/tearing down/, there's so many 'destroy' words in the comment that it's hard to read.


>+nsNodeUtils::ReleaseProperties(nsINode *aNode)
>+{
>+  NS_ASSERTION(aNode->HasProperties(),
>+               "Call to ReleaseProperties not needed.");
>+
>+  // Strong reference to the document so that deleting properties can't
>+  // delete the document.
>+  nsCOMPtr<nsIDocument> document = aNode->GetOwnerDoc();
>+  if (document) {
>+    document->PropertyTable()->DeleteAllPropertiesFor(aNode);
>+  }
>+  aNode->UnsetFlags(NODE_HAS_PROPERTIES);
>+}

I think you should only unlink the userdata here. Other properties might be needed by other code. And call the function something like UnlinkUserData or some such that includes "UserData".

r/sr=me with that
Attachment #259142 - Flags: superreview+
Attachment #259142 - Flags: review?(jonas)
Attachment #259142 - Flags: review+
(Assignee)

Comment 21

11 years ago
(In reply to comment #20)
> Aren't all properties bound to nsINode objects?

No, some are bound to nsIFrame objects.

I'll change ReleaseProperties to UnlinkUserData, but it won't change much. If ReleaseProperties/UnlinkUserData is called then the node is in a cycle that we're unlinking, ideally we'll end up in LastRelease for that node which will call DeleteAllPropertiesFor(aNode).
Status: NEW → ASSIGNED
Flags: in-testsuite?
(Assignee)

Comment 22

11 years ago
Created attachment 264601 [details] [diff] [review]
Patch that was checked in
Attachment #259142 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 23

10 years ago
I checked the testcase in as a crashtest.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.