1477561 - Improve handling of shared memory preference data when spawning recording processes

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Currently, shared memory preference data from the UI process is conveyed to a recording process via a fair amount of glue when the middleman starts that recording process.  After a recent rebase there is an additional block of preferences data that is now conveyed to child processes, which also needs handling.

Rather than making the record/replay glue even more complicated, this patch removes all the associated instrumentation and glue, and simply maps the file descriptors in the middleman so that the recording process can access them.  The existing redirections used to record/replay mmap'ed file descriptor contents are sufficient to take care of recording/replaying this data.

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 8994028 [details] [diff] [review]
patch

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

It is always nice to see things get simpler.

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8994028 [details] [diff] [review]
patch

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

::: toolkit/recordreplay/ipc/ChildProcess.cpp
@@ +465,5 @@
>      MOZ_RELEASE_ASSERT(!gRecordingProcess);
>      gRecordingProcess = new ipc::GeckoChildProcessHost(GeckoProcessType_Content);
> +
> +    gRecordingProcess->AddFdToRemap(kPrefsFileDescriptor, kPrefsFileDescriptor);
> +    gRecordingProcess->AddFdToRemap(kPrefMapFileDescriptor, kPrefMapFileDescriptor);

We clone this FD and close the original immediately at child process startup. The IPC FileDescriptor stuff is weird enough that we wind up cloning it again soon afterwards, and that probably winds up reusing the original descriptor that we closed, but that isn't something we should rely on.

You'll probably need to call Preferences::EnsureSnapshot again, and move the IsParentProcess assertion inside the `if (!gSharedMap)`

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Explicitly pass pref handles around when spawning recording processes.

As far as I can tell, with the existing patch the middleman is still using the original file descriptors when spawning the recording process --- this is done synchronously under the InitializeMiddleman() call made by ContentProcess::Init(), before the descriptors have been closed.  It sure is opaque and fragile, though.  This supplemental patch explicitly passes the handles around to make the code clearer and get some assurance the handles won't be closed before they can be passed down to the recording process.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Explicitly pass pref handles around when spawning recording processes.

Sorry, the patch I just attached has some extraneous changes.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8994374 [details] [diff] [review]
Part 2 - Explicitly pass pref handles around when spawning recording processes.

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

Thanks

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc6db674236
Improve handling of shared memory preference data when spawning recording processes, r=mccr8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c47a0754222c
Part 2 - Explicitly pass pref handles around when spawning recording processes, r=kmag.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5dc6db674236
https://hg.mozilla.org/mozilla-central/rev/c47a0754222c