372960 - Make XPConnect traverse more JS edges

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Peter Van der Beken [:peterv]]

We're still missing edges for the cycle collection graph. I have a patch that makes the cycle collection code in nsXPConnect traverse more edges:

* for Script objects, traverse objects held in atoms in the atommap
* for Function objects, if FUN_INTERPRETED(fun) traverse objects held in atoms
  in the atommap of the script
* for an object's properties, walk scope property ids and any getters and setters

The patches combined seem to resolve the leaking of two windows when going to www.google.com/ig. I'll try them separately to see if any one is enough. This probably depends on the landing of bug 368773 to be really effective.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Both changes (atoms and getters/setters) are needed for this to be effective, so ended up combining everything into one patch. I can attach a diff -w, but it's not much more readable.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 257684 [details] [diff] [review]
v1

> {
>     jsval key;
> 
>-    if (atom->flags & ATOM_MARK)
>+    if (atom->flags & ATOM_MARK) {
>+        if (ATOM_IS_OBJECT(atom) && cx->runtime->gcThingCallback) {
>+            void *thing = ATOM_TO_OBJECT(atom);
>+            uint8 *flagp = js_GetGCThingFlags(thing);
>+            cx->runtime->gcThingCallback(thing, *flagp,
>+                                         cx->runtime->gcThingCallbackClosure);

Nit: house style avoids block locals, or at least (lately) puts a blank line between decls and stmts.

>-/* We avoid a large number of unnecessary calls by doing the flag check first */
> #define GC_MARK_ATOM(cx, atom)                                                \
>     JS_BEGIN_MACRO                                                            \
>-        if (!((atom)->flags & ATOM_MARK))                                     \
>-            js_MarkAtom(cx, atom);                                            \
>+        js_MarkAtom(cx, atom);                                                \
>     JS_END_MACRO

So there's no point in GC_MARK_ATOM; maybe we can remove it (more a reminder to igor and myself).

>+++ js/src/xpconnect/src/nsXPConnect.cpp	7 Mar 2007 15:07:14 -0000
>@@ -47,6 +47,9 @@
> #include "nsBaseHashtable.h"
> #include "nsHashKeys.h"
> #include "jsobj.h"
>+#include "jsscript.h"
>+#include "jsfun.h"
>+#include "jsatom.h"

Nit: alphabetize "js*.h" files as well as grouping them.

>+static void
>+TraverseJSScript(JSScript *script, nsCycleCollectionTraversalCallback &cb)
>+{
>+    JSAtomMap *map = &script->atomMap;
>+    uintN i, length = map->length;
>+    JSAtom **vector = map->vector;

Nit: xpconnect style would put a blank line here, I think.

>+            if(count > 0)
>+            {
>+                JS_snprintf(name, sizeof(name), "XPCNativeWrapper (%s)",
>+                            array[0]->GetNameString());
>+            }

Does XPConnect have a helper for this? Could you use nsIClassInfo to get the impl name instead of using the first interface's name?

>+            if (FUN_INTERPRETED(fun) && fun->u.i.script)
>+                TraverseJSScript(fun->u.i.script, cb);

Could just use FUN_SCRIPT(fun) in the if condition and the call.

>+    JSScope *scope = OBJ_SCOPE(obj);
>+    if (scope)
>+    {
>+        JSScopeProperty *sprop;
>+        for (sprop = SCOPE_LAST_PROP(scope); sprop; sprop = sprop->parent)
>+        {
>+            if (SCOPE_HAD_MIDDLE_DELETE(scope) &&
>+                !SCOPE_HAS_PROPERTY(scope, sprop))
>+                continue;
>+            jsid id = sprop->id;
>+            if (JSID_IS_OBJECT(id))
>+                cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT,
>+                                   JSID_TO_OBJECT(id));
>+            if (sprop->attrs & JSPROP_GETTER)
>+                cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT,
>+                                   JSVAL_TO_GCTHING((jsval)sprop->getter));
>+            if (sprop->attrs & JSPROP_SETTER)
>+                cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT,
>+                                   JSVAL_TO_GCTHING((jsval)sprop->setter));
>+        }
>+    }
>+

The scope may be shared among objects along the prototype chain, where the proto-children have not been mutated yet. Also, the sprops are shared in a runtime (heap) wide lexicographic tree. So I'm not sure that you want to note each as a child. To handle the first case (unmutated proto-kids), instead of

    if (scope)

you would test (and in xpconnect house style):

    if(scope && scope->object == obj)

To handle the sharing in rt->propertyTree of sprop nodes, I'm not sure what to do. It may not break Bacon-Rajan to treat each shared sprop as a child of every sharing object. Thoughts?

/be

Comment 3

[Brendan Eich [:brendan]]

Cc'ing Igor -- see comment 2.

/be

Comment 4

[Peter Van der Beken [:peterv]]

(In reply to comment #2)
> The scope may be shared among objects along the prototype chain, where the
> proto-children have not been mutated yet. Also, the sprops are shared in a
> runtime (heap) wide lexicographic tree. So I'm not sure that you want to note
> each as a child. To handle the first case (unmutated proto-kids), instead of
> 
>     if (scope)
> 
> you would test (and in xpconnect house style):
> 
>     if(scope && scope->object == obj)
> 
> To handle the sharing in rt->propertyTree of sprop nodes, I'm not sure what to
> do. It may not break Bacon-Rajan to treat each shared sprop as a child of every
> sharing object. Thoughts?

I don't think either is a problem, afaict js_Mark just marks all sprops for the object, so the gcThinkCallback is called for all the sprops of an object's scope, regardless of sharing. The traversal code needs to mimic the marking that happens, so I think the patch should be ok on this point. Make sense?
I'll take care of the other comments.

Comment 5

[Brendan Eich [:brendan]]

Yes, if we model everything in JS's heap as ref-counted, then the high degree of sharing via the property tree means high modeled ref-counts for objects owned by the sprops (id, getter, setter).

/be

Comment 6

[Peter Van der Beken [:peterv]]

I see some weird crashes, I think I should be checking OBJ_IS_NATIVE(obj) before traversing the scope.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> I see some weird crashes, I think I should be checking OBJ_IS_NATIVE(obj)
> before traversing the scope.

Quite -- sorry I missed that.

/be

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1.1

PConnect doesn't really have a helper to print the classname, XPCWrappedNative::ToString generates strings that are too long to be useful for the graphs.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 257888 [details] [diff] [review]
v1.1

Looks good -- only nit is that the xpconnect patches mix Foo* and Bar * pointer style (cuddling the * with the type sometimes, with the declarator name other times).

/be

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Yeah, I'm definitely having trouble using the correct coding styles in all these modules, sorry about that :-/.
Brendan: did you mean to mark this r+? I'm also going to need help checking in the mozilla/js/src part of this patch.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 257947 [details] [diff] [review]
v1.2

r=me, and I'm making you a member of the js partition.

/be

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 257947 [details] [diff] [review]
v1.2

sr=jst

Comment 13

[Peter Van der Beken [:peterv]]

Checked this in, but had to back out the nsXPConnect chunk because it turned the OS X test tinderbox orange, toolkit/themes/pinstripe/tests/test_bug371080.xul started timing out.

Comment 14

[Robert Sayre]

Attachment: console excerpts while quickly reloading the pinstripe xul test

###!!! ASSERTION: Fault in cycle collector: traversed refs exceed refcount (ptr: 1c92080)

I learned from the mp3 of the cycle collector conference call that is bad news. This is with peter's patch applied, but I think his patch might just make it easier to trigger. I had to back out my parser tests because they triggered this same test failure, probably by creating a lot of churn through data: iframes.

Comment 15

[Robert Sayre]

Attachment: live test case

this will assert and eventually crash the trunk if you reload enough, at least with all of attachment 257947 [details] [diff] [review] applied.

Comment 16

[Robert Sayre]

Attachment: stack of crash induced by frantic reloading of the test case

Comment 17

[Robert Sayre]

I should add that I am running linux at the moment. The test cases will fail if you are on linux or windows. This is turning the mac tinderbox orange because it is the only one that runs pinstripe tests--if we had an equivalent test on linux or windows, we would probably see it there too.

Comment 18

[Rob Campbell [:rc] (:robcee)]

I just saw something similar occur on the windows unit test box. The error message in the js console was that script="foo" was or contained a syntax error, even though this test has been run hundreds of times before.

see the <a href="http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1173720974.1173723925.11618.gz&fulltext=1">full log</a> for the freeze, though details there aren't really useful.

Comment 19

[Robert Sayre]

robcee has the mac unit test box running debug builds, so perhaps we can reland this and observe?

Comment 20

[Igor Bukanov]

AFAICS the last patch is incomplete. It does not traverse XML objects which store their XML children/parents/qnames in their private structures. Since one can store, for example, in qname or in XML functions an arbitrary object including a DOM wrapper, the traversal code must deal with them.

Comment 21

[Igor Bukanov]

Maybe we should really separate GC marking and code traversal in SpderMonkey? 

I.e. instead of traversal and marking at the same time the code would borrow a cycle collector idea about NoteChild and replace the marking semantic in various Mark function via a traversal one. That is, instead of implementing JS GC marking the GC-related things should provide a way to enumerate their GC-controllable children. 

Then js_GC would use the traversal code to implement the marking phase while the cycle collector would bridge it to make JS entities to participate in the cycle collection. 

I think it should be even possible without breaking public API compatibility as the arg argument of JS_MarkGCThing/JSMarkOp can be used to pass the child notification callback.

Comment 22

[Igor Bukanov]

I filed bug 375270 about making the marking phase of JS GC just one particular user of the traversal code.

Comment 23

[Peter Van der Beken [:peterv]]

The patch has been completely landed, but we did see some tinderbox orange. Both sayrer and I tried to add debugging code to figure out why the tinderbox goes orange, but adding debugging code fixed the orange :-/.

Comment 24

[Johnny Stenback (:jst)]

Any thoughts yet on whether the orange was due to actual problems or some weirdness with the testing system? Are we ready to close this bug yet?

Comment 25

[Peter Van der Beken [:peterv]]

I'm going to mark this fixed. I don't think we're still seeing the orange that we saw when this landed. Let's file a new bug if it returns.