407963 - [FUEL] Factor out generic toolkit aspects of FUEL into a reusable form

Status: RESOLVED FIXED

(Firefox :: General, defect, P4)

Description

[Joey Minta]

The current FUEL interfaces include both Firefox-specific interfaces (e.g. fuelIBookmark) and generic toolkit interfaces (e.g. fuelIConsole) that other applications would likely find useful when creating a similar library.  The current implementation should be factored in such a way that these other applications can re-use these parts without the need to cut-and-paste the code.

Possible solutions include a js-module, use of mozISubScriptLoader, or even a simple #include file.

I believe the following is a complete list of the current toolkit-generic FUEL interfaces:
    * fuelIConsole
    * fuelIExtensions
    * fuelIExtension
    * fuelIPreference
    * fuelIPreferenceBranch
    * fuelIEvents
    * fuelIEventListener
    * fuelIEventItem

See also bug 380168 for some relevant discussion of the issue.

Comment 1

[Joey Minta]

Despite the general rule against confirmed bugs in Firefox:General, there doesn't seem to be a better place for FUEL bugs...

Comment 2

[Joey Minta]

Attachment: WIP

This factors out the toolkit elements into a separate js object that can be inherited by an Application implementation.  It works great until I try to move the objects into their own file to be loaded as a subscript.  Then, it only fails because I can't seem to get the build system to package my extra script.

To do:
*Figure out enough Make to package the extra script
*Place extra script in toolkit directory so other applications will check it out from cvs
*Factor out fuelIApplication

Any help people can offer with the Make stuff would be appreciated.

Comment 3

[Joey Minta]

Attachment: WIP2

This time with interfaces factored out.  All that's left is properly getting these things into the toolkit/ realm and making the build system realize they exist.

Comment 4

[Joey Minta]

Attachment: patch v1

It seems Make and I have come to a truce.  This patch fully separates the toolkit parts of FUEL into a reusable form.  In a moment, I'll attach the patch I used to get identical toolkit functionality into Thunderbird.  Other toolkit consumers should be able to create analogous patches for their apps.

This patch passes all the mochitests.

mfinkle expressed some concern that the subscript approach may impact startup time.  I don't really know a good way to test this short of landing, but we should definitely keep an eye out.  A less dangerous approach would be to #include all the objects (Console, Extensions, etc), but this would still require re-writing getters into every application's Application object.  I passed on the js-module approach since we don't expect this file to ever be loaded more than once per runtime.

Comment 5

[Joey Minta]

Attachment: Thunderbird patch

This patch would add FUEL functionality to Thunderbird.  Similar patches could be written for Sunbird and other toolkit consumers.  I won't land this patch until I've also implemented some mail-specific bits into the steel Application.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 293032 [details] [diff] [review]
patch v1

Can you add a comment to this so its clear to others whats happening?
+    this.__proto__.observe(aSubject, aTopic, aData);

We'll have to watch the Ts hit, if any.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

> +    this.__proto__.observe(aSubject, aTopic, aData);
If you are planning to call the observe method of the base class, shouldn't this be:

> this.__proto__.__proto__.observe.call(this, aSubject, aTopic, aData);

Comment 8

[Joey Minta]

(In reply to comment #7)
You are exactly right.  Thanks for the catch!  New patch coming soon.

Comment 9

[Joey Minta]

Attachment: patch v2

This version fixes the __proto__ issue and adds a comment as requested by mfinkle.  Additionally, I did some seat-of-the-pants profiling with Date.now() and was seeing an unfortunate hit of about 8-10ms during the call to loadSubScript().  While small, this seemed likely to be unacceptable.  As a result, this patch also switches to #include-ing the toolkit file, rather than loading it at runtime.  The result is, of course, equivalent.

Carrying over r+.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 293328 [details] [diff] [review]
patch v2

You should get Benjamin to approve the addition of toolkit/components/exthelper .

>Index: toolkit/components/exthelper/extIApplication.idl

No license header?

Comment 11

[Joey Minta]

Comment on attachment 293328 [details] [diff] [review]
patch v2

The fuel idl didn't have a license, so I didn't add one here either.  I can throw one in if people want.

bsmedberg: Please see gavin's comment #10 re adding the exthelper folder.

Comment 12

[Benjamin Smedberg]

Comment on attachment 293328 [details] [diff] [review]
patch v2

I just skimmed this: I'm just giving MOA based on existing reviews.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #11)
> (From update of attachment 293328 [details] [diff] [review])
> The fuel idl didn't have a license, so I didn't add one here either.  I can
> throw one in if people want.

I am going to patch fuelIApplication to add a license header - or you could add it as part of this patch :)

up to you

Comment 14

[Joey Minta]

Comment on attachment 293328 [details] [diff] [review]
patch v2

Requesting approval.  This is fairly low risk in that the vast majority of it is just moving code around.  The patch passes all the FUEL mochitests.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 293328 [details] [diff] [review]
patch v2

a=beltzner for 1.9

Comment 16

[Joey Minta]

I'm in the midst of exams so I'm going to get someone else to land this.  Re: the license header, the fuelApplication.js contributors list should be sufficient.

Comment 17

[Reed Loden [:reed]]

Added missing license header to fuelIApplication.idl and extIApplication.idl.

Checking in browser/fuel/public/fuelIApplication.idl;
/cvsroot/mozilla/browser/fuel/public/fuelIApplication.idl,v  <--  fuelIApplication.idl
new revision: 1.10; previous revision: 1.9
done
Checking in browser/fuel/src/fuelApplication.js;
/cvsroot/mozilla/browser/fuel/src/fuelApplication.js,v  <--  fuelApplication.js
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/components/Makefile.in;
/cvsroot/mozilla/toolkit/components/Makefile.in,v  <--  Makefile.in
new revision: 1.76; previous revision: 1.75
done
RCS file: /cvsroot/mozilla/toolkit/components/exthelper/Makefile.in,v
done
Checking in toolkit/components/exthelper/Makefile.in;
/cvsroot/mozilla/toolkit/components/exthelper/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/exthelper/extApplication.js,v
done
Checking in toolkit/components/exthelper/extApplication.js;
/cvsroot/mozilla/toolkit/components/exthelper/extApplication.js,v  <--  extApplication.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/exthelper/extIApplication.idl,v
done
Checking in toolkit/components/exthelper/extIApplication.idl;
/cvsroot/mozilla/toolkit/components/exthelper/extIApplication.idl,v  <--  extIApplication.idl
initial revision: 1.1
done

Comment 18

[Reed Loden [:reed]]

Backed this out due to orange. See http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox&errorparser=unix&logfile=1200210780.1200211853.15867.gz&buildtime=1200210780&buildname=Linux%20fxdbug-linux-tbox%20Depend&fulltext=1 for full log.


  ###!!! ASSERTION: Oops!  You're asking for a weak reference to an object that doesn't support that.: 'factoryPtr', file nsWeakReference.cpp, line 109
  NS_GetWeakReference(nsISupports*, unsigned int*)+0x000000A3 [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0002C8F1]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0004A622]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x000495F0]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0004AC82]
  NS_InvokeByIndex_P+0x00000029 [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x000BFF45]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x0005D27C]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x0006C128]
  js_Invoke+0x00000BC7 [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libmozjs.so +0x00064B47]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0007669D]
  js_Invoke+0x00000C51 [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libmozjs.so +0x00064BD1]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x000561DC]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x0004D7A5]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x000C0DD6]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x00049497]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0004A985]
  NS_ShutdownXPCOM_P+0x000001DA [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x00039A94]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxul.so +0x00027F0E]
  XRE_main+0x000031B1 [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/libxul.so +0x0002E0C1]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/dist/bin/firefox-bin +0x00000DC1]
  __libc_start_main+0x000000DC [/lib/libc.so.6 +0x00015DEC]
  nsDOMNodeAllocator leaked 118616 bytes 
  nsStringStats
   => mAllocCount:          27190
   => mReallocCount:         5189
   => mFreeCount:           22466  --  LEAKED 4724 !!!
   => mShareCount:          28373
   => mAdoptCount:           2062
   => mAdoptFreeCount:       2059  --  LEAKED 3 !!!

Comment 19

[Reed Loden [:reed]]

Oh, and please don't forget to update toolkit/toolkit-makefiles.sh in your next revision of the patch.

Comment 20

[Reed Loden [:reed]]

While the patch was landed, it did cause a 8-9ms Ts win on Linux. Hopefully the fixed patch will do the same thing. :)

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: FUEL refactoring patch, v3 (thanks to emre)

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #21)
> Created an attachment (id=310402) [details]
> FUEL refactoring patch, v3 (thanks to emre)
> 

+    os.addObserver(this, "quit-application-granted", false);
+    os.addObserver(this, "quit-application", false);

Thanks for updating this patch. Note, we removed the use of these notification on trunk and the leak is probably still present.

Comment 23

[Emre Birol]

Attachment: FUEL refactoring patch, v4

- This patch does merge the latest (trunk) fuel library and Jminta's extHelper library.
- It doesn't fix the memory leaks, it only fixes the WeakReference problem. The root of this problem is passing extApplication reference to nsIObserverService. Even when we pass extApplication's reference to addObserver with ownsWeak = false, it gives this error.  Adding XPCOMUtils.generateQI([Ci.nsISupportsWeakReference]) to extApplication fixes the problem. Don't know why -- if somebody explains would be great.
- When I run mochitest on the trunk (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032010 Minefield/3.0b5pre), I get leaks. I guess these leaks nothing have to do wih this refactored library. You can find the mochitest results before and after the patch.

Comment 24

[Emre Birol]

Attachment: Mochitest results before and after the patch.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 310811 [details] [diff] [review]
FUEL refactoring patch, v4

>Index: browser/fuel/public/fuelIApplication.idl

>-
>-/**
>- * Interface representing a container for bookmark roots. Roots
>- * are the top level parents for the various types of bookmarks in the system.
>- */
>-[scriptable, uuid(c9a80870-eb3c-11dc-95ff-0800200c9a66)]
>-interface fuelIBookmarkRoots : nsISupports
>-{
>-  /**
>-   * The folder for the 'bookmarks menu' root.
>-   */
>-  readonly attribute fuelIBookmarkFolder menu;
>-
>-  /**
>-   * The folder for the 'personal toolbar' root.
>-   */
>-  readonly attribute fuelIBookmarkFolder toolbar;
>-
>-  /**
>-   * The folder for the 'tags' root.
>-   */
>-  readonly attribute fuelIBookmarkFolder tags;
>-
>-  /**
>-   * The folder for the 'unfiled bookmarks' root.
>-   */
>-  readonly attribute fuelIBookmarkFolder unfiled;
>+/**                                                               
>+ * Interface representing a container for bookmark roots. Roots   
>+ * are the top level parents for the various types of bookmarks in the system.  
>+ */                                                               
>+[scriptable, uuid(c9a80870-eb3c-11dc-95ff-0800200c9a66)]          
>+interface fuelIBookmarkRoots : nsISupports                        
>+{                                                                 
>+  /**                                                             
>+   * The folder for the 'bookmarks menu' root.                    
>+   */                                                             
>+  readonly attribute fuelIBookmarkFolder menu;                    
>+                                                                  
>+  /**                                                             
>+   * The folder for the 'personal toolbar' root.                  
>+   */                                                             
>+  readonly attribute fuelIBookmarkFolder toolbar;                 
>+                                                                  
>+  /**                                                             
>+   * The folder for the 'tags' root.                              
>+   */                                                             
>+  readonly attribute fuelIBookmarkFolder tags;                    
>+                                                                  
>+  /**                                                             
>+   * The folder for the 'unfiled bookmarks' root.                 
>+   */                                                             
>+  readonly attribute fuelIBookmarkFolder unfiled;                 
> };

Weird diff, but should be ok

> //=================================================
> // Application implementation
> Application.prototype = {
>   // for nsIClassInfo + XPCOMUtils
>   classDescription: "Application",
>   classID:          Components.ID("fe74cf80-aa2d-11db-abbd-0800200c9a66"),
>   contractID:       "@mozilla.org/fuel/application;1",
> 
>   // redefine the default factory for XPCOMUtils
>   _xpcom_factory: ApplicationFactory,
> 
>-  // get this contractID registered for certain categories via XPCOMUtils
>-  _xpcom_categories: [
>-    // make Application a startup observer
>-    { category: "app-startup", service: true },
>-
>-    // add Application as a global property for easy access
>-    { category: "JavaScript global privileged property" }
>-  ],
>-
>-  get id() {
>-    return this._info.ID;
>-  },
>-
>-  get name() {
>-    return this._info.name;
>-  },
>+  // for nsISupports
>+  QueryInterface : XPCOMUtils.generateQI([Ci.fuelIApplication, Ci.nsIObserver, Ci.nsIClassInfo]),

nsIClassInfo is here, but do you impl all the methods?

>+  getInterfaces : function app_gi(aCount) {
>+    var interfaces = [Ci.fuelIApplication, Ci.nsIObserver, Ci.nsIClassInfo];
>+    aCount.value = interfaces.length;
>+    return interfaces;

Here too, in fact this is a nsIClassInfo method, but others have been removed.


>+#include ../../../toolkit/components/exthelper/extApplication.js
>Index: toolkit/components/Makefile.in

>+  // for nsIClassInfo
>+  flags : Ci.nsIClassInfo.SINGLETON,
>+  implementationLanguage : Ci.nsIProgrammingLanguage.JAVASCRIPT,
>+
>+  getInterfaces : function app_gi(aCount) {
>+    var interfaces = [Ci.extIApplication, Ci.nsIObserver, Ci.nsIClassInfo];
>+    aCount.value = interfaces.length;
>+    return interfaces;
>+  },
>+
>+  getHelperForLanguage : function app_ghfl(aCount) {
>+    return null;
>+  },

So the methods are here, but getInterfaces is sorta duplicated from fuelApplication. I wonder what the proper thing to do here is? Its probably ok, but fuelApplication should have the other methods too, I think.

>+
>+  QueryInterface : XPCOMUtils.generateQI([Ci.nsISupportsWeakReference])
>+};

You should probably include sxtIApplication here too.

>Index: toolkit/components/exthelper/extIApplication.idl

>+/**
>+ * Interface holds information about an event.
>+ */
>+[scriptable, function, uuid(05281820-ab62-11db-abbd-0800200c9a66)]
>+interface extIEventItem : nsISupports
>+{
>+  /**
>+   * The name of the event
>+   */
>+  readonly attribute AString type;
>+
>+  /**
>+   * Can hold extra details and data associated with the event. This
>+   * is optional and event specific. If the event does not send extra
>+   * details, this is null.
>+   */
>+  readonly attribute AString data;

This was changed to a nsIVariant in another patch (jminta did the change)

Try cleaning up the QI's and the nsIClassInfo situation. Also fix (sync) the extIEventItem.data change.

r- for now

Comment 26

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #25)
> Weird diff, but should be ok

Actually it looks like that's adding a bunch of trailing whitespace, which should be avoided. There are other places in the patch where you're adding extra space on blank lines, too.

Comment 27

[Joey Minta]

(In reply to comment #25)
> 
> nsIClassInfo is here, but do you impl all the methods?
extApplication implements all of the methods, and the FUEL Application object overrides those methods it wants to change.

> Here too, in fact this is a nsIClassInfo method, but others have been removed.
Not removed, just moved to the proto.

> This was changed to a nsIVariant in another patch (jminta did the change)
Hasn't landed yet. 

Comment 28

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #27)
> > This was changed to a nsIVariant in another patch (jminta did the change)
> Hasn't landed yet. 

(just adding extra info)
patch is in attachment 310833 [details] [diff] [review] on bug 424203 

Comment 29

[Emre Birol]

Attachment: Revision 5 of the refactoring patch

Based on Jminta's comments I didn't changed the nsIClassInfo interface implementation in Application and extApplication. This is what is different for revision 5:

- clears the white spaces as much as possible
- adds extIApplication into QueryInterface property
- changes event attribute to nsIVariant (bug 424203 - thanks Bryan )

thanks for the review

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 311426 [details] [diff] [review]
Revision 5 of the refactoring patch

Looks better. Let's do this!

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 311426 [details] [diff] [review]
Revision 5 of the refactoring patch

Requesting approval for 1.9b5.  

Reward: this allows all xulrunner consumers to use and extend FUEL.  It's a pre-requisite for STEEL, the Thunderbird version of this, which we want for Tb3.  Additionally, extensions which are compatible with both (e.g.) Firefox and Thunderbird can use the generic parts of FUEL in both.

Risk: low: passes all tests, this is mostly code motion, the main substantive change is the nsIVariant switch (already approved by mfinkle).  Because of an extension compatibility question*, it would be slightly preferable for this to have a beta to bake in.

* if an existing consumer of FUEL cares about the names of the interfaces that the FUEL objects implement (as I understand it, most [all?] shouldn't), this patch changes many of those.

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311426 [details] [diff] [review]
Revision 5 of the refactoring patch

a1.9b5=beltzner

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Landed; I go to commute, and jminta is keeping an eye on the tree.

Comment 34

[Mark Banner (:standard8)]

From what I can tell, the Firefox 3 (and later) FUEL documentation missed the interface name changes that went on here (https://developer.mozilla.org/en/FUEL) and the fact these interfaces moved to toolkit so that all apps have access to them. Therefore adding dev-doc-needed keyword as I believe devmo needs an update for this bug.

Comment 35

[Eric Shepherd [:sheppy]]

I've seen a lot of work done on the FUEL docs in the last few weeks. Does that work result in this being done?

Comment 36

[Jean-Yves Perrier [:teoli]]

FUEL is dying (if not dead), we plan to archive its documentation as soon as it will be removed. We are not maintaining the current one.