Closed
Bug 1035654
Opened 10 years ago
Closed 10 years ago
Leak with 'new TrackEvent'
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: jruderman, Assigned: aknow, Mentored)
References
Details
(Keywords: memory-leak, testcase, Whiteboard: [lang=python][MemShrink:P3])
Attachments
(5 files, 10 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
1. Run with XPCOM_MEM_LEAK_LOG=2
2. Load the testcase
3. Quit
Result: leak nsGlobalWindow, nsDocument
Reporter | ||
Comment 1•10 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
Comment 3•10 years ago
|
||
Hmm, yes, indeed.
Comment 4•10 years ago
|
||
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?
Updated•10 years ago
|
Mentor: bzbarsky
Whiteboard: [lang=python]
Assignee | ||
Comment 6•10 years ago
|
||
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]
Assignee | ||
Comment 8•10 years ago
|
||
(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?
Assignee | ||
Comment 9•10 years ago
|
||
Any suggestion for the CC related functions in Comment 8? If everything is fine, I could keep working on codegen script.
Flags: needinfo?(bzbarsky)
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
Convert the testcase into a crashtest
Attachment #8459417 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8459418 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•10 years ago
|
||
A diff showing the expected fix on generated files.
Attachment #8459422 -
Flags: feedback?(bzbarsky)
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
Comment on attachment 8459418 [details] [diff] [review]
Part 2: Add ImplCycleCollectionTraverse for OwningNonNull<T>
r=me
Attachment #8459418 -
Flags: review?(bzbarsky) → review+
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
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•10 years ago
|
||
mach build dom/bindings/export
should work.
Assignee | ||
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
> I am still working on generating trace/unlink for union types.
^^^^^ fix: should be traverse
Comment 21•10 years ago
|
||
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-
Assignee | ||
Comment 22•10 years ago
|
||
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 23•10 years ago
|
||
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-
Assignee | ||
Comment 24•10 years ago
|
||
Address the comment. How about this version?
Attachment #8462306 -
Attachment is obsolete: true
Attachment #8462360 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 25•10 years ago
|
||
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•10 years ago
|
||
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+
Assignee | ||
Comment 27•10 years ago
|
||
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)
Assignee | ||
Comment 28•10 years ago
|
||
Diff results after patch Part 3-2
Comment 29•10 years ago
|
||
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-
Assignee | ||
Comment 30•10 years ago
|
||
(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•10 years ago
|
||
> 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.
Assignee | ||
Comment 32•10 years ago
|
||
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 33•10 years ago
|
||
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-
Assignee | ||
Comment 34•10 years ago
|
||
> 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•10 years ago
|
||
> 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?
Assignee | ||
Comment 36•10 years ago
|
||
> 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•10 years ago
|
||
> 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.
Assignee | ||
Comment 38•10 years ago
|
||
(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...
Assignee | ||
Comment 39•10 years ago
|
||
This is the version w/o inline.
Attachment #8464566 -
Attachment is obsolete: true
Attachment #8465317 -
Flags: review?(bzbarsky)
Comment 40•10 years ago
|
||
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+
Assignee | ||
Comment 41•10 years ago
|
||
Attachment #8459417 -
Attachment is obsolete: true
Attachment #8466098 -
Flags: review+
Assignee | ||
Comment 42•10 years ago
|
||
Attachment #8459418 -
Attachment is obsolete: true
Attachment #8466099 -
Flags: review+
Assignee | ||
Comment 43•10 years ago
|
||
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+
Assignee | ||
Comment 44•10 years ago
|
||
Keywords: checkin-needed
Comment 45•10 years ago
|
||
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
Flags: in-testsuite+
Keywords: checkin-needed
Comment 46•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3e0ffdf2da54
https://hg.mozilla.org/mozilla-central/rev/540f9f9f3072
https://hg.mozilla.org/mozilla-central/rev/56323c08e889
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•