Nicholas Nethercote [inactive] Assignee
	Description • 8 years ago

Let's use MOZ_MUST_USE as much as is sensible in nsMemoryReporterManager.cpp.

	Nicholas Nethercote [inactive] Assignee
	Comment 2 • 8 years ago

(Previous patch included an unintentional change.)

	Eric Rahm [:erahm]
	Comment 3 • 8 years ago

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

	Nicholas Nethercote [inactive] Assignee
	Comment 4 • 8 years ago

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!

	Nicholas Nethercote [inactive] Assignee
	Comment 5 • 8 years ago

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.

	Eric Rahm [:erahm]
	Comment 6 • 8 years ago

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?

	Eric Rahm [:erahm]
	Comment 7 • 8 years ago

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

	Nicholas Nethercote [inactive] Assignee
	Comment 8 • 8 years ago

> Is this just extra cleanup of an unused type?

Yes.

	Nicholas Nethercote [inactive] Assignee
	Comment 9 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6612a73409a9029aff2461b9ab9f451b4677f51d
Bug 1291970 (part 1) - Use MOZ_MUST_USE in nsMemoryReporterManager. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8117f4a3659120291ecbfcc07650fed632dda02
Bug 1291970 (part 2) - Tweak nsCategoryManager memory reporter handling. r=erahm.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6612a73409a9
https://hg.mozilla.org/mozilla-central/rev/b8117f4a3659