324871 - UserData can cause reference cycles

Status: RESOLVED FIXED

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

Description

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

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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++?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We're breaking the cycle by exposing underlying C++ reachability to the information to the JS GC instead of rooting.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This is the rooting in nsXPCWrappedJS, right?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes.

Comment 7

[Jesse Ruderman]

*** Bug 326635 has been marked as a duplicate of this bug. ***

Comment 8

[Jesse Ruderman]

Bug 326635 has a testcase, so adding testcase keyword.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, I think the first and the last are the same thing, so just two problems.

Comment 12

[Peter Van der Beken [:peterv]]

Sure, we can back it out now if it makes you feel more comfortable.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think there's a pretty good chance graydon's stuff will fix this, actually...

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: Testcase (leak through handler)

This shows the leak through the handler with a closure.

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: 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)

Comment 16

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

Peter, now that graydon landed the cycle collector you could maybe hook it up to that instead.

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: 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.

Comment 18

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

I'm fine with removing NODE_DELETED. Seems like a source of confusion and hardship for developers anyway since it exposes GC details.

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 20

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

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

Comment 21

[Peter Van der Beken [:peterv]]

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

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: Patch that was checked in

Comment 23

[Jesse Ruderman]

I checked the testcase in as a crashtest.