1035654 - Leak with 'new TrackEvent'

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Run with XPCOM_MEM_LEAK_LOG=2
2. Load the testcase
3. Quit

Result: leak nsGlobalWindow, nsDocument

Comment 1

[Jesse Ruderman]

Attachment: leak info (trace-refcnt)

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The generated TrackEvent doesn't cycle collect anything.  That's unfortunate.

I suspect the problem is that we unroll once but here we have interfaces inside a union inside a nullable which requires unrolling twice.

http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#13908

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm, yes, indeed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So I think we have two fundamental implementation options here:

1)  Add some sort of traverse/trace/unlink methods to unions that CC would call and have NS_IMPL_CYCLE_COLLECTION_TRAVERSE/NS_IMPL_CYCLE_COLLECTION_UNLINK call them.  That doesn't really work for the tracing bit, though.

2)  Just hardcode the if-checks and getting of the value in the dictionary CC methods.

I guess we're ok otherwise because we don't support dictionary members of events yet, right?

Comment 5

[Boris Zbarsky [:bzbarsky]]

I suspect that approach #2 from comment 4 is uglier but simpler.

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

I'd like to try this bug. Since this is my first time working on DOM bug, it might take me some time to work out a solution.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Szu-Yu Chen [:aknow] from comment #6)
> I'd like to try this bug. Since this is my first time working on DOM bug, it
> might take me some time to work out a solution.

No worries.  This bug is in a new feature that is not used much so it's not urgent.

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> So I think we have two fundamental implementation options here:
> 
> 1)  Add some sort of traverse/trace/unlink methods to unions that CC would
> call and have
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE/NS_IMPL_CYCLE_COLLECTION_UNLINK call them.
> That doesn't really work for the tracing bit, though.
 
I'm thinking about option 1 by adding these kind of traverse/unlink functions for unions.
http://pastebin.mozilla.org/5565445

Because the union member are OwningNonNull, the pointers could not be directly point to null.
The only way I know to change the refcnt is calling destructor explicitly.  Is it a proper way?

Comment 9

[Szu-Yu Chen [:aknow] (inactive)]

Any suggestion for the CC related functions in Comment 8? If everything is fine, I could keep working on codegen script.

Comment 10

[Boris Zbarsky [:bzbarsky]]

You can probably Uninit() to unlink.  Calling the destructor of the union type itself is wrong.  If the union type friends the ImplCycleCollectionUnlink function, it should work out, I think.

The traverse _might_ work better if it delegates to ImplCycleCollectionTraverse on the union members instead of manually trying to do it.  That should work better if the union member is a sequence of interface pointers, say.  We presumably need to add an overload of ImplCycleCollectionTraverse that takes an OwningNonNull<T>, just like we have for nsRefPtr<T> already.

I was worrying about tracing, but looks like we don't allow the sort of unions that would need tracing in event members anyway so far. So let's not worry about that bit.

Comment 11

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Add a crashtest

Convert the testcase into a crashtest

Comment 12

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Add ImplCycleCollectionTraverse for OwningNonNull<T>

Comment 13

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Fix on generated files

A diff showing the expected fix on generated files.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8459417 [details] [diff] [review]
Part 1: Add a crashtest

This seems ok, though the cycle is a bit indirect.  Might be good to also add this to the testcase:

  track.expando = new TrackEvent("t", { "track": track })

in case other parts of the object graph change.  This should definitely catch the cycle in this case, right?

Maybe just have two crashtests, one with the cycle via window and one with the cycle directly like the above and verify both fail without the patch?

And add the missing newline at the end of the file, please.

r=me with that.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8459418 [details] [diff] [review]
Part 2: Add ImplCycleCollectionTraverse for OwningNonNull<T>

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8459422 [details] [diff] [review]
Fix on generated files

I think the forward-declaration for the function should come before the class declaration that friends the function.  Otherwise some compilers treat the friend declaration as a forward-declaration in the class scope and things get pretty confused.

In the ImplCycleCollectionTraverse impl it might be nice to take aName into account somehow, but I don't see a sane way to do that, so what you have is fine.

The rest looks great!  Thank you.

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

How could I run the codegen itself without the whole build?
I got some text in dom/bindings/doc/* but don't know how to let it work.

"""
Explicit method for performing codegen                               
   There needs to be an explicit method for invoking code generation.
   It needs to cover regular and test files.                         
                                                                     
   This is implemented via ``make export`` in ``dom/bindings``.      
"""

Comment 18

[Boris Zbarsky [:bzbarsky]]

  mach build dom/bindings/export

should work.

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3-1: Add hasGeckoInterface() in IDLType to fix the unroll() problem in IDLUnionType

The patch adds traverse/unlink in generated webidl type.

 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(TrackEvent, Event)  
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTrack)                           
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END                                 
              
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(TrackEvent, Event)    
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mTrack)                             
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END                                   

In the original code, IDLUnionType is not unrollable. However, it contains several different types. There is no simple way to unroll it and return just a single type. So, I introduce a new function hasGeckoInterface() for the purpose used in codegen.

I am still working on generating trace/unlink for union types.

Comment 20

[Szu-Yu Chen [:aknow] (inactive)]

> I am still working on generating trace/unlink for union types.
                                   ^^^^^ fix: should be traverse

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8461376 [details] [diff] [review]
Part 3-1: Add hasGeckoInterface() in IDLType to fix the unroll() problem in IDLUnionType

I think we should use a function in codegen instead of adding a new method on IDLType.  Something that would basically parallel the cases in getNativeTypeForIDLTType, so we make sure we don't miss anything.  Maybe something like this:

def idlTypeNeedsCycleCollection(type):
    type = type.unroll() # Takes care of sequences and nullables
    if ((type.isPrimitive and type.tag() in builtinNames) or
        type.isEnum() or
        # domstring, bytestring, etc
        ):
        return False
    elif type.isGeckoInterface():
        return True
    elif type.isUnion():
        return any(self.idlTypeNeedsCycleCollection(t) for t in type.flatMemberTypes)
    else:
        raise TypeError("Don't know whethr to cycle-collect type %s" %
                        type)

Comment 22

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3-1#2: Add cycle collection check for union

Yes, modifying in codegen is much better!!

I found that it is not so easy to list all the types that we don't have to do cycle collection with. In the beginning, I tried the proposal code but got a problem with wrapper type, which cannot be unrolled. At that time, it's neither a union nor a gecko interface, so it fall into the error case. I have no idea about how to work on this type.

So, in my patch, I choose a simple way to do it. Except union type, all the logic should be the same with previous design.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8462306 [details] [diff] [review]
Part 3-1#2: Add cycle collection check for union

> I found that it is not so easy to list all the types that we don't have to do
> cycle collection with.

You want to list the same exact set of types as getNativeTypeForIDLType does, as I said in comment 21.  Those are the types for which we might have members.

> At that time, it's neither a union nor a gecko interface, so it fall into the
> error case.

This is why you need the case of things that are known to not need cycle collection.  That case is pretty important!

> So, in my patch, I choose a simple way to do it.

This will cause leaks if we add support for a non-gecko-interface type that still needs CC, like dictionaries or mozmap with gecko interfaces in them.  Avoiding those leaks is why I suggested explicitly listing the types we know don't need CC, so if a new type of event attribute is added we will get codegen failures (and fix the codegen) instead of leaks.

Comment 24

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3-1#3: Add cycle collection check for union

Address the comment. How about this version?

Comment 25

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8462360 [details] [diff] [review]
Part 3-1#3: Add cycle collection check for union

Review of attachment 8462360 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +49,5 @@
>              # Interface types are only copy-constructible if they're Gecko
>              # interfaces.  SpiderMonkey interfaces are not copy-constructible
>              # because of rooting issues.
>              (type.isInterface() and type.isGeckoInterface()))
>  

I will add the empty line back

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8462360 [details] [diff] [review]
Part 3-1#3: Add cycle collection check for union

>-
> def wantsAddProperty(desc):

That blank line was there on purpose.  Please don't take it out.

The rest looks great. Thanks!  r=me

Comment 27

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3-2: Generate cycle collection traverse/unlink for unions

I have moved idlTypeNeedsCycleCollection into the global scope before I work on this patch. So the change is not explicitly shown here.

Three additional types: Dictionary, MozMap, Callback are added into the non-cycle-collected group. Is it correct? They are used as the inner type of a union. Therefore I need to determine whether they need cycle-collected.

Comment 28

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Diff of generated UnionType.h and UnionType.cpp

Diff results after patch Part 3-2

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8463258 [details] [diff] [review]
Part 3-2: Generate cycle collection traverse/unlink for unions

A callback _does_ need cycle collection, and a dictionary or mozmap might need it as well, depending on their members.  You basically want to check whether the types of the dictionary members, and the type the mozmap is parametrized over, need CC (and if they do, probably just fail codegen for the moment).

Comment 30

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #29)
> Comment on attachment 8463258 [details] [diff] [review]
> Part 3-2: Generate cycle collection traverse/unlink for unions
> 
> A callback _does_ need cycle collection, and a dictionary or mozmap might
> need it as well, depending on their members.  You basically want to check
> whether the types of the dictionary members, and the type the mozmap is
> parametrized over, need CC (and if they do, probably just fail codegen for
> the moment).

OK but I still have a problem. Some unions might contain a "EventHandlerNonNull". Does it need cycle collection?

And for a dictionary or mozmap, we have to check their members. That means we can add two cases in idlTypeNeedsCycleCollection, just like we do for union type, right?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Does it need cycle collection?

Yes, most definitely.

> That means we can add two cases in idlTypeNeedsCycleCollection, just like we do for union
> type, right?

Yep.

Comment 32

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3-2#2: Generate cycle collection traverse/unlink for unions

The generated code is a bit ugly. Will it be better to create a macro for declaring these travese/unlink functions?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8464566 [details] [diff] [review]
Part 3-2#2: Generate cycle collection traverse/unlink for unions

>+    if not type.isType():

You should never see a non-type here.

>+    elif type.isDictionary() and type.isWrapper():

Why do you need the isWrapper() bit?  I don't think you do.  Is this because you were passing non-types in and exprimenting?

>+        if any(idlTypeNeedsCycleCollection(t) for t in type.inner.members):

You want 

  any(idlTypeNeedsCycleCollection(m.type) for m in type.inner.members)

no?  Is this why you were seeing non-types coming into the method?

>+                declarations.add(("mozilla::dom::Owning" + name, False))

Using "name" here is not obviously right to me.  What ensures this matches the actual type name that will be used for the union?

Seems better to use:

   declarations.add(("mozilla::dom::%s" % CGUnionStruct.unionTypeName(t, True),
                     False))

>+class CGCycleCollectionTraverseForOwningUnionMethod(CGAbstractMethod):

>+        self.ownsMembers = True

Why is this needed?  Just pass True at the one place you use it.

>+                Argument("Owning%s&" % str(type), "aUnion"),

Again, use CGUnionStruct.unionTypeName.

>+        CGAbstractMethod.__init__(self, None, "ImplCycleCollectionTraverse", "void", args)

I wonder whether it makes sense to make this inline...

>+    def definition_body(self):
>+        traverseTypes = []

More like memberNames, right?

>+                vars = getUnionTypeTemplateVars(self.type,

This seems like overkill if all you need is the member name.  Just use getUnionMemberName(t).  Then you don't need a descriptorProvider either.

Please assert that at the end of the loop over flatMemberTypes you end up with a nonempty memberNames.

>+        ifStaments = [CGIfWrapper(CGGeneric(functionCallTemplate % (t, t)),

Probably want parens instead of square brackets around this definition (to produce a generator, not a list, since an iterable is all CGElseChain wants).

>+            friend = "  friend void ImplCycleCollectionUnlink(Owning%s& aUnion);\n" % str(self.type)

Again, CGUnionStruct.unionTypeName.

>+++ b/dom/bindings/parser/WebIDL.py

You don't need the changes to this file.

Does this patch address the first sentence of comment 16?  It doesn't seem to...

Comment 34

[Szu-Yu Chen [:aknow] (inactive)]

> You want 
> 
>   any(idlTypeNeedsCycleCollection(m.type) for m in type.inner.members)
> 
> no?  Is this why you were seeing non-types coming into the method?

Oh Yes! It's indeed the problem I got. Thanks for the correction.

> >+        CGAbstractMethod.__init__(self, None, "ImplCycleCollectionTraverse", "void", args)
> 
> I wonder whether it makes sense to make this inline...

Did you mean it might be better to inline the function? If so, unlink function should also be inlined, right?
 
> Does this patch address the first sentence of comment 16?  It doesn't seem
> to...

Yes, did I miss anything or misunderstand your point?

In generated UnionTypes.h, it will be something like this

====
// forward declaration for the class which is used in the function
class OwningVideoTrackOrAudioTrackOrTextTrack;

// forward declaration for the function
void
ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& aCallback, OwningVideoTrackOrAudioTrackOrTextTrack aUnion, const char* aName, uint32_t aFlags = 0);

void
ImplCycleCollectionUnlink(OwningVideoTrackOrAudioTrackOrTextTrack aUnion);

// class declaration that friend the function
class OwningVideoTrackOrAudioTrackOrTextTrack : public AllOwningUnionBase
{
  friend void ImplCycleCollectionUnlink(OwningVideoTrackOrAudioTrackOrTextTrack& aUnion);
  enum Type
  {
    eUninitialized,
====

Comment 35

[Boris Zbarsky [:bzbarsky]]

> Did you mean it might be better to inline the function? 

I mean just pass inline=True to the CGAbstractMethod.__init__.

> If so, unlink function should also be inlined, right?

Yup.

>// forward declaration for the function

This is what I want, yes (though I think you only need the forward-decl for unlink).  What's generating that forward-declaration?

Comment 36

[Szu-Yu Chen [:aknow] (inactive)]

> This is what I want, yes (though I think you only need the forward-decl for
> unlink).  What's generating that forward-declaration?

Here in UnionTypes(), just put the function before class.

    return (headers, implheaders, declarations,
            CGList(SortedDictValues(traverseMethods) +
                   SortedDictValues(unlinkMethods) +
                   SortedDictValues(unionStructs) +
                   SortedDictValues(owningUnionStructs),

But it seems that I have to find another way for forward declaration.
In order to inline the function, we have to modify the order of code:

forward-decl for unlink
union class that friend the unlink
inline definition of unlink

We can't put the function definition before the union class. The union class should be completed first to be used in unlink.

So, both decl and definition of unlink will appear the different location of UnionTypes.h. Is there an easy way to achieve it by current codegen architecture?

The idea in my mind is to create another CGClass for forward-decl function. Then change the return statement to

    return (headers, implheaders, declarations,
            CGList(SortedDictValues(unlinkMethodsForwardDelcaration) +
                   SortedDictValues(unionStructs) +
                   SortedDictValues(owningUnionStructs) +
                   SortedDictValues(traverseMethods) +
                   SortedDictValues(unlinkMethods) +

Comment 37

[Boris Zbarsky [:bzbarsky]]

> Here in UnionTypes(), just put the function before class.

Aha!  Right, that's a good-ish reason to have the unlink function not inline, with a comment saying why it is so and that the ordering here is really important.  Let's not worry about overcomplicating things just so we can inline the unlink.

On the other hand, do please go ahead and inline the traverse, since it can just come after the structs.

Comment 38

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #37)
> > Here in UnionTypes(), just put the function before class.
> 
> Aha!  Right, that's a good-ish reason to have the unlink function not
> inline, with a comment saying why it is so and that the ordering here is
> really important.  Let's not worry about overcomplicating things just so we
> can inline the unlink.
> 
> On the other hand, do please go ahead and inline the traverse, since it can
> just come after the structs.

I'm not sure whether it is a good idea to inline traverse functions.

Because all of the functions are inlined: traverse, ImplCycleCollectionTraverse, CycleCollectionNoteChild, ... In the end, in UnionTypes.h, instead of forward declaration of the class used in union, we have to really include their header.  Then, the worse part is that not all the added headers are viewable by cpp that including UnionTypes.h.  That means we have to modify the include path for that cpp but it might not really make sense...

Comment 39

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3-2#3: Generate cycle collection traverse/unlink for unions

This is the version w/o inline.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465317 [details] [diff] [review]
Part 3-2#3: Generate cycle collection traverse/unlink for unions

OK, I agree that if we'd have to change the set of headers we include we shouldn't inline the methods.

>+    # The order of itmes in CGList is important.

"items".

>+    # for theses methods should come before the class declaration. Otherwise

"for these methods".

>+class CGCycleCollectionTraverseForOwningUnionMethod(CGAbstractMethod):
>+    def __init__(self, type, descriptorProvider):

The descriptorProvider bit here is unused.  Please take it out.

r=me with that.  Thanks!

Comment 41

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 1: Add a crashtest. r=bzbarsky

Comment 42

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 2: Add ImplCycleCollectionTraverse for OwningNonNull<T>. r=bzbarsky

Comment 43

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 3: Generate cycle collection traverse/unlink for unions. r=bzbarsky

Part 3-1 and Part 3-2 are squashed together.

Comment 44

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=a07bda3cb31f

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0ffdf2da54
https://hg.mozilla.org/integration/mozilla-inbound/rev/540f9f9f3072
https://hg.mozilla.org/integration/mozilla-inbound/rev/56323c08e889

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e0ffdf2da54
https://hg.mozilla.org/mozilla-central/rev/540f9f9f3072
https://hg.mozilla.org/mozilla-central/rev/56323c08e889