Add message manager listeners to the event-counts tree

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: khuey, Assigned: wchen)

Tracking

unspecified
mozilla27
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [MemShrink:P2][qa-])

Attachments

(1 attachment)

We're tracking DOM event listeners, we should track message manager listeners too.
William is this something you would be interested in working on?
blocking-b2g: --- → koi+
Flags: needinfo?(wchen)
Whiteboard: [MemShrink] → [MemShrink:P2]
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> William is this something you would be interested in working on?

Yeah, I can take this.
Assignee: nobody → wchen
Flags: needinfo?(wchen)
I'm a bit confused by the title of this bug, why do message listeners belong in the event-counts tree? I created a new tree in this patch, but that can be easily changed. Is there even an event-count tree because I can't seem to find it.
Attachment #815614 - Flags: review?(n.nethercote)
Comment on attachment 815614 [details] [diff] [review]
Add memory reporter for message manager listeners.

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

The event-counts tree, IIRC, was in a patch that got modified before landing.

::: content/base/src/nsFrameMessageManager.cpp
@@ +1016,5 @@
> +  MessageManagerReferentCount() : strong(0), weakAlive(0), weakDead(0) {}
> +  size_t strong;
> +  size_t weakAlive;
> +  size_t weakDead;
> +  nsTArray<nsIAtom*> suspectMessages;

Should this be nsCOMArray?

@@ +1103,5 @@
> +    } while (0)
> +
> +  REPORT(nsPrintfCString("message-manager/referent/%s/strong", aManagerType),
> +         aReferentCount.strong,
> +         nsPrintfCString("The number of strong referents held by the message "

You could move the nsPrintfCString() calls into REPORT.

@@ +1143,5 @@
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    nsCOMPtr<nsIMessageBroadcaster> globalmm =
> +      do_GetService("@mozilla.org/globalmessagemanager;1");
> +    if (globalmm) {
> +      nsFrameMessageManager* mm =

Should this be nsRefPtr<nsFrameMessageManager>?  Likewise above in CountReferents().
Attachment #815614 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #4)
> Comment on attachment 815614 [details] [diff] [review]
> Add memory reporter for message manager listeners.
> 
> Review of attachment 815614 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The event-counts tree, IIRC, was in a patch that got modified before landing.
> 
> ::: content/base/src/nsFrameMessageManager.cpp
> @@ +1016,5 @@
> > +  MessageManagerReferentCount() : strong(0), weakAlive(0), weakDead(0) {}
> > +  size_t strong;
> > +  size_t weakAlive;
> > +  size_t weakDead;
> > +  nsTArray<nsIAtom*> suspectMessages;
> 
> Should this be nsCOMArray?
> 
It shouldn't be necessary because the struct only lives on the stack and the atom is kept alive during the lifetime by the message manager. But I've changed it to nsCOMArray just to be on the safe side.
> @@ +1103,5 @@
> > +    } while (0)
> > +
> > +  REPORT(nsPrintfCString("message-manager/referent/%s/strong", aManagerType),
> > +         aReferentCount.strong,
> > +         nsPrintfCString("The number of strong referents held by the message "
> 
> You could move the nsPrintfCString() calls into REPORT.
> 
I left it the way it is because I think it's a bit easier to read as opposed to passing a string with a format specifier into a macro without immediately being able to see what it is replaced with.
> @@ +1143,5 @@
> > +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> > +    nsCOMPtr<nsIMessageBroadcaster> globalmm =
> > +      do_GetService("@mozilla.org/globalmessagemanager;1");
> > +    if (globalmm) {
> > +      nsFrameMessageManager* mm =
> 
> Should this be nsRefPtr<nsFrameMessageManager>?  Likewise above in
> CountReferents().
It shouldn't be necessary but I've changed it to a nsRefPtr just to be on the safe side.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8854b9662405
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/8854b9662405
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Whiteboard: [MemShrink:P2] → [MemShrink:P2][qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.