1343863 - precompile_cache.js leaks the places shutdown blockers

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[(no longer active)]

See this try job, for example:

https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-2d216591dbf733fdb1acf11aa3065bdaa46dbfc0/try-linux64-st-an-debug/try-linux64-st-an-debug-bm76-try1-build6677.txt.gz

STR:

$ export XPCOM_MEM_LEAK_LOG=1
$ ./objdir/dist/bin/xpcshell -g objdir/dist/bin/ -a objdir/dist/bin/ -f toolkit/mozapps/installer/precompile_cache.js -e 'precompile_startupcache("resource://gre/");'

The issue is that there is no profile directory, and InitDatabaseFile() fails as a result, which makes us bail out early from here: <http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/toolkit/components/places/Database.cpp#458>

But this is before we have set up our shutdown blockers: <http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/toolkit/components/places/Database.cpp#500>.  So there is nothing that the script can do in order to break the cycle between the Database object and their shutdown blockers, so we leak them all.

== BloatView: ALL (cumulative) LEAK STATISTICS, default process 33502

     |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
     |                                      | Per-Inst   Leaked|   Total      Rem|
   0 |TOTAL                                 |       53     2392|  211437       42|
   6 |BackstagePass                         |      104      312|     722        3|
  17 |ClientsShutdownBlocker                |       80       80|       1        1|
  19 |ConnectionShutdownBlocker             |       88       88|       1        1|
  25 |Database                              |      248      248|       1        1|
  65 |PlacesShutdownBlocker                 |       72      144|       2        2|
 114 |XPCNativeInterface                    |       56      112|    1784        2|
 115 |XPCNativeMember                       |       16       32|   39659        2|
 116 |XPCNativeSet                          |       32       64|    1106        2|
 117 |XPCWrappedNative                      |       96      480|    7603        5|
 118 |XPCWrappedNativeProto                 |       40      160|    2877        4|
 120 |XPCWrappedNativeTearOff               |       32      160|   10645        5|
 173 |nsJSID                                |       56      112|     310        2|
 175 |nsJSPrincipals                        |       24       24|       4        1|
 183 |nsMainThreadPtrHolder<T>              |       24       24|       2        1|
 220 |nsStringBuffer                        |        8        8|   49410        1|
 226 |nsTArray_base                         |        8       32|   45466        4|
 253 |nsXPCWrappedJS                        |      120      120|     101        1|
 254 |nsXPCWrappedJSClass                   |       72       72|      10        1|
 259 |xptiInterfaceInfo                     |       40      120|     270        3|

nsTraceRefcnt::DumpStatistics: 260 entries

The leak goes away if we Shutdown() before bailing out, due to Shutdown() breaking that cycle for us.

Comment 1

[(no longer active)]

Attachment: Make sure that places Database object doesn't get leaked if the profile folder doesn't exist or is locked

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8842858 [details] [diff] [review]
Make sure that places Database object doesn't get leaked if the profile folder doesn't exist or is locked

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

::: toolkit/components/places/Database.cpp
@@ +459,5 @@
> +
> +    // If we bail out early here without doing anything else, the Database object
> +    // will leak due to the cycle between the shutdown blockers and itself, so
> +    // let's break the cycle first.
> +    Shutdown();

This may be fine, but looks like any failure in Init() will in general cause this.
I wonder if we should rather assign to mClientsShutdown and mConnectionShutdown just at the end of the Init() function, before the final return NS_OK;

Comment 3

[Marco Bonardo [:mak]]

Or at least where we add the blockers, later in Init()

Comment 4

[(no longer active)]

The current code doesn't handle mConnectionShutdown being null, and Database::GetClientsShutdow() expects mClientsShutdown to be non-null.  But you're right about the other early bail outs from this function.

New patch coming up!

Comment 5

[(no longer active)]

Attachment: Make sure that places Database object doesn't get leaked if the profile folder doesn't exist or is locked

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8843797 [details] [diff] [review]
Make sure that places Database object doesn't get leaked if the profile folder doesn't exist or is locked

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

LGTM

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d1b4db2ae35
Make sure that places Database object doesn't get leaked if the profile folder doesn't exist or is locked; r=mak

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d1b4db2ae35