Last Comment Bug 653627 - Don't use char** in nsIMemoryReporter
: Don't use char** in nsIMemoryReporter
Status: RESOLVED FIXED
[inbound]
: dev-doc-complete
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-28 18:06 PDT by Nicholas Nethercote [:njn]
Modified: 2011-09-06 06:50 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (against m-i 70732:96fac50b57ed) (15.69 KB, patch)
2011-06-26 23:31 PDT, Nicholas Nethercote [:njn]
bzbarsky: superreview+
Details | Diff | Splinter Review
patch v2 (against m-i 71842:c04708b8007f) (15.92 KB, patch)
2011-06-27 18:42 PDT, Nicholas Nethercote [:njn]
n.nethercote: superreview+
Details | Diff | Splinter Review
patch v3 (20.72 KB, patch)
2011-07-04 23:32 PDT, Nicholas Nethercote [:njn]
khuey: review+
n.nethercote: superreview+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-04-28 18:06:25 PDT
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 2011-04-28 18:08:54 PDT
XPCOM rules say whatever calls GetDescription.
Comment 2 Boris Zbarsky [:bz] 2011-04-28 19:48:11 PDT
> Would that get rid of the strdup calls?

Yes.
Comment 3 Nicholas Nethercote [:njn] 2011-06-26 23:31:55 PDT
Created attachment 542098 [details] [diff] [review]
patch (against m-i 70732:96fac50b57ed)

My mozString-fu is weak, so please check this carefully :)
Comment 4 Boris Zbarsky [:bz] 2011-06-27 14:38:43 PDT
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 [:njn] 2011-06-27 18:42:45 PDT
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+.
Comment 6 Nicholas Nethercote [:njn] 2011-07-04 23:32:41 PDT
Created attachment 543883 [details] [diff] [review]
patch v3

Updated;  some new memory reporter code has been added.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-06 19:41:55 PDT
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 [:njn] 2011-07-06 20:08:06 PDT
(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 [:khuey] (khuey@mozilla.com) 2011-07-06 20:10:28 PDT
(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 [:njn] 2011-07-07 19:46:43 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/43fa785282e3
Comment 11 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-07-08 06:00:12 PDT
http://hg.mozilla.org/mozilla-central/rev/43fa785282e3
Comment 12 Eric Shepherd [:sheppy] 2011-09-06 06:50:01 PDT
Docs updated:

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

Note You need to log in before you can comment on or make changes to this bug.