944807 - fix memory reporting in nsMsgDatabase.cpp after removal of NS_RegisterMemoryReporter in bug 936964

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[:aceman]

It seems mozilla-central removed NS_RegisterMemoryReporter and co.
It is still used here: 
/mailnews/db/msgdb/src/nsMsgDatabase.cpp
    line 1145 -- NS_RegisterMemoryReporter(mMemReporter);

So the build fails now, see:
https://tbpl.mozilla.org/php/getParsedLog.php?id=31257138&tree=Thunderbird-Trunk

See http://hg.mozilla.org/mozilla-central/rev/ece8c99958a6 on how to migrate to the new schema.

Comment 1

[:aceman]

Attachment: WIP patch

This is as far as I could progress. Can anybody move this further? It still has some compile failures:
In file included from ../../../../mozilla/dist/include/nsMailDatabase.h:10:0,
                 from /var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:15:
../../../../mozilla/dist/include/nsMsgDatabase.h:428:60: error: 'virtual nsresult nsMsgDatabase::CollectReports(nsIMemoryReporterCallback*, nsISupports*)' cannot be overloaded
   NS_IMETHOD CollectReports(nsIMemoryReporterCallback*aCb,
                                                            ^
In file included from ../../../../mozilla/dist/include/nsMailDatabase.h:10:0,
                 from /var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:15:
../../../../mozilla/dist/include/nsMsgDatabase.h:118:60: error: with 'virtual nsresult nsMsgDatabase::CollectReports(nsIMemoryReporterCallback*, nsISupports*)'
   NS_DECL_NSIMEMORYREPORTER
                                                            ^
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function 'virtual nsrefcnt nsMsgDBService::Release()':
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:78:1847: warning: deleting object of polymorphic class type 'nsMsgDBService' which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]
 NS_IMPL_ISUPPORTS1(nsMsgDBService, nsIMsgDBService)
                                      ^
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function 'virtual nsresult nsMsgDBService::GetOpenDBs(nsIArray**)':
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:434:73: error: 'nsISupports' is an ambiguous base of 'nsMsgDatabase'
     openDBs->AppendElement(nsMsgDatabase::m_dbCache->ElementAt(i), false);
                                                                         ^
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function 'int64_t nsMsgDatabase::Amount()':
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:1036:51: error: expected primary-expression before ')' token
   return SizeOfIncludingThis(mozilla::MallocSizeOf);
                                                   ^
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp: In constructor 'nsMsgDatabase::nsMsgDatabase()':
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:1188:34: error: 'RegisterWeakMemoryReporter' was not declared in this scope
   RegisterWeakMemoryReporter(this);
                                  ^
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:1188:34: note: suggested alternative:
In file included from ../../../../mozilla/dist/include/nsMsgDatabase.h:11:0,
                 from ../../../../mozilla/dist/include/nsMailDatabase.h:10,
                 from /var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:15:
../../../../mozilla/dist/include/nsIMemoryReporter.h:687:62: note:   'mozilla::RegisterWeakMemoryReporter'
 XPCOM_API(nsresult) RegisterWeakMemoryReporter(nsIMemoryReporter* aReporter);
                                                              ^
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp: In destructor 'virtual nsMsgDatabase::~nsMsgDatabase()':
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:1193:36: error: 'UnregisterWeakMemoryReporter' was not declared in this scope
   UnregisterWeakMemoryReporter(this);
                                    ^
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:1193:36: note: suggested alternative:
In file included from ../../../../mozilla/dist/include/nsMsgDatabase.h:11:0,
                 from ../../../../mozilla/dist/include/nsMailDatabase.h:10,
                 from /var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:15:
../../../../mozilla/dist/include/nsIMemoryReporter.h:689:62: note:   'mozilla::UnregisterWeakMemoryReporter'
 XPCOM_API(nsresult) UnregisterWeakMemoryReporter(nsIMemoryReporter* aReporter);
                                                              ^
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function 'virtual nsresult nsMsgDatabase::RemoveHeaderFromThread(nsMsgHdr*)':
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:2120:71: error: 'nsISupports' is an ambiguous base of 'nsMsgDatabase'
     nsCOMPtr <nsIDBChangeAnnouncer> announcer = do_QueryInterface(this);
                                                                       ^
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function 'virtual nsresult nsMsgDatabase::AddToThread(nsMsgHdr*, nsIMsgThread*, nsIMsgDBHdr*, bool)':
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:4702:69: error: 'nsISupports' is an ambiguous base of 'nsMsgDatabase'
   nsCOMPtr <nsIDBChangeAnnouncer> announcer = do_QueryInterface(this);
                                                                     ^
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function 'int64_t nsMsgDatabase::Amount()':
/var/SSD/TB-hg/mailnews/db/msgdb/src/nsMsgDatabase.cpp:1037:1: error: control reaches end of non-void function [-Werror=return-type]
 }

Comment 2

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8340551 [details] [diff] [review]
WIP patch

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

I did a simpler fix instead to get it just building. Adopting the not-creating-singletons is still a worthwhile patch, though.

(The problems with nsISupports ambiguous-case conversions generally can be solved by adding strategic static_cast<nsIMsgDatabase> casts).

Comment 3

[:aceman]

The simpler patch being http://hg.mozilla.org/comm-central/rev/bfec48e0479c. I can confirm trunk builds again for me.

Ok, I have no idea which approach is good so you can keep this bug and finish it when you feel like it.

Comment 4

[[e-mail]]

Another approach is to move memory reporting to the nsMsgDBService, although that would probably require moving the nsMsgDatabase cache there too.

Comment 5

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #3)
> The simpler patch being http://hg.mozilla.org/comm-central/rev/bfec48e0479c.

2013-11-30  Bustage fix for bug 936964's changes to memory reporting, r=bustage-fix.

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #3)
> The simpler patch being http://hg.mozilla.org/comm-central/rev/bfec48e0479c.
> I can confirm trunk builds again for me.
> 
> Ok, I have no idea which approach is good so you can keep this bug and
> finish it when you feel like it.

What is left to finish, your WIP from comment 1?

Comment 7

[Jorg K (CEST = GMT+2)]

Looks like stuff already got implemented here:
https://hg.mozilla.org/comm-central/rev/bfec48e0479c
https://hg.mozilla.org/comm-central/rev/7ef8dd6bfe35
https://hg.mozilla.org/comm-central/rev/71de34b00fc2

I can see
+    return aCb->Callback(EmptyCString(), path,
+                         nsIMemoryReporter::KIND_HEAP,
+                         nsIMemoryReporter::UNITS_BYTES,
+                         mDatabase->SizeOfIncludingThis(GetMallocSize),           
+                         NS_LITERAL_CSTRING("Memory used for the folder database."),
+                         aClosure);

and

+  NS_IMETHOD GetPath(nsACString &memoryPath)
+  {
+    memoryPath.AssignLiteral("explicit/maildb/database(");
+    nsCOMPtr<nsIMsgFolder> folder;

from Aceman's patch. Did they all steal from Aceman?

I think we're done here.

Comment 8

[:aceman]

And I have stolen it from the m-c changes :)

I don't know what is missing here. I wanted to fix the compile failure, which got fixed (comment 3).
The patch (which I just blindly adapted from m-c to c-c) supposedly contains other worthwhile changes (comment 2). If bug 936964 implements those changes (but really also for c-c?), we could be done here.
But I am not able to decide that, jcranmer can.

Comment 9

[Wayne Mery (:wsmwk)]

At this point I agree we should close this as fixed by the bustage patch, and do any "proper" fixing and followup in new bugs. So the remaining questions ...

(In reply to Joshua Cranmer [:jcranmer] from comment #2)

Adopting the not-creating-singletons is still a worthwhile patch, though.

a) Did bug 936964 resolve this?

(In reply to [e-mail] from comment #4)

Another approach is to move memory reporting to the nsMsgDBService, although
that would probably require moving the nsMsgDatabase cache there too.

b) Even if other issues have been resolved, from code organization, stability and maintenance POV, is there value in doing this?

c) does the above help us move toward resolving bug 805023 and bug 857373 ?

Comment 10

[Magnus Melin [:mkmelin]]

It's been so long since those landed and so much has changed since that it's quite hard to say how it impacted at the time or what the status is now.