Closed Bug 695475 Opened 13 years ago Closed 6 years ago

Create a tool to help users identify a problematic add-on by bisecting the list of installed add-ons

Categories

(Toolkit :: Add-ons Manager, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED INACTIVE

People

(Reporter: ehsan.akhgari, Assigned: matrix.org)

References

Details

(Whiteboard: [MemShrink:P2][lang=js])

Attachments

(1 file, 3 obsolete files)

Here is what I have in mind.  We can create a tool which takes a Firefox profile, and helps the user bisect its add-ons to see if they can reproduce memory leaks.  It can be implemented as an add-on itself or as a standalone tool, or as a combination.  Here's how you would use it:

Initiate the tool.  It will first disable all of your add-ons, and then will restart Firefox.  When Firefox is restarted, it shows up a page instructing the user that all of the add-ons are disabled, and they should try to see if they can reproduce the memory leak bug.  If they can't, then the tool will enable the first half of the add-ons and restart and show another page stating what happened.  And the bisection goes on like that.  All that the user needs to do here is to just test to see if they can repro the bug under the different Firefox instances.
...and then once you find a leaky addon, you submit it to AMO, which they use to mark the add-on as leaky, per bug 695481!
Josh: do you know of any contributors who might be interested in working on this tool?  It should be fairly easy to implement as an add-on...
Whiteboard: [MemShrink] → [MemShrink:P2][mentor=ehsan]
Whiteboard: [MemShrink:P2][mentor=ehsan] → [MemShrink:P2][mentor=ehsan][lang=js]
Blocks: 677861
Who would be responsible for leak identification -- the end user or the addon?

Is the intent of the addon to simplify bisection for end users who are sufficiently knowledgeable to identify leaks, and could benefit from a tool to bisect installed addons to find the addon(s) that trigger a specific leak?  (Easier to implement, but narrows target audience).

Or would the addon be responsible for implementing one or more of the leak tools [1], and then notifying the user that it detected a leak, and offering to bisect if the user will repeat what they did to trigger the leak?  Would it try to isolate and identify a specific "leak signature"?  (Harder to implement, but accommodates a larger target audience).

Ultimately, who is the target audience -- memory leak savvy Developers or general End Users?

The addon should probably first warn the user that bisection will require log2(N) iterations (with N installed and enabled addons), and potentially more if the user has difficulty reproducing or re-identifying the given leak, although perhaps less if the user can provide additional input (that proves accurate), eg which addons are suspected as possibly leaking or are suspected as not leaking.  The addon should also restore the initial addon enabled/disabled state after bisection, offering to additionally disable/uninstall/report the leaky addon (if positively identified through bisection).

Additionally, while some of the addon memory usage may be leaks, a given addon may simply consume significant memory in its operation.  Users may also be interested in an addon to profile memory usage of each addon in turn to realize the memory "cost" of leaving a given addon enabled.  Users may want to leave memory intensive addons installed, but disable them until needed, depending on the memory footprint.  However, this angle may be out of scope?

Lastly, it should also be noted that while an addon may trigger a leak, the cause of the leak may be firefox itself.  Identifying addons that expose leaks in firefox may be more accurately regarded as firefox leaks rather than classifying the addon itself as leaky?

[1] https://wiki.mozilla.org/Performance:Leak_Tools (there may be others, but this list was cited in another [MemShrink] bug)
The scope of this bug is focused: provide a way to bisect the set of addons.  Other aspects of finding leaky addons, like figuring out whether an addon is leaking, or is using too much memory, are left to existing tools.  The idea is to implement a tool like hg bisect ( http://mercurial.selenic.com/wiki/BisectExtension ), except for addons.  This would help the user figure out which addon may be causing a particular problem they are having, leak or otherwise.  I'll update the summary to reflect this.

The scenario this is intended to help solve is a user files a bug saying they have a memory leak of some sort, but they happen to have a dozen or so addons.  First we ask them to try to reproduce the leak without any addons, because it is easy to restart with no addons.  If the leak doesn't reproduce, then what do you do?  Manually bisecting the set of addons is tedious and error prone.  We've seen users with multiple leaky addons (I think?), but we haven't seen any cases where the interaction of two addons causes a leak.
OS: Mac OS X → All
Hardware: x86 → All
Summary: Create a tool which users can use to bisect the list of add-ons that they have installed to find the leaking one → Create a tool to help users identify a problematic add-on by bisecting the list of installed add-ons
You have a lot of good points here.  I'll directly address a few.

(In reply to linux.user.since.2002 from comment #3)
> However, this angle may be out of scope?

That's a good idea, but I think even a general addon bisection tool wold help with this scenario.  A user could run bisection to find a problematic addon.  Once that is done, they can manually disable it or enable it as needed using the existing tools.  That's easy to do, it is just annoying to figure out what is causing the problem.

> Identifying addons that expose
> leaks in firefox may be more accurately regarded as firefox leaks rather
> than classifying the addon itself as leaky?

That's okay.  Our goal here is not really to blame anybody, but to provide a way to create a simpler test case to figure out what is wrong.
Will this work for addons that crash the browser?

What about addons that only leak when used together? A binary search won't work for those, so it's probably beyond the scope of this bug, but is there a plan to handle those? (e.g. when one range reproduces the bug but its two halves don't, systematically try out different partitions within that range)
(In reply to Emanuel Hoogeveen from comment #6)
> Will this work for addons that crash the browser?

It seems to me that it would work for any kind of addon problem, though I guess with crashing addons you have to worry about whether you can save the state afterwards I guess?
 
> What about addons that only leak when used together?

I don't think we've really come across any cases like that, so I don't know how common it is.  For a first go at this, I think it is okay to not worry about it, and just tell the user this may be the problem.  Bonus points for giving the last set of addons that reproduced the problem.

If we have this tool available, and addon interaction is really a problem in practice, we can think about how to best deal with that.
(In reply to Andrew McCreight [:mccr8] from comment #7)
> I don't think we've really come across any cases like that, so I don't know
> how common it is.
I didn't really mean to use the word 'leak' there in retrospect. I seem to remember some cases where a combination of addons caused breakage on the web - but yeah, they're probably (hopefully) rare.

Thinking about a potential way to handle cases like that a bit more, the first departure from a normal binary search is that you have to try both halves to see if they reproduce, rather than assuming the other half will reproduce when the first half doesn't. After that, if neither half reproduces I guess you'd try the first and last quarter, then the second and third quarters, and so on.
(In reply to Emanuel Hoogeveen from comment #6)
> What about addons that only leak when used together?

There are also addons that depend on others.  Firebug extensions would be a classic example; however, there are other such addons.  Such an addon may leak memory, but might only leak if its dependency is also enabled -- perhaps otherwise just throwing an alert() telling the user to install/enable the dependent addon and then stopping without running the leaky code.  Further, the depending addon may simply expose/trigger a leak in its dependency.

While I cannot speak to leaks specifically, I do know there are such addons that I ended up disabling and removing for perceived poor performance/interoperability.  Do not recall if that happened to be memory related however.

Two non-depending addons could theoretically result in a memory leak used in tandem, if the first addon puts firefox in some state that the second addon would not normally trigger, but while in that state, exposes a leak in firefox itself.  However, this is perceived as unlikely?

(In reply to Emanuel Hoogeveen from comment #8)
> I seem to
> remember some cases where a combination of addons caused breakage on the web
> - but yeah, they're probably (hopefully) rare.

This may depend on the nature of the addons installed.  If an end user decides to install numerous addons from a specific category, the combination of those addons can negatively interact and result in UI as well as web breakage (addon-addon conflicts relations).

This is likely rare amongst developers; however, I wonder how many end users just keep installing related addons until they achieve a desired effect (or get frustrated and give up)?  Not sure how many end users would fall into that category to guess frequency; however, this may be remote as well.

Regardless, simple bisection seems most prudent here (at least for an initial implementation), as it would likely isolate the majority of leaking addons...
I think everyone is worrying about 1% cases.  I suspect in 99% of cases this will be very useful.  Let's not overthink it.
Hello Ehsan, I'm interested in picking this up. Are you available for mentoring?
Yes!  Are you familiar with the Add-on Manager API? <https://developer.mozilla.org/en-US/docs/Addons/Add-on_Manager>
Also, do you have any experience developing Firefox extensions?
No to both the questions.
Hmm, I think this project is best for somebody who's familiar with developing add-ons for Firefox, and cannot be a good introduction point to become familiar with writing add-ons...
Hello Ehsan, I would like to work on this, I am getting started with reading about addons, what else do I need to read about and what all hands on experience on tools or procedures do I need to have to start implementation.
This would be my first contribution and loved the support given by you to newbies!
(In reply to comment #16)
> Hello Ehsan, I would like to work on this, I am getting started with reading
> about addons, what else do I need to read about and what all hands on
> experience on tools or procedures do I need to have to start implementation.
> This would be my first contribution and loved the support given by you to
> newbies!

Sorry for the delay in getting back to you -- this happened to coincide with my vacation.

I suggest looking into the Add-on Manager APIs once you're done reading about the basics of creating new extensions.
Hello,

I would like to give this a shot.  It looks like a good way to get involved.  I have developed an number of addons and I can say that many users would love this when debugging issues.

Would anyone be available to mentor me? (Ehsan?).
Argh sorry Kevin but I'm too busy these days.  Josh, can you please see if you can find somebody else who is interested in mentoring Kevin?  Thanks!
Flags: needinfo?(josh)
Whiteboard: [MemShrink:P2][mentor=ehsan][lang=js] → [MemShrink:P2][mentor=?][lang=js]
Blair, is this something you could mentor?
Flags: needinfo?(josh) → needinfo?(bmcbride)
Sure :) I'm a bit swamped myself, so there may be the odd time where I'm a little slow to respond.

Kevin: Have you familiarized yourself with the Add-ons Manager API (https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/Add-on_Manager)? And is the aim of this bug clear?


If you want me to see something on this bug, be sure to needinfo? me (put my email in the "Need more information from" field below the comment box on this bug). Otherwise just email or find me on IRC (Unfocused) - note that I'm in New Zealand, which makes timezones extra fun.
Assignee: nobody → kevincox
Status: NEW → ASSIGNED
Flags: needinfo?(bmcbride)
Whiteboard: [MemShrink:P2][mentor=?][lang=js] → [MemShrink:P2][mentor=bmcbride@mozilla.com][lang=js]
(In reply to Blair McBride [:Unfocused] from comment #21)
> Sure :) I'm a bit swamped myself, so there may be the odd time where I'm a
> little slow to respond.
> 

No worries.

> Kevin: Have you familiarized yourself with the Add-ons Manager API
> (https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/Add-on_Manager)?

Yes, I used most of it developing an addon.

> And is the aim of this bug clear?
> 

Yup, sounds reasonably straight forward to me.

I am envisioning an API that deals with restarting the browser with different addons enabled and disabled.  I plan to make it generic and callback based so that it could be scripted (if you can problematically reproduce and detect the error) but for the first go it would just have a simple gui driver that asked the user to reproduce the bug and report if it occurred.

> 
> If you want me to see something on this bug, be sure to needinfo? me (put my
> email in the "Need more information from" field below the comment box on
> this bug). Otherwise just email or find me on IRC (Unfocused) - note that
> I'm in New Zealand, which makes timezones extra fun.

I just am wondering where to put the code.  Both physically in the tree and in the firefox API and UI.  Do you have any recommendations.

/toolkit/modules/ would be a nice place for `Cu.import` but I'm not sure it fits with what's in there.
Flags: needinfo?(bmcbride)
(Ugh, sorry - took a couple of days off)


(In reply to Kevin Cox from comment #22)
> I just am wondering where to put the code.  Both physically in the tree and
> in the firefox API and UI.  Do you have any recommendations.
> 
> /toolkit/modules/ would be a nice place for `Cu.import` but I'm not sure it
> fits with what's in there.

Lets put it with the Add-ons Manager, and have the Add-ons Manager UI as an entry-point to starting this tool. So the main API of the tool would go in /toolkit/mozapps/extensions/ (AddonTroubleshooting.jsm?), and the UI would go in /toolkit/mozapps/extensions/content/

I think your idea of concentrating on the base functionality first and only having a very simple UI for now is a good one. That'll give us some time to figure out what kind of UI we want for this, which is the main unknown thing here IMO.
Flags: needinfo?(bmcbride)
Attached patch Inital AddonBisector API work. (obsolete) — Splinter Review
Here is some initial work on the API.  Here is a quick example.

////////////////////////////////////
// Starting.
Cu.import("resource://gre/modules/AddonBisector.jsm");
if (AddonBisector.ongoing) {
  // Oops, someone else is bisecting.
} else {
  AddonBisector.start(function(cb, stats){
    console.log("Starting bisection!");
    cb(); // The callback sets up the addons and restarts the browser.
  });
}
////////////////////////////////////
// Continuing.
let errorp = doesErrorOccur();

Cu.import("resource://gre/modules/AddonBisector.jsm");
AddonBisector.mark(function(cb, stats){
  if (stats.done) {
    console.log("The bad addon was: "+stats.bad);
  }
  cb(); // If not done, sets up the next iteration and restarts.
        // If done restores previous browser state and restarts.

}, !errorp); // Takes a value if the browser is "good" in this state.
////////////////////////////////////

I left calling up to the user.  They are responsible for starting up after the browser restarts and calling `mark()` with the results.

Both the functions have a similar API and the stats object is identical so it is likely that the handler for each can be the same, or at least very similar.

Any thoughts?  There will likely be a couple of changes before the UI is finished.

No rush, I have exams I need to study for :D
Flags: needinfo?(bmcbride)
Attached patch Complete patch. (obsolete) — Splinter Review
Here is a patch that contains an update the the API as well as the GUI.

I removed the state objects from the API and made them properties on the AddonBisector object.  They were global anyways and they were useful outside of the callbacks.

I also added an initial iteration with no addons enabled to ensure it is not a firefox problem.

The UI is fairly simple (and a little ugly) but it provides a nice overview of what is happening and should be pretty straight forward to use.

Any recommendations?
Attachment #8344946 - Attachment is obsolete: true
Comment on attachment 8346900 [details] [diff] [review]
Complete patch.

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

Discussed over IRC. Relevant part of the log:


<Unfocused> general structure of the API you've built looks good
<Unfocused> just struggled following some of the code due to the abbreviated names of things - eg, hard to see what addonBisector.js is doing without also referring to AddonBisector.jsm at the same time
<kevincox> ok. Do you mean local variable names?
<Unfocused> yea
<Unfocused> and the properties on AddonBisector too. eg, AddonBisector.bad is only a getter, but you cant tell that just from looking at the name (just based on the name, you could also think it could be used to mark an addon as bad)
<kevincox> Ah, ok I see. `badAddon` is probably better?
<kevincox> Then I rename good -> goodAddons
<kevincox> Same with unknown.
<Unfocused> yea, that'd be an improvement
<kevincox> kk
<Unfocused> also wonder if instead of AddonBisector.ongoing, we should have AddonBisector.state - which would be one of several AddonBisector.STATE_* constants (see AddonManager.jsm for examples of this)
<kevincox> I thought about that but I could only come up with STATE_ONGOING and STATE_NONE.
<Unfocused> for now we wouldn'tneed many states, but in the future it'd be great if we could detect if two addons conflict with one another (STATE_CONFLICT)
<Unfocused> but there's also the case where the user gives us input that ends up not making any sense, so we can't actually figure out what's wrong
<Unfocused> or if we find a problem when all addons are disabled
<kevincox> That seams more like a result than a state.
<Unfocused> oh, yea - you could have a separate result property, that'd work nicely
<kevincox> Currently we just return the bad addon ID or undefined if no addon caused the bug (ie firefox bug). Should we return a more elaborate object with a reason and the findings?
<Unfocused> yea, i think so
<kevincox> ok.
<kevincox> If we eventually detect conflicts between two addons we will have multiple results, so should we make badAddons an array?
<kevincox> ie bad -> badAddons instead of badAddon
<Unfocused> yep
<kevincox> ok
<kevincox> And do you still think we need state constants or keep it boolean? I thought of allowing apps to "tag" a bisection with a name so that they wouldn't mess up someone else's bisection. Do you think that would be useful?
<kevincox> The reason I didn't add it first version is the user will probably be prompted to start a bisection, so they can do their own "locking".
<Unfocused> hm. isn;t there 3 states? no bisection started, bisection in progress, and bisection finished with results available?
<kevincox> Good call. I have `ongoing` and `done` which should be merged into a `state` constant.
<Unfocused> multiple bisections is too confusing, imo. if one is currently running, we should just not allow another to start. but there should be an API to stop a bisection mid-way
<kevincox> multiple: I was just worried about stale bisections messing everything up, so I made `start()` always succeed. As I said, the user will almost certainly be prompted. Although a more forceful lockout could be added.
<kevincox> As for clearing a bisection, this is a definite should have, but it isn't strictly necessary with the always-succeeds `start()`. I'll add it next patch.
<kevincox> *next patch version.
<Unfocused> kevincox: cool :)
<kevincox> I think that keeping start() as is is probably better as it saves users from checking if they have "control" it also allows for generic UI's. I think that if the user requests a bisection we should just obey. The only "danger" I see of aborting a bisection is that enabled/disabled addons are messed up.
<Unfocused> kevincox: ah, yep. in that case, we should figure out a way to reset the current bisection and start fresh
<kevincox> Yes, I'll create the abort() talked about earlier that would provide a clean exit.
<Unfocused> sounds good :)
<kevincox> Why I don't want start to deal with this is I imagine a user uncleanly leaving a bisection then starting a new one much later with different addons and geting a crazy enabled/disabled state upon it trying to "abort" an old bisection.
<Unfocused> kevincox: yep, makes sense
Attachment #8346900 - Flags: feedback+
Some other thoughts:
* We didn't discuss the UI. I'll have a ponder about that over the next couple of days, but next year  we'll want to get someone from the UX team involved for that part.
* You should have a look through AddonManager.jsm for general code style - it's much easier to maintain code when it all *feels* the same.
* When building AddonBisector.jsm, think about how you would write tests for the module. This is the type of module that we should be aiming to make testable via XPCShell [1], which means only API calls - no UI.


[1] https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/XPCShell
Flags: needinfo?(bmcbride)
Attached patch Updated patch. (obsolete) — Splinter Review
Here is an updated patch.

Notable changes:
- Added init() method as in the future the state will probably be put somewhere other than the preferences tree and will likely be async.
- Renamed properties on AddonBisector to be more descriptive.
- Added abort() method.
- Catch errors in callbacks and log useful messages.
- Changed definition style to match AddonManager.

About the additional comments.

I looked into the testing and it appears pretty straight forwards.  I just need to see how exactly dealing with addons works within XPCShell.  I will work on this next.

I don't claim to know anything about UX so I would love some advise there.  What I have now is just intended to be simple.
Attachment #8346900 - Attachment is obsolete: true
Here is another patch.  I feel that it is getting fairly complete.  Any feedback is of course welcome.

Notably there are tests.  They exercise most of the logic in the AddonBisector module.  The exception is actually restarting the browser as the harness appears to ignore the restart attempt.

I also made further effort to make the code look and feel like AddonManager.
Attachment #8351051 - Attachment is obsolete: true
Attachment #8355422 - Flags: review?(bmcbride)
Flags: needinfo?(bmcbride)
Mentor: bmcbride
Whiteboard: [MemShrink:P2][mentor=bmcbride@mozilla.com][lang=js] → [MemShrink:P2][lang=js]
Flags: needinfo?(bmcbride)
Blair, have you had a chance to review the latest patch? If you don't have time for this, can you recommend someone else to review it?
Flags: needinfo?(bmcbride)
Flags: needinfo?(bmcbride)
Attachment #8355422 - Flags: review?(bmcbride)
(In reply to Mike Cooper [:mythmon] from comment #30)
> Blair, have you had a chance to review the latest patch? If you don't have
> time for this, can you recommend someone else to review it?

My apologies - this got lost while I was swamped with work, then continued being lost while I've been on extended leave for the past year.

In terms of technical review, I'd suggest Jared Wien/Gijs Kruitbosch/Matthew Noorenberghe. Although, I'd suggest a fresh look at this from a UX perspective too.
Mentor: bmcbride
(In reply to Blair McBride [:Unfocused] (unavailable) from comment #31)
> In terms of technical review, I'd suggest Jared Wien/Gijs Kruitbosch/Matthew
> Noorenberghe

Er, and Dave Townsend, of course - but IIRC this didn't touch any deep Add-ons Manager internals.
This product looks more suitable for it, like mozregression.
Severity: normal → enhancement
Product: Core → Testing
No, this is specifically about an in-product tool in the Add-ons Manager (see the work-in-progress patch), and I don't want to derail that. If doing something similar in mozregression would be useful, then please file a new bug.
Component: General → Add-ons Manager
Product: Testing → Toolkit
(In reply to :Ehsan Akhgari from comment #0)
> Here is what I have in mind.  We can create a tool which takes a Firefox
> profile, and helps the user bisect its add-ons to see if they can reproduce
> memory leaks.  It can be implemented as an add-on itself or as a standalone
> tool, or as a combination.  Here's how you would use it:
> 
> Initiate the tool.  It will first disable all of your add-ons, and then will
> restart Firefox.  When Firefox is restarted, it shows up a page instructing
> the user that all of the add-ons are disabled, and they should try to see if
> they can reproduce the memory leak bug.  If they can't, then the tool will
> enable the first half of the add-ons and restart and show another page
> stating what happened.  And the bisection goes on like that.  All that the
> user needs to do here is to just test to see if they can repro the bug under
> the different Firefox instances.

I suppose that this tool would be usable, not only with Firefox itself, but also with any application using the same Add-ons Manager? (Thunderbird, SeaMonkey, I'm not sure about Firefox for Mobile and FxOS)
(In reply to Tony Mechelynck [:tonymec] from comment #35)
> I suppose that this tool would be usable, not only with Firefox itself, but
> also with any application using the same Add-ons Manager? (Thunderbird,
> SeaMonkey, I'm not sure about Firefox for Mobile and FxOS)

Yes. And while mobile would need a new UI, the logic can be shared.
did this effort migrate elsewhere? or is it still 'here', just buried under busy?
(In reply to PGNet Dev from comment #37)
> did this effort migrate elsewhere? or is it still 'here', just buried under
> busy?

The latter, as far as I know :(
That's how I understand it as well.
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE

Does this functionality still exist in firefox?

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

Attachment

General

Created:
Updated:
Size: