Closed Bug 1438678 Opened 6 years ago Closed 6 years ago

Don't send prefs via the command line

Categories

(Core :: Preferences: Backend, enhancement)

Product:
Core
Component:
Preferences: Backend
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1438678 - Pass early prefs via shared memory instead of the command line. .
59 bytes, text/x-review-board-request
bobowen
: review+
jld
: review+
glandium
: review+
Details 
Doing it via shared memory will be better:
- It doesn't bloat the command line (when viewed via `ps` or similar programs).
- Size restrictions will be less than on the command line.
Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try

Looks good on Linux, Mac, Android. Windows is all red but the jobs don't have any error lines reported. Not sure what to make of that. It works locally on my Windows machine.
(In reply to Nicholas Nethercote [:njn] from comment #3)
> Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try
> 
> Looks good on Linux, Mac, Android. Windows is all red but the jobs don't
> have any error lines reported. Not sure what to make of that. It works
> locally on my Windows machine.

[taskcluster 2018-02-16T04:49:28.757Z] Aborting task - max run time exceeded!
[taskcluster 2018-02-16T04:49:28.928Z]    Exit Code: 0
[taskcluster 2018-02-16T04:49:28.928Z] Success Code: 0x4
[taskcluster 2018-02-16T04:49:28.928Z]    User Time: 0s
[taskcluster 2018-02-16T04:49:28.928Z]  Kernel Time: 15.625ms
[taskcluster 2018-02-16T04:49:28.928Z]    Wall Time: 2h23m29.223008s
[taskcluster 2018-02-16T04:49:28.928Z]  Peak Memory: 1679360
[taskcluster 2018-02-16T04:49:28.928Z]       Result: IDLENESS_LIMIT_EXCEEDED

That happens during/after make check. That suggests something might be deadlocking.
Comment on attachment 8951514 [details]
Bug 1438678 - Pass early prefs via shared memory instead of the command line. .

https://reviewboard.mozilla.org/r/220812/#review227280

::: commit-message-3f22f:4
(Diff revision 1)
> +Bug 1438678 - Pass early prefs via a shared memory file instead of the command line. r=aklotz,jld,glandium.
> +
> +This patch replaces the large -intPrefs/-boolPrefs/-stringPrefs flags with
> +-prefsName (on Unix), -prefsHandle (on Windows), and -prefsLen (on both), which

I haven't looked too much in detail, but I'll start with a few remarks:
- From reading shared_memory.h, it seems it supports names *and* handles on unix *and* Windows. Why not use either on both systems, instead of using different code paths?
- The code handling the "decoding" of the shared memory region should be in the same file as the "encoding".
- Not being limited by what can go on the command line anymore, why keep values as strings in shared memory?
> - From reading shared_memory.h, it seems it supports names *and* handles on
> unix *and* Windows. Why not use either on both systems, instead of using
> different code paths?

I tried using names throughout, and then handles throughout, and I couldn't use either uniformaly due to the way SharedMemory::Delete() works.

- On Unix, the file exists until Delete() is called, even if there are no live references to it.

- On Windows, Delete() is a no-op, because the file is auto-deleted once its handle refcount drops to zero.

So when I tried using names on Windows, the parent would close its handle and the file would be auto-deleted before the child could open a handle. I couldn't see a way for the parent to keep a handle alive until the child has opened a handle. So instead the parent passes a duplicate handle to the child, ensuring the file stays alive until the child is done with it.

And when I tried using handles (file descriptors) on Unix, it didn't work. The child is responsible for calling Delete(), and Delete() takes a filename, and I couldn't work out a nice way to get the filename from the handle, so I stuck with passing the name.

I'm happy to put comments in about this if it helps. In general, shared_memory.h isn't as nice to use as it first looks. This Delete() inconsistency is one reason. The mixed use of std::string and std::wstring is another.


> - The code handling the "decoding" of the shared memory region should be in
> the same file as the "encoding".

It doesn't make sense to put both encoding and decoding into either ContentParent.cpp or ContentProcess.cpp. I was considering moving it into libpref as a follow-up, I guess I can do that in this bug.


> - Not being limited by what can go on the command line anymore, why keep
> values as strings in shared memory?

You think I should use binary data? I wanted this bug to focus on the shared memory mechanism and keep the encoding/decoding changes minimal. (That's also why I kept the encoding and decoding in their current locations.) Once this lands, I can do follow-ups. But note that bug 1436911 will require using pref names instead of indices into a static list of prefs, so a lot of the data will be strings anyway.
(In reply to Mike Hommey [:glandium] from comment #4)
> (In reply to Nicholas Nethercote [:njn] from comment #3)
> > Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try

Whoops, here is the correct link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3405869e6d640200029f537a6baca213873de388

> [taskcluster 2018-02-16T04:49:28.757Z] Aborting task - max run time exceeded!
> 
> That happens during/after make check. That suggests something might be
> deadlocking.

Thank you for the tip!
(In reply to Nicholas Nethercote [:njn] from comment #6)
> And when I tried using handles (file descriptors) on Unix, it didn't work.
> The child is responsible for calling Delete(), and Delete() takes a
> filename, and I couldn't work out a nice way to get the filename from the
> handle, so I stuck with passing the name.

Why should the child be responsible for calling Delete()?

> > - The code handling the "decoding" of the shared memory region should be in
> > the same file as the "encoding".
> 
> It doesn't make sense to put both encoding and decoding into either
> ContentParent.cpp or ContentProcess.cpp. I was considering moving it into
> libpref as a follow-up, I guess I can do that in this bug.

I agree libpref would be a better place for both ends. 
 
> > - Not being limited by what can go on the command line anymore, why keep
> > values as strings in shared memory?
> 
> You think I should use binary data? I wanted this bug to focus on the shared
> memory mechanism and keep the encoding/decoding changes minimal. (That's
> also why I kept the encoding and decoding in their current locations.) Once
> this lands, I can do follow-ups. But note that bug 1436911 will require
> using pref names instead of indices into a static list of prefs, so a lot of
> the data will be strings anyway.

At least for numeric value, it seems weird to pass them as strings and parse them with strtol. But I'm fine calling this scope bloat, and it does make things easier to review that the changes are limited.
> Why should the child be responsible for calling Delete()?

The child uses the data and knows when it has finished with it. The parent doesn't know when the child has finished with the data. So it makes sense for the child to be responsible for destroying it. (It's like a C++ move constructor, more or less.)

Unless I'm overlooking something?
(In reply to Nicholas Nethercote [:njn] from comment #9)
> > Why should the child be responsible for calling Delete()?
> 
> The child uses the data and knows when it has finished with it. The parent
> doesn't know when the child has finished with the data. So it makes sense
> for the child to be responsible for destroying it. (It's like a C++ move
> constructor, more or less.)
> 
> Unless I'm overlooking something?

If you share the file via "handle" (aka file descriptor on unix), the file doesn't need to exist on the file system. You only need it to exist when you open it (and even that is not true anymore on Linux, where there is now O_TMPFILE), so you can delete it straight away and then pass the file descriptor around.
> you can delete it straight away and then pass the file descriptor around.

Ok, I'll try that. Thanks!
(In reply to Mike Hommey [:glandium] from comment #10)
> If you share the file via "handle" (aka file descriptor on unix), the file
> doesn't need to exist on the file system. You only need it to exist when you
> open it (and even that is not true anymore on Linux, where there is now
> O_TMPFILE), so you can delete it straight away and then pass the file
> descriptor around.

Or pass an empty name to SharedMemory::Create, in which case it's deleted immediately: https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/ipc/chromium/src/base/shared_memory_posix.cc#154-164

I'd strongly suggest not using named shared memory, because nothing else in the tree uses it and I'd like to remove it.  In general it's a problem for sandboxing: we don't want a process to be able to access shared memory unless that specific process is explicitly given access, and we already have fd/handle passing which does exactly that.  (Also, the less Chromium IPC code we have, the easier it is to try to clean up what remains.)  See also bug 1439057.
> Or pass an empty name to SharedMemory::Create, in which case it's deleted
> immediately:
> https://searchfox.org/mozilla-central/rev/
> 47cb352984bac15c476dcd75f8360f902673cb98/ipc/chromium/src/base/
> shared_memory_posix.cc#154-164

Good suggestion, I'll try that.

(This demonstrates another way that shared_memory.h isn't nice to use: you pretty much have to read shared_memory_{posix,win}.cc to use it effectively.)
I've updated the patches according to the suggestions:

- The shared memory is now anonymous because I'm passing an empty name string to SharedMemory::Create(). On Unix this also means that the shared memory file is unlinked immediately after creation, so no Delete() calls are necessary.

- On Unix, I'm now using handles (file descriptors). I don't need to pass the handle because it's hard-coded to 8. (Other IPC fds are similarly hard-coded.)

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a85c9972984669653e9c37f22ef9cc9d24a941bc

Again, Mac and Linux look good, while Windows has those timeouts. Still not sure about them.

Android has some assertion failures in LaunchAndroidService(). Line numbers aren't shown, but I think the assertion right at the top is failing because fds_to_remap.size() is too large. It's weird that only a few tests have problems; I would have thought any problems would manifest more reliably. I see two options:

- Duplicate all the "add an fd" stuff from https://hg.mozilla.org/mozilla-central/rev/bcc0a91dd43f.

- Don't use a hard-coded fd and fds_to_remap. Instead use dup() (rather than dup2()) and pass the resulting fd to the child via -prefsHandle, which Windows needs anyway. Or will the sandbox code close that fd in the child?

jld, any suggestions about the Android stuff? Thanks.

(BTW, does Android even use content processes? My understanding of the Android process model is poor.)
Flags: needinfo?(jld)
(In reply to Nicholas Nethercote [:njn] from comment #16)
> - Don't use a hard-coded fd and fds_to_remap. Instead use dup() (rather than
> dup2()) and pass the resulting fd to the child via -prefsHandle, which
> Windows needs anyway. Or will the sandbox code close that fd in the child?

Normally, the IPC code closes any fds that aren't explicitly mapped (and aren't stdin/out/err); this is necessary because, in general, trying to use the close-on-exec flag to do the same thing introduces a race condition where fds could be unintentionally leaked into the child.

But Android does something entirely different[1] involving JNI and Android-specific things.  And XML[2] that describes a “service” being invoked via RPC, or something like that.  The file descriptors seem to eventually wind up as part of a Parcel[3] being passed over Binder IPC, rather than left open across execve().

[1] https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/ipc/glue/GeckoChildProcessHost.cpp#806-811
[2] https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/mobile/android/geckoview/src/main/AndroidManifest.xml#57-63
[3] https://developer.android.com/reference/android/os/Parcel.html

> (BTW, does Android even use content processes? My understanding of the
> Android process model is poor.)

Firefox for Android doesn't, as I understand it (although it might use plugins?), but GeckoView does.  GeckoView is a project for embedding Gecko in Android apps, it *does* use e10s, and we might wind up using it in a browser product at some point in the future.

So… either duplicate the “add an fd” stuff (and see also bug 1440034), or volunteer to do some complicated cross-platform refactoring to how these things are handled.  The former is probably the more practical approach.
Flags: needinfo?(jld)
Attachment #8951514 - Attachment is obsolete: true
Attachment #8951514 - Flags: review?(mh+mozilla)
Attachment #8951514 - Flags: review?(jld)
Attachment #8951514 - Flags: review?(aklotz)
This latest version now works on all tier 1 platforms. I also moved the serialization/deserialization code into libpref, as glandium requested in comment 5. (It's still strings rather than binary data, though.)
Comment on attachment 8951513 [details]
Bug 1438678 - Pass early prefs via shared memory instead of the command line. .

https://reviewboard.mozilla.org/r/220810/#review229428

If you can, it would probably be better to separate the IPC changes from the rest.

::: commit-message-5adf3:33
(Diff revision 4)
> +    inheritable handles. We also ensure that list is processed on all paths
> +    (MOZ_SANDBOX with sandbox, MOZ_SANDBOX without sandbox, non-MOZ_SANDBOX) so
> +    that the handles are marked as inheritable.
> +
> +  - Android: like Linux and Mac, but the handles get passed via "parcels", so
> +    we the -prefsHandle value isn't correct and we have to use the new

"so we the"?

::: dom/ipc/ContentParent.cpp:2030
(Diff revision 4)
>  
> -  nsCString schedulerPrefs = Scheduler::GetPrefs();
> -
> -  // Only do these ones if they're non-empty.
> -  if (!intPrefs.IsEmpty()) {
> -    extraArgs.push_back("-intPrefs");
> +  // Finish the serialization by combining the strings in the shared memory.
> +  Preferences::SerializeEarlyPreferences2(
> +    boolPrefs, intPrefs, stringPrefs, static_cast<char*>(shm.memory()));
> +
> +#ifndef MOZ_WIDGET_ANDROID

s/MOZ_WIDGET_ANDROID/ANDROID/ here and everywhere you use MOZ_WIDGET_ANDROID.

::: dom/ipc/ContentProcess.cpp:154
(Diff revision 4)
>        foundIsForBrowser = true;
> -    } else if (!strcmp(aArgv[idx], "-intPrefs")) {
> +#ifndef MOZ_WIDGET_ANDROID
> +    } else if (!strcmp(aArgv[idx], "-prefsHandle")) {
>        char* str = aArgv[idx + 1];
> -      while (*str) {
> -        int32_t index = strtol(str, &str, 10);
> +      MOZ_ASSERT(str[0] != '\0');
> +      uintptr_t h = strtoll(str, &str, 10);

fwiw, strtoll returns a long long, which is larger than uintptr_t on 32-bit platforms.

::: dom/ipc/ContentProcess.cpp:155
(Diff revision 4)
> -        MOZ_ASSERT(str[0] == ':');
> -        str++;
> -        MaybePrefValue value(PrefValue(static_cast<int32_t>(strtol(str, &str, 10))));
> -        MOZ_ASSERT(str[0] == '|');
> -        str++;
> -        // XXX: we assume these values as default values, which may not be
> -        // true. We also assume they are unlocked. Fortunately, these prefs
> +#if defined(XP_UNIX)
> +      prefsHandle = base::FileDescriptor(h, /* auto_close */ true);
> +#elif defined(XP_WIN)
> +      prefsHandle = reinterpret_cast<HANDLE>(h);
> +#else
> +# error "Unsupported platform"
> +#endif

IIRC, XP_WIN and XP_UNIX are mutually exclusive, making the #else effectively dead code. Also, I'm pretty sure that we have plenty of code around that already assume they can do #ifdef XP_WIN/#else

::: dom/ipc/ContentProcess.cpp:168
(Diff revision 4)
> -      }
> -      foundBoolPrefs = true;
> -    } else if (!strcmp(aArgv[idx], "-stringPrefs")) {
>        char* str = aArgv[idx + 1];
> -      while (*str) {
> -        int32_t index = strtol(str, &str, 10);
> +      MOZ_ASSERT(str[0] != '\0');
> +      prefsLen = strtol(str, &str, 10);

fwiw, strtol returns a long, which is smaller than a size_t.

::: modules/libpref/Preferences.cpp:3067
(Diff revision 4)
> +    }
> +  }
> +
> +  // +2 for two newlines and +1 for the terminating null (see
> +  // WriteEarlyPreferences()).
> +  return aBoolPrefs.Length() + aIntPrefs.Length() + aStringPrefs.Length() + 3;

It seems to me you don't need to use 3 buffers, here, and could just as well use 1, which would make SerializeEarlyPreferences2 only do a memcpy from that buffer to the shared memory, meaning it wouldn't even need to be a prefs-api method.
Attachment #8951513 - Flags: review?(mh+mozilla)
> > +  // +2 for two newlines and +1 for the terminating null (see
> > +  // WriteEarlyPreferences()).
> > +  return aBoolPrefs.Length() + aIntPrefs.Length() + aStringPrefs.Length() + 3;
> 
> It seems to me you don't need to use 3 buffers, here, and could just as well
> use 1, which would make SerializeEarlyPreferences2 only do a memcpy from
> that buffer to the shared memory, meaning it wouldn't even need to be a
> prefs-api method.

With the current code it would be possible to gather the three strings and then combine them all within libpref, but it would require extra memory.

But in bug 1436911 I'm overhauling that part so there will only be a single string anyway. So I'll defer that change to that bug to minimize the changes here.
(In reply to Nicholas Nethercote [:njn] from comment #23)
> > > +  // +2 for two newlines and +1 for the terminating null (see
> > > +  // WriteEarlyPreferences()).
> > > +  return aBoolPrefs.Length() + aIntPrefs.Length() + aStringPrefs.Length() + 3;
> > 
> > It seems to me you don't need to use 3 buffers, here, and could just as well
> > use 1, which would make SerializeEarlyPreferences2 only do a memcpy from
> > that buffer to the shared memory, meaning it wouldn't even need to be a
> > prefs-api method.
> 
> With the current code it would be possible to gather the three strings and
> then combine them all within libpref, but it would require extra memory.

I'm not following how this would require extra memory. You're allocating 3 nsCStrings with all the content you then copy one nsCString at a time into shared memory. I'm saying you could/should just use 1, which would use the same amount of memory (+ the \n's), while simplifying the API (and not requiring the artificial split in two numbered methods).
Comment on attachment 8951513 [details]
Bug 1438678 - Pass early prefs via shared memory instead of the command line. .

https://reviewboard.mozilla.org/r/220810/#review229390

This mostly looks good, but I'm concerned about the fd leak.

::: dom/ipc/ContentParent.cpp
(Diff revision 3)
> -      boolPrefs.Append(nsPrintfCString("%u:%d|", i, Preferences::GetBool(prefName)));
> -      break;
> -    case nsIPrefBranch::PREF_STRING: {
> -      nsAutoCString value;
> -      Preferences::GetCString(prefName, value);
> -      stringPrefs.Append(nsPrintfCString("%u:%d;%s|", i, value.Length(), value.get()));

Always good to see less subsystem-specific code inline in ContentParent/ContentProcess etc.

::: dom/ipc/ContentParent.cpp:2043
(Diff revision 3)
> -    extraArgs.push_back("-stringPrefs");
> -    extraArgs.push_back(stringPrefs.get());
> +  // identify mapping. That works on Linux but not on Mac, for unclear reasons
> +  // (see bug 1440207).
> +  //
> +  // Note: on Android, AddFdToRemap() sets up the fd to be passed via a Parcel,
> +  // and the -prefsHandle argument isn't used.
> +  base::SharedMemoryHandle newHandle;

This will leak a file descriptor, unless I'm missing something.  `base::FileDescriptor` is not a RAII class / has no destructor; the `auto_close` member is, confusingly, just a suggestion.

But I think you don't even need to dup this — the value is arbitrary and it just needs to (1) not collide with any of the other dstFds and (2) not be the same as `shm.handle().fd` because of whatever's going on on Mac.  (And it's not used at all on Android, it looks like?)

You could avoid the Mac problem by using `shm.handle().fd + 1`, but that's not a guarantee it won't collide with one of the hard-coded low fds.  For that matter, neither is `dup`ing it.  (Although, by the same token, there's no guarantee that the Mac thing isn't occasionally breaking on release right now, because srcFd and dstFd happen to match.  Sigh.)

Personally I'd just hard-code `8`, pass that as `-prefsHandle` even though it's constant because the code for parsing it is already there, and add yet another FIXME comment pointing to bug 1440207.

::: dom/ipc/ContentProcess.cpp:154
(Diff revision 3)
>        foundIsForBrowser = true;
> -    } else if (!strcmp(aArgv[idx], "-intPrefs")) {
> +#ifndef MOZ_WIDGET_ANDROID
> +    } else if (!strcmp(aArgv[idx], "-prefsHandle")) {
>        char* str = aArgv[idx + 1];
> -      while (*str) {
> -        int32_t index = strtol(str, &str, 10);
> +      MOZ_ASSERT(str[0] != '\0');
> +      uintptr_t h = strtoll(str, &str, 10);

Shouldn't this be `strtoull`?  Otherwise 64-bit `HANDLE`s with the high bit set might not work, if I understand correctly.

::: ipc/glue/GeckoChildProcessHost.cpp:1046
(Diff revision 3)
> +  // Mark the handles to inherit as inheritable.
> +  for (HANDLE h : mLaunchOptions->handles_to_inherit) {
> +# if defined(MOZ_SANDBOX)
> +    mSandboxBroker.AddHandleToShare(h);
> +# else
> +    SetHandleInformation(h, HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT);

This seems like as good a way as any to handle this.

::: ipc/glue/GeckoChildProcessHost.cpp:1199
(Diff revision 3)
>    jni::ObjectArray::LocalRef jargs = jni::ObjectArray::New<jni::String>(argvSize);
>    for (int ix = 0; ix < argvSize; ix++) {
>      jargs->SetElement(ix, jni::StringParam(argv[ix].c_str(), env));
>    }
> -  base::file_handle_mapping_vector::const_iterator it = fds_to_remap.begin();
> -  int32_t ipcFd = it->first;
> +
> +  // XXX: this processing depends entirely on the internals of

This comment could point to bug 1440207.

::: modules/libpref/Preferences.cpp:3060
(Diff revision 3)
> +          nsPrintfCString("%u:%d;%s|", i, value.Length(), value.get()));
> +      } break;
> +      case PrefType::None:
> +        break;
> +      default:
> +        printf("preference type: %d\n", int(pref->Type()));

Nit: `printf_stderr`?
Attachment #8951513 - Flags: review?(jld)
(In reply to Mike Hommey [:glandium] from comment #22)
> > +      uintptr_t h = strtoll(str, &str, 10);
> 
> fwiw, strtoll returns a long long, which is larger than uintptr_t on 32-bit
> platforms.

That should be fine, I think?  The string will always be the representation of a uintptr_t; the problem would be on Win64, where signed long long *isn't* a superset of uintptr_t.

> > +      prefsLen = strtol(str, &str, 10);
> 
> fwiw, strtol returns a long, which is smaller than a size_t.

I noticed this too, but hopefully the size of the early prefs fits in a long?  On the other hand it doesn't really hurt anything to use strtoull here too.
> I'm not following how this would require extra memory. You're allocating 3
> nsCStrings with all the content you then copy one nsCString at a time into
> shared memory. I'm saying you could/should just use 1, which would use the
> same amount of memory (+ the \n's), while simplifying the API (and not
> requiring the artificial split in two numbered methods).

With the current code the bool prefs all need to be serialized together, then the int prefs together, then the string prefs together. Either we build them separately and then combine them (more memory) or we do three passes over the table. Or we wait for the follow-up bug.
Comment on attachment 8951513 [details]
Bug 1438678 - Pass early prefs via shared memory instead of the command line. .

https://reviewboard.mozilla.org/r/220810/#review229572

::: commit-message-5adf3:27
(Diff revision 4)
> +    in combination with the new GeckoChildProcessHost::AddFdToRemap() function
> +    (which ensures the child won't close the fd).
> +
> +  - Windows: there is no need to duplicate the handle because Windows handles
> +    are system-wide. But we do use the new
> +    GeckoChildProcessHost::AddFdToRemap() function to add it to the list of

AddHandleToShare()

::: dom/ipc/ContentParent.cpp:2054
(Diff revision 4)
> +  mSubprocess->AddFdToRemap(shm.handle().fd, newHandle.fd);
> +#ifndef MOZ_WIDGET_ANDROID
> +  prefsHandle = newHandle.fd;
> +#endif
> +#elif defined(XP_WIN)
> +  // Mark the handle as shareable, and pass it via the command line.

nit: maybe we should just drop this comment, this AddHandleToShare doesn't currently mark the handle as shareable and the fact that is is passed via the command line is implementation detail that this code doesn't necessarily care about.

::: ipc/glue/GeckoChildProcessHost.cpp:1044
(Diff revision 4)
>    cmdLine.AppendLooseValue(UTF8ToWide(childProcessType));
>  
> +  // Mark the handles to inherit as inheritable.
> +  for (HANDLE h : mLaunchOptions->handles_to_inherit) {
> +# if defined(MOZ_SANDBOX)
> +    mSandboxBroker.AddHandleToShare(h);

I wonder if just moving LaunchOptions into its own header would make it possible to pass that into SandboxBroker::LaunchApp, anyway I've also lost enough time on chasing those sorts of conflicts to not want you to waste more time here.

I do think that the loop and AddHandleToShare calls should move into the |if (shouldSandboxCurrentProcess) {|.

The SetHandleInformation call should move to before [1] or maybe to GeckoChildProcessHost::AddHandleToShare, although that would mean we do it twice in the sandboxed case. We should probably check or explicitly ignore the return value as well.

[1] https://hg.mozilla.org/mozilla-central/file/b184be598740/ipc/chromium/src/base/process_util_win.cc#l357
(In reply to Bob Owen (:bobowen) from comment #28)
> I wonder if just moving LaunchOptions into its own header would make it
> possible to pass that into SandboxBroker::LaunchApp, anyway I've also lost
> enough time on chasing those sorts of conflicts to not want you to waste
> more time here.

For what it's worth, breaking up process_util.h more wouldn't hurt — a lot of the tree indirectly includes it via IPC stuff just to get the pid/handle types, so touching the process launch parts causes a lot of unnecessary rebuilding.
> I do think that the loop and AddHandleToShare calls should move into the |if
> (shouldSandboxCurrentProcess) {|.
> 
> The SetHandleInformation call should move to before [1] or maybe to
> GeckoChildProcessHost::AddHandleToShare, although that would mean we do it
> twice in the sandboxed case.

The thing I like about the current structure is that the inheritability processing is in a single spot. If I take your suggestion, that processing is done for the sandboxed case in PerformAsyncLaunchInternal() (prior to calling SandboxBroker::LaunchApp()) and done for the non-sandboxed case in base::LaunchApp().

> We should probably check or explicitly ignore the return value as well.

Sure.
Changes in the latest version:

- On Linux/Mac, it no longer dups the fd, instead hardcoding 8 as the dest fd.
  This means that -prefsHandle is not used on any Unix platform.

- Serialization is now done in a single step. It uses more intermediate memory
  that way, but bug will fix that 1436911.
  
- I'm using strtoull() for both -prefsHandle and -prefsLen arguments. There's a
  comment explaining why it should be ok.
  
- Various other minor fixes done, as suggested.

- Bob: I didn't move the handle processing code as you suggested, for the
  reasons explained in comment 30. But I can do it if you still feel strongly 
  about it.
(In reply to Nicholas Nethercote [:njn] from comment #30)
> > I do think that the loop and AddHandleToShare calls should move into the |if
> > (shouldSandboxCurrentProcess) {|.
> > 
> > The SetHandleInformation call should move to before [1] or maybe to
> > GeckoChildProcessHost::AddHandleToShare, although that would mean we do it
> > twice in the sandboxed case.
> 
> The thing I like about the current structure is that the inheritability
> processing is in a single spot. If I take your suggestion, that processing
> is done for the sandboxed case in PerformAsyncLaunchInternal() (prior to
> calling SandboxBroker::LaunchApp()) and done for the non-sandboxed case in
> base::LaunchApp().

That is one plus point, but actually I think it's better to have that internal to the API.
Then if you want to share to a sandboxed process you have to call SandboxBroker::AddHandleToShare and it makes sure it's shareable. If you want to share to a non-sandboxed process you have to pass in the launch options and it will make sure they're shareable. Probably unlikely, but if we decided to use these two |LaunchApp|s elsewhere the caller would need to know that in the one case you definitely need to make the handle shareable yourself in the other you don't.

Also, given the usual case of MOZ_SANDBOX defined, having those extra calls to the broker in the non-sandboxed case is unnecessary and in the sandboxed case the SetHandleInformation is unnecessary. I think that adds to the confusion of the code paths here.
 
> > We should probably check or explicitly ignore the return value as well.
> 
> Sure.

Perhaps a diagnostic assert would be good on that, I suspect it should never actually fail, but if it does it will be easier to track down the potentially non-usable browser if we crash on Nightly.
> I think it's better to have that internal to the API.
> Then if you want to share to a sandboxed process you have to call
> SandboxBroker::AddHandleToShare and it makes sure it's shareable. If you
> want to share to a non-sandboxed process you have to pass in the launch
> options and it will make sure they're shareable. Probably unlikely, but if
> we decided to use these two |LaunchApp|s elsewhere the caller would need to
> know that in the one case you definitely need to make the handle shareable
> yourself in the other you don't.

Sorry, I'm now confused. Are you saying that the sandbox case should require explicit handle processing, while the non-sandbox case should do handle processing itself? If so, that inconsistency seems odd to me.

Another possibility is to move the handle processing inside each of SandboxBroker::Launch() and base::Launch(). It would require passing handles_to_inherit to SandboxBroker::Launch(), but otherwise it feels sensible to me.
Flags: needinfo?(bobowencode)
Comment on attachment 8951513 [details]
Bug 1438678 - Pass early prefs via shared memory instead of the command line. .

https://reviewboard.mozilla.org/r/220810/#review230546

I'll leave the IPC details to jld and bobowen.
Attachment #8951513 - Flags: review?(mh+mozilla) → review+
(In reply to Nicholas Nethercote [:njn] from comment #34)
> > I think it's better to have that internal to the API.
> > Then if you want to share to a sandboxed process you have to call
> > SandboxBroker::AddHandleToShare and it makes sure it's shareable. If you
> > want to share to a non-sandboxed process you have to pass in the launch
> > options and it will make sure they're shareable. Probably unlikely, but if
> > we decided to use these two |LaunchApp|s elsewhere the caller would need to
> > know that in the one case you definitely need to make the handle shareable
> > yourself in the other you don't.
> 
> Sorry, I'm now confused. Are you saying that the sandbox case should require
> explicit handle processing, while the non-sandbox case should do handle
> processing itself? If so, that inconsistency seems odd to me.

No, the sandboxed case already sets the handle as shareable here [1], called through SandboxBroker::AddHandleToShare.
So, I'm suggesting the non-sandboxed case base::LaunchApp should do the same when it runs through the vector that is passed to it.

[1] https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/security/sandbox/chromium/sandbox/win/src/sandbox_policy_base.cc#412
Flags: needinfo?(bobowencode)
Comment on attachment 8951513 [details]
Bug 1438678 - Pass early prefs via shared memory instead of the command line. .

https://reviewboard.mozilla.org/r/220810/#review231510
Attachment #8951513 - Flags: review?(jld) → review+
Comment on attachment 8951513 [details]
Bug 1438678 - Pass early prefs via shared memory instead of the command line. .

https://reviewboard.mozilla.org/r/220810/#review231526
Attachment #8951513 - Flags: review?(bobowencode) → review+
Thank you all for the reviews. This patch is in a lot better shape as a result of all your comments.

I'm going to wait until the 61 dev cycle begins (March 13) to land this, because it's reasonably risky.
https://hg.mozilla.org/integration/mozilla-inbound/rev/32d6774930e55be5c03e8d631fc067a995623c1e
Bug 1438678 - Pass early prefs via shared memory instead of the command line. r=bobowen,jld,glandium.
https://hg.mozilla.org/mozilla-central/rev/32d6774930e5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Blocks: 1451986
Depends on: 1481139
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: