Last Comment Bug 751466 - Get add-ons to upgrade from old versions of the SDK
: Get add-ons to upgrade from old versions of the SDK
Status: RESOLVED DUPLICATE of bug 783046
:
Product: Tech Evangelism
Classification: Other
Component: Add-ons (show other bugs)
: unspecified
: All All
: -- normal with 3 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 753091
Blocks: 752468 752497 752500 783046
  Show dependency treegraph
 
Reported: 2012-05-02 22:36 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-08-21 16:37 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
MXR search results (328.47 KB, text/html)
2012-05-04 04:43 PDT, Andrew Williamson [:eviljeff]
no flags Details
Table of add-ons using old SDK (195.35 KB, text/html)
2012-05-07 00:41 PDT, Wladimir Palant
no flags Details
Same table without SDK 1.4 and 1.5 (124.07 KB, text/html)
2012-05-07 22:39 PDT, Wladimir Palant
no flags Details

Description Justin Lebar (not reading bugmail) 2012-05-02 22:36:53 PDT
Old versions of the Jetpack SDK are known to leak.  Moreover, these leaks appear not to be fixed by severing chrome -> content leaks (i.e., bug 695480 does not fix e.g. bug 751420).

We should therefore aggressively move add-ons off old versions of the SDK.

SDK-based add-ons should not be approved for AMO if they're not built against the latest version of the SDK.  And we should communicate that, at some date, we'll downgrade to preliminary review all add-ons which use an old SDK.

Let's do these things and then see after some time if we still have a significant number of add-ons built off an old SDK being used.
Comment 1 Justin Lebar (not reading bugmail) 2012-05-02 22:37:56 PDT
Jorge, do you agree with this course of action?  Is there anything we (MemShrink) can help with here?
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-03 01:37:50 PDT
(In reply to Justin Lebar [:jlebar] from comment #0)
> Old versions of the Jetpack SDK are known to leak.  Moreover, these leaks
> appear not to be fixed by severing chrome -> content leaks (i.e., bug 695480
> does not fix e.g. bug 751420).

In fact bug 695480 appears to have caused bug 751420.
Comment 3 Andrew Williamson [:eviljeff] 2012-05-03 03:37:39 PDT
We already require a recent release of the SDK to be used for fully reviewed addons.  Recent is normally taken to be the current release, or perhaps the previous one (its not really fair to ask a developer to use a release that wasn't available when they submitted the addon for review)

As for existing approved versions, we have an SDK upgrade function on AMO. (I assume it still works though I've not heard anything about it for a while)
Comment 4 Jorge Villalobos [:jorgev] 2012-05-03 09:22:55 PDT
As Andrew said, we already require recent versions of the SDK for new submissions.

I disagree with downgrading add-ons using old versions of the SDK just because. If they are found to be leaky, we should follow the regular process of getting them fixed.

As for automatically upgrading SDK add-ons, this has been done before. The SDK team handle this, so I'm CC'ing a couple of people who can help us. Can we schedule an SDK repackaging that moves AMO add-ons to version 1.6.1 or later?
Comment 5 Jeff Griffiths (:canuckistani) (:⚡︎) 2012-05-03 09:28:52 PDT
My current understanding is that re-packing SDK-based add-ons that weren't created with Builder was too error-prone to continue with, dcm might have more details on that.
Comment 6 Justin Lebar (not reading bugmail) 2012-05-03 09:46:23 PDT
> I disagree with downgrading add-ons using old versions of the SDK just because. If they 
> are found to be leaky, we should follow the regular process of getting them fixed.

This would make sense if we had a concerted effort to test old add-ons for leaks.  Are you saying that you want to go through and ensure that all the add-ons with old versions of the SDK don't leak?

I think at this point we can say that old versions of Jetpack are incompatible with new versions of Firefox.  I'd be happy to mark all add-ons using the old SDK as incompatible, and I think I have a strong argument for doing so, but I thought asking nicely and downgrading to preliminary would be more friendly.
Comment 7 Jorge Villalobos [:jorgev] 2012-05-03 10:08:16 PDT
(In reply to Justin Lebar [:jlebar] from comment #6)
> This would make sense if we had a concerted effort to test old add-ons for
> leaks.  Are you saying that you want to go through and ensure that all the
> add-ons with old versions of the SDK don't leak?

If I had the time to do so, I'd be happy to.

> I think at this point we can say that old versions of Jetpack are
> incompatible with new versions of Firefox.

Not really, no. They still work, and many work very well.

> I'd be happy to mark all add-ons
> using the old SDK as incompatible, and I think I have a strong argument for
> doing so, but I thought asking nicely and downgrading to preliminary would
> be more friendly.

As usual, we disagree on the methods to get things resolved.
Comment 8 Andrew McCreight [:mccr8] 2012-05-03 10:23:57 PDT
(In reply to Justin Lebar [:jlebar] from comment #6)
> I think at this point we can say that old versions of Jetpack are
> incompatible with new versions of Firefox.  I'd be happy to mark all add-ons
> using the old SDK as incompatible, and I think I have a strong argument for
> doing so, but I thought asking nicely and downgrading to preliminary would
> be more friendly.
The question is, how often do these older Jetpack addons cause leaks?  It could be that only some features cause leaks.
Comment 9 Justin Lebar (not reading bugmail) 2012-05-03 10:33:15 PDT
> The question is, how often do these older Jetpack addons cause leaks?  It could be that 
> only some features cause leaks.

Here's the complete source code for an add-on built off the old SDK which causes leaks (bug 751420):

main.js:

  const pm = require('page-mod');
  pm.PageMod({
    include: '*',
    contentScriptWhen: 'start',
    contentScriptFile: require('self').data.url('wallflower.js')
  });

wallflower.js:

> (function killIframes() {
>   var defaultURL = 'about:blank';
>   if (inIframe() && (
>     window.location.href.indexOf('facebook.com/extern/login_status') != -1 ||
>     window.location.href.indexOf('plusone.google.com/u/0/_/+1/fastbutton') != -1 ||
>     window.location.href.indexOf('plusone.google.com/_/+1/hover') != -1 ||
>     window.location.href.indexOf('facebook.com/plugins/like.php') != -1 ||
>     window.location.href.indexOf('fbcdn.net/connect') != -1)) {
>     window.location.href = defaultURL;
>   }
> })()

function inIframe() {
  return window.location != window.parent.location ? true : false;
}

https://github.com/autonome/Wallflower/blob/master/lib/main.js

From the bug:

> After disabling Wallflower, I'm seeing a 4x memory reduction, the ghost window table 
> is empty, and FF feels significantly snappier.
Comment 10 Justin Lebar (not reading bugmail) 2012-05-03 10:52:08 PDT
I mean to say, it appears that extraordinarily simple add-ons cause massive leaks and slowdowns when coupled with new Firefox and an old SDK.

But I get the impression that this is not up for debate.  Is that right, Jorge?  If so, please close this bug.  I don't want to waste your time trying to convince you if you've made up your mind.
Comment 11 Dave Townsend [:mossop] 2012-05-03 10:55:07 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #4)
> As Andrew said, we already require recent versions of the SDK for new
> submissions.
> 
> I disagree with downgrading add-ons using old versions of the SDK just
> because. If they are found to be leaky, we should follow the regular process
> of getting them fixed.

What do you think to blacklisting them for DTC?
Comment 12 Jorge Villalobos [:jorgev] 2012-05-03 11:08:26 PDT
(In reply to Justin Lebar [:jlebar] from comment #10)
> But I get the impression that this is not up for debate.  Is that right,
> Jorge?

Preemptively downgrading add-ons without a demonstrated problem is not up for debate, that's right.

> If so, please close this bug.

I'd like to wait for dcm's take on comment #5, to see if there's anything we can do automatically. If the automatic repackaging isn't an option, we can always try emailing devs and doing outreach through the Add-ons Blog.

(In reply to Dave Townsend (:Mossop) from comment #11)
> What do you think to blacklisting them for DTC?

The DTC override list has a pretty high barrier of entry. We only add add-ons that have proven usability problems. If we can identify the SDK add-ons that have these memory problems after 15, then we can consider it. However, that depends on how serious the problem is. Creating a zombie compartment is not sufficient IMO. Reliably producing multiple compartments is something I'd consider serious.
Comment 13 Justin Lebar (not reading bugmail) 2012-05-03 15:25:21 PDT
Can you please post the list of jetpack addons using an old SDK so we can test them?
Comment 14 Matt Brubeck (:mbrubeck) 2012-05-03 15:59:28 PDT
Could we also try some immediate communication with all authors of old-SDK add-ons, encouraging them to update and rebuild?  This is a good idea regardless of whether we eventually test/downgrade/DTC-blacklist any add-ons ourselves.  Hopefully many authors will cooperate without such threats.
Comment 15 Justin Lebar (not reading bugmail) 2012-05-03 16:09:39 PDT
In our experience addressing leaks (bug 700547), a considerable fraction of add-on authors are not responsive.  I suspect our experience here will be similar.
Comment 16 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-03 17:34:24 PDT
(In reply to Justin Lebar [:jlebar] from comment #13)
> Can you please post the list of jetpack addons using an old SDK so we can
> test them?

Yes, this would be really useful info.  Can it be obtained?
Comment 17 Dave Townsend [:mossop] 2012-05-03 17:45:17 PDT
One thought, even the latest release version of the SDK has leaks that are mostly fixed in the current development version. Further once we land in Firefox we'll want to make sure to get as many add-ons updated to work with that as possible as that should remove the need to do it ever again. It might be worth waiting till that is complete before reaching out to the devs.
Comment 18 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-03 20:20:20 PDT
(In reply to Dave Townsend (:Mossop) from comment #17)
> One thought, even the latest release version of the SDK has leaks that are
> mostly fixed in the current development version.

Oh?  The current version is 1.6.1 according to https://addons.mozilla.org/en-US/developers/builder.  I thought that fixed all known leaks.  Have new leaks been found?
Comment 19 Dave Townsend [:mossop] 2012-05-03 21:18:19 PDT
(In reply to Nicholas Nethercote [:njn] from comment #18)
> (In reply to Dave Townsend (:Mossop) from comment #17)
> > One thought, even the latest release version of the SDK has leaks that are
> > mostly fixed in the current development version.
> 
> Oh?  The current version is 1.6.1 according to
> https://addons.mozilla.org/en-US/developers/builder.  I thought that fixed
> all known leaks.  Have new leaks been found?

As far as I know there are no more serious ones, but there are still things like bug 745376 and we're still tracking all the issues that are showing up since bug 695480 landed, which may have actually made SDK add-ons leak more, though thankfully only if you disabled the add-on before shutting down Firefox.
Comment 20 Justin Lebar (not reading bugmail) 2012-05-03 22:03:04 PDT
We've put ourselves in an unenviable position.  We know that there exists a pre-1.6.1 version which leaks massively upon content script injection (bug 751420).  There are potentially myriad other problems, but the add-ons team doesn't have the resources to find out.

So if begging authors to upgrade is our chosen method of addressing this problem, the longer we wait before we start, the fewer add-ons will be upgraded by the time we release FF15, and the more people will experience the corresponding massive slow-downs.

On the other hand, if we tell add-on authors to upgrade their SDKs too often, they'll tire of it, and again, without a way to automatically update them or the will to disable add-ons built against old SDKs, again users suffer.

It's a tough trade-off, but I feel like picking the right answer is particularly tough in this case because we don't know if 10, 100, or 1000 add-ons are affected (both by the old SDK and by leaky usage of the SDK post bug 695480).  If we can get that list of add-ons (comment 16), then MemShrink will try to find resources to do the relevant testing.
Comment 21 Justin Dolske [:Dolske] 2012-05-04 00:56:39 PDT
Data would help; I'm afraid this might be a big problem. I recently tracked down a massive ghost-window leak in my own profile to a recently-installed jetpack addon.

(In reply to Jorge Villalobos [:jorgev] from comment #12)
> Preemptively downgrading add-ons without a demonstrated problem is not up
> for debate, that's right.

I don't think it's quite that cut-and-dry! [Conversely: I'd like to understand why you think it is.]

That's not to suggest we should do so lightly or as a first-strike tool. And I would suggest that the _minimal_ requirements would be get data on how widespread and severe the problem is, attempt developer outreach, and see if we can target addons more precisely (eg, scan for usage of known-leaky modules). But if we should find that a majority of pre-1.6.1 SDK addons are causing massive leaks and can't find a better remedy, I think we'd have to consider a blanket downgrade (or even blocklist!) as a possibility to consider.
Comment 22 Andrew Williamson [:eviljeff] 2012-05-04 04:41:09 PDT
(In reply to Justin Lebar [:jlebar] from comment #13)
> Can you please post the list of jetpack addons using an old SDK so we can
> test them?

https://mxr.mozilla.org/addons/search?string=\%22sdkVersion\%22\%3A+\%221\.[012345]&regexp=1&find=harness-options\.json%24&findi=\.xul%24&filter=^[^\0]*%24&hitlimit=&tree=addons

771 matches.  Some may be disabled/have trivial usage numbers/be abandoned, etc.
Comment 23 Andrew Williamson [:eviljeff] 2012-05-04 04:43:46 PDT
Created attachment 621010 [details]
MXR search results

See attached for MXR result for those who don't have access.  I've changed the /<amo-id>/ href to point to AMO so navigating to the url will load the relevant addon's listing.
Comment 24 [Baboo] 2012-05-05 04:08:51 PDT
There is even at least one add-on from Mozilla (Mozilla Labs to be precise).
It might be great idea to set a good example.
Comment 25 Wladimir Palant 2012-05-07 00:41:42 PDT
Created attachment 621525 [details]
Table of add-ons using old SDK

I converted the search results above into a table (added the metadata from AMO). Only 171 add-ons on the lists are currently fully reviewed, 20 more are awaiting full review. Rest of them only has a preliminary review if any.

There is indeed one fully reviewed Mozilla add-on on the list. It's MemChaser :)
Comment 26 Andrew Ducker 2012-05-07 02:12:41 PDT
Speaking as one of the Add-on creators (with a measly 122 users), I have to say I've never touched the add-on since I first uploaded it.  There didn't seem to be much point upgrading versions of the SDK when it worked just fine the way it was, with no complaints.

Possibly what we need is a way of uploading just the source, and having AMO do a recompile/rebuild every time the SDK is updated, to move it automatically to the latest version?
Comment 27 Alexandre Poirot [:ochameau] 2012-05-07 06:50:54 PDT
dcm: I thought we were automatically repacking old addons that are using only high level APIs (addon-kit ones)?

I wrote some details on Wallflower leak in bug 751420 comment 19.
Here is a quick summary: this leak is really bad, it will leak content documents for addons using PageMod API or tab.attach() method with SDK up to 1.3 and running on upcoming FF15 (because it includes bug 679363)

This issue highlight something wrong happening around the SDK. With train release cycle, SDK releases are meant to be used with only a couple of Firefox version. For example, Wallflower was using SDK 1.0b6 and this SDK version was meant to be working only for FF4.0b7 up to FF7.0. These limited version support are here just to prevent facing such platform regression!
So it sounds like we should either automatically repack addons for real -or- revise these compatibility ranges and see how SDK should work in the long run.
Comment 28 David Mason [:dcm :dmason] 2012-05-07 07:34:18 PDT
What we decided was that we would only repack add-ons that were hosted on Builder, mainly because we have the source there. The two problems we ran into were: 1) resources, planning and testing the first time we did it. 2) perhaps more importantly, no access to source code to *safely* repack and determine if we could repack.  We've talked about a number of ways we can do this safely (having a source package and an add-on package, etc) but haven't had the resources to investigate it further. We do still want to repack but we need time and resources, and we want to land the high-level APIs in Fx to see where that gets us.

We talked about this the other day and are going to look into other ways to get the work done - see if we can find something feasible. We'll report back here and elsewhere.
Comment 29 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-07 20:15:40 PDT
> I converted the search results above into a table (added the metadata from
> AMO). Only 171 add-ons on the lists are currently fully reviewed, 20 more
> are awaiting full review. Rest of them only has a preliminary review if any.

Thanks, Wladimir!

That search link doesn't work for me -- I get zero matches.  Hmm.

Anyway, it searches for all versions prior to 1.6, but Alexandre says in bug 751420 comment 19 that this problem was fixed in 1.4.  If we exclude 1.4 and 1.5 from the search how much does that reduce the size of the list?
Comment 30 Wladimir Palant 2012-05-07 22:39:02 PDT
Created attachment 621888 [details]
Same table without SDK 1.4 and 1.5

This is the filtered version of the table above - only SDK versions older than 1.4 are present. This leaves 472 addons in the list, 111 of them fully reviewed.
Comment 31 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-05-08 11:10:29 PDT
FWIW, as far as I can tell the patch in bug 752877 fixes the post-bug 695480 jetpack leaks.
Comment 32 Jorge Villalobos [:jorgev] 2012-05-08 12:53:44 PDT
So if this lands on 15 we won't need to push devs to upgrade their SDK? (Though it'd still be good to do regularly)
Comment 33 Alexandre Poirot [:ochameau] 2012-05-08 13:30:50 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #32)
> So if this lands on 15 we won't need to push devs to upgrade their SDK?
> (Though it'd still be good to do regularly)

It seems to fix leak introduced by bug 679363, but doesn't solve leaks that happened on SDK before 1.6.1 version (various leaks around context-menu API).
As Jetpack developer it is kind of scary to know that addons are still using SDK version before final 1.0 version (1.0bx)!
Comment 34 Matt Brubeck (:mbrubeck) 2012-05-08 13:46:42 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #33)
> As Jetpack developer it is kind of scary to know that addons are still using
> SDK version before final 1.0 version (1.0bx)!

Three of those add-ons are published by Mozilla Labs.  I filed bug 753091 to update them.

The "sort table" bookmarklet from https://www.squarefree.com/bookmarklets/pagedata.html is useful for browsing Wladimir's tables of add-ons.
Comment 35 Justin Lebar (not reading bugmail) 2012-05-08 14:09:42 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #32)
> So if this lands on 15 we won't need to push devs to upgrade their SDK?
> (Though it'd still be good to do regularly)

Do you have a proposal for pushing devs to upgrade their SDKs?  That was what I was hoping to get out of this bug; I'd certainly be curious to hear how you think we could/should do it.
Comment 36 Jorge Villalobos [:jorgev] 2012-05-08 15:25:10 PDT
There was a Jetpack team meeting today where some of these options were discussed. The possibility of an automatic repack wasn't totally discarded, for what I understood, possibly limited to the simplest add-ons, so that remains the ideal option in my mind.

The second best approach is direct emailing, which is something we can certainly do if the the repacking is impossible or insufficient.

Downgrading currently public add-ons would be step 3, giving at least a few weeks for developers to respond. By this point we can at least say that we tried contacting the developers and they weren't responsive.
Comment 37 Justin Lebar (not reading bugmail) 2012-05-08 15:29:58 PDT
Can I ask what made you change your stance from comment 12?  Or did I misunderstand something, and this isn't a change of position?
Comment 38 Jorge Villalobos [:jorgev] 2012-05-08 16:28:46 PDT
Actually I might have been to broad on my last comment. I would be OK downgrading add-ons that have really old versions of the SDK (like pre-1.0) or versions that have proven problems.

From comment #31 it appears that we don't have to worry about the post-bug 695480 leaks if it lands.

The other only concern that I know about are memory leaks on pre-1.6.1 versions of the SDK, and that comes down to detecting the add-ons using the leaky APIs to limit the scope of the problem. I think that can be done, either using the Add-ons MXR or maybe using the bulk add-on validator or the SDK repackager. These are the cases where I think it's OK to notify and then downgrade if there's no response.

If there are other concerns with old versions of the SDK, other than the fact that they are old, it'd be good to know about them.
Comment 39 Jeff Griffiths (:canuckistani) (:⚡︎) 2012-05-09 12:03:02 PDT
As part of the outreach on this effort ( I think this can also be useful as a more general 're-pack your add-ons!' campaign ) it would be great to be able to re-generate the stats attached and expose the data set and some shiny graphs or something. It would be great to track this on-going as part of 'areweaddeduponyet', although that site has yet to be updated for 2012 KPIs.
Comment 40 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-09 17:44:24 PDT
> From comment #31 it appears that we don't have to worry about the post-bug
> 695480 leaks if it lands.

That's the idea... it'll need some testing.

I heard somewhere that there's a plan to eventually put the SDK in Firefox itself, and then add-ons would automatically use that and wouldn't need to be packed with an SDK version.  Is that correct?  It would obviate this whole problem.
Comment 41 Dave Townsend [:mossop] 2012-05-09 17:47:38 PDT
(In reply to Nicholas Nethercote [:njn] from comment #40)
> > From comment #31 it appears that we don't have to worry about the post-bug
> > 695480 leaks if it lands.
> 
> That's the idea... it'll need some testing.
> 
> I heard somewhere that there's a plan to eventually put the SDK in Firefox
> itself, and then add-ons would automatically use that and wouldn't need to
> be packed with an SDK version.  Is that correct?  It would obviate this
> whole problem.

There is a plan, add-ons will have to repack using a version of the SDK after that to start using the code that ships in Firefox. It should significantly (if not completely) reduce the need to repack after that though.
Comment 42 Justin Lebar (not reading bugmail) 2012-08-21 16:37:31 PDT

*** This bug has been marked as a duplicate of bug 783046 ***

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