thread callbacks through MemoryReportRequestClient

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

6 months ago
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.
Assignee

Comment 1

6 months ago
Attachment #9033981 - Flags: review?(continuation)
Assignee

Updated

6 months ago
Blocks: 1514039
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.
Attachment #9033981 - Flags: review?(continuation) → review-
Assignee

Comment 3

6 months ago
Updated with GetSingleton() calls.
Attachment #9034013 - Flags: review?(continuation)
Assignee

Updated

6 months ago
Attachment #9033981 - Attachment is obsolete: true
Assignee

Comment 4

6 months ago
Now with include deletions and passing std::function by reference, as OS X
would prefer.
Attachment #9034015 - Flags: review?(continuation)
Assignee

Updated

6 months ago
Attachment #9034013 - Attachment is obsolete: true
Attachment #9034013 - Flags: review?(continuation)
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?
Attachment #9034015 - Flags: review?(continuation) → review+

Comment 6

6 months ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b71897ff6be
thread callbacks through MemoryReportRequestClient; r=mccr8

Comment 7

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7b71897ff6be
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.