Last Comment Bug 750454 - FUEL causes lots of leaks until shutdown, can also cause 10+minute shutdown times
: FUEL causes lots of leaks until shutdown, can also cause 10+minute shutdown t...
Status: RESOLVED FIXED
[MemShrink:P2][Snappy]
: mlk
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- major with 4 votes (vote)
: Firefox 16
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
: 717724 (view as bug list)
Depends on: 750583
Blocks: 533290 758082 760779 730546 750953 758248 762506 767682 770477
  Show dependency treegraph
 
Reported: 2012-04-30 13:23 PDT by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2012-07-03 05:41 PDT (History)
32 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix the array clearing. (1.10 KB, patch)
2012-04-30 19:06 PDT, Patrick Walton (:pcwalton)
no flags Details | Diff | Splinter Review
WIP v1 (29.48 KB, patch)
2012-05-04 08:42 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
WIP v2 (37.28 KB, patch)
2012-05-04 14:02 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
WIP v3 (39.02 KB, patch)
2012-05-07 15:30 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v1 (42.68 KB, patch)
2012-05-14 12:51 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
FUEL - Part 1, Use weak refs in Application. (3.94 KB, patch)
2012-05-14 18:36 PDT, Justin Lebar (not reading bugmail)
mak77: review+
Details | Diff | Splinter Review
FUEL - Part 2, Fix browser leaks. (6.19 KB, patch)
2012-05-14 18:37 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
FUEL - Part 3, Fix preferences leaks. (6.75 KB, patch)
2012-05-14 18:37 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
FUEL - Part 4, Fix bookmarks leaks. (18.60 KB, patch)
2012-05-14 18:37 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
FUEL - Part 5, Fix extensions. (6.04 KB, patch)
2012-05-14 18:37 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
FUEL - Part 6, Remove gShutdown. (1.85 KB, patch)
2012-05-14 18:37 PDT, Justin Lebar (not reading bugmail)
mak77: review+
Details | Diff | Splinter Review
FUEL - Part 7, Test fixes (3.48 KB, patch)
2012-05-14 18:37 PDT, Justin Lebar (not reading bugmail)
mak77: review+
Details | Diff | Splinter Review
Part 1a, review comments (comment 52). (1.38 KB, patch)
2012-06-04 21:14 PDT, Justin Lebar (not reading bugmail)
mak77: review+
Details | Diff | Splinter Review
Part 2a, review comment (comment 53): Use handleEvent instead of rolling it ourselves. (5.36 KB, patch)
2012-06-04 21:14 PDT, Justin Lebar (not reading bugmail)
mak77: review+
Details | Diff | Splinter Review
Part 3a, review comments from comment 54. (3.96 KB, patch)
2012-06-04 21:15 PDT, Justin Lebar (not reading bugmail)
mak77: review+
Details | Diff | Splinter Review
Part 4a, review comments (comment 55) (8.05 KB, patch)
2012-06-04 21:15 PDT, Justin Lebar (not reading bugmail)
mak77: review+
Details | Diff | Splinter Review
Part 5a, review comments (comment 56). (1.34 KB, patch)
2012-06-04 21:15 PDT, Justin Lebar (not reading bugmail)
mak77: review+
Details | Diff | Splinter Review
Part 8: Reformat FUEL code. (37.06 KB, patch)
2012-06-04 21:16 PDT, Justin Lebar (not reading bugmail)
mak77: review+
Details | Diff | Splinter Review
Part 9: Add javadoc to PreferenceObserver.{add,remove}EventListener. (2.15 KB, patch)
2012-06-04 21:16 PDT, Justin Lebar (not reading bugmail)
mak77: review+
Details | Diff | Splinter Review
Roll-up patch (53.09 KB, patch)
2012-06-05 12:29 PDT, Justin Lebar (not reading bugmail)
mak77: review+
Details | Diff | Splinter Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-30 13:23:57 PDT
Short story: We should nuke the gShutdown mechanism entirely. FUEL adds a bunch of closures to the gShutdown array, and those closures keep their respective FUEL objects alive until shutdown. For things like pref observers this should be unnecessary (since they request to be used as weak observers). Other things (Window, BrowserTab) should figure out some other way to clean up in a more timely fashion.

Longer story: One of my favorite addons does this: 

  for(var window_index in Application.windows){
    if(Application.windows.hasOwnProperty(window_index)){
      var window = Application.windows[window_index];
      for(var tab_index in window.tabs){
        if(window.tabs.hasOwnProperty(tab_index)){
          var tab = window.tabs[tab_index]
  ...

I tend to keep a lot of tabs open (though not loaded) and every time this function runs I have 2N+1 calls to Application.windows (where N is number of windows). Then for every window I have 2M+1 calls to Window.tabs (where M is the number of tabs). Every Window object created adds an item to the gShutdown array, as does every BrowserTab object.

This quickly gets out of control if you have a large number of tabs. On a very short browsing session this array contained > 600k entries.

And then those closures just... sit there... forever... until shutdown. DOM windows cannot be cleaned up, so they hang around, get included in the CC graph, etc.

And then it gets even worse. At shutdown, we run this code:

  while (gShutdown.length) {
    gShutdown.shift()();

This pulls one item out of the array and then memmove's everything else, for every iteration (quadratic in number of elements according to bz). This is causing me to experience 10+ minute shutdown times, and I have a very zippy machine.

We _really_ need to fix this. Any addon using FUEL to access windows or tabs are going to hit this, as are preference observers, extension inspectors, etc.
Comment 1 Dão Gottwald [:dao] 2012-04-30 13:26:30 PDT
Do we know how many add-ons use FUEL?
Comment 2 Justin Lebar (not reading bugmail) 2012-04-30 13:34:46 PDT
> DOM windows cannot be cleaned up, so they hang around, get included in the CC graph, etc.

Ben tells me on IRC that he hasn't actually observed this; it's a hypothesis based on the other things which are happening.
Comment 3 Jorge Villalobos [:jorgev] 2012-04-30 13:39:23 PDT
(In reply to Dão Gottwald [:dao] from comment #1)
> Do we know how many add-ons use FUEL?

Many add-ons use FUEL, possibly hundreds:
https://mxr.mozilla.org/addons/search?string=fuel

What exactly does an add-on need to do to trigger this bug?
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-30 13:46:48 PDT
(In reply to Justin Lebar [:jlebar] from comment #2)
> Ben tells me on IRC that he hasn't actually observed this; it's a hypothesis
> based on the other things which are happening.

Well, unclear about *content* windows. We definitely leak the chrome windows.
Comment 5 Dão Gottwald [:dao] 2012-04-30 13:48:32 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #3)
> (In reply to Dão Gottwald [:dao] from comment #1)
> > Do we know how many add-ons use FUEL?
> 
> Many add-ons use FUEL, possibly hundreds:
> https://mxr.mozilla.org/addons/search?string=fuel

That's not going to be comprehensive. The "Application" object is fuel, for instance...
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-30 13:50:26 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #3)
> What exactly does an add-on need to do to trigger this bug?

Use "Application.windows" or "Application.windows.tabs", basically.
Comment 7 Boris Zbarsky [:bz] 2012-04-30 13:59:00 PDT
> What exactly does an add-on need to do to trigger this bug?

Iterate over Application.windows and then the .tabs for each window in the "obvious" way (the one that involves doing the get on every loop of the iteration).

Assuming the common case of 1 window with M tabs, any add-on that does that will end up with 2*M*(M+1)*(number of times it tried to walk the tabs) entries in gShutdown.

For Ben's case, M is about 100, as I understand, so to hit 600k entries would require about 30 walks over the tabs.  If the addon just walks over the tabs periodically, that's easy to hit.

To put that 600k number in perspective, I wrote a small C++ program that tests how long it takes to just do the memmoving involved in the shutdown observer.  It took 3 seconds for 100k entries.  I stopped it at about 8 minutes for 1000k entries.  Note that the expected time for 1000k entries based on the 100k time was about 5 minutes, so clearly 1000k entries doesn't fit in cache as well, and there is a jump in the time needed at some point depending on cache size.

In either case, the smallest time one would expect for 600k entries based on the 100k entry time is about a minute and a half, which is already completely unacceptable for shutdown....
Comment 8 Justin Lebar (not reading bugmail) 2012-04-30 14:05:23 PDT
You know, we should just get rid of this gShutdown business entirely and without regard for leaking (assuming we pass our tests).

At best, gShutdown makes us not "leak" at shutdown.  But we leaked *right up until shutdown* regardless!  So it's buying us exactly zero.
Comment 9 Boris Zbarsky [:bz] 2012-04-30 14:07:58 PDT
Yes, 100% agreed.
Comment 10 Justin Lebar (not reading bugmail) 2012-04-30 14:14:33 PDT
Let's see how orange it turns the tree.  :)  https://tbpl.mozilla.org/?tree=Try&rev=df130256b169
Comment 11 Patrick Walton (:pcwalton) 2012-04-30 19:06:32 PDT
Created attachment 619810 [details] [diff] [review]
Patch to fix the array clearing.

Here's a patch to fix the array clearing (moved from bug 750583). Although I'd rather just nuke gShutdown entirely.
Comment 12 Patrick Walton (:pcwalton) 2012-04-30 19:07:48 PDT
*** Bug 750583 has been marked as a duplicate of this bug. ***
Comment 13 Mozilla RelEng Bot 2012-05-01 00:00:21 PDT
Try run for df130256b169 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=df130256b169
Results (out of 222 total builds):
    success: 181
    warnings: 41
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-df130256b169
Comment 14 Justin Lebar (not reading bugmail) 2012-05-01 10:48:22 PDT
I think I can fix everything except the Extension{,s} objects via judicious use of weak references.

But it's not clear to me how to fix the Extensions.  The Extension objects are kept alive by JS code implementing an event listener pattern, so weakmaps don't fit.

I think we could solve the problem if we never created more than one Extension object for a given add-on, but then add-ons could trample on each other by modifying that object.  We could freeze the object, but that would change the API.

I thought about a solution where we do:

  add-on (non-FUEL object) ---> ExtensionProxy singleton <--- Extension objects

That is, one add-on object keeps this one ExtensionProxy alive (via a WeakMap keyed on the add-on), and the extension objects are implemented by forwarding their requests to the ExtensionProxy.  But this doesn't work either, because each Extension object needs to be able to fire events triggered by the add-on, so we need to have path from the add-on to the Extension objects.

So...I'm not sure what to do here.
Comment 15 Boris Zbarsky [:bz] 2012-05-01 10:52:48 PDT
Are the Extension objects used in practice?

Quite honestly, I think that at first glance the "never create more than one Extension object for a given add-on" approach seems safest.

Though if add-ons add event listeners to the Extension objects, that would just mean those listeners leak...
Comment 16 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-01 16:54:59 PDT
I filed bug 750953 on the crazy behavior that is killing the readability extension. Basically once you get a FUEL Window object the system is doomed, if it runs long enough.
Comment 17 Justin Lebar (not reading bugmail) 2012-05-01 21:51:40 PDT
> Quite honestly, I think that at first glance the "never create more than one Extension 
> object for a given add-on" approach seems safest.

You don't think it's a problem that the changes one add-on might make to an Extension object would be visible to other add-ons?  Or do you think we should freeze the objects?

I'm now less confident that weak references are going to solve even most of these leaks.  There are a lot of event listeners...

Alternatively, I think an enumerable list of weak refs might work.  I'm kind of surprised we don't already have one, actually...
Comment 18 Boris Zbarsky [:bz] 2012-05-01 22:01:58 PDT
> You don't think it's a problem that the changes one add-on might make to an Extension
> object would be visible to other add-ons?

I think it's a smaller problem than the other things we're looking at here, and I'm willing to bet add-ons make such changes very rarely if ever.

And yes, we really need to solve the event listener issues.  I think that the only sane way to do that is to just minimize the number of objects we create, hence my support for singleton objects.
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-03 16:28:02 PDT
(In reply to Justin Lebar [:jlebar] from comment #17)
> You don't think it's a problem that the changes one add-on might make to an
> Extension object would be visible to other add-ons?  Or do you think we
> should freeze the objects?

What changes do you envision add-ons making to these objects? All of its useful properties are read-only, so I think it's quite unlikely that add-ons are changing these objects at all to begin with. Even if they are, it's even more unlikely that another add-on would break because of it (unless you're doing something dumb like replacing _item, but I really don't see any reasonable reason why anyone would do that).
Comment 20 Justin Lebar (not reading bugmail) 2012-05-03 17:16:15 PDT
> What changes do you envision add-ons making to these objects?

I didn't have a specific case in mind.  I agree that modifying the objects doesn't make much sense; I just hoping we wouldn't have to change any semantics.  But it looks like that's not doable, at least, not easily.
Comment 21 Justin Lebar (not reading bugmail) 2012-05-04 08:42:25 PDT
Created attachment 621060 [details] [diff] [review]
WIP v1

Gets rid of gShutdown.  This passes the tests locally, without leaking.  But I know that at least the Extensions code and part of the Bookmarks code is untested.

So I need to investigate testing, and also clean up the patch some.
Comment 22 Justin Lebar (not reading bugmail) 2012-05-04 14:02:25 PDT
Created attachment 621163 [details] [diff] [review]
WIP v2

Cleaned up mostly; still needs a bunch of additional tests before I'm confident that it works properly.

A notable change: Preference registered a weakref listener on the pref branch, so the intent was apparently for the listener to die when the Preference object died.  That is, you'd have to keep the Preference object alive in order for the listener objects you registered using it to do anything.

However, gShutdown foiled that plan -- it kept a strong reference to every Preference object.  So you could register a listener on a Preference object, drop all your refs to it, and still have that listener fire.

I decided to keep the current semantics by making listeners registered via a Preference object stay alive forever, because that matches our semantics around bookmarks, and I think it's sane.
Comment 23 Justin Lebar (not reading bugmail) 2012-05-07 15:30:42 PDT
Created attachment 621760 [details] [diff] [review]
WIP v3

Re-enabled a preferences test, made it work.
Comment 24 Justin Lebar (not reading bugmail) 2012-05-07 15:33:14 PDT
I think this should fix bug 533290.  That seems to have been caused by weakref weirdness, which I'm removing here.
Comment 25 neil@parkwaycc.co.uk 2012-05-07 17:02:25 PDT
(In reply to Justin Lebar from comment #24)
> I think this should fix bug 533290.  That seems to have been caused by
> weakref weirdness, which I'm removing here.

I don't see you removing weakrefs, c.f. this patch excerpt:
>+        QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
>+                                               Ci.nsISupportsWeakReference])
>+      };
>+      this._observersDict[aBranch] = observer;
>+      this._prefs.addObserver(aBranch, observer, /* ownsWeak = */ true);
Comment 26 Justin Lebar (not reading bugmail) 2012-05-07 17:50:02 PDT
Well, see the first line here:

>+      this._observersDict[aBranch] = observer;
>+      this._prefs.addObserver(aBranch, observer, /* ownsWeak = */ true);

We're now holding a strong ref to the observer.  The prefbranch also holds a weakref to it, it's true, but the object is now explicitly held alive until the PreferenceObserver goes away, and the PreferenceObserver lives until shutdown.
Comment 27 Justin Lebar (not reading bugmail) 2012-05-08 17:51:59 PDT
I can't figure out how to test this extension code without shoving hooks pretty deep into AddonManager.jsm.  I'm not sure that we want to do that.

If we're afraid of changing this untested code, we could leave it as-is.  We might need to leave gShutdown, in this case -- the extension objects are held alive by strong listeners from AddonManager.jsm, and I'm not sure if we'll leak if we leave those listeners until shutdown.

An add-on shouldn't create bazillions of Extension objects unless they call Application.getExtensions() in a loop.  It returns via a callback, so I presume extensions won't call this too many times.

So the options I have are: Leave the grossness alone because we think it's likely to work properly, test our new code by injecting fake extensions into the add-on manager, or look closely at the code and let our users do the testing for us.

Thoughts?
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-08 18:29:35 PDT
Why do you need to shove hooks into the addon manager to test this?
Comment 29 Justin Lebar (not reading bugmail) 2012-05-08 21:14:00 PDT
I don't fully understand the scope of the objects defined in these files, but it seems that I can't do |new Extensions| from a browser chrome test -- that seems pretty reasonable.

So to get a handle on a non-empty Extensions object, AddonManager.getAddonsByTypes needs to return a non-empty list.  It's currently empty when I run the browser-chrome tests.

Then, once I get an Extension object, I want to trigger a relevant onenable (or whatever) event.  The object listening to that event is the singleton ExtensionObserver, which does AddonManager.addAddonListener(this).  Again, I don't understand how things are scoped here, but it seems that I can't get a handle on gExtensionObserver from the test (and this is again reasonable; we don't want an extension messing with this global).  So even if I wanted to, I can't call gExtensionObserver.onEnabling() myself -- I'd need to trigger that via the addon manager.
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-08 23:09:05 PDT
Couldn't your test just install an addon and enable it? toolkit/mozapps/extensions/test/xpcshell/test_fuel.js does something similar, but it could also be done from a browser chrome test. Or maybe tweaking that test would be sufficient.
Comment 31 Justin Lebar (not reading bugmail) 2012-05-09 12:42:34 PDT
There was some chatter on IRC:

bz: pdf.js uses FUEL?
bdahl: pdf.js uses fuel
gavin: yes
gavin: so does testpilot

:(
Comment 32 Nicholas Nethercote [:njn] 2012-05-09 16:43:48 PDT
> bz: pdf.js uses FUEL?
> bdahl: pdf.js uses fuel
> gavin: yes
> gavin: so does testpilot

Oh, so the two add-ons we ship by default to users?  Sigh.
Comment 33 Nicholas Nethercote [:njn] 2012-05-09 16:44:54 PDT
(And yes, I know Test Pilot is not shipped on normal releases and pdf.js isn't enabled yet.  Still...)
Comment 34 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-09 17:28:05 PDT
I "requested a pull" to fix pdf.js at https://github.com/mozilla/pdf.js/pull/1683 .
Comment 35 Justin Lebar 2012-05-09 18:19:13 PDT
I'm really under the gun here at the b2g work week.  Doing my best, but I may not be able to get back to this until next week.
Comment 36 Justin Lebar (not reading bugmail) 2012-05-09 22:20:13 PDT
(Comment 35 was really me; I signed in with browser-id on my phone and didn't realize it was going to post as a new user.)
Comment 37 Justin Lebar (not reading bugmail) 2012-05-14 12:50:45 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #30)
> Couldn't your test just install an addon and enable it?
> toolkit/mozapps/extensions/test/xpcshell/test_fuel.js does something
> similar, but it could also be done from a browser chrome test. Or maybe
> tweaking that test would be sufficient.

Indeed, it was sufficient.  I didn't even realize that test existed.  Thanks!
Comment 38 Justin Lebar (not reading bugmail) 2012-05-14 12:51:29 PDT
Created attachment 623777 [details] [diff] [review]
Patch v1
Comment 39 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-14 16:09:16 PDT
Can you split this up into pieces? The bookmarks observer changes are more complicated and don't necessary to fix the primary issue (lots of Tab/Window objects being accumulated in gShutdown).
Comment 40 Justin Lebar (not reading bugmail) 2012-05-14 17:08:20 PDT
To be clear, the bookmarks and pref observer changes are necessary to fix the primary issue of memory leaks.  For example, accessing Preferences.all leaks an object for every pref.  (Plus listeners for each object, etc.)  Depending on how one uses this API, that could be quite bad.

I'll split this up however you want, but can you be specific about how you want it split?  Do you want the window leak fixes split out from the non-window leak fixes?  Do you want one patch per object I changed?
Comment 41 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-14 17:18:16 PDT
Splitting it into three parts would probably be good. In order of importance:
1) Window/Tab changes
2) Prefs changes
3) Bookmarks/Extensions
Comment 42 Justin Lebar (not reading bugmail) 2012-05-14 18:36:58 PDT
Created attachment 623893 [details] [diff] [review]
FUEL - Part 1, Use weak refs in Application.
Comment 43 Justin Lebar (not reading bugmail) 2012-05-14 18:37:05 PDT
Created attachment 623894 [details] [diff] [review]
FUEL - Part 2, Fix browser leaks.
Comment 44 Justin Lebar (not reading bugmail) 2012-05-14 18:37:11 PDT
Created attachment 623895 [details] [diff] [review]
FUEL - Part 3, Fix preferences leaks.
Comment 45 Justin Lebar (not reading bugmail) 2012-05-14 18:37:17 PDT
Created attachment 623896 [details] [diff] [review]
FUEL - Part 4, Fix bookmarks leaks.
Comment 46 Justin Lebar (not reading bugmail) 2012-05-14 18:37:24 PDT
Created attachment 623897 [details] [diff] [review]
FUEL - Part 5, Fix extensions.
Comment 47 Justin Lebar (not reading bugmail) 2012-05-14 18:37:31 PDT
Created attachment 623898 [details] [diff] [review]
FUEL - Part 6, Remove gShutdown.
Comment 48 Justin Lebar (not reading bugmail) 2012-05-14 18:37:38 PDT
Created attachment 623899 [details] [diff] [review]
FUEL - Part 7, Test fixes
Comment 49 Justin Lebar (not reading bugmail) 2012-05-30 06:45:15 PDT
Any chance we're going to get this done by uplift (6/4)?
Comment 50 Justin Lebar (not reading bugmail) 2012-05-30 06:45:37 PDT
Or, Gavin, if you don't think you'll have time, perhaps you can suggest another reviewer?
Comment 51 Marco Bonardo [::mak] 2012-05-31 02:01:55 PDT
I can do this, though I'm not going to flip all requests cause would just be useless spam.
Comment 52 Marco Bonardo [::mak] 2012-05-31 08:47:32 PDT
Comment on attachment 623893 [details] [diff] [review]
FUEL - Part 1, Use weak refs in Application.

Review of attachment 623893 [details] [diff] [review]:
-----------------------------------------------------------------

basically positive, though I think there's potential space for further simplification in the "unload" case.

::: toolkit/components/exthelper/extApplication.js
@@ +550,2 @@
>      // references to various services, and to remove itself as
>      // observer of any other notifications.

This comment doesn't apply anymore, according to what you do here and what you are doing in the "remove gShutdown" patch.
In the end the shutdown observer will only be used to notify the "unload" event and cleanup a couple singletons, so the comments should be updated.

I we'd not need to nullify those singletons, you may even also just completely get rid of this code and just add "unload": "xpcom-shutdown" to rmap in the events getter, so that it's observed only when the implementer requires to observe it, as all the other events.
So, do we really need to nullify gExtensionObserver and gPreferenceObserver there? Do they keep anything alive more than usual in case we don't (not sure that really matters cause we are so late there, plus when we'll exit(0) that code won't be called at all).

@@ +550,4 @@
>      // references to various services, and to remove itself as
>      // observer of any other notifications.
>      this._obs = Cc["@mozilla.org/observer-service;1"].
>                  getService(Ci.nsIObserverService);

OT: Do we already have a bug to use Services.jsm in this old code?

@@ +550,5 @@
>      // references to various services, and to remove itself as
>      // observer of any other notifications.
>      this._obs = Cc["@mozilla.org/observer-service;1"].
>                  getService(Ci.nsIObserverService);
> +    this._obs.addObserver(this, "xpcom-shutdown", /* ownsWeak = */ true);

nit: I don't think commenting on each weak ref is really useful, all observer-like interfaces have the same behavior of false=strong, true=weak.  If it's unclear, we should evaluate adding addWeakObserver :)  Though, if you think it clarifies things, fine!
Comment 53 Marco Bonardo [::mak] 2012-05-31 09:57:59 PDT
Comment on attachment 623894 [details] [diff] [review]
FUEL - Part 2, Fix browser leaks.

Review of attachment 623894 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/fuel/src/fuelApplication.js
@@ +116,1 @@
>  function Window(aWindow) {

I think (actually just tried in Scratchpad) you can use the map inside the constructor itself and keep using new Window(), since this constructor is not exposed there's less risk of misuse.

  if (map.has(aWindow)) {
    return map.get(aWindow);
  }
  // init stuff
  map.set(aWindow, this);

The same for GetBrowserTab... alternatively, if you did this to be clearer about the fact this may be a new or existing instance, you may name it getOrCreateWindow().

Also, we don't use to uppercase functions in js, unless they are constructors.

@@ +138,5 @@
>     */
>    _watch : function win_watch(aType) {
>      var self = this;
>      this._tabbrowser.tabContainer.addEventListener(aType,
> +      function(e){ self._event(e); },

why doesn't this object implement nsIDOMEventListener and provide handleEvent instead of its own custom implementation?

@@ +177,5 @@
> +    fuelBrowserTabMap.set(aBrowser, fuelBrowserTab);
> +  }
> +  else {
> +    // A tab may change windows, so make sure our tab cached tab has the right
> +    // window.

"A tab may move to another window, so make sure the cached window is the current ones."

@@ +195,5 @@
>  }
>  
>  BrowserTab.prototype = {
> +  get _tabbrowser() {
> +    return this._window._tabbrowser;

accessing "private" property of Window :( Though I see this was there already
Comment 54 Marco Bonardo [::mak] 2012-05-31 11:32:33 PDT
Comment on attachment 623895 [details] [diff] [review]
FUEL - Part 3, Fix preferences leaks.

Review of attachment 623895 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/exthelper/extApplication.js
@@ +165,5 @@
> +
> +function PreferenceObserver() {
> +  this._observersDict = {};
> +  this._prefs = Components.classes["@mozilla.org/preferences-service;1"]
> +                          .getService(Ci.nsIPrefService);

may be a lazyServiceGetter... though, looks unused?

@@ +168,5 @@
> +  this._prefs = Components.classes["@mozilla.org/preferences-service;1"]
> +                          .getService(Ci.nsIPrefService);
> +}
> +
> +PreferenceObserver.prototype = {

is there a specific reasons this is an instanceable object than just a plain let gPreferenceObserver = Object.freeze({...}); ?

@@ +170,5 @@
> +}
> +
> +PreferenceObserver.prototype = {
> +  // aPrefs is the nsIPrefBranch onto which we'll install our listener.
> +  addListener: function(aPrefs, aDomain, aEvent, aListener) {

please, proper javadoc and name the methods

@@ +180,5 @@
> +
> +    if (!observer) {
> +      observer = {
> +        events: new Events(),
> +        observe: function(aSubject, aTopic, aData) {

please name the method

@@ +191,5 @@
> +      observer.prefBranch.addObserver(aDomain, observer, /* ownsWeak = */ true);
> +
> +      if (!this._observersDict[root]) {
> +        this._observersDict[root] = {};
> +      }

we just made this at the beginning of the method...

@@ +192,5 @@
> +
> +      if (!this._observersDict[root]) {
> +        this._observersDict[root] = {};
> +      }
> +      this._observersDict[root][aDomain] = observer;

please add a comment clarifying the ownership here, cause it's really easy to miss the fact the dict is keeping the observer alive, rather than the pref branch.

@@ +205,5 @@
> +      return;
> +    }
> +    var observer = this._observersDict[root][aDomain];
> +
> +    observer.events.removeListener(aEvent, aListener);

nit: move newline one line above

@@ +213,5 @@
> +      // aPrefs is the same object as observer.prefBranch, so we have to call
> +      // removeObserver on observer.prefBranch.
> +      observer.prefBranch.removeObserver(aDomain, observer);
> +      delete this._observersDict[root][aDomain];
> +      if (this._observersDict[root] === {}) {

are you sure this works? doesn't look like it would
I think you should rather check Object.keys(this._observersDict).length === 0

@@ +236,1 @@
>    this._prefs.QueryInterface(Ci.nsIPrefBranch);

hm, getBranch already returns nsIPrefBranch, the getService above could just do getService(Ci.nsIPrefBranch). Not sure what's the point of this QI then.

@@ +236,3 @@
>    this._prefs.QueryInterface(Ci.nsIPrefBranch);
>  
> +  let self = this;

since you only use self._prefs, would be cleaner to have let prefs = this._prefs; and use that.

@@ +236,5 @@
>    this._prefs.QueryInterface(Ci.nsIPrefBranch);
>  
> +  let self = this;
> +  this._events = {
> +    addListener: function(aEvent, aListener) {

space after "function" for anonymous functions, but probably better naming these
Comment 55 Marco Bonardo [::mak] 2012-05-31 12:21:08 PDT
Comment on attachment 623896 [details] [diff] [review]
FUEL - Part 4, Fix bookmarks leaks.

Review of attachment 623896 [details] [diff] [review]:
-----------------------------------------------------------------

I admit I had to close more than 1 eye on this part, this bookmarks abstraction is terrible... but not your fault and not related to this bug so... whatever :(

::: browser/fuel/src/fuelApplication.js
@@ +346,5 @@
> +function BookmarksObserver() {
> +  this._eventsDict = {};
> +  this._folderEventsDict = {};
> +  this._rootEvents = new Events();
> +  Utilities.bookmarks.addObserver(this, /* ownsWeak = */ true);

We should stop using Utilities for places services and actually use defineLazyModuleGetter and use PlacesUtils

@@ +347,5 @@
> +  this._eventsDict = {};
> +  this._folderEventsDict = {};
> +  this._rootEvents = new Events();
> +  Utilities.bookmarks.addObserver(this, /* ownsWeak = */ true);
> +}

same question as before, couldn't this just be a gBookmarksObserver and effectively add the observer first time an addxxx method is invoked?

@@ +352,5 @@
> +
> +BookmarksObserver.prototype = {
> +  onBeginUpdateBatch : function() {},
> +  onEndUpdateBatch : function() {},
> +  onBeforeItemRemoved : function(aId) {},

please don't define unused args (not going to repeat)

@@ +354,5 @@
> +  onBeginUpdateBatch : function() {},
> +  onEndUpdateBatch : function() {},
> +  onBeforeItemRemoved : function(aId) {},
> +
> +  onItemAdded : function(aId, aFolder, aIndex, aItemType, aURI) {

please name actually used methods
nit: s/aId/aItemId/, s/aFolder/aParentId/ everywhere

@@ +373,5 @@
> +    this._dispatchToEvents("change", aProperty, this._eventsDict[aId]);
> +  },
> +
> +  onItemMoved: function(aId, aOldParent, aOldIndex, aNewParent, aNewIndex) {
> +    this._dispatchToEvents("move", aId, this._eventsDict[aId]);

I wonder what's the usefulness of knowing something moved without knowing from where and to where... probably we should rather notify "remove" and "add" instead of "move", at least you can do something with it.  btw, I actually hope nobody uses this crappy fuel listener...

@@ +457,1 @@
>  function Bookmark(aId, aParent, aType) {

Sorry, I don't understand why new Bookmark() would be bad in this case, is it not the same?

@@ +464,1 @@
>    var self = this;

better to have let id = this._id; and use id...
nit: s/aId/aItemId/, s/aParent/aParentId/, s/aType/aItemType/

@@ +543,5 @@
> +  onItemAdded : function(aId, aFolder, aIndex, aItemType, aURI) {},
> +  onBeforeItemRemoved : function(aId) {},
> +  onItemVisited: function(aId, aVisitID, aTime) {},
> +  onItemRemoved : function(aId, aFolder, aIndex) {},
> +  onItemChanged : function(aId, aProperty, aIsAnnotationProperty, aValue) {},

remove unused args

@@ +573,1 @@
>  function BookmarkFolder(aId, aParent) {

don't get the difference again, sorry

@@ +721,5 @@
> +  onEndUpdateBatch : function bmf_oeub() {},
> +  onItemAdded : function bmf_oia(aId, aFolder, aIndex, aItemType, aURI) {},
> +  onBeforeItemRemoved : function bmf_oir(aId) {},
> +  onItemRemoved : function bmf_oir(aId, aFolder, aIndex) {},
> +  onItemChanged : function bmf_oic(aId, aProperty, aIsAnnotationProperty, aValue) {},

pls remove all naming and args

@@ +744,3 @@
>    get menu() {
>      if (!this._menu)
> +      this._menu = GetBookmarkFolder(Utilities.bookmarks.bookmarksMenuFolder, null);

Sigh, all of these should use PlacesUtils.bookmarksMenuFolderId, PlacesUtils.toolbarFolderId and so on, to avoid crossing xpcom :(
Comment 56 Marco Bonardo [::mak] 2012-05-31 12:55:38 PDT
Comment on attachment 623897 [details] [diff] [review]
FUEL - Part 5, Fix extensions.

Review of attachment 623897 [details] [diff] [review]:
-----------------------------------------------------------------

mostly same comments apply but globally looks fine

::: toolkit/components/exthelper/extApplication.js
@@ +454,5 @@
> +  this._eventsDict = {};
> +
> +  AddonManager.addAddonListener(this);
> +  AddonManager.addInstallListener(this);
> +}

not going to repeat the same question again :)

@@ +518,5 @@
> +
> +  var self = this;
> +  this._events = {
> +    addListener: function(aEvent, aListener) {
> +      gExtensionObserver.addListener(self.id, aEvent, aListener);

let id = this.id and use id.
Comment 57 Marco Bonardo [::mak] 2012-05-31 12:56:10 PDT
Comment on attachment 623898 [details] [diff] [review]
FUEL - Part 6, Remove gShutdown.

basically what I said in https://bugzilla.mozilla.org/show_bug.cgi?id=750454#c52
Comment 58 Marco Bonardo [::mak] 2012-05-31 12:57:36 PDT
Comment on attachment 623899 [details] [diff] [review]
FUEL - Part 7, Test fixes

Review of attachment 623899 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/fuel/test/browser_Bookmarks.js
@@ +204,5 @@
> +  testFolderC.remove();
> +  is(gLastRootAction, "remove");
> +
> +  testFolderD.description = "Foo";
> +  is(gLastRootAction, "bookmarkProperties/description");

my god, I never figured we expose description through fuel.
Comment 59 Justin Lebar (not reading bugmail) 2012-06-01 13:13:29 PDT
Thanks for the review!  Some questions and comments:

> nit: I don't think commenting on each weak ref is really useful, all observer-like interfaces have 
> the same behavior of false=strong, true=weak.  If it's unclear, we should evaluate adding 
>  addWeakObserver :)  Though, if you think it clarifies things, fine!

I have a personal vendetta against public APIs which take opaque arguments (particularly booleans), so if it's OK with you, I'd very much like to keep these /* ownsWeak = */ comments in there.

http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html

I'm also really curious why we name these methods.  If we compare

  doFoo: function() {...}

to

  doFoo: function FOP_doFoo() { ... }

the latter adds verbiage with nearly zero benefit.  (The only case where it matters is when doFoo wants to reference itself and can't do this.doFoo for whatever reason, right?)

> (From comment 52)
> I [rather?] we'd not need to nullify those singletons

I believe I tested and we need to set the singletons to null, otherwise we leak and the testsuite turns orange.  I'll test again.  Note that the observer singletons necessarily keep a lot of objects alive.

> (From comment 52)
> OT: Do we already have a bug to use Services.jsm in this old code?

I guess this is a rhetorical question?  I don't think anybody has looked carefully at this code for some time, so it seems unlikely that such a bug exists.

> (From comment 53)
> I think (actually just tried in Scratchpad) you can use the map inside the constructor itself and keep using new Window(), since this 
> constructor is not exposed there's less risk of misuse.

Huh, that's a cool trick.  But I'm afraid this still creates a new Window object, which is immediately garbage.  Creating a bit more garbage for Windows would probably be fine, but it likely wouldn't be fine for e.g. tabs (you could have hundreds of tabs and access them in a loop).  So my preference would be to use the straightforward get-or-create functions, unless we know that this is identical.

> (From comment 53)
>> +    // A tab may change windows, so make sure our tab cached tab has the right
>> +    // window.
>
> "A tab may move to another window, so make sure the cached window is the current ones."

How would you feel about: "A tab may move to another window, so make sure that our cached tab's _window property is up to date."?

>> +  onBeforeItemRemoved : function(aId) {},
>
> please don't define unused args (not going to repeat)

How strongly do you feel about this?

If I came in and decided to add an implementation to onBeforeItemRemoved, and it didn't have the aId param, I'd be pretty confused.  I think having the unused args here is good documentation.

If you really don't want to have the args, I'd be OK with

   onBeforeItemRemoved : function(/* args omitted */) {}

But I don't think that's an improvement, personally.
Comment 60 Marco Bonardo [::mak] 2012-06-01 13:38:55 PDT
(In reply to Justin Lebar [:jlebar] from comment #59)
> I have a personal vendetta against public APIs which take opaque arguments
> (particularly booleans), so if it's OK with you, I'd very much like to keep
> these /* ownsWeak = */ comments in there.

sure, I feel the same vs blind booleans args/

> I'm also really curious why we name these methods.  If we compare

I think the main reason was for exception stacks showing anonymous functions, not sure if something changed in that regards lately.

> > (From comment 52)
> > I [rather?] we'd not need to nullify those singletons
> 
> I believe I tested and we need to set the singletons to null, otherwise we
> leak and the testsuite turns orange.  I'll test again.  Note that the
> observer singletons necessarily keep a lot of objects alive.

ok, thanks for checking, it's unfortunate we can't simplify code due to those :(

> > (From comment 52)
> > OT: Do we already have a bug to use Services.jsm in this old code?
> 
> I guess this is a rhetorical question?  I don't think anybody has looked

sort of a rant mostly, yeah, this code is unmaintained mostly, that makes me think we should start saying users it's deprecated, and remove it soon or later, would it even be in 1 or 2 years...

> Huh, that's a cool trick.  But I'm afraid this still creates a new Window
> object, which is immediately garbage.  Creating a bit more garbage for
> Windows would probably be fine, but it likely wouldn't be fine for e.g. tabs
> (you could have hundreds of tabs and access them in a loop).  So my
> preference would be to use the straightforward get-or-create functions,
> unless we know that this is identical.

I don't have data to tell you that off-hand, for windows the overhead is likely unimportant, so maybe you could keep new for Window and use getOrCreate for tabs, adding a comment about why we don't use the same trick? (overhead)

> How would you feel about: "A tab may move to another window, so make sure
> that our cached tab's _window property is up to date."?

Fine, regardless I'm not english mother tongue, just the original phrase was sounding unclear.

> >> +  onBeforeItemRemoved : function(aId) {},
> >
> > please don't define unused args (not going to repeat)
> 
> How strongly do you feel about this?

I think there may be a tiny overhead for defining single args (missing data though), but my actual motivation is mostly readability and code coherence, we usually do that around the codebase, afaict.  This is not blocking a positive review, btw.
Comment 61 Marco Bonardo [::mak] 2012-06-01 13:42:37 PDT
ah, fwiw, the args to mostly of those observer methods are wrong, or better, they are already missing many.
Comment 62 Justin Lebar (not reading bugmail) 2012-06-01 17:56:08 PDT
> alternatively, if you did this to be clearer about the fact this may be a new or existing instance, you may name it
> getOrCreateWindow().

I think whether we're "getting" or "creating" a Window object here is an implementation detail, so I've left this as getWindow/getBrowserTab.

> please, proper javadoc and name the methods

None of the methods in either fuelApplication.js or extApplication.js has a javadoc.

I know two wrongs don't make a right, but could you please clarify exactly which methods you want me to document with javadocs?

> space after "function" for anonymous functions, but probably better naming these

By my count, the no-space variant appears twice as often in our tree than the space variant.

  $ find . -name '*.js' | xargs grep 'function(' | wc -l
  23655
  $ find . -name '*.js' | xargs grep 'function (' | wc -l
  10250

Moreover, "function (" does not match how we write named functions (it's "function foo()", not "function foo ()").

Are you sure that "function (" is the right style to enforce?

> (From comment 55)
> We should stop using Utilities for places services and actually use defineLazyModuleGetter and use PlacesUtils

Are you gating the review on this, or is this something you think we can file a follow-up bug on?  (To be clear, I do not volunteer to write this follow-up patch.)

> nit: s/aId/aItemId/, s/aParent/aParentId/, s/aType/aItemType/

This touches a lot of code that I didn't touch here.  I think it's a good change, but I think it should be a separate bug.

> (From comment 55)
>> +  onEndUpdateBatch : function bmf_oeub() {},
>> +  onItemAdded : function bmf_oia(aId, aFolder, aIndex, aItemType, aURI) {},
>> +  onBeforeItemRemoved : function bmf_oir(aId) {},
>> +  onItemRemoved : function bmf_oir(aId, aFolder, aIndex) {},
>> +  onItemChanged : function bmf_oic(aId, aProperty, aIsAnnotationProperty, aValue) {},
> pls remove all naming and args

I don't get why these shouldn't have names while the other anonymous functions should, but I'm happy to remove their names if you want me to!  :)

> couldn't this just be a gBookmarksObserver and effectively add the observer first time an addxxx method is invoked?

I thought this was a good suggestion, and I tried it.

Unfortunately it's complicated to make this work.  extApplication.js is not included until the end of fuelApplication.js.  That means that gBookmarksObserver cannot call, for example, |new Events()| inside its object definition.  Getting around this requires adding a lot of lazy initialization, and I don't think it's worthwhile.

Are you OK leaving things the way they are?


As promised, I went through and investigated which of the global singletons must be cleared at shutdown so as to avoid "leaks" and corresponding orange.

 - BookmarksObsever   <-- must be cleared
 - ExtensionObserver  <-- does not need to be cleared
 - PreferenceObserver <-- must be cleared

If you want, I can nuke the |gExtensionObserver = null| call.
Comment 63 Justin Lebar (not reading bugmail) 2012-06-01 17:58:26 PDT
(In reply to Marco Bonardo [:mak] from comment #61)
> ah, fwiw, the args to mostly of those observer methods are wrong, or better,
> they are already missing many.

Well, that's quite bad.  :)

> I don't get why these shouldn't have names while the other anonymous functions should, but I'm happy 
> to remove their names if you want me to!  :)

I guess you want the names for stack traces, but you don't care about getting stack traces on these functions?  I thought stack traces now said "[anonymous function at foo.js:42]", but if not, I'm totally in favor of naming all functions we care about.
Comment 64 Marco Bonardo [::mak] 2012-06-02 02:19:02 PDT
(In reply to Justin Lebar [:jlebar] from comment #62)
> I know two wrongs don't make a right, but could you please clarify exactly
> which methods you want me to document with javadocs?

Only the ones where you are actually adding a comment before them. I don't want you to fix all the code, just the new one.

> Are you sure that "function (" is the right style to enforce?

yes, at least in browser

> > We should stop using Utilities for places services and actually use defineLazyModuleGetter and use PlacesUtils
> 
> Are you gating the review on this, or is this something you think we can
> file a follow-up bug on?  (To be clear, I do not volunteer to write this
> follow-up patch.)

Not gating, though I think it would make the patch simpler in certain places, and would improve performances and memory. If you don't think you have the time for that, nvm.

> > couldn't this just be a gBookmarksObserver and effectively add the observer first time an addxxx method is invoked?
> 
> I thought this was a good suggestion, and I tried it.
> 
> Getting around this requires adding a lot of lazy
> initialization, and I don't think it's worthwhile.
> 
> Are you OK leaving things the way they are?

ok, we should evaluate it in a follow-up, there is so much that can be improved in this code :/ (though, it may not be worth it, we should take a decision regarding deprecation).

> If you want, I can nuke the |gExtensionObserver = null| call.

my point was to remove the shutdown path, if regardless we can't do that, there's poor benefit.

(In reply to Justin Lebar [:jlebar] from comment #63)
> (In reply to Marco Bonardo [:mak] from comment #61)
> > ah, fwiw, the args to mostly of those observer methods are wrong, or better,
> > they are already missing many.
> 
> Well, that's quite bad.  :)

The fact is that when someone changes listener APIs it's hard he will go through each single listener in the codebase (may be tens or hundreds) to add new args, so defining all args often doesn't improve anything regarding the need to go looking at the idl/docs.

> I guess you want the names for stack traces, but you don't care about
> getting stack traces on these functions?

they are empty, so would be crazy to get a stack from there :)
Comment 65 Justin Lebar (not reading bugmail) 2012-06-04 20:09:37 PDT
>> Are you sure that "function (" is the right style to enforce?
> yes, at least in browser

I'm going to fix this, but please note that "function(" appears twice as often in browser/ than "function (".

I make a big deal of this because style nits waste everyone's time.  We minimize this wasted time by acknowledging that our codebase conforms to certain conventions, whether we like them or not, and that it's easier to write code which matches existing conventions than it is to guess what a reviewer thinks is the right style.

Literally every reviewer who's ever looked at a patch of mine has a different personal style guide.  I have to keep notes in order to keep things straight.
Comment 66 Justin Lebar (not reading bugmail) 2012-06-04 21:14:24 PDT
Created attachment 630049 [details] [diff] [review]
Part 1a, review comments (comment 52).
Comment 67 Justin Lebar (not reading bugmail) 2012-06-04 21:14:48 PDT
Created attachment 630050 [details] [diff] [review]
Part 2a, review comment (comment 53): Use handleEvent instead of rolling it ourselves.
Comment 68 Justin Lebar (not reading bugmail) 2012-06-04 21:15:09 PDT
Created attachment 630051 [details] [diff] [review]
Part 3a, review comments from comment 54.
Comment 69 Justin Lebar (not reading bugmail) 2012-06-04 21:15:31 PDT
Created attachment 630052 [details] [diff] [review]
Part 4a, review comments (comment 55)
Comment 70 Justin Lebar (not reading bugmail) 2012-06-04 21:15:49 PDT
Created attachment 630053 [details] [diff] [review]
Part 5a, review comments (comment 56).
Comment 71 Justin Lebar (not reading bugmail) 2012-06-04 21:16:17 PDT
Created attachment 630054 [details] [diff] [review]
Part 8: Reformat FUEL code.

I reformatted everything so there would be a consistent style.
Comment 72 Justin Lebar (not reading bugmail) 2012-06-04 21:16:32 PDT
Created attachment 630055 [details] [diff] [review]
Part 9: Add javadoc to PreferenceObserver.{add,remove}EventListener.
Comment 73 Justin Lebar (not reading bugmail) 2012-06-04 21:20:53 PDT
If you feel that this patch queue has gotten a bit out of hand, the github UI might make things more manageable.  https://github.com/jlebar/mozilla-central/compare/c2139e29efc1e1ff665867e75add7433cc913edd...jlebar:FUEL-750454
Comment 74 Marco Bonardo [::mak] 2012-06-05 12:18:45 PDT
Comment on attachment 630050 [details] [diff] [review]
Part 2a, review comment (comment 53): Use handleEvent instead of rolling it ourselves.

Review of attachment 630050 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/fuel/src/fuelApplication.js
@@ +78,5 @@
>    }
>    return fuelWindow;
>  }
>  
> +// Don't call new Winodw() directly; use getWindow instead.

typo: Winodw
Comment 75 Marco Bonardo [::mak] 2012-06-05 12:27:50 PDT
if you wish I may take a quick look to the coalesced patch, otherwise I think we're basically done.
Comment 76 Justin Lebar (not reading bugmail) 2012-06-05 12:29:02 PDT
Created attachment 630274 [details] [diff] [review]
Roll-up patch
Comment 77 Marco Bonardo [::mak] 2012-06-05 12:38:51 PDT
Comment on attachment 630274 [details] [diff] [review]
Roll-up patch

Review of attachment 630274 [details] [diff] [review]:
-----------------------------------------------------------------

just a couple nits, thanks for touching this old unmaintained code.

::: browser/fuel/src/fuelApplication.js
@@ +263,4 @@
>      return Utilities.annotations.itemHasAnnotation(this._id, aName);
>    },
>  
> +  get: function(aName) {

other methods have been names in this prototype

@@ +268,5 @@
>        return Utilities.annotations.getItemAnnotation(this._id, aName);
>      return null;
>    },
>  
> +  set: function(aName, aValue, aExpiration) {

ditto
Comment 78 Justin Lebar (not reading bugmail) 2012-06-05 13:00:55 PDT
One last try push, for good measure.  https://tbpl.mozilla.org/?tree=Try&rev=ac72aecb0cf0
Comment 79 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-05 15:55:54 PDT
FWIW, I agree that "function()" vs. "function ()" is not a distinction worth worrying about. I don't think it would have held up review either way, though :)
Comment 81 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-05 17:15:30 PDT
Do we have any telemetry stuff in place that might indicate how much this helps?
Comment 82 Justin Lebar (not reading bugmail) 2012-06-05 17:19:49 PDT
We measure RSS, but that needle is unlikely to move in aggregate.

If we could make a list of the relevant extensions, we could look at how RSS changes for people who have at least one FUEL-using extension installed.
Comment 84 Jonathan Protzenko [:protz] 2012-06-12 23:49:18 PDT
*** Bug 717724 has been marked as a duplicate of this bug. ***

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