1517276 - thread callbacks through MemoryReportRequestClient

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Nathan Froyd [:froydnj]]

The guts of MemoryReportRequestClient's supporting runnables contain
switches on the particular type of process we're running.  If you're
bringing up a new process type, having to add extra cases for your
process type here is a bit onerous.  These runnables really shouldn't
know anything about the process types that they're running on, either.

The easiest thing to do is modify MemoryReportRequestClient::Start to
take callbacks for what to do when a report is created and when
reporting is finished.  Then all process-specific knowledge can be
pushed out to the clients themselves, leaving MemoryReportRequestClient
and friends process-type agnostic.  We could even, at some later date,
move this code into xpcom/base/ to sit near nsMemoryReporterManager,
where it belongs.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: thread callbacks through MemoryReportRequestClient

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 9033981 [details] [diff] [review]
thread callbacks through MemoryReportRequestClient

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

This makes a lot of sense! r- for the UAF issue I think I spotted.

::: dom/ipc/ContentChild.cpp
@@ +1238,5 @@
>  
>    MemoryReportRequestClient::Start(aGeneration, aAnonymize,
> +                                   aMinimizeMemoryUsage, aDMDFile, process,
> +                                   [&](const MemoryReport& aReport) {
> +                                     Unused << SendAddMemoryReport(aReport);

Maybe I'm reading this wrong, but isn't this (for all three process types) going from a situation where we get a null deref crash if we use the callback after the actors have been shut down to one where we get a UAF? (I guess some static analysis should complain about a raw ref to |this|?) That doesn't seem great to me. Could we just continue to use the GetSingleton() methods in these callbacks, though it does make the code uglier.

::: dom/ipc/MemoryReportRequest.h
@@ +65,5 @@
>    bool mAnonymize;
>    mozilla::ipc::FileDescriptor mDMDFile;
>    nsCString mProcessString;
> +  ReportCallback mReportCallback;
> +  FinishCallback mFinishCallback;

It might be a little nicer if this was structured as a single OO interface rather than two functions, but I guess we're probably not going to add additional callbacks, so this is okay. Let's think about changing this if more stuff gets added.

It is too bad, for this specific case, that there's no inheritance in IPDL or this could all be encapsulated in a GetSingleton() method for some parent type that has the two methods we need.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: thread callbacks through MemoryReportRequestClient

Updated with GetSingleton() calls.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: thread callbacks through MemoryReportRequestClient

Now with include deletions and passing std::function by reference, as OS X
would prefer.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 9034015 [details] [diff] [review]
thread callbacks through MemoryReportRequestClient

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

::: dom/ipc/MemoryReportRequest.cpp
@@ +132,3 @@
>  
>    NS_IMETHOD Callback(nsISupports* aUnused) override {
> +    bool sent = mFinishCallback(mGeneration);

Maybe inline |sent| now that it is trivial?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b71897ff6be
thread callbacks through MemoryReportRequestClient; r=mccr8

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7b71897ff6be