Closed Bug 716163 Opened 12 years ago Closed 12 years ago

"Delicious Bookmarks" add-on leaks 100s of places.sqlite connections

Categories

(WebExtensions :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: n.nethercote, Assigned: jorgev)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

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.)
Blocks: DarkMatter
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.
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.
I can confirm bug, in FF8 with AwesomeBar integration enabled.
urykhy: do you have a Delicious account?
(In reply to Nicholas Nethercote [:njn] from comment #4)
> urykhy: do you have a Delicious account?

sure, and ~ 1.5K bookmarks.
urykhy, thanks for the confirmation!
Whiteboard: [MemShrink] → [MemShrink:P2]
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.
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.
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.
Maybe they're opening a connection for each new document.  That's at least conceivable...
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?
If they don't do thinks like call stopSearch on the nsIAutoCompleteSearch they could probably get leaks.
it's possible the factory is not managing the search as a service with createInstance, that would be fixable on our side.
indeed I think it doesn't.
Why should it?

IMO the underlying bug here is that XPCOM lets you createInstance services.
provided they are declared as services, that i can't find for autocomplete, afaict
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
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.
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.
Depends on: 722254
Did we reach out the extension developer, or should we rather try to port bug 722254 to Aurora?
(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.
(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.
> 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 :)
(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!
> 
> 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.
(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
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
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.
wait, you tested 2012-02-08-mozilla-release, while the fix is in Nightly (FF13). is your version 10?
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 ?
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.
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
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
(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.
Fine, I requested approval.
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.
Assignee: nobody → jorge
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.
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.
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.
(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?
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.
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..
If that's the case, I think the extension should be downgraded to preliminary review status.
(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.
Component: General → Add-ons
Product: Core → Tech Evangelism
WFM, per comment #44.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
Component: Add-ons → General
Product: Tech Evangelism → WebExtensions
You need to log in before you can comment on or make changes to this bug.