Last Comment Bug 658632 - Provide easy way to add non-leaky classinfo to JS components
: Provide easy way to add non-leaky classinfo to JS components
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks: 637099
  Show dependency treegraph
 
Reported: 2011-05-20 12:49 PDT by Peter Van der Beken [:peterv]
Modified: 2012-03-14 02:30 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (26.36 KB, patch)
2011-05-20 12:49 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Review
v1.1 (26.94 KB, patch)
2011-06-06 14:32 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Review
v1.2 (31.17 KB, patch)
2011-06-08 09:33 PDT, Peter Van der Beken [:peterv]
gavin.sharp: review+
Details | Diff | Review

Description Peter Van der Beken [:peterv] 2011-05-20 12:49:42 PDT
Created attachment 534072 [details] [diff] [review]
v1

A number of JS components implement nsIClassInfo, but they implement it in the component itself. This can easily lead to bloat, as we often hold on to nsIClassInfo for much longer than we need the component (until shutdown). It can also lead to shutdown leaks, if we end up with a cycle involving the component. We should add support to XPCOMUtils to easily add nsIClassInfo support to a JS component, implemented in a separate (singleton) object.
Patch coming up (still need to run some tests).
Comment 1 Peter Van der Beken [:peterv] 2011-06-06 14:32:34 PDT
Created attachment 537645 [details] [diff] [review]
v1.1
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-06 17:39:13 PDT
Comment on attachment 537645 [details] [diff] [review]
v1.1

>diff --git a/js/src/xpconnect/loader/XPCOMUtils.jsm b/js/src/xpconnect/loader/XPCOMUtils.jsm

>+  generateCI: function XPCU_generateCI(classInfo)

>+    _interfaces.push(Ci.nsISupports);

Should this also add nsIClassInfo?

>+    return {

>+      contractID: classInfo.contractID,

You seem to not be specifying contractID properties on the objects passed to generateCI... I guess that apparently doesn't matter in practice (I see some components just use "null", others omit it). If that's the case, is there use to allowing it to be specified?

>+      QueryInterface: this.generateQI([Ci.nsIClassInfo])

Hmm, is this going to break getService(nsIClassInfo).QI(nsIFoo) for these components? Presumably that also never matters in practice, I guess. Is there precedent for doing something like this (tear-off nsIClassInfo without a QI impl that will go the other way)?

>   generateNSGetFactory: function XPCU_generateNSGetFactory(componentsArray) {

>-        if (!(component.prototype.classID instanceof Components.ID))
>+        var classID;
>+        if ("classInfo" in component.prototype)
>+          classID = component.prototype.classInfo.classID;
>+        else
>+          classID = component.prototype.classID;

You could avoid this change (and the classID removals) by having generateCI return a getClassInfo function rather than an object (as with generateQI), and have that function just use this.classID (and change makeQI accordingly). That removes some of the potential confusion from some components not having any visible classID properties (which has always been a requirement).

It would be nice to also be able to avoid specifying the interface list twice, though I guess that's a bit harder. I suppose you could just add an "_interfaces" property on the relevant components and have them pass that to both generateCI and generateQI.
Comment 3 Peter Van der Beken [:peterv] 2011-06-07 02:43:12 PDT
(In reply to comment #2)
> Should this also add nsIClassInfo?

I don't think it makes sense to expose nsIClassInfo, especially since it looks like a number of these objects are exposed through the DOM. In fact, we probably shouldn't even expose nsISupports, I'll remove the auto-adding of nsISupports.

> You seem to not be specifying contractID properties on the objects passed to
> generateCI... I guess that apparently doesn't matter in practice (I see some
> components just use "null", others omit it). If that's the case, is there
> use to allowing it to be specified?

I just converted the existing properties, but I guess it makes sense to actually expose these contract IDs. I've copied the ones I found from the manifests.

> 
> >+      QueryInterface: this.generateQI([Ci.nsIClassInfo])
> 
> Hmm, is this going to break getService(nsIClassInfo).QI(nsIFoo) for these
> components?

Yes.

> Presumably that also never matters in practice, I guess. Is
> there precedent for doing something like this (tear-off nsIClassInfo without
> a QI impl that will go the other way)?

Yes, it's how the C++ implementation using macros does it too (see xpcom/glue/nsClassInfoImpl.cpp). It was explicitly decided to allow breaking XPCOM identity rules for this when nsIClassInfo was originally introduced.

> You could avoid this change (and the classID removals) by having generateCI
> return a getClassInfo function rather than an object (as with generateQI),
> and have that function just use this.classID (and change makeQI
> accordingly). That removes some of the potential confusion from some
> components not having any visible classID properties (which has always been
> a requirement).

What makes classID special? You could argue that we'd specify all the properties on the component's prototype. We'd have to cache the classinfo object somewhere to avoid recreating it all the time, but not on the component itself or we're back where we started. I guess we could do in the component's prototype:

|get classInfo() { return XPCOMUtils.generateCI(this); },|

with generateCI a lazy getter redefining classInfo on the prototype. Not sure I like this syntax though.

> It would be nice to also be able to avoid specifying the interface list
> twice, though I guess that's a bit harder.

I don't think this should be a goal, I think we need to explicitly decide which interfaces to expose on the flattened object (again, some of these seem to be exposed through the DOM).
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-07 13:59:14 PDT
(In reply to comment #3)
> I don't think it makes sense to expose nsIClassInfo, especially since it
> looks like a number of these objects are exposed through the DOM. In fact,
> we probably shouldn't even expose nsISupports, I'll remove the auto-adding
> of nsISupports.

I don't understand this. What's the advantage to having the list of interfaces be incomplete?

> What makes classID special? You could argue that we'd specify all the
> properties on the component's prototype.

It's special because there is a current requirement for all components to have that property anyways - not true of any of the others.

> We'd have to cache the classinfo
> object somewhere to avoid recreating it all the time, but not on the
> component itself or we're back where we started.

Does "recreating it all the time" actually make a significant difference? I guess it could. How about just making the classID a global constant and then using that to specify it both on the prototype and for passing to generateCI? I guess this doesn't matter that much either way, but the magic of having generateNSGetFactory depend on side-effects of generateCI bothers me (very non-obvious dependency).

> I don't think this should be a goal, I think we need to explicitly decide
> which interfaces to expose on the flattened object (again, some of these
> seem to be exposed through the DOM).

Well, in the examples in that patch the list seems to always be the same, so I'm not sure I understand the benefit to this extra flexibility (I guess this goes back to my first question).
Comment 5 Peter Van der Beken [:peterv] 2011-06-07 14:24:44 PDT
(In reply to comment #4)
> I don't understand this. What's the advantage to having the list of
> interfaces be incomplete?

The list of interfaces in the classinfo is used to determine what properties are exposed on the flattened object, accessible without QIing the object. For objects exposed through the DOM to content those are the interfaces whose properties are accessible to content. There's probably not much harm in exposing nsIClassInfo's properties though it does increase the possibility of something going wrong, and I don't see what the benefit is. What benefit is there to exposing classID on window.sidebar (as an example)?

> It's special because there is a current requirement for all components to
> have that property anyways - not true of any of the others.

Sure, but only to make generateNSGetFactory work, and I fixed generateNSGetFactory.

> Does "recreating it all the time" actually make a significant difference? I
> guess it could. How about just making the classID a global constant and then
> using that to specify it both on the prototype and for passing to
> generateCI? I guess this doesn't matter that much either way, but the magic
> of having generateNSGetFactory depend on side-effects of generateCI bothers
> me (very non-obvious dependency).

How does it depend on it? Either generateCI is used and we get the classID through the classInfo or we get it off the component, generateNSGetFactory returns exactly the same result in both cases. Personally I don't see why classID should be exposed on components, it something that clearly belongs in classinfo but I guess there are too many components that don't have classinfo but that want to use generateNSGetFactory.
Comment 6 Peter Van der Beken [:peterv] 2011-06-07 14:28:35 PDT
(In reply to comment #4)
> I don't understand this. What's the advantage to having the list of
> interfaces be incomplete?

BTW, I don't know of any C++ class with classinfo that returns either nsIClassInfo or nsISupports in its interfaces list. Why should JS components be different?
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-07 16:18:21 PDT
(In reply to comment #5)
> What benefit is there to exposing classID on window.sidebar (as an example)?

There's no benefit, obviously - that's a good concrete example of why omitting it is a good idea. You see nsIClassInfo as nothing other than a way for xpconnect to determine what interfaces to flatten, whereas I was looking at it from a different point of view (it should fully/accurately describe the component's implemented interfaces). I suppose that isn't particularly useful in practice, though, so I concede the point. 

> How does it depend on it?

"you need a classID property for use with generateNSGetFactory, but only if you don't also use generateCI" is a complicated requirement for users of XPCOMUtils to sort out (people will get it wrong). "you need a classID property" is much simpler - that's all. I don't really think the win from only specifying it once trumps that, so that's why I suggested just using a constant and putting it in both places.
Comment 8 Peter Van der Beken [:peterv] 2011-06-08 09:33:52 PDT
Created attachment 538038 [details] [diff] [review]
v1.2

(In reply to comment #7)
> I was looking
> at it from a different point of view (it should fully/accurately describe
> the component's implemented interfaces).

I guess another way to look at it would be that nsISupports and nsIClassInfo are "infrastructure" interfaces, plus you already know the component supports them (nsISupports because it's an XPCOM component, nsIClassInfo because you're using it to get the component's implemented interfaces).

Personally I think needing to specify the classID twice is much easier to get wrong, but whatever.

Added some more docs and added support for specifying interfaces by name (since that's supported in generateQI). Removed nsISupports from the interfaces array. Added constants for the CIDs and added them both to the components and the classinfos.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-08 12:08:58 PDT
Comment on attachment 538038 [details] [diff] [review]
v1.2

>diff --git a/browser/components/feeds/src/WebContentConverter.js b/browser/components/feeds/src/WebContentConverter.js

>+  classInfo: XPCOMUtils.generateCI({classID: WCCR_CLASSID,
>+                                    contractID: WCCR_CONTRACTID,
>+                                    interfaces: [Ci.nsIWebContentConverterService,
>+                                                 Ci.nsIWebContentHandlerRegistrar,
>+                                                 Ci.nsIObserver, Ci.nsIFactory],

nsIFactory here doesn't seem useful (I don't know why this component tries to implement it to begin with). But I guess that can be cleaned up separately in a followup.

>diff --git a/js/src/xpconnect/loader/XPCOMUtils.jsm b/js/src/xpconnect/loader/XPCOMUtils.jsm

>+ *    // [optional] classInfo implementation, e.g. using the generateCIhelper.

space before "helper"

>   /**
>+   * Generate a ClassInfo implementation for a component. The returned object
>+   * must be assigned to the 'classInfo' property of a JS object. The first and
>+   * only argument should be an object that contains a number of optional
>+   * properties: "interfaces", "contractID", "classDescription", "classID" and
>+   * "flags". The values of the properties will be returned as the values of the
>+   * various properties of the nsIClassInfo implementation.
>+   */

"interfaces" isn't really optional (arguably "classID" isn't either, but I guess things might not actually break)

> function makeQI(interfaceNames) {
>   return function XPCOMUtils_QueryInterface(iid) {
>     if (iid.equals(Ci.nsISupports))
>       return this;
>+    if (iid.equals(Ci.nsIClassInfo) && "classInfo" in this)
>+      return this.classInfo;

Little bit worried that this migth cause trouble if people have existing "classInfo" properties on their components, but I guess that's unlikely.

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js

>+  classID:

this is broken

r=me with those addressed.
Comment 10 Peter Van der Beken [:peterv] 2011-06-09 02:40:06 PDT
(In reply to comment #9)
> "interfaces" isn't really optional (arguably "classID" isn't either, but I
> guess things might not actually break)

Both are optional, if a class shouldn't be instantiated through XPCOM classID can be omitted (see the geolocation stuff) and not having interfaces might make sense too (opaque object that can be passed to/from content).
Comment 11 Peter Van der Beken [:peterv] 2011-06-09 07:59:35 PDT
http://hg.mozilla.org/mozilla-central/rev/c6b2050b04ef
Comment 12 neil@parkwaycc.co.uk 2011-08-28 09:25:37 PDT
This broke Thunderbird and SeaMonkey. Thunderbird was luckier because they had tests that this broke...
Comment 13 Peter Van der Beken [:peterv] 2011-08-29 00:14:16 PDT
(In reply to neil@parkwaycc.co.uk from comment #12)
> This broke Thunderbird and SeaMonkey. Thunderbird was luckier because they
> had tests that this broke...

Maybe you could post how or reference the bug? This wasn't supposed to break anything, so knowing how would actually be helpful.
Comment 14 Wladimir Palant 2012-03-14 02:30:50 PDT
Added documentation: https://developer.mozilla.org/en/JavaScript_code_modules/XPCOMUtils.jsm#generateCI%28%29 (also added to the "Class declaration" code example).

Note You need to log in before you can comment on or make changes to this bug.