The default bug view has changed. See this FAQ.

Don't use char** in nsIMemoryReporter

RESOLVED FIXED in mozilla8

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

({dev-doc-complete})

unspecified
mozilla8
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
xpcom/base/nsIMemoryReporter.idl has this code:

#define NS_MEMORY_REPORTER_IMPLEMENT(_classname,_path,_desc,_usageFunction,_data
ptr) \
    class MemoryReporter_##_classname : public nsIMemoryReporter {      \
    public:                                                             \
      NS_DECL_ISUPPORTS                                                 \
      NS_IMETHOD GetPath(char **memoryPath) { *memoryPath = strdup(_path); retur
n NS_OK; } \
      NS_IMETHOD GetDescription(char **desc) { *desc = strdup(_desc); return NS_
OK; } \
      NS_IMETHOD GetMemoryUsed(PRInt64 *memoryUsed) { *memoryUsed = _usageFuncti
on(_dataptr); return NS_OK; } \
    };                                                                  \
    NS_IMPL_ISUPPORTS1(MemoryReporter_##_classname, nsIMemoryReporter)

sdwilsh suggests that the char** arguments should instead be nsACString& arguments.  (Would that get rid of the strdup calls?  It's unclear to me who's responsible for freeing those duplicated strings in the current code.)
(Assignee)

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All
XPCOM rules say whatever calls GetDescription.
> Would that get rid of the strdup calls?

Yes.
(Assignee)

Comment 3

6 years ago
Created attachment 542098 [details] [diff] [review]
patch (against m-i 70732:96fac50b57ed)

My mozString-fu is weak, so please check this carefully :)
Attachment #542098 - Flags: superreview?(bzbarsky)
Attachment #542098 - Flags: review?(sdwilsh)
Comment on attachment 542098 [details] [diff] [review]
patch (against m-i 70732:96fac50b57ed)

>+        process.AssignLiteral("");

  process.Truncate();

This applies in a few places in the patch.

>+     path.Assign(nsDependentCString(SurfaceMemoryReporterPathForType(mType)));

You shouldn't need the nsDependentCString.  nsACString has an Assign overload taking const char*.

You need to change the nsIMemoryReporter interface id.

The process ID is probably OK to leave as ACString, since we can guarantee it will be ASCII.  The description is better as AUTF8String in case we ever decide to localize.  This doesn't affect the C++ code at all.

sr=me with those nits.
Attachment #542098 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 5

6 years ago
Created attachment 542354 [details] [diff] [review]
patch v2 (against m-i 71842:c04708b8007f)

This patch addresses bz's comments.  I've carried over his sr+.
Attachment #542098 - Attachment is obsolete: true
Attachment #542098 - Flags: review?(sdwilsh)
Attachment #542354 - Flags: superreview+
Attachment #542354 - Flags: review?(sdwilsh)
(Assignee)

Comment 6

6 years ago
Created attachment 543883 [details] [diff] [review]
patch v3

Updated;  some new memory reporter code has been added.
Attachment #542354 - Attachment is obsolete: true
Attachment #542354 - Flags: review?(sdwilsh)
Attachment #543883 - Flags: superreview+
Attachment #543883 - Flags: review?(khuey)
Comment on attachment 543883 [details] [diff] [review]
patch v3

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

Looks good.  One nit.

::: dom/base/nsDOMMemoryReporter.h
@@ +53,3 @@
>    NS_IMETHOD GetUnits(PRInt32* aUnits);
>    NS_IMETHOD GetAmount(PRInt64* aAmount);
> +  NS_IMETHOD GetDescription(nsACString &aDescription);

Is there a reason you're not using NS_DECL_NSIMEMORYREPORTER here?

::: xpcom/base/nsIMemoryReporter.idl
@@ +256,4 @@
>        NS_IMETHOD GetKind(int *kind) { *kind = _kind; return NS_OK; }                          \
>        NS_IMETHOD GetUnits(int *units) { *units = _units; return NS_OK; }                      \
> +      NS_IMETHOD GetAmount(PRInt64 *amount) { *amount = _amountFunction(); return NS_OK; }     \
> +      NS_IMETHOD GetDescription(nsACString &desc) { desc.Assign(_desc); return NS_OK; } \

and here?
Attachment #543883 - Flags: review?(khuey) → review+
(Assignee)

Comment 8

6 years ago
(In reply to comment #7)
> Comment on attachment 543883 [details] [diff] [review] [review]
> patch v3
> 
> Review of attachment 543883 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks good.  One nit.
> 
> ::: dom/base/nsDOMMemoryReporter.h
> @@ +53,3 @@
> >    NS_IMETHOD GetUnits(PRInt32* aUnits);
> >    NS_IMETHOD GetAmount(PRInt64* aAmount);
> > +  NS_IMETHOD GetDescription(nsACString &aDescription);
> 
> Is there a reason you're not using NS_DECL_NSIMEMORYREPORTER here?

I didn't write that code, but I can change it.

> ::: xpcom/base/nsIMemoryReporter.idl
> @@ +256,4 @@
> >        NS_IMETHOD GetKind(int *kind) { *kind = _kind; return NS_OK; }                          \
> >        NS_IMETHOD GetUnits(int *units) { *units = _units; return NS_OK; }                      \
> > +      NS_IMETHOD GetAmount(PRInt64 *amount) { *amount = _amountFunction(); return NS_OK; }     \
> > +      NS_IMETHOD GetDescription(nsACString &desc) { desc.Assign(_desc); return NS_OK; } \
> 
> and here?

That's definitions, not declarations -- I don't think you can use NS_DECL_NSIMEMORYREPORTER as a result?
(In reply to comment #8)
> 
> > ::: xpcom/base/nsIMemoryReporter.idl
> > @@ +256,4 @@
> > >        NS_IMETHOD GetKind(int *kind) { *kind = _kind; return NS_OK; }                          \
> > >        NS_IMETHOD GetUnits(int *units) { *units = _units; return NS_OK; }                      \
> > > +      NS_IMETHOD GetAmount(PRInt64 *amount) { *amount = _amountFunction(); return NS_OK; }     \
> > > +      NS_IMETHOD GetDescription(nsACString &desc) { desc.Assign(_desc); return NS_OK; } \
> > 
> > and here?
> 
> That's definitions, not declarations -- I don't think you can use
> NS_DECL_NSIMEMORYREPORTER as a result?

Yeah, I wasn't reading closely enough, you're correct here.
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/43fa785282e3
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/43fa785282e3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Keywords: dev-doc-needed
Docs updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryReporter
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.