Closed Bug 653627 Opened 9 years ago Closed 9 years ago

Don't use char** in nsIMemoryReporter

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: njn, Assigned: njn)

Details

(Keywords: dev-doc-complete, Whiteboard: [inbound])

Attachments

(1 file, 2 obsolete files)

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.)
OS: Linux → All
Hardware: x86_64 → All
XPCOM rules say whatever calls GetDescription.
> Would that get rid of the strdup calls?

Yes.
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+
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)
Attached patch patch v3Splinter Review
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+
(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.
http://hg.mozilla.org/mozilla-central/rev/43fa785282e3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.