Closed Bug 944807 Opened 8 years ago Closed 2 years ago

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

Categories

(MailNews Core :: Database, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aceman, Unassigned)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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.
Attached patch WIP patchSplinter Review
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]
 }
Attachment #8340551 - Flags: feedback?(Pidgeot18)
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).
Attachment #8340551 - Flags: feedback?(Pidgeot18)
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.
Severity: blocker → normal
Another approach is to move memory reporting to the nsMsgDBService, although that would probably require moving the nsMsgDatabase cache there too.
Severity: normal → minor
(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.
Blocks: 857373, 805023
Severity: minor → normal
See Also: → 1330872
Summary: Update nsMsgDatabase.cpp after removal of NS_RegisterMemoryReporter → fix memory reporting in nsMsgDatabase.cpp after removal of NS_RegisterMemoryReporter
Whiteboard: [patchlove]
(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?
Flags: needinfo?(acelists)
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.
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.
Flags: needinfo?(acelists) → needinfo?(Pidgeot18)
Flags: needinfo?(Pidgeot18)

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 neil@parkwaycc.co.uk 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 ?

No longer blocks: 805023, 857373
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(mkmelin+mozilla)
Keywords: regression
Regressed by: 936964
Resolution: --- → FIXED
See Also: 1330872936964
Summary: fix memory reporting in nsMsgDatabase.cpp after removal of NS_RegisterMemoryReporter → fix memory reporting in nsMsgDatabase.cpp after removal of NS_RegisterMemoryReporter in bug 936964
Whiteboard: [patchlove]

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.

Flags: needinfo?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.