1334558 - Don't use M-C NS_RegisterStaticAtoms() since atoms table is sealed and attempts to add to it result in a MOZ_CRASH() after bug 1276669

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Jorg K (CEST = GMT+2)]

Debug builds don't even start:

Assertion failure: !gStaticAtomTableSealed (Atom table has already been sealed!), at c:/mozilla-source/comm-central/mozilla/xpcom/ds/nsAtomTable.cpp:591
#01: nsMsgDBFolder::nsMsgDBFolder (c:\mozilla-source\comm-central\mailnews\base\util\nsmsgdbfolder.cpp:154)
#02: nsImapMailFolder::nsImapMailFolder (c:\mozilla-source\comm-central\mailnews\imap\src\nsimapmailfolder.cpp:179)
#03: nsImapMailFolderConstructor (c:\mozilla-source\comm-central\mailnews\build\nsmailmodule.cpp:625)
#04: RDFServiceImpl::GetResource (c:\mozilla-source\comm-central\mozilla\rdf\base\nsrdfservice.cpp:904)
#05: nsMsgIncomingServer::CreateRootFolder (c:\mozilla-source\comm-central\mailnews\base\util\nsmsgincomingserver.cpp:366)
#06: nsMsgIncomingServer::GetRootFolder (c:\mozilla-source\comm-central\mailnews\base\util\nsmsgincomingserver.cpp:134)
#07: nsMsgAccountManager::createKeyedServer (c:\mozilla-source\comm-central\mailnews\base\src\nsmsgaccountmanager.cpp:610)
#08: nsMsgAccountManager::GetIncomingServer (c:\mozilla-source\comm-central\mailnews\base\src\nsmsgaccountmanager.cpp:495)
#09: nsMsgAccount::createIncomingServer (c:\mozilla-source\comm-central\mailnews\base\src\nsmsgaccount.cpp:104)
#10: nsMsgAccountManager::LoadAccounts (c:\mozilla-source\comm-central\mailnews\base\src\nsmsgaccountmanager.cpp:1225)
#11: nsMsgAccountManager::GetAllIdentities (c:\mozilla-source\comm-central\mailnews\base\src\nsmsgaccountmanager.cpp:937)
#12: XPTC__InvokebyIndex (c:\mozilla-source\comm-central\mozilla\xpcom\reflect\xptcall\md\win32\xptcinvoke_asm_x86_64.asm:99)

Comment 1

[Jorg K (CEST = GMT+2)]

OK, we register atoms here:
https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#148
https://dxr.mozilla.org/comm-central/rev/c0e5b93e1369ec16a2c0747df816fbcae5084fff/mailnews/base/util/nsMsgDBFolder.cpp#148

But the atoms table is now "sealed" here:
https://dxr.mozilla.org/comm-central/rev/a338e596b1d9f37186aaeddcfaa572ae043e578d/mozilla/layout/build/nsLayoutStatics.cpp#270
or here:
https://hg.mozilla.org/mozilla-central/rev/c7456484433f#l1.12

And later additions crash here:
https://hg.mozilla.org/mozilla-central/rev/5a6fc9e02f9d#l1.14

Comment 2

[Jorg K (CEST = GMT+2)]

OK, I discussed that at length with Kent on IRC. We decided to rip out atoms altogether and use strings.
If you want to see the size of the problem:
https://dxr.mozilla.org/comm-central/search?q=nsIAtom&redirect=false

Starting with nsIFolderListener.idl and nsIMsgFolder.idl.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (v1)

This gets rid of the use of nsIAtom from those interfaces and related code.

TB starts and seems to work, however, there is a long long long litany of complains when the profile manager closes (I use this to select my testing profiles) and when TB closes.

I don't think we register all those atoms, or do we. Here's a little excerpt:

ds/nsAtomTable.cpp, line 397
[6252] ###!!! ASSERTION: dynamic atom with non-zero refcount lc-333333!: 'false'
, file c:/mozilla-source/comm-central/mozilla/xpcom/ds/nsAtomTable.cpp, line 397

[6252] ###!!! ASSERTION: dynamic atom with non-zero refcount preferencesTab0!: '
false', file c:/mozilla-source/comm-central/mozilla/xpcom/ds/nsAtomTable.cpp, li
ne 397
[6252] ###!!! ASSERTION: dynamic atom with non-zero refcount appmenu_forwardAsMe
nu!: 'false', file c:/mozilla-source/comm-central/mozilla/xpcom/ds/nsAtomTable.c
pp, line 397
[6252] ###!!! ASSERTION: dynamic atom with non-zero refcount scrollbox-innerbox!
: 'false', file c:/mozilla-source/comm-central/mozilla/xpcom/ds/nsAtomTable.cpp,
 line 397
[6252] ###!!! ASSERTION: dynamic atom with non-zero refcount onDOMMetaRemoved!:
'false', file c:/mozilla-source/comm-central/mozilla/xpcom/ds/nsAtomTable.cpp, l
ine 397
[6252] ###!!! ASSERTION: dynamic atom with non-zero refcount appmenu_tagMenu!: '
false', file c:/mozilla-source/comm-central/mozilla/xpcom/ds/nsAtomTable.cpp, li
ne 397

Now, picking lc-333333 from the list we see that that only occurs in CSS:
https://dxr.mozilla.org/comm-central/search?q=lc-333333&redirect=false
So if that doesn't get "free'ed", than that's nothing we can fix, right?

Nathan, we've stopped registering our strings/atoms late now, but what what can we do about this endless list of errors?

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8831342 [details] [diff] [review]
1334558-remove-atoms.patch (v1)

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

::: mailnews/base/src/nsMsgFolderDataSource.cpp
@@ +787,2 @@
>      OnFolderSortOrderPropertyChanged(resource, oldValue, newValue);
> +  else if (strcmp("BiffStateAtom", property) == 0) {

Three wrong strings. Will fix later.

Comment 5

[Jorg K (CEST = GMT+2)]

Actually, four more which should be lowercase:
-    else if (kIsDeferredAtom == property)
+    else if (strcmp("IsDeferred", property) == 0)
       NotifyPropertyChanged(resource, kNC_IsDeferred, literalNode, oldLiteralNode);
-    else if (kIsSecureAtom == property)
+    else if (strcmp("IsSecure", property) == 0)
       NotifyPropertyChanged(resource, kNC_IsSecure, literalNode, oldLiteralNode);
-    else if (kCanFileMessagesAtom == property)
+    else if (strcmp("CanFileMessages", property) == 0)
       NotifyPropertyChanged(resource, kNC_CanFileMessages, literalNode, oldLiteralNode);
-    else if (kInVFEditSearchScopeAtom == property)
+    else if (strcmp("InVFEditSearchScope", property) == 0)
       NotifyPropertyChanged(resource, kNC_InVFEditSearchScope, literalNode);

Comment 6

[Jorg K (CEST = GMT+2)]

And "open", too. Easy to get wrong.

Try run with those 3 + 4 + 1 wrong strings fixed:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bfb3d0721e420483712d04a3d9bbd540297ef860

Comment 7

[Kent James (:rkent)]

Comment on attachment 8831342 [details] [diff] [review]
1334558-remove-atoms.patch (v1)

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

I looked this over and it seems fine. I did not check each atom to see if it was correct (so I would also miss the incorrect case errors).

Since we are changing everything, it seems to me it would be a good time to fix any inconsistencies. That is, make sure that all former atoms are replaced with camel case strings, with first letter capitalized. If the string includes "Atom" then don't include that.

It would be good if someone had the time to check each atom name, but I do not have that time (nor did I try to compile it). Still, the changes are straightforward, I think this is the right thing to do, so r+=me once you have the try tests sorted.

Comment 8

[Jorg K (CEST = GMT+2)]

Hmm, last comment for the day: There were:
-    kCanFileMessagesAtom         = MsgNewAtom("canFileMessages").take();
and
-        nsCOMPtr <nsIAtom> canFileAtom = MsgGetAtom("CanFileMessages");
So let's hope they weren't meant to be the same.

Looks like we should have all those strings as defines somewhere.

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #7)
> Since we are changing everything, it seems to me it would be a good time to
> fix any inconsistencies. That is, make sure that all former atoms are
> replaced with camel case strings, with first letter capitalized. If the
> string includes "Atom" then don't include that.
How do we know those strings aren't used in JS somewhere hard-coded?

Comment 10

[Kent James (:rkent)]

Yes strings as defines (or some sort of global constants) would let C++ locate case or misspelling errors.

"How do we know those strings aren't used in JS somewhere hard-coded?"  They probably are, but then the JS also has to change since it would be referring to atoms, not strings.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Jorg K (GMT+1) from comment #3)
> Nathan, we've stopped registering our strings/atoms late now, but what what
> can we do about this endless list of errors?

Destroy the atoms before XPCOM shutdown.  I don't know who is holding those atoms alive, given that they're referenced from CSS.  Do you still have live documents at XPCOM shutdown (?!) ?

It's pretty unfortunate that you had to replace all those atom comparisons with string comparisons.  Are you sure there's not a better way to do that?

Comment 12

[Kent James (:rkent)]

Comment on attachment 8831342 [details] [diff] [review]
1334558-remove-atoms.patch (v1)

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

OK I was too aggressive in approving this. There are JS locations that need fixing. Some examples:

https://dxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/index_msg.js#2774

Comment 13

[ISHIKAWA, Chiaki]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> (In reply to Jorg K (GMT+1) from comment #3)
> > Nathan, we've stopped registering our strings/atoms late now, but what what
> > can we do about this endless list of errors?
> 
> Destroy the atoms before XPCOM shutdown.  I don't know who is holding those
> atoms alive, given that they're referenced from CSS.  Do you still have live
> documents at XPCOM shutdown (?!) ?

Sorry for intrusion. But the shutdown order or non-released object issue has been observed for the last several years. For example, I see the following in the C-C TB DEBUG version local test ALL the time.

...
WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and everything alive inside it, that is) AT JS_ShutDown TIME.  FIX THIS!
[19810] ###!!! ASSERTION: Component Manager being held past XPCOM shutdown.: 'cnt == 0', file /NREF-COMM-CENTRAL/comm-central/mozilla/xpcom/build/XPCOMInit.cpp, line 1074
#01: ???[/NREF-COMM-CENTRAL/objdir-tb3/dist/bin/libxul.so +0x10588bd]
#02: NS_ShutdownXPCOM[/NREF-COMM-CENTRAL/objdir-tb3/dist/bin/libxul.so +0x10588dc]
#03: ???[/NREF-COMM-CENTRAL/objdir-tb3/dist/bin/libxul.so +0x49463cb]
#04: ???[/NREF-COMM-CENTRAL/objdir-tb3/dist/bin/libxul.so +0x4950d76]
#05: XRE_main[/NREF-COMM-CENTRAL/objdir-tb3/dist/bin/libxul.so +0x4951186]
#06: ???[/NREF-COMM-CENTRAL/objdir-tb3/dist/bin/thunderbird +0x551e]
#07: ???[/NREF-COMM-CENTRAL/objdir-tb3/dist/bin/thunderbird +0x59e2]
#08: __libc_start_main[/lib/x86_64-linux-gnu/libc.so.6 +0x202b1]
#09: _start[/NREF-COMM-CENTRAL/objdir-tb3/dist/bin/thunderbird +0x471a]
#10: ??? (???:???)
[19810] WARNING: '!compMgr', file /NREF-COMM-CENTRAL/comm-central/mozilla/xpcom/glue/nsComponentManagerUtils.cpp, line 63
Leaked URLs:
...


> It's pretty unfortunate that you had to replace all those atom comparisons
> with string comparisons.  Are you sure there's not a better way to do that?

I would think we could create a fake atom class, to be used by C-C TB only,
which the basic functions necessary for C-C TB (simply offers registration/comparison, etc.),
and rename all the usage of Atomm class to this new reduced class, etc.
Then rewriting the bulk of the code is not necessary.
But the use of Atoms in JS makes it tough...
There seem to be plenty of them.
https://dxr.mozilla.org/comm-central/search?q=nsIAtomService&redirect=false

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> Destroy the atoms before XPCOM shutdown.  I don't know who is holding those
> atoms alive, given that they're referenced from CSS.  Do you still have live
> documents at XPCOM shutdown (?!) ?
Good question. As mentioned in comment #3:
  TB starts and seems to work, however, there is a long long long
  litany of complains when the *profile manager* closes (I use this
  to select my testing profiles) and when TB closes.
this also happens in the profile manager. Now, the TB profile manager suffers the same problem as the FF profile manager, in that it gets an error at the end when exiting, see bug 1303637 (and formerly bug 1223574). No one was interested. Looks like this is coming back to bite us. Have you ever run the profile manager in debug mode after landing bug 1276669?

> It's pretty unfortunate that you had to replace all those atom comparisons
> with string comparisons.  Are you sure there's not a better way to do that?
It's pretty unfortunate that this was thrust upon us with no prior notice and no post on dev-platform. nsIAtoms is a public interface, so how good is using this interface if you have no core service to manage your atoms since you basically closed NS_RegisterStaticAtoms() for others to use. The only thing we can come up in a rush is to use strings. That said, I'll look into using all dynamic atoms via NS_Atomize()/MsgGetAtom() although Kent appears to be more in favour of going the string path.

Comment 15

[Jorg K (CEST = GMT+2)]

Oops, NI got lost by mid-air collision.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-dynamic-atoms.patch (v1)

OK, the "use dynamic atoms" approach is a lot less painful ;-)

We already use this approach here:
https://dxr.mozilla.org/comm-central/rev/aa9ec692549119aac1abf473af2056adb97ad7cb/mailnews/base/src/nsMsgFolderDataSource.cpp#188
with the free at:
https://dxr.mozilla.org/comm-central/rev/aa9ec692549119aac1abf473af2056adb97ad7cb/mailnews/base/src/nsMsgFolderDataSource.cpp#266

Application starts and runs. However, exiting the profile manager gives, as already known:
WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and everything alive
inside it, that is) AT JS_ShutDown TIME.  FIX THIS!
[2472] WARNING: '!compMgr', file c:/mozilla-source/comm-central/mozilla/xpcom/co
mponents/nsComponentManagerUtils.cpp, line 63
[2472] ###!!! ASSERTION: dynamic atom with non-zero refcount stringBundleBinding
s!: 'false', file c:/mozilla-source/comm-central/mozilla/xpcom/ds/nsAtomTable.cp
p, line 397
followed by a million more.

And exiting TB gives the same:
[3760] WARNING: unable to Flush() dirty datasource during XPCOM shutdown: file c
:/mozilla-source/comm-central/mozilla/rdf/base/nsRDFXMLDataSource.cpp, line 745

WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and everything alive
inside it, that is) AT JS_ShutDown TIME.  FIX THIS!
[3760] ###!!! ASSERTION: Component Manager being held past XPCOM shutdown.: 'cnt
 == 0', file c:/mozilla-source/comm-central/mozilla/xpcom/build/XPCOMInit.cpp, l
ine 1075
[3760] WARNING: '!compMgr', file c:/mozilla-source/comm-central/mozilla/xpcom/co
mponents/nsComponentManagerUtils.cpp, line 63
[3760] ###!!! ASSERTION: dynamic atom with non-zero refcount key_previousMsg!: '
false', file c:/mozilla-source/comm-central/mozilla/xpcom/ds/nsAtomTable.cpp, li
ne 397

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=531fc1f6b72d417a98235d11a54a058ec9227fd5
(with a previous version of the patch where MsgNewPermanentAtom() wasn't remove yet, makes no difference)

Comment 17

[Jorg K (CEST = GMT+2)]

So the debug builds fail since they seem to actually run the application and that fails with:

Executing c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/objdir-tb/dist\bin\xpcshell.exe -g c:\builds\moz2_slave\tb-try-c-cen-w32-d-00000000000\build\objdir-tb\dist\bin/ -a c:\builds\moz2_slave\tb-try-c-cen-w32-d-00000000000\build\objdir-tb\dist\bin/ -f c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mozilla/toolkit/mozapps/installer\precompile_cache.js -e precompile_startupcache("resource://gre/");
resource://gre/components/BrowserElementParent.js
resource://gre/components/CSSUnprefixingService.js
resource://gre/components/ColorAnalyzer.js

[stuff deleted]

resource://gre/modules/xmpp-xml.jsm
resource://gre/modules/xmpp.jsm
[2084] WARNING: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS', file c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mozilla/xpcom/base/nsCycleCollector.cpp, line 3595
[2084] WARNING: '!compMgr', file c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mozilla/xpcom/components/nsComponentManagerUtils.cpp, line 63
[2084] ###!!! ASSERTION: dynamic atom with non-zero refcount FolderFlag!: 'false', file c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mozilla/xpcom/ds/nsAtomTable.cpp, line 397
#01: NS_ShutdownAtomTable (c:\builds\moz2_slave\tb-try-c-cen-w32-d-00000000000\build\mozilla\xpcom\ds\nsatomtable.cpp:539)
#02: mozilla::ShutdownXPCOM (c:\builds\moz2_slave\tb-try-c-cen-w32-d-00000000000\build\mozilla\xpcom\build\xpcominit.cpp:1082)
#03: XRE_XPCShellMain (c:\builds\moz2_slave\tb-try-c-cen-w32-d-00000000000\build\mozilla\js\xpconnect\src\xpcshellimpl.cpp:1631)
#04: mozilla::BootstrapImpl::XRE_XPCShellMain (c:\builds\moz2_slave\tb-try-c-cen-w32-d-00000000000\build\mozilla\toolkit\xre\bootstrap.cpp:53)
#05: NS_internal_main (c:\builds\moz2_slave\tb-try-c-cen-w32-d-00000000000\build\mozilla\js\xpconnect\shell\xpcshell.cpp:74)
#06: wmain (c:\builds\moz2_slave\tb-try-c-cen-w32-d-00000000000\build\mozilla\toolkit\xre\nswindowswmain.cpp:118)
#07: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253)
#08: BaseThreadInitThunk[C:\Windows\syswow64\kernel32.dll +0x1338a]
#09: RtlInitializeExceptionChain[C:\Windows\SysWOW64\ntdll.dll +0x397f2]
#10: RtlInitializeExceptionChain[C:\Windows\SysWOW64\ntdll.dll +0x397c5]
[2084] ###!!! ASSERTION: dynamic atom with non-zero refcount FolderFlag!: 'false', file c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mozilla/xpcom/ds/nsAtomTable.cpp, line 397
Hit MOZ_CRASH() at c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mozilla/memory/mozalloc/mozalloc_abort.cpp:33
c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mozilla/toolkit/mozapps/installer/packager.mk:41: recipe for target 'stage-package' failed
mozmake.exe[3]: Leaving directory 'c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/objdir-tb/mail/installer'
c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mozilla/toolkit/mozapps/installer/packager.mk:97: recipe for target 'make-package' failed

So basically that runs into the same problem that we see on debug builds with the profile manager and the application itself.

non-debug builds went through and we'll know in an hour or so whether something works.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8831407 [details] [diff] [review]
1334558-dynamic-atoms.patch (v1)

OK, let's go with the dynamic atoms, Kent inquired about those on IRC. You'll have to make that a post-landing review.

Comment 19

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/f469e0a1b35e13268d0a8ed06240cc364258a5f0

This will give us a green tree at least for Windows opt so we get some working(!) Dailies and can see where we stand with other bustage.

The issues from comment #14, comment #16 and comment #17 have been moved to bug 1334771, the NI goes there, too.

Comment 20

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/f469e0a1b35e13268d0a8ed06240cc364258a5f0 (comment #19)
https://hg.mozilla.org/comm-central/rev/5723e169a8f2ba512caccde4ac84aab83349b57e (follow up)

I think we're done here. Leaking atoms left, right and centre is covered by bug 1303637 comment #2 and bug 1334771.

Comment 21

[Kent James (:rkent)]

Comment on attachment 8831407 [details] [diff] [review]
1334558-dynamic-atoms.patch (v1)

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

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +105,5 @@
>                               nsIDBChangeListener, nsIUrlListener,
>                               nsIJunkMailClassificationListener,
>                               nsIMsgTraitClassificationListener)
>  
>  #define MSGDBFOLDER_ATOM(name_, value_) nsIAtom* nsMsgDBFolder::name_ = nullptr;

Here you are defining the atoms as static properties of nsMsgDBFolder, meaning there will be one value for all instances of nsMsgDBFolder. But later you release those values in the destructor of a particular instance. That is not going to work, as other folders will then also see their nsIAtom* values released, and you may have the atom destroyed.

If you are going to use the dynamic atoms, what you  will need to do is to make them instance variables.

While you are at it, you should go ahead and just make the nsCOMPtr<nsIAtom> and let the smart pointer automatic release work.

@@ +131,5 @@
>    mIsServer(false),
>    mInVFEditSearchScope (false)
>  {
>    if (mInstanceCount++ <=0) {
> +#define MSGDBFOLDER_ATOM(name_, value_) name_ = MsgNewAtom(value_).take();

When you use nsCOMPtr, I do not believe you will need the .take() (but I have never actually used .take() myself)

@@ +166,5 @@
>      NS_Free(kLocalizedJunkName);
>      NS_Free(kLocalizedArchivesName);
>      NS_Free(kLocalizedBrandShortName);
> +
> +#define MSGDBFOLDER_ATOM(name_, value_) NS_RELEASE(name_);

This is the problematic release. It will not be needed if you use nsCOMPtr

Comment 22

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #21)
> Here you are defining the atoms as static properties of nsMsgDBFolder,
> meaning there will be one value for all instances of nsMsgDBFolder. But
> later you release those values in the destructor of a particular instance.
You got yourself confused.

> That is not going to work, as other folders will then also see their
> nsIAtom* values released, and you may have the atom destroyed.
How can it not be working, you have a green run in opt and in debug also most test pass unless they leak folders.

As you correctly pointed out, the atoms are static properties of nsMsgDBFolder, and there therefore is one global set of atoms for all folder instances.

That's why the get allocated in the constructor of the *first* instance and released in the destructor of the *last* instance. If course, if the last instance never gets released, we have a problem, as can be seen in bug 1334874:
https://dxr.mozilla.org/comm-central/rev/07ccae5d880ba9f0e798b061abfe00fb100568d0/mailnews/base/util/nsMsgDBFolder.cpp#134

In fact, the current code is an excellent way to find leaked folders, which previously we just ignored.

So please put r+ onto this or I really have to take it to the engineering committee ;-)

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8831407 [details] [diff] [review]
1334558-dynamic-atoms.patch (v1)

Second round.

Comment 24

[Jorg K (CEST = GMT+2)]

Please note that what was implemented here is already used elsewhere in the system as noted in comment #16:
https://dxr.mozilla.org/comm-central/rev/aa9ec692549119aac1abf473af2056adb97ad7cb/mailnews/base/src/nsMsgFolderDataSource.cpp#188
with the free at:
https://dxr.mozilla.org/comm-central/rev/aa9ec692549119aac1abf473af2056adb97ad7cb/mailnews/base/src/nsMsgFolderDataSource.cpp#266

Static class members here:
https://dxr.mozilla.org/comm-central/rev/07ccae5d880ba9f0e798b061abfe00fb100568d0/mailnews/base/src/nsMsgFolderDataSource.h#260

What is there not to like ;-)

Comment 25

[Kent James (:rkent)]

Comment on attachment 8831407 [details] [diff] [review]
1334558-dynamic-atoms.patch (v1)

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

OK, I missed the "mInstanceCount" stuff, I see that is supposed to overcome the objections that I had (though as I thought about it more, it might work even without the mInstanceCount trick). So I can approve it.

Re your "So please put r+ onto this or I really have to take it to the engineering committee ;-)" yes you were joking, but to be clear the normal path in a simple dispute would be to get a third party involved (like Aceman or Magnus). In theory the Engineering Committee could resolve disputes, but that is not their main function, and should be a last resort rather than a common occurrence.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8831407 [details] [diff] [review]
1334558-dynamic-atoms.patch (v1)

Excellent, since we all agree now, we're well and truly done here. Migration from atoms to strings to be considered in bug 1334834.

Comment 27

[Jorg K (CEST = GMT+2)]

Aurora (TB 53):
https://hg.mozilla.org/releases/comm-aurora/rev/1696e54a643054c8e3263870f5c1898a13180a62
https://hg.mozilla.org/releases/comm-aurora/rev/f1156f0adef920c22fc317a95e0d73a084bbee02

I had to uplift this since bug 1276669 just got uplifted.

Comment 28

[Jorg K (CEST = GMT+2)]

Beta (TB 52):
https://hg.mozilla.org/releases/comm-beta/rev/a4c1629a5108273f1e0a4b81d8b07a4d9cf2a955
https://hg.mozilla.org/releases/comm-beta/rev/965a622fb389f262cdb0119a7bf8fa3fa0de72ac

I had to uplift this since bug 1276669 will be uplifted.