Leak with 'new TrackEvent'

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jruderman, Assigned: aknow, Mentored)

Tracking

(Blocks 2 bugs, {memory-leak, testcase})

Trunk
mozilla34
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=python][MemShrink:P3])

Attachments

(5 attachments, 10 obsolete attachments)

325 bytes, text/html
Details
29.12 KB, text/plain
Details
1.69 KB, patch
aknow
: review+
Details | Diff | Splinter Review
1.23 KB, patch
aknow
: review+
Details | Diff | Splinter Review
10.62 KB, patch
aknow
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
Posted file testcase
1. Run with XPCOM_MEM_LEAK_LOG=2
2. Load the testcase
3. Quit

Result: leak nsGlobalWindow, nsDocument
Reporter

Comment 1

5 years ago
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
Component: Video/Audio → DOM: Events
Hmm, yes, indeed.
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?
Mentor: bzbarsky
Whiteboard: [lang=python]
I suspect that approach #2 from comment 4 is uglier but simpler.
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.
Assignee: nobody → szchen
(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.
Whiteboard: [lang=python] → [lang=python][MemShrink:P3]
(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?
Any suggestion for the CC related functions in Comment 8? If everything is fine, I could keep working on codegen script.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Posted patch Part 1: Add a crashtest (obsolete) — Splinter Review
Convert the testcase into a crashtest
Attachment #8459417 - Flags: review?(bzbarsky)
Posted patch Fix on generated files (obsolete) — Splinter Review
A diff showing the expected fix on generated files.
Attachment #8459422 - Flags: feedback?(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.
Attachment #8459417 - Flags: review?(bzbarsky) → review+
Comment on attachment 8459418 [details] [diff] [review]
Part 2: Add ImplCycleCollectionTraverse for OwningNonNull<T>

r=me
Attachment #8459418 - Flags: review?(bzbarsky) → review+
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.
Attachment #8459422 - Flags: feedback?(bzbarsky) → feedback+
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``.      
"""
  mach build dom/bindings/export

should work.
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.
Attachment #8461376 - Flags: review?(bzbarsky)
> I am still working on generating trace/unlink for union types.
                                   ^^^^^ fix: should be traverse
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)
Attachment #8461376 - Flags: review?(bzbarsky) → review-
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.
Attachment #8461376 - Attachment is obsolete: true
Attachment #8462306 - Flags: review?(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.
Attachment #8462306 - Flags: review?(bzbarsky) → review-
Address the comment. How about this version?
Attachment #8462306 - Attachment is obsolete: true
Attachment #8462360 - Flags: review?(bzbarsky)
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 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
Attachment #8462360 - Flags: review?(bzbarsky) → review+
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.
Attachment #8463258 - Flags: review?(bzbarsky)
Diff results after patch Part 3-2
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).
Attachment #8463258 - Flags: review?(bzbarsky) → review-
(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?
> 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.
The generated code is a bit ugly. Will it be better to create a macro for declaring these travese/unlink functions?
Attachment #8463258 - Attachment is obsolete: true
Attachment #8464566 - Flags: review?(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...
Attachment #8464566 - Flags: review?(bzbarsky) → review-
> 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,
====
> 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?
> 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) +
> 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.
(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...
This is the version w/o inline.
Attachment #8464566 - Attachment is obsolete: true
Attachment #8465317 - Flags: review?(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!
Attachment #8465317 - Flags: review?(bzbarsky) → review+
Attachment #8459417 - Attachment is obsolete: true
Attachment #8466098 - Flags: review+
Part 3-1 and Part 3-2 are squashed together.
Attachment #8459422 - Attachment is obsolete: true
Attachment #8462360 - Attachment is obsolete: true
Attachment #8463259 - Attachment is obsolete: true
Attachment #8465317 - Attachment is obsolete: true
Attachment #8466100 - Flags: review+
You need to log in before you can comment on or make changes to this bug.