1029337 - Use IPC for content process profiler file writing

Status: RESOLVED WONTFIX

(Core :: Gecko Profiler, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Currently, for profiling on B2G, when we want processes to write out their profiles, the profile.sh script sends a signal to each process, which opens a file in TmpD to write to.  This will be a problem for sandboxing.

Instead, we can have the parent process, when signalled, send each of the children a message with a FileDescriptor to write the profile data to.  (For compatibility, the children can ignore the signal.)

It would also be good to IPCify the reading of the profiler options on profile start, although not as important for bug 930258 given that there's going to be some facility for whitelisting specific fixed paths.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Now that I've actually tried to do this, I think it makes more sense to solve this problem by using a child→parent sync IPC to open the file, and leave restructuring of the profiler control mechanisms for a followup bug.  I have a patch that works locally on B2G (and desktop e10s, by attaching to the content process with gdb to call the profiler functions), and I'm currently ironing out breakages on try.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

So there are several problems.  On Windows, the C++ library has no way to make an ostream from a handle or fd or stdio FILE*; if we switch JSStreamWriter to stdio, then we have no way to translate the call site that uses stringstream, because fmemopen() isn't everywhere yet.  This seems to work if I ifdef the code so that Windows opens the file directly and Unix uses IPC, which is not ideal.

Also, letting the child request arbitrary amounts of storage space is bad (although this isn't the only such case); see bug 1079566.  This change wouldn't be making that problem worse (vs. directly opening the file), but it would be nice to improve it.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug1029337-profiler-ipc-fileopen-hg0.diff

Passed try after some trial-and-error (see above about Windows and iostreams): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2f7867bf9291

Also deals with a few weird things that broke because of added header inclusions combined with unified builds.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8501408 [details] [diff] [review]
bug1029337-profiler-ipc-fileopen-hg0.diff

Here's a reason not to do it this way: the file storage allocated by the child process can outlive the process itself, which is not the case with OpenAnonymousTemporaryFile, so this would be a significant increase in child process permissions.  And it's not actually necessary to do it like that.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

It's certainly possible to make the IPC protocol do something reasonable, but the problem is the communication with B2G profile.sh — it will have to recognize that the Gecko is newer than this change and send information to the parent process somehow (using the devtools protocol instead of yet more tempfile-and-signal hacks would be nice) rather than the old SIGUSR2 stuff.  But the code for that still has to stick around in the B2G repo in case it's an older Gecko.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Any suggestions about what to do with the B2G profile.sh signaling?  I'd like to not make the situation worse for any future cleanup, but I also don't want to turn this into a huge ambitious project if it doesn't need to be.

Comment 7

[Benoit Girard (:BenWa)]

Hey Jed, profile.sh and the multi-process profling method of b2g was always a hack to get profiling working on b2g ASAP. The current design is unique to b2g and conflicts with sandbox & e10s.

I believe that instead of building a hack to support sandboxing we instead build proper e10s support and have b2g use it. I suggest that we solve bug 1008435 which should fix sandboxing:

- We have the chrome process' profiler instance be the parent profiler instance. All requests are sent to that process and that process is responsible for contacting children instance over IPDL IPC.
- When a child process is started it registers observable events. When the profiler state changes it sends an observable events which sends the profiler IPC message to child instances.

Doing it this way will fix this bug, fix e10s support, will make it such that b2g doesn't have it's own unique multi-process implementation. It sounds like sandbox will already need to move things to work over IPC anyways so doing it properly might not be a lot more work and e10s is interested in supporting the profiler.

What do you think?

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Interesting.  I'd been thinking in terms of the minimum amount of work necessary to keep profile.sh working… but then profile.sh has never been a good user experience for web developers.

So maybe the place to start is desktop e10s profiling (while keeping in mind that infinite RAM isn't a safe assumption for the backend), and then make that work for a tethered B2G device, and *then* figure out what to do about profile.sh.

As for sandboxing: we're already going to need to intercept open() and proxy it to another process for some things (including closed-source drivers, so this is probably never going away), and the filename the profiler opens is predictable from the pid.  The only problem is that the child could still unilaterally open the file and write arbitrary junk to it even if profiling isn't happening, but that could be papered over (maybe a special-case check for /data/local/tmp/profiler.options).  All of which is to say that this doesn't actually need to block progress on sandboxing, so that's not a strong reason to favor a fast solution over a better one.

Comment 9

[Benoit Girard (:BenWa)]

mconley has already started the e10s profiling work. Right now I assume the profiles will be sent as a single string so we would cause memory peaks and risk OOM. But I assume we can follow up with a stream base saving but that might be a bit complicated to get working over IPC.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This should happen, but it's not on my agenda anymore.  It's also been a while since I've looked at this, or what's been happening with profiling on e10s desktop and how to adapt it for use here.

Comment 11

[Markus Stange [:mstange]]

B2G no longer exists, so profile.sh doesn't need to be fixed.

On Desktop e10s, we capture subprocess profiles using some methods on PContent and send the profiles as strings.