653627 - Don't use char** in nsIMemoryReporter

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

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.)

Comment 1

[Shawn Wilsher :sdwilsh]

XPCOM rules say whatever calls GetDescription.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Would that get rid of the strdup calls?

Yes.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch (against m-i 70732:96fac50b57ed)

My mozString-fu is weak, so please check this carefully :)

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: patch v2 (against m-i 71842:c04708b8007f)

This patch addresses bz's comments.  I've carried over his sr+.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: patch v3

Updated;  some new memory reporter code has been added.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 8

[Nicholas Nethercote [inactive]]

(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?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 10

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/43fa785282e3

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/43fa785282e3

Comment 12

[Eric Shepherd [:sheppy]]

Docs updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryReporter