Last Comment Bug 716163 - "Delicious Bookmarks" add-on leaks 100s of places.sqlite connections
: "Delicious Bookmarks" add-on leaks 100s of places.sqlite connections
Status: RESOLVED WORKSFORME
[MemShrink:P2]
:
Product: Tech Evangelism
Classification: Other
Component: Add-ons (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Jorge Villalobos [:jorgev]
:
Mentors:
Depends on: 722254
Blocks: DarkMatter LeakyAddons
  Show dependency treegraph
 
Reported: 2012-01-06 17:58 PST by Nicholas Nethercote [:njn]
Modified: 2012-04-26 14:29 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments

Description Nicholas Nethercote [:njn] 2012-01-06 17:58:39 PST
I got the following report from a comment on my blog: http://blog.mozilla.com/nnethercote/2012/01/04/memshrink-progress-weeks-28-29/#comment-4754.  I haven't yet tried to reproduce it myself.

The add-on's AMO page is here: https://addons.mozilla.org/en-US/firefox/addon/delicious-bookmarks/.  The developers are listed as "Yahoo! Inc." and "AVOS".  I will email support@avos.com to let them know.

It's the 90th most popular add-on on AMO, with ~156,000 users.

----

I've been having really bad memory usage. When Firefox is open, it will gradually use more and more memory, until it reaches 3 GB and crashes. (Sometimes in just a couple days.) I've had it crash many times. Here's the about:memory I copied when I was running Firefox 8:

2,743.75 MB (100.0%) -- explicit
+--1,254.47 MB (45.72%) -- heap-unclassified
+--1,236.69 MB (45.07%) -- storage
¦  +--1,236.69 MB (45.07%) -- sqlite
¦     +--1,204.00 MB (43.88%) -- places.sqlite
¦     ¦  +--1,190.83 MB (43.40%) -- cache-used [222]
¦     ¦  +-----13.18 MB (00.48%) -- (2 omitted)
¦     +-----16.91 MB (00.62%) -- other
¦     +-----15.77 MB (00.57%) -- (14 omitted)
+----190.17 MB (06.93%) -- js
¦    +---80.66 MB (02.94%) -- (65 omitted)
¦    +---69.66 MB (02.54%) -- compartment([System Principal], 0x5759000)
¦    ¦   +--33.23 MB (01.21%) -- gc-heap
¦    ¦   ¦  +--16.70 MB (00.61%) -- objects
¦    ¦   ¦  +--16.53 MB (00.60%) -- (6 omitted)
¦    ¦   +--19.84 MB (00.72%) -- (7 omitted)
¦    ¦   +--16.58 MB (00.60%) -- mjit-code
¦    +---20.63 MB (00.75%) -- gc-heap-chunk-dirty-unused
¦    +---19.23 MB (00.70%) -- compartment(http://camelegg.com/product/N82E16822148...)
¦        +--19.23 MB (00.70%) -- (8 omitted)
+-----37.36 MB (01.36%) -- layout
¦     +--24.03 MB (00.88%) -- styledata
¦     +--13.33 MB (00.49%) -- (1 omitted)
+-----25.06 MB (00.91%) -- (6 omitted)

I noticed that the places.sqlite cache is always the largest problem. While reading a bug report, I found out that the number after "cache-used" is the number of open DB connections. And that each connection can have a 5 MB cache. I tried running Firefox with different add-ons disabled and found out it is Delicious Bookmarks that is leaking the connections. It leaks connections whenever I type into the awesome bar. I can type a few letters and it leaks several connections. (I have its awesome bar integration turned on; turning it off might help, but I haven't tried.)
Comment 1 Nicholas Nethercote [:njn] 2012-01-06 18:19:35 PST
BTW, I suspect "heap-unclassified" is so high because the user is using Firefox 9 or earlier, which lacks the patch fixing bug 678977, and so the sqlite usage is being under-reported, and the under-reported bytes are falling into "heap-unclassified".

If/when somebody reproduces this bug with Firefox 10 or later, hopefully they'll see that heap-unclassified isn't so high.  If so, there's not need for this bug to block bug 563700.
Comment 2 Nicholas Nethercote [:njn] 2012-01-10 15:05:22 PST
Jorge, would any of the AMO tester people be able to reproduce this?  I tried to reproduce and was unable to but I don't have a Delicious account and that might be necessary.
Comment 3 urykhy 2012-01-11 00:48:26 PST
I can confirm bug, in FF8 with AwesomeBar integration enabled.
Comment 4 Nicholas Nethercote [:njn] 2012-01-11 00:58:33 PST
urykhy: do you have a Delicious account?
Comment 5 urykhy 2012-01-11 01:33:04 PST
(In reply to Nicholas Nethercote [:njn] from comment #4)
> urykhy: do you have a Delicious account?

sure, and ~ 1.5K bookmarks.
Comment 6 Nicholas Nethercote [:njn] 2012-01-11 02:29:53 PST
urykhy, thanks for the confirmation!
Comment 7 Nicholas Nethercote [:njn] 2012-01-17 17:18:24 PST
I've received no response to my email to the authors, which I sent 11 days ago.

Jorge, fligtar:  this would be a great candidate for the proposed "this add-on leaks memory" flag on AMO.
Comment 8 ashwinjmathew 2012-01-22 15:24:09 PST
Just to confirm - I had memory usage jumping to about 1.3GB on Firefox 9.0.1/Ubuntu 11.10 with Delicious bookmarks enabled. I have a few thousand bookmarks saved in Delicious, and the places.salite memory usage was rising to about 40% of overall usage, as reported in this bug. Once the Delicious extension is disabled, memory usage remains under control.
Comment 9 Marco Bonardo [::mak] 2012-01-25 03:28:30 PST
ugh, I wonder if they are opening connections in a loop and never closing them, sounds quite crazy, we need just 3-4 for all the product, I can't see anything that may need more than a couple connections.
Comment 10 Justin Lebar (not reading bugmail) 2012-01-25 06:53:47 PST
Maybe they're opening a connection for each new document.  That's at least conceivable...
Comment 11 Kris Maglione [:kmag] 2012-01-29 07:26:48 PST
Unless I'm mistaken, this results from the following line in their autocomplete provider:

		this.historyAutoComplete		= Components.classes["@mozilla.org/autocomplete/search;1?name=history"].createInstance(Components.interfaces.nsIAutoCompleteSearch);

given that autocomplete providers are meant to be used as services rather than individual instances. I'm a bit surprised that the cloned DB connections it creates are never GCed, though.

I've sent a message via the AMO review system that their next update won't be approved until this is addressed.

P.S. Why is this bug in the Core component? Do we have a canonical component for these kinds of add-on bugs?
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-29 07:32:15 PST
If they don't do thinks like call stopSearch on the nsIAutoCompleteSearch they could probably get leaks.
Comment 13 Marco Bonardo [::mak] 2012-01-29 10:26:01 PST
it's possible the factory is not managing the search as a service with createInstance, that would be fixable on our side.
Comment 14 Marco Bonardo [::mak] 2012-01-29 10:27:46 PST
indeed I think it doesn't.
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-29 10:31:41 PST
Why should it?

IMO the underlying bug here is that XPCOM lets you createInstance services.
Comment 16 Marco Bonardo [::mak] 2012-01-29 10:37:46 PST
provided they are declared as services, that i can't find for autocomplete, afaict
Comment 17 Kris Maglione [:kmag] 2012-01-29 10:55:10 PST
I'm not sure what you mean by declared as services. Classes implementing these contracts are meant to be accessed as services. At least, they are accessed as services everywhere they're used in core code, and everywhere mentioned on MDC. The example for JS implementations given on MDC[1] likewise assures that multiple instances aren't created, which the builtin history provider does not.

[1] https://developer.mozilla.org/en/How_to_implement_custom_autocomplete_search_component#section_4
Comment 18 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-29 10:57:51 PST
Binary components have a flag determining whether or not they are a service (though createInstance and getService don't actually check that flag)

JS components may or may not have a similar thing.
Comment 19 Kris Maglione [:kmag] 2012-01-29 13:08:50 PST
JS components have always been more ad-hoc than binary components. Until XPCOMUtils, there wasn't any standardization in the implementation of JS components, and it doesn't have any means of flagging services (other than implementing your own factory that only returns a singleton). If you mean the SINGLETON flag of nsIClassInfo, JS components can do that, but as far as I know, most of the services that are only meant to be accessed as services don't even implement nsIClassInfo.
Comment 20 Marco Bonardo [::mak] 2012-02-07 01:24:38 PST
Did we reach out the extension developer, or should we rather try to port bug 722254 to Aurora?
Comment 21 Nicholas Nethercote [:njn] 2012-02-07 15:28:59 PST
(In reply to Marco Bonardo [:mak] from comment #20)
> Did we reach out the extension developer

Yes, see comment 0 and comment 7.  No response.
Comment 22 Marco Bonardo [::mak] 2012-02-08 00:21:48 PST
(In reply to Nicholas Nethercote [:njn] from comment #21)
> (In reply to Marco Bonardo [:mak] from comment #20)
> > Did we reach out the extension developer
> 
> Yes, see comment 0 and comment 7.  No response.

So, do you think it's worth to try to fix the issue on Aurora? While the fix should be pretty safe, it's also true it changes all factories of all Places components. Though all of those are used in tests.
Comment 23 Nicholas Nethercote [:njn] 2012-02-08 00:53:31 PST
> So, do you think it's worth to try to fix the issue on Aurora? While the fix
> should be pretty safe, it's also true it changes all factories of all Places
> components. Though all of those are used in tests.

It's hard to judge that when we don't yet even have a patch that will fix it on Nightly :)
Comment 24 Marco Bonardo [::mak] 2012-02-08 00:57:49 PST
(In reply to Nicholas Nethercote [:njn] from comment #23)
> It's hard to judge that when we don't yet even have a patch that will fix it
> on Nightly :)

bug 722254 will be in today's Nightly and will fix it!
Comment 25 Nicholas Nethercote [:njn] 2012-02-08 01:05:06 PST
> 
> bug 722254 will be in today's Nightly and will fix it!

Ah!  urykhy / ashwinjmathew:  if either of you use Nightly and could confirm that it's fixed, that would be great.

As for Aurora, this add-on isn't popular enough for it to be worthwhile, IMO.
Comment 26 Marco Bonardo [::mak] 2012-02-08 01:12:14 PST
(In reply to Nicholas Nethercote [:njn] from comment #25)
> > 
> > bug 722254 will be in today's Nightly and will fix it!
> 
> Ah!  urykhy / ashwinjmathew:  if either of you use Nightly and could confirm
> that it's fixed, that would be great.
> 
> As for Aurora, this add-on isn't popular enough for it to be worthwhile, IMO.

ok..just to clarify the Nightly to test is the 2012-02-08 one
Comment 27 urykhy 2012-02-08 06:57:17 PST
2012-02-08-mozilla-release-debug version, with latest delicious plugin + nosy

after some navigations, i close all tabs, press GC/CC/MMU some times and got results:
from about:memory

105.12 MB (100.0%) -- explicit
├───67.69 MB (64.39%) -- storage
│   └──67.69 MB (64.39%) -- sqlite
│      ├──57.46 MB (54.66%) -- places.sqlite
│      │  ├──48.62 MB (46.25%) -- cache-used [109]
│      │  ├───7.59 MB (07.22%) -- stmt-used [109]
│      │  └───1.25 MB (01.19%) -- schema-used [109]
│      ├───6.60 MB (06.28%) -- other
│      ├───2.14 MB (02.03%) -- (11 omitted)
│      └───1.49 MB (01.42%) -- ybookmarks.sqlite
│          ├──1.10 MB (01.05%) -- cache-used
│          └──0.39 MB (00.37%) -- (2 omitted)

tail:
  206.79 MB -- resident
1,340.93 MB -- vsize


now i made some navigations:

114.52 MB (100.0%) -- explicit
├───76.04 MB (66.39%) -- storage
│   └──76.04 MB (66.39%) -- sqlite
│      ├──64.84 MB (56.62%) -- places.sqlite
│      │  ├──54.81 MB (47.86%) -- cache-used [123]
│      │  ├───8.61 MB (07.52%) -- stmt-used [123]
│      │  └───1.41 MB (01.24%) -- schema-used [123]
│      ├───7.40 MB (06.46%) -- other
│      ├───2.28 MB (01.99%) -- (11 omitted)
│      └───1.52 MB (01.33%) -- ybookmarks.sqlite
│          ├──1.13 MB (00.99%) -- cache-used
│          └──0.39 MB (00.34%) -- (2 omitted)
  229.21 MB -- resident
1,441.60 MB -- vsize

vsize is really growing.

more verbose display:
125,325,910 B (100.0%) -- explicit
├───79,730,216 B (63.62%) -- storage
│   └──79,730,216 B (63.62%) -- sqlite
│      ├──67,989,000 B (54.25%) -- places.sqlite
│      │  ├──57,473,840 B (45.86%) -- cache-used [123]
│      │  ├───9,032,032 B (07.21%) -- stmt-used [123]
│      │  └───1,483,128 B (01.18%) -- schema-used [123]
│      ├───7,755,640 B (06.19%) -- other
│      ├───1,596,392 B (01.27%) -- ybookmarks.sqlite
│      │   ├──1,185,000 B (00.95%) -- cache-used
│      │   ├────403,280 B (00.32%) -- stmt-used
│      │   └──────8,112 B (00.01%) -- schema-used
│      ├─────392,168 B (00.31%) -- extensions.sqlite
│      │     ├──296,592 B (00.24%) -- cache-used
│      │     ├───88,600 B (00.07%) -- stmt-used
│      │     └────6,976 B (00.01%) -- schema-used
│      ├─────347,760 B (00.28%) -- chromeappsstore.sqlite
│      │     ├──263,984 B (00.21%) -- cache-used
│      │     ├───79,848 B (00.06%) -- stmt-used
│      │     └────3,928 B (00.00%) -- schema-used
│      ├─────295,104 B (00.24%) -- addons.sqlite
│      │     ├──263,680 B (00.21%) -- cache-used
│      │     ├───25,696 B (00.02%) -- stmt-used
│      │     └────5,728 B (00.00%) -- schema-used
│      ├─────291,368 B (00.23%) -- content-prefs.sqlite
│      │     ├──263,696 B (00.21%) -- cache-used
│      │     ├───25,248 B (00.02%) -- stmt-used
│      │     └────2,424 B (00.00%) -- schema-used
│      ├─────281,944 B (00.22%) -- webappsstore.sqlite
│      │     ├──198,168 B (00.16%) -- cache-used
│      │     ├───79,848 B (00.06%) -- stmt-used
│      │     └────3,928 B (00.00%) -- schema-used
│      ├─────179,880 B (00.14%) -- cookies.sqlite
│      │     ├──164,968 B (00.13%) -- cache-used
│      │     ├───13,072 B (00.01%) -- stmt-used
│      │     └────1,840 B (00.00%) -- schema-used
│      ├─────179,000 B (00.14%) -- urlclassifier3.sqlite
│      │     ├──110,056 B (00.09%) -- stmt-used
│      │     ├───66,280 B (00.05%) -- cache-used
│      │     └────2,664 B (00.00%) -- schema-used
│      ├─────108,096 B (00.09%) -- downloads.sqlite
│      │     ├───99,168 B (00.08%) -- cache-used
│      │     ├────7,144 B (00.01%) -- stmt-used
│      │     └────1,784 B (00.00%) -- schema-used
│      ├─────107,376 B (00.09%) -- permissions.sqlite
│      │     ├───99,176 B (00.08%) -- cache-used
│      │     ├────6,936 B (00.01%) -- stmt-used
│      │     └────1,264 B (00.00%) -- schema-used
│      ├─────103,960 B (00.08%) -- signons.sqlite
│      │     ├───99,160 B (00.08%) -- cache-used
│      │     ├────2,824 B (00.00%) -- schema-used
│      │     └────1,976 B (00.00%) -- stmt-used
│      └─────102,528 B (00.08%) -- search.sqlite
│            ├───99,160 B (00.08%) -- cache-used
│            ├────2,160 B (00.00%) -- stmt-used
│            └────1,208 B (00.00%) -- schema-used
Comment 28 Marco Bonardo [::mak] 2012-02-08 07:19:17 PST
hm, I can't really see how's that possible if they are just instancing our services and not creating connections by themselves.

which steps did you follow? I tried to install it locally and navigate and I don't see any additional connections.
Comment 29 Marco Bonardo [::mak] 2012-02-08 07:20:20 PST
wait, you tested 2012-02-08-mozilla-release, while the fix is in Nightly (FF13). is your version 10?
Comment 30 urykhy 2012-02-08 07:25:21 PST
you can configure it:

tools->delicious options->awesome bar
* enable awesome bar integration
* show delicious bookmarks in awesomebar search result

other options if off.

ps: i download it from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/02/2012-02-08-mozilla-release-debug/ 
if this is wrong location - can you point me to the correct one ?
Comment 31 Marco Bonardo [::mak] 2012-02-08 07:29:25 PST
yeah, you tested a build of the release channel.
The right build is either http://nightly.mozilla.org/ or http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
Thanks.
Comment 32 urykhy 2012-02-08 08:04:36 PST
it works like a charm )
thanx.

after some navigations:
├──22.36 MB (26.27%) ── heap-unclassified
├───9.43 MB (11.08%) -- storage
│   ├──8.83 MB (10.38%) -- sqlite
│   │  ├──2.74 MB (03.22%) ++ (11 tiny)
│   │  ├──2.30 MB (02.70%) -- ybookmarks.sqlite
│   │  │  ├──1.92 MB (02.25%) ── cache-used
│   │  │  └──0.39 MB (00.45%) ++ (2 tiny)
│   │  ├──1.98 MB (02.32%) -- places.sqlite
│   │  │  ├──1.67 MB (01.96%) ── cache-used [3]
│   │  │  └──0.31 MB (00.37%) ++ (2 tiny)
│   │  └──1.81 MB (02.13%) ── other
│   └──0.60 MB (00.71%) ++ prefixset

143.80 MB ── resident
512.71 MB ── vsize
Comment 33 Marco Bonardo [::mak] 2012-02-08 08:17:44 PST
Thank you for the confirmation urykhy!

So, what we can do:
- port the fix to Aurora. Nicholas pointed out this add-on is not so widely used, so may not be worth it, it's easy to do though.
- mark the add-on as "leaking" on AMO. Jorge?
- neither of those and just resolve this bug
Comment 34 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-08 08:23:43 PST
(In reply to Marco Bonardo [:mak] from comment #33)
> Thank you for the confirmation urykhy!
> 
> So, what we can do:
> - port the fix to Aurora. Nicholas pointed out this add-on is not so widely
> used, so may not be worth it, it's easy to do though.
> - mark the add-on as "leaking" on AMO. Jorge?
> - neither of those and just resolve this bug

The fix is simple enough and the failure mode bad enough that I think we should backport to Aurora.
Comment 35 Marco Bonardo [::mak] 2012-02-08 08:31:50 PST
Fine, I requested approval.
Comment 36 Marco Bonardo [::mak] 2012-02-08 08:33:16 PST
I'm assigning the bug to Jorge since from development point of view here there is nothing more we can do, AMO may want to add checks and mark add-ons. Otherwise just close this.
Comment 37 Kris Maglione [:kmag] 2012-02-08 08:36:21 PST
The developer has already been informed via AMO that this needs to be fixed, along with a litany of other issues. His next update won't be approved without it. That said, as the add-on seems to be updated only once a year and the developer has been generally unresponsive, there might be an argument for demoting the add-on to preliminary status.
Comment 38 Justin Lebar (not reading bugmail) 2012-02-08 08:40:23 PST
We release a new version of Firefox every six weeks; I think six weeks should be an upper bound on the amount of time we're willing to wait for an add-on developer to take action before we escalate.
Comment 39 Jorge Villalobos [:jorgev] 2012-02-08 16:44:33 PST
I sent a message to some contacts I have on AVOS. I'll give them some time to reply and then decide what to do here.
Comment 40 Nicholas Nethercote [:njn] 2012-02-14 20:28:29 PST
(In reply to Kris Maglione [:kmag] from comment #37)
> The developer has already been informed via AMO that this needs to be fixed,
> along with a litany of other issues. His next update won't be approved
> without it. That said, as the add-on seems to be updated only once a year
> and the developer has been generally unresponsive, there might be an
> argument for demoting the add-on to preliminary status.

That's not relevant for this bug, though.  Can we mark this bug as fixed?
Comment 41 Kris Maglione [:kmag] 2012-02-14 21:24:57 PST
This is still an issue with the current public version of the add-on on the current public Firefox. Since it seems that this isn't going to land in a release version until 12, I'd rather it stay open until we've determined that we can't get it fixed sooner in the add-on.
Comment 42 Zlip792 2012-02-20 22:02:13 PST
I think this is worth sharing:
http://blog.delicious.com/2012/01/update-on-the-browser-extensions/

Important Part:
We’re putting the extensions “on ice”.  We will remove some broken options to avoid having non-functioning features deployed. This point is most relevant to the Firefox extension which always had more features than the others. Beyond this we have no current plans to continue developing them.

So some decision has to be made..
Comment 43 Kris Maglione [:kmag] 2012-02-20 22:15:58 PST
If that's the case, I think the extension should be downgraded to preliminary review status.
Comment 44 Jorge Villalobos [:jorgev] 2012-02-22 11:46:39 PST
(In reply to Kris Maglione [:kmag] from comment #43)
> If that's the case, I think the extension should be downgraded to
> preliminary review status.

Done. Since I don't expect much to happen in the future (didn't get any useful response from AVOS or Yahoo), I think we should wait for Firefox 12 to be rolled out before calling this WFM.
Comment 45 Jorge Villalobos [:jorgev] 2012-04-26 14:29:52 PDT
WFM, per comment #44.

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