Use MOZ_MUST_USE in nsMemoryReporterManager.cpp

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Let's use MOZ_MUST_USE as much as is sensible in nsMemoryReporterManager.cpp.
Attachment #8777633 - Flags: review?(erahm)
(Previous patch included an unintentional change.)
Attachment #8777634 - Flags: review?(erahm)
Attachment #8777633 - Attachment is obsolete: true
Attachment #8777633 - Flags: review?(erahm)
Comment on attachment 8777634 [details] [diff] [review]
Use MOZ_MUST_USE in nsMemoryReporterManager.cpp

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

This seems fine, but I'm not sure it's have the effect you want. Either I'm guessing you need another patch that fixes all the bustage from calls to |RegisterWeakMemoryReporter| or tacking |MOZ_MUST_USE| to the definition (vs declaration) doesn't propagate outside of the file?

So if the intent is just local r+, if not f+.
Attachment #8777634 - Flags: review?(erahm) → review+
I've kept the MOZ_MUST_USE annotations on static functions within the .cpp
file, and added some to the header. Thanks for catching the oversight in the
previous patch!
Attachment #8779919 - Flags: review?(erahm)
This patch adds an assertion that makes sure that the nsCategoryManager is
destroyed after the nsMemoryReporterManager, because bad things would happen
otherwise.

Also, nsCategoryManager uses manual memory management (it's AddRef/Release are
hardwired to always return 2 and 1 respectively) so it doesn't matter if we
register it as a strong or weak memory reporter. But it's more common to use
RegisterWeakMemoryReporter when the argument is |this|, so this patch does
that.
Attachment #8779920 - Flags: review?(erahm)
Comment on attachment 8779919 [details] [diff] [review]
(part 1) - Use MOZ_MUST_USE in nsMemoryReporterManager

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

r=me

::: xpcom/base/nsIMemoryReporter.idl
@@ -472,5 @@
>  // the following functions for those components to be registered with the
>  // manager.
>  
>  typedef int64_t (*InfallibleAmountFn)();
> -typedef nsresult (*FallibleAmountFn)(int64_t* aAmount);

Is this just extra cleanup of an unused type?
Attachment #8779919 - Flags: review?(erahm) → review+
Comment on attachment 8779920 [details] [diff] [review]
(part 2) - Tweak nsCategoryManager memory reporter handling

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

This seems fine, although I'm not sure what it has to do with this bug :)
Attachment #8779920 - Flags: review?(erahm) → review+
> Is this just extra cleanup of an unused type?

Yes.
https://hg.mozilla.org/mozilla-central/rev/6612a73409a9
https://hg.mozilla.org/mozilla-central/rev/b8117f4a3659
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.