455853 - Javascript: User defined properties disappear after 3 to 10 seconds from nodes that are added to a DOM document that is created using document.implementation.createDocument()

Status: RESOLVED WORKSFORME

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

Description

[Luke Van In]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

This is an error with the DOM in JavaScript - possibly due to the garbage collector collecting variables that are still in scope.

When using a user-defined property on a node that belongs to a DOM document, the user-defined property becomes undefined without reason or explanation. 

The error seems to be time-related because the property which is defined for a few seconds later becomes undefined. In my tests the property becomes undefined typically after 3 to 10 seconds, although it may take longer.

The DOM document mentioned here is one created using document.implementation.createDocument( "", "", null );

Please see the "Steps to Reproduce", "Additional Information", and "URL" for example scripts to re-create the error.


Reproducible: Always

Steps to Reproduce:
1. A DOM document is created using document.implementation.createDocument( "", "", null) (call this MyDom)

2. A node is created (call this MyDocument), and added to the DOM document with MyDom.appendChild( MyDocument)

3. A node is created using document.createElement( "B" ) or  MyDoc.createElement( "B" ) (call this MyNode)

4. A user defined property is added to the node from step 3 (MyNode)

5. The node from step 3 (MyNode) is added to the node from step 2 (MyDocument)

6. After an arbitrary time interval (3 to 10 seconds on my browser) the user defined property becomes undefined 
Actual Results:  
For the first few seconds of the script's lifetime the user-defined property exists (that is, it is defined). After 3 to 10 seconds the property becomes undefined.

Expected Results:  
The property should stay defined as long as there is a variable that holds a reference to it (in this case, the containing node).

I have created a sample script (written in JavaScript) to reproduce the error.

Copy and paste the code below into an HTML file. Click the "test" button when the page has loaded. Clicking the button will run code which will check if a specific user-defined property exists on a node created using the above-mentioned techniques. 

For the first few seconds clicking the button yields the result "property exists: true", indicating that the user-defined property does exist.

After 3 to 10 seconds, clicking the button yields the result "property exists: false" indicating that the user-defined property no longer exists.



[-----SNIP-----]
<script language="javascript">
//
var doc = document.implementation.createDocument( "", "", null );

var lib = doc.createElement( 'nodes' );

doc.appendChild( lib );	


function MakeNode()
{
	var node = document.createElement( 'b' );
	
	node.extendedElement = true;

	doc.importNode( node, true );
		
	lib.appendChild( node );
}


MakeNode();
	
	
	
//	
function TestCode()
{
	//
	var n = lib.firstChild;
	
	alert( 'property exists: ' + ( n.extendedElement != undefined )  );

}

</script>


<input type="button" onClick="TestCode();"  value="test" />
[-----SNIP-----]

Comment 1

[Ria Klaassen (not reading all bugmail)]

Did a few attempts to find a regression range but the script failed in older builds with an error.

Comment 2

[chas peters]

most bugs seem to come from vista related programs.

Comment 3

[Peter Van der Beken [:peterv]]

Probably related to the importNode call.

Comment 4

[Luke Van In]

Wow I didn't expect such a quick response. This is awesome!

Peter: I don't think it is the importNode() call. Although importNode() would remove the user-defined properties I only included it here for 'correctness'. 

The script still seems to execute without the call, although the bug still appears.

I have uploaded another test script (sans the importNode() call) to http://www.aliotech.com/lab/javascript/v0.09/test-gc-3.html

Comment 5

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #1)
> Did a few attempts to find a regression range but the script failed in older
> builds with an error.

What I meant to say is that this bug is a regression but that the date is unclear because of that wrong document error being thrown.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Need real regression range, assuming this is a regression.  I would suspect this never worked, since there is no script global object on the document, but it might be a cycle collection regression.

Comment 7

[Jason Orendorff [:jorendorff]]

Slightly smaller test case:

<script language="javascript">
var doc = document.implementation.createDocument("", "", null);
var root = doc.createElement('nodes');
doc.appendChild(root);
root.expando = "ok";
root = null;
</script>
<input type=button onclick="alert(doc.documentElement.expando || 'gone')" value="test">

Comment 8

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

Attachment: testcase, works on ff2

Comment 9

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

Attachment: possible fix

Check the existence of script object.

Comment 10

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

This seemed to cause leaks on some (but not all) tboxes.
I think the problem might be that document doesn't traverse
references.
Trying to write a reliable testcase for that.

Comment 11

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

Or perhaps the reason is that globalobject can't access the wrappers
created for data documents.
...still trying to find what is causing the leaks and ensuring that
backing out the patch did actually fix the problem.

Comment 12

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

http://mxr.mozilla.org/mozilla-central/source/content/xml/document/test/test_bug399502.xhtml and
http://mxr.mozilla.org/mozilla-central/source/content/xml/document/test/test_bug445330.html?force=1
cause a document to leak with the patch for this bug.

Comment 13

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

Attachment: proposed patch

This way I don't get any document/element/attr leaks when running mochitest.

Comment 14

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

Attachment: or this one

This might be better still. Document doesn't own the keys of mContentWrapperHash
(== content nodes), only the wrappers, so I think there might be cases when
the cycle doesn't contain document. If content node can inform about the wrapper,
and even unlink it, the cycle can be broken down before unlinking/destroying
the document.

Comment 15

[Johnny Stenback (:jst)]

Not blocking on this, but it'd be awesome to get one of the patches here reviewed and checked in!

Comment 16

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

Comment on attachment 340170 [details] [diff] [review]
or this one

>@@ -1287,16 +1299,20 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(
>   tmp->mRadioGroups.Enumerate(RadioGroupsTraverser, &cb);
> 
>   // The boxobject for an element will only exist as long as it's in the
>   // document, so we'll traverse the table here instead of from the element.
>   if (tmp->mBoxObjectTable) {
>     tmp->mBoxObjectTable->EnumerateRead(BoxObjectTraverser, &cb);
>   }
> 
>+  if (tmp->mContentWrapperHash) {
>+    tmp->mContentWrapperHash->EnumerateRead(ContentWrapperTraverser, &cb);
>+  }

These references are already traversed from nodes, so I think this would result in them being traversed twice. The advantage of traversing from the node is that if there is nothing referencing the node or the wrapper it can get cycle collected before the document is.

> nsDocument::AddReference(void *aKey, nsISupports *aReference)
> {
>-  if (mScriptGlobalObject) {
>+  PRBool dummy;
>+  nsIScriptGlobalObject* scriptObject = GetScriptHandlingObject(dummy);
>+  if (scriptObject) {

Not sure why there is a scriptObject check needed at all? If it was just to prevent leaks the cycle collector should take care of that now.

Comment 17

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

(In reply to comment #16)
> These references are already traversed from nodes, so I think this would result
> in them being traversed twice. 
Yes, that is a small problem. Could be fixed so that traverse from
nodes only when not bound to document.

> The advantage of traversing from the node is
> that if there is nothing referencing the node or the wrapper it can get cycle
> collected before the document is.
That is the change which fixes the leak I saw when checked in the original patch. Document owns all the references, but it can't traverse all them
via childNodes, because there can be disconnected subtrees.

 
> Not sure why there is a scriptObject check needed at all? If it was just to
> prevent leaks the cycle collector should take care of that now.
Well, possibly not needed. It was added when CC landed.

Comment 18

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

(In reply to comment #17)
> Yes, that is a small problem. Could be fixed so that traverse from
> nodes only when not bound to document.
Bah, doesn't quite work. Way too late here.

Comment 19

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

This should be fixed now by some peterv's patch, IIRC.

Comment 20

[Joshua Mitchell (Inactive)]

Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task