Closed Bug 751466 Opened 12 years ago Closed 12 years ago

Get add-ons to upgrade from old versions of the SDK

Categories

(WebExtensions :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 783046

People

(Reporter: justin.lebar+bug, Unassigned)

References

Details

Attachments

(3 files)

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.
Jorge, do you agree with this course of action?  Is there anything we (MemShrink) can help with here?
(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.
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)
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?
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.
> 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.
(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.
(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.
> 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.
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.
(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?
(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.
Can you please post the list of jetpack addons using an old SDK so we can test them?
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.
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.
(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?
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.
(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?
(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.
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.
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.
(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.
Attached file 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.
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.
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 :)
Attachment #621525 - Attachment mime type: text/plain → text/html
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?
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.
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.
Blocks: 752497
Blocks: 752500
Blocks: 752468
> 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?
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.
FWIW, as far as I can tell the patch in bug 752877 fixes the post-bug 695480 jetpack leaks.
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)
(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)!
Depends on: 753091
(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.
(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.
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.
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?
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.
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.
> 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.
(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.
Whiteboard: [MemShrink]
Blocks: 783046
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Component: Add-ons → General
Product: Tech Evangelism → WebExtensions
You need to log in before you can comment on or make changes to this bug.