590057 - [Mac OS X] properly select architecture for child process

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

We're planning to support using i386 plugins from an x86_64 host. The host will need to decide what architecture its child plugin process should be and launch it properly.

Comment 1

[Josh Aas]

Attachment: fix v0.8

Comment 2

[Josh Aas]

Attachment: fix v0.9

Comment 3

[Josh Aas]

Attachment: fix v0.95

Linux build fixes.

Comment 4

[Josh Aas]

Attachment: fix v1.0

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 474335 [details] [diff] [review]
fix v1.0

I think all the changes to chromium code should be wrapped around '#if defined(CHROMIUM_MOZILLA_BUILD)'. Perhaps cjones can clarify the rules for that.

+    if (posix_spawnattr_init(&spawnattr) == 0 && spawnattr) {

This doesn't have a corresponding posix_spawnattr_destroy method. If it's not needed then perhaps a comment should clear it up. Also we should prefix '::' for system calls.

Comment 6

[Josh Aas]

(In reply to comment #5)
> Comment on attachment 474335 [details] [diff] [review]
> fix v1.0
> 
> I think all the changes to chromium code should be wrapped around '#if
> defined(CHROMIUM_MOZILLA_BUILD)'. Perhaps cjones can clarify the rules for
> that.

I think that effort has been recognized as futile at this point and it is probably a waste of time, but if people still care they can speak up and I'll add it back.

> +    if (posix_spawnattr_init(&spawnattr) == 0 && spawnattr) {
> 
> This doesn't have a corresponding posix_spawnattr_destroy method. 

I'll look into it.

> Also we should prefix '::' for system calls.

We only do that for Apple framework calls, not for posix-ish/unixy calls. I think the idea behind the prefix was to make it easier to spot calls that non-Apple developers might not recognize (whereas we'd expect developers to recognize the more common and portable posix/unix calls).

Comment 7

[Josh Aas]

Attachment: fix v1.1

Comment 8

[Josh Aas]

Attachment: fix v1.2

This includes a fix that allows us to compile on PPC for now. Easiest to just add some selection logic for PPC even though we won't use it.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 474782 [details] [diff] [review]
fix v1.2

>diff --git a/dom/plugins/PluginProcessParent.cpp b/dom/plugins/PluginProcessParent.cpp
>--- a/dom/plugins/PluginProcessParent.cpp
>+++ b/dom/plugins/PluginProcessParent.cpp
>@@ -35,16 +35,18 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "mozilla/plugins/PluginProcessParent.h"
> 
> #include "base/string_util.h"
>+#include "base/process_util.h"
>+
> #include "mozilla/ipc/BrowserProcessSubThread.h"
> #include "mozilla/plugins/PluginMessageUtils.h"
> 
> using std::vector;
> using std::string;
> 
> using mozilla::ipc::BrowserProcessSubThread;
> using mozilla::ipc::GeckoChildProcessHost;
>@@ -65,19 +67,67 @@ PluginProcessParent::PluginProcessParent
> 
> PluginProcessParent::~PluginProcessParent()
> {
> }
> 
> bool
> PluginProcessParent::Launch(PRInt32 timeoutMs)
> {
>+    base::ProcessArchitecture arch = base::PROCESS_ARCH_DEFAULT;
>+
>+#ifdef XP_MACOSX

Instead of making this code Mac-only, we should define no-op stubs
that always return DEFAULT.

>+    // Get the plugin architecture mask.
>+    uint32 pluginArchitectures = 0;
>+    nsresult rv = mozilla::ipc::GeckoChildProcessHost::GetArchitecturesForBinary(mPluginFilePath.c_str(), &pluginArchitectures);

|using mozilla::ipc::GeckoChildProcessHost;|.  I'd be more specific
here with the name |pluginLibArches| or something.  See comments for
GeckoChildProcessHost.

>+#if defined(__i386__)
>+    base::ProcessArchitecture parentArch = base::PROCESS_ARCH_I386;
>+#elif defined(__x86_64__)
>+    base::ProcessArchitecture parentArch = base::PROCESS_ARCH_X86_64;
>+#elif defined(__PPC__)
>+    base::ProcessArchitecture parentArch = base::PROCESS_ARCH_PPC;
>+#else
>+#error "You are building on an unrecognized architecture"
>+#endif
>+

|using base::ProcessArchitecture;|.

Please make this a file-static helper |CurrentProcessArchitecture()|.
Really this should be a global helper, but we don't have a great place
to stick it at the moment.

I'd use |curProcArch| instead of |parentArch|.  The latter will be
confusing in content processes.

The style

  ProcessArchitecture curProcArch;
#if FOO
  pluginHostArch = ARCH_FOO;
#...

is easier to maintain.

Please use ARCH_CPU_X86_64/ARCH_CPU_X86/ARCH_CPU_PPC here.

>+    // Prefer the parent architecture.
>+    if (pluginArchitectures & parentArch) {
>+        arch = parentArch;
>+    }

Instead of |arch|, use |pluginContainerArch| or something like that.

>+    else {
>+        // Get host architecture mask.
>+        uint32 hostArchitectures = 0;

|= PROCESS_ARCH_DEFAULT;|

"Host" is pretty overloaded.  Maybe use |pluginContainerArches|
instead?

>diff --git a/ipc/chromium/src/base/process_util_mac.mm b/ipc/chromium/src/base/process_util_mac.mm
>--- a/ipc/chromium/src/base/process_util_mac.mm
>+++ b/ipc/chromium/src/base/process_util_mac.mm
>@@ -17,27 +17,36 @@
> #include "base/eintr_wrapper.h"
> #include "base/logging.h"
> #include "base/rand_util.h"
> #include "base/string_util.h"
> #include "base/time.h"
> 
> namespace base {
> 
>+void FreeEnvVarsArray(char** array, int length)

|char* array[]|

> 
>+  // Set up the CPU preference array.
>+  cpu_type_t cpu_types[1];
>+  switch (arch) {
>+    case PROCESS_ARCH_I386:
>+      cpu_types[0] = CPU_TYPE_X86;
>+      break;
>+    case PROCESS_ARCH_X86_64:
>+      cpu_types[0] = CPU_TYPE_X86_64;
>+      break;
>+    case PROCESS_ARCH_PPC:
>+      cpu_types[0] = CPU_TYPE_POWERPC;
>+    default:
>+      cpu_types[0] = CPU_TYPE_ANY;
>+      break;
>+  }
>+
>+  // Initialize spawn attributes.
>+  posix_spawnattr_t spawnattr;
>+  if (posix_spawnattr_init(&spawnattr) != 0) {
>+    FreeEnvVarsArray(vars, varsLen);
>+    return false;
>+  }
>+
>+  // Set spawn attributes.
>+  size_t attr_count = 1;
>+  size_t attr_ocount = 0;
>+  if (posix_spawnattr_setbinpref_np(&spawnattr, attr_count, cpu_types, &attr_ocount) != 0 ||
>+      attr_ocount != attr_count) {
>+    FreeEnvVarsArray(vars, varsLen);
>+    posix_spawnattr_destroy(&spawnattr);
>+    return false;
>+  }
>+

I don't feel qualified to review this, but it looks OK AFAICT.

>diff --git a/ipc/glue/GeckoChildProcessHost.cpp b/ipc/glue/GeckoChildProcessHost.cpp
>--- a/ipc/glue/GeckoChildProcessHost.cpp
>+++ b/ipc/glue/GeckoChildProcessHost.cpp

We're accumulating enough platform-specific code in here that I think
it's time to start splitting it into separate files.  We only need
GeckoChildProcessHost_mac.cpp so far.

>@@ -41,16 +41,17 @@
> #include "base/command_line.h"
> #include "base/path_service.h"
> #include "base/string_util.h"
> #include "chrome/common/chrome_switches.h"
> #include "chrome/common/process_watcher.h"
> #ifdef XP_MACOSX
> #include "chrome/common/mach_ipc_mac.h"
> #include "base/rand_util.h"
>+#include "nsILocalFileMac.h"
> #endif
> 
> #include "prprf.h"
> 
> #if defined(OS_LINUX)
> #  define XP_LINUX 1
> #endif
> #include "nsExceptionHandler.h"
>@@ -114,16 +115,121 @@ GeckoChildProcessHost::~GeckoChildProces
>     );
> 
> #if defined(XP_MACOSX)
>   if (mChildTask != MACH_PORT_NULL)
>     mach_port_deallocate(mach_task_self(), mChildTask);
> #endif
> }
> 
>+#ifdef XP_MACOSX
>+class AutoCFTypeObject {
>+public:
>+  AutoCFTypeObject(CFTypeRef object)
>+  {
>+    mObject = object;
>+  }
>+  ~AutoCFTypeObject()
>+  {
>+    ::CFRelease(mObject);
>+  }
>+private:
>+  CFTypeRef mObject;
>+};
>+
>+nsresult GeckoChildProcessHost::GetArchitecturesForBinary(const char *path, uint32 *result)
>+{
>+  *result = 0;
>+
>+  CFURLRef url = ::CFURLCreateFromFileSystemRepresentation(kCFAllocatorDefault,
>+                                                           (const UInt8*)path,
>+                                                           strlen(path),
>+                                                           false);
>+  if (!url) {
>+    return NS_ERROR_FAILURE;
>+  }
>+  AutoCFTypeObject autoPluginContainerURL(url);
>+
>+  CFArrayRef pluginContainerArchs = ::CFBundleCopyExecutableArchitecturesForURL(url);
>+  if (!pluginContainerArchs) {
>+    return NS_ERROR_FAILURE;
>+  }
>+  AutoCFTypeObject autoPluginContainerArchs(pluginContainerArchs);
>+
>+  CFIndex pluginArchCount = ::CFArrayGetCount(pluginContainerArchs);
>+  for (CFIndex i = 0; i < pluginArchCount; i++) {
>+    CFNumberRef currentArch = static_cast<CFNumberRef>(::CFArrayGetValueAtIndex(pluginContainerArchs, i));
>+    int currentArchInt = 0;
>+    if (!::CFNumberGetValue(currentArch, kCFNumberIntType, &currentArchInt)) {
>+      continue;
>+    }
>+    switch (currentArchInt) {
>+      case kCFBundleExecutableArchitectureI386:
>+        *result |= base::PROCESS_ARCH_I386;
>+        break;
>+      case kCFBundleExecutableArchitectureX86_64:
>+        *result |= base::PROCESS_ARCH_X86_64;
>+      case kCFBundleExecutableArchitecturePPC:
>+        *result |= base::PROCESS_ARCH_PPC;
>+        break;
>+      default:
>+        break;
>+    }
>+  }
>+
>+  return NS_OK;
>+}
>+
>+nsresult GeckoChildProcessHost::GetChildBinaryArchitectures(uint32 *result)
>+{
>+  nsresult rv = NS_OK;
>+
>+  // Cache this, it shouldn't ever change.

Let's go even further and bake this in at compile time.

>+}
>+#endif
>+

Please move these new functions into GeckoChildProcessHost_mac.cpp.

>diff --git a/ipc/glue/GeckoChildProcessHost.h b/ipc/glue/GeckoChildProcessHost.h
>--- a/ipc/glue/GeckoChildProcessHost.h
>+++ b/ipc/glue/GeckoChildProcessHost.h
>@@ -59,19 +59,27 @@ protected:
> public:
>   typedef base::ProcessHandle ProcessHandle;
> 
>   GeckoChildProcessHost(GeckoProcessType aProcessType=GeckoProcessType_Default,
>                         base::WaitableEventWatcher::Delegate* aDelegate=nsnull);
> 
>   ~GeckoChildProcessHost();
> 
>-  bool SyncLaunch(std::vector<std::string> aExtraOpts=std::vector<std::string>(), int32 timeoutMs=0);
>+#ifdef XP_MACOSX
>+  static nsresult GetArchitecturesForBinary(const char *path, uint32 *result);
>+  static nsresult GetChildBinaryArchitectures(uint32 *result);

Let's keep these interfaces defined for all platforms and add stubs for non-mac.

Instead of |GetChildBinaryArchitectures(uint32 *result);|, let's do 

  uint32 GetSupportedArchitecturesForProcessType(GeckoProcessType type)

I don't think function should be fallible.

>diff --git a/modules/plugin/base/src/nsPluginsDirDarwin.cpp b/modules/plugin/base/src/nsNPAPIPlugin.cpp
>--- a/modules/plugin/base/src/nsNPAPIPlugin.cpp
>+++ b/modules/plugin/base/src/nsNPAPIPlugin.cpp

The plugin tag cleanup looks OK.  This would be better served as a
separate patch, would prefer not to see it again.

Comment 10

[Josh Aas]

Attachment: fix v1.3

Comment 11

[Josh Aas]

Attachment: fix v1.4

Fix a bug I introduced in the last version of the patch.

Comment 12

[Josh Aas]

Attachment: fix v1.5

Broke out part of the patch into bug 596434.

Comment 13

[Josh Aas]

Attachment: fix v1.6

Comment 14

[Guillermo López :willyaranda (probably SLOW response)]

(Just to add, Adobe has released a 64-bit plugin for all three platforms on http://labs.adobe.com/technologies/flashplayer10/ and I have it working correctly on mac)

Comment 15

[Guillermo López :willyaranda (probably SLOW response)]

(obviously, wrong bug)

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 475603 [details] [diff] [review]
fix v1.6

>diff --git a/dom/plugins/PluginProcessParent.cpp b/dom/plugins/PluginProcessParent.cpp
>--- a/dom/plugins/PluginProcessParent.cpp
>+++ b/dom/plugins/PluginProcessParent.cpp
> bool
> PluginProcessParent::Launch(PRInt32 timeoutMs)
> {
>+    ProcessArchitecture currentArchitecture = base::GetCurrentProcessArchitecture();
>+    uint32 containerArchitectures = GetSupportedArchitecturesForProcessType(GeckoProcessType_Plugin);
>+
>+    uint32 pluginLibArchitectures = currentArchitecture;
>+#ifdef XP_MACOSX
>+    nsresult rv = GetArchitecturesForBinary(mPluginFilePath.c_str(), &pluginLibArchitectures);
>+    if (NS_FAILED(rv)) {
>+        // If the call failed just assume that we want the current architecture.
>+        pluginLibArchitectures = currentArchitecture;
>+    }
>+#endif

You don't need the #ifdef, this function is defined for all platforms now.

Looks good, thanks.

Comment 17

[Josh Aas]

(In reply to comment #16)

> You don't need the #ifdef, this function is defined for all platforms now.

The ifdef is there because "c_str()" doesn't return the same thing on all platforms so that code won't work everywhere.

Comment 18

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/e45446b74099