381651 - Tweak XPCOMUtils.jsm

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Nickolay_Ponomarev]

Attachment: patch

Three main changes:
1. Eliminate the separate 'class descriptor' objects. Instead XPCOMUtils now uses the various properties on ComponentConstructor.prototype. The properties are named after the relevant nsIClassInfo attributes, so that implementing that takes less effort.
2. Use the default factory that instantiates the component via |new Component| by default. Always calls QI on the component instance.
3. Provide a generateQI utility.

These changes together mean smaller registration boilerplate in the common case. Also, personally I think it's easier to read the code when the contractID is listed near the component implementation.

I also plan to provide a simplified way to register for categories, but decided to do this in a separate patch.

Comment 1

[Robert Sayre]

The functionality looks good, but it's going to leak IIDs at the very least. First, it's important for the functions returned by getFactory and GenerateQI to be declared outside of the block. Otherwise, the closure will contain IIDs, creating a cycle. The way to beat this is to get the string names of the interfaces, and pass them to a function declared outside the block.

function makeQI() {
  return function {...}
}

function generateQI(interfaces) {
  return makeQI([i.name for each(i in interfaces)]); 
}

Comment 2

[Alex Vincent [:WeirdAl]]

Comment on attachment 265730 [details] [diff] [review]
patch

>Index: js/src/xpconnect/loader/XPCOMUtils.jsm
>+  generateQI: function(interfaces) {
>+  generateModule: function(componentsArray, postRegister, preUnregister) {

Please name your functions:
generateQI: function generateQI(interfaces)

>+      classes[proto.classID] = {
>+        constructor: component,
>+        className: proto.classDescription,
>+        contractID: proto.contractID,
>+        factory: this._getFactory(component),
>+      };
>+    }

...

>+          compMgr.registerFactoryLocation(classes[i].cid,
>+                                          classes[i].className,
>+                                          classes[i].contractID,

...
>+          compMgr.unregisterFactoryLocation(classes[i].cid, fileSpec);

Hm, I don't think you actually define a .cid property on each member of the classes array.

>+  _getFactory: function(component) {

>+        createInstance: function(outer, iid) {
Again, please name your functions.

Comment 3

[Brendan Eich [:brendan]]

Name your functions for venkman or firebug? Is that really necessary for firebug? It adds redundancy for no other benefit. I think debuggers should cope.

/be

Comment 4

[Nickolay_Ponomarev]

(In reply to comment #1)
> The functionality looks good

Thanks, I mainly wanted to see if there are objections to the functional changes.

> but it's going to leak IIDs at the very least.

That makes little sense to me, though I haven't investigated this. What cycle are you talking about - what does nsJS(I)ID keep alive while we're referencing it?

(In reply to comment #2)
> Hm, I don't think you actually define a .cid property on each member of the
> classes array.
> 
That's a good catch, thanks! Over-relying on tests passing is a bad thing.

Comment 5

[Robert Sayre]

(In reply to comment #4)
> 
> That makes little sense to me, though I haven't investigated this. What cycle
> are you talking about - what does nsJS(I)ID keep alive while we're referencing
> it?

The best explanation I have seen is given here:
http://www.mozilla.org/scriptable/avoiding-leaks.html#dont-cycles

These types of leaks are extremely hard to visualize, because JS is normally a GCed language. It is worth it to solve this once, for everyone.

Comment 6

[Nickolay_Ponomarev]

Attachment: patch, v2

OK, I went with your suggestion about keeping an array of interface names internally, but I really don't think this should be necessary (btw I've read the document you point to long ago, I don't think it answers the question of why this leak happens).

Comment 7

[Alex Vincent [:WeirdAl]]

Comment on attachment 266266 [details] [diff] [review]
patch, v2

Unofficial r+ from me: I don't spot anything really wrong this time, except for styling nits:

>Index: js/src/xpconnect/loader/XPCOMUtils.jsm
>+    for each(let component in componentsArray) {

A space before the ( character, please

>+        compMgr.QueryInterface(Ci.nsIComponentRegistrar);

I'm not sure this line's necessary, but it doesn't hurt either.  I would normally suggest (compMgr instanceof Ci.nsIComponentRegistrar), but here I think you really do want it to propagate an exception.  So this is good imho.

>+
>       unregisterSelf: function(compMgr, fileSpec, location) {

Might I suggest putting in a comment identifying which interface each method comes from?  Sure, it's well known to us advanced people that registerSelf, unregisterSelf, etc., come from nsIModule, but someone just starting out probably wouldn't:

+      // nsIModule
       unregisterSelf: function(compMgr, fileSpec, location) {

>+    for each(let component in componentsArray) {
>+      classes.push({
>+        cid:          component.prototype.classID,
>+        constructor:  component,
>+        className:    component.prototype.classDescription,
>+        contractID:   component.prototype.contractID,
>+        factory:      this._getFactory(component),
>+      });
>+    }

I don't see why you store the component in the constructor property.  You've got everything else you need for the classes here.


>+        for each(let classDesc in classes) {

A space before the ( character, please.

>+  /**
>+   * Returns a factory for |component|.
>+   */
>+  _getFactory: function(component) {

Give it a complete JavaDoc, please (@param, @return).  Again, sure, it's obvious, but let's do it right if we're going to do it.

>+    var factory = component.prototype._xpcom_factory;
>+    if (!factory) {

I worry about a strict warning here, and making sure it really is a factory.

try {
  if (("_xpcom_factory" in component.prototype) &&
      (component.prototype._xpcom_factory.QueryInterface(Ci.nsIFactory))) {
    return component.prototype._xpcom_factory;
  }
} catch (e) {
  // do nothing.
}

Whether you should explicitly check that the suggested factory is a nsIFactory object I won't recommend for or against.

Comment 8

[Nickolay_Ponomarev]

(In reply to comment #7)
> >+        compMgr.QueryInterface(Ci.nsIComponentRegistrar);
> 
> I'm not sure this line's necessary, but it doesn't hurt either.
That line was there, but anyway: compMgr is a nsIComponentManager, so you do need a QI.

> Might I suggest putting in a comment identifying which interface each method
> comes from?

That whole object implements nsIModule. One can guess it does, but I'll add a single comment for the object.

> >+  _getFactory: function(component) {
> 
> Give it a complete JavaDoc, please (@param, @return).

Why? It's not even a public method.

> >+    var factory = component.prototype._xpcom_factory;
> >+    if (!factory) {
> 
> I worry about a strict warning here

This usage (as well as if(obj.undefinedProperty)) doesn't cause a strict warning:

> xpcshell.exe -s
js> var o={}
js> o.a
typein:2: strict warning: reference to undefined property o.a
js> var a=o.a
js>

> and making sure it really is a factory.
> 
This object does not have to implement QI... I could check if |"createInstance" in factory|, but I don't think this is necessary.

Comment 9

[Nickolay_Ponomarev]

Attachment: patch v3

With the rest of weirdal's nits fixed.

Comment 10

[Robert Sayre]

Comment on attachment 266270 [details] [diff] [review]
patch v3

This looks great, thanks.

Comment 11

[Steve Krulewitz]

There is a small bug here -- in the calls to postRegister() and preUnregister(), the third argument should be |componentsArray| not |classArray|

Comment 12

[Nickolay_Ponomarev]

Attachment: patch v4

Good catch, fixed.

sayrer, should I get another review from someone else or is yours enough?

Comment 13

[Robert Sayre]

(In reply to comment #12)
>
> sayrer, should I get another review from someone else or is yours enough?

needs review from an xpconnect peer. bsmedberg would be good.

Comment 14

[Benjamin Smedberg]

Comment on attachment 267634 [details] [diff] [review]
patch v4

I'm not sure about "makeQI"... can we pass it the actual interface objects instead of the names, to avoid doing the Ci lookup? That will avoid a hashtable penalty whenever we call QI on these objects, which could be a significant win.

Other than that, r=me

Comment 15

[Nickolay_Ponomarev]

(In reply to comment #14)
> I'm not sure about "makeQI"... can we pass it the actual interface objects
> instead of the names, to avoid doing the Ci lookup?

That results in the leak stats increase, whatever that actually means. I don't think I'll get to working on that, and the current version of XPCOMUtils suffers from the same leak (bug 380969).

Can we land the leaking version of the patch (I really don't think passing the actual Ci.* objects should leak) since that doesn't make things worse? Or use the version with poorer performance and file a bug on fixing that?

The point is, I think stabilizing the XPCOMUtils' interface is more important at this point.

Comment 16

[Benjamin Smedberg]

That's interesting... I guess this version is ok, but could you file a followup bug for the optimized version?

Comment 17

[Robert Sayre]

(In reply to comment #15)
> (In reply to comment #14)
> > I'm not sure about "makeQI"... can we pass it the actual interface objects
> > instead of the names, to avoid doing the Ci lookup?
> 
> Can we land the leaking version of the patch (I really don't think passing the
> actual Ci.* objects should leak) since that doesn't make things worse? Or use
> the version with poorer performance and file a bug on fixing that?

I know it leaks, because I measured it, and then wrote code that prevented the return QI implementation from closing over the interfaces.

Comment 18

[Nickolay_Ponomarev]

(In reply to comment #17)
> I know it leaks, because I measured it, and then wrote code that prevented the
> return QI implementation from closing over the interfaces.
> 
I don't argue that, I also measured the leaks in both cases. I'm saying it _shouldn't_ leak in the ideal case - there's no obvious reason for it to leak.

If it's okay with you to land this (not leaking Cis, but slower) version and file the bug on figuring out the leaks and reverting the makeQI hack, could you review the latest version of the patch?

Comment 19

[Jeff Walden [:Waldo]]

Couldn't you make the method accept interfaces externally but deal with strings internally, via |.name|, to avoid the leak but still present a sane interface, until we figure out how to stop the leak?  I think this only runs into problems with interfaces created from the number alone, but I think that's a reasonable compromise for what's hopefully only a temporary problem.

I spent some time looking at nsJSIID today to figure out what mInfo (xptiInterfaceInfo*) was leaking.  It's a big huge mess of shared data with ownership models which complicate analysis, and I haven't gotten very far yet.  Most of mInfo's functionality is unneeded, so I'm thinking maybe it's possible to just copy the necessary data into it or perhaps to just copy the parts of mInfo that are needed for nsJSIID and not hold a strong reference to mInfo.

Comment 20

[Nickolay_Ponomarev]

(In reply to comment #19)
> Couldn't you make the method accept interfaces externally but deal with strings
> internally, via |.name|, to avoid the leak but still present a sane interface,
> until we figure out how to stop the leak?

That's how the patch works; generateQI is the public method that takes the interface objects and converts them to strings.

Comment 21

[Nickolay_Ponomarev]

Attachment: patch v4.1

Updated, also commented out one check, since it sometimes fails in my testing. I wanted to get a version ready for check-in, since others depend on it. Robert, could you check the patch and maybe check it in when the tree reopens? My free time didn't coincide with the open tree lately...

Comment 22

[Robert Sayre]

Comment on attachment 269005 [details] [diff] [review]
patch v4.1

ok, I'll check this in later today

Comment 23

[Nickolay_Ponomarev]

This has been checked in on 2007-06-20 21:53. Thanks, sayrer.

mozilla/js/src/xpconnect/loader/XPCOMUtils.jsm 	1.3
mozilla/js/src/xpconnect/tests/unit/component_import.js 	1.3
mozilla/js/src/xpconnect/tests/unit/test_import.js 	1.4 

Comment 24

[Alex Vincent [:WeirdAl]]

CR is not defined: http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/loader/XPCOMUtils.jsm#204

Comment 25

[Alex Vincent [:WeirdAl]]

Attachment: define CR