From b3befb9df9084d137146f56e2125d9a03fc02921 Mon Sep 17 00:00:00 2001 From: Dave Hylands Date: Fri, 4 Apr 2014 12:16:16 -0700 Subject: [PATCH] Bug 988410 - Move directory service calls onto MainThread. r=bent --- ipc/glue/GeckoChildProcessHost.cpp | 151 ++++++++++++++++++----------- ipc/glue/GeckoChildProcessHost.h | 11 +++ 2 files changed, 104 insertions(+), 58 deletions(-) diff --git a/ipc/glue/GeckoChildProcessHost.cpp b/ipc/glue/GeckoChildProcessHost.cpp index f74f27fd3fba..1cb32a226e64 100644 --- a/ipc/glue/GeckoChildProcessHost.cpp +++ b/ipc/glue/GeckoChildProcessHost.cpp @@ -21,6 +21,7 @@ #include "nsILocalFileMac.h" #endif +#include "MainThreadUtils.h" #include "prprf.h" #include "prenv.h" @@ -29,6 +30,7 @@ #include "nsDirectoryServiceDefs.h" #include "nsIFile.h" +#include "mozilla/ClearOnShutdown.h" #include "mozilla/ipc/BrowserProcessSubThread.h" #include "mozilla/Omnijar.h" #include @@ -62,6 +64,9 @@ static const bool kLowRightsSubprocesses = #endif ; +mozilla::StaticRefPtr GeckoChildProcessHost::sGreDir; +mozilla::DebugOnly GeckoChildProcessHost::sGreDirCached; + static bool ShouldHaveDirectoryService() { @@ -120,29 +125,26 @@ GeckoChildProcessHost::~GeckoChildProcessHost() #endif } -void GetPathToBinary(FilePath& exePath) +//static +void +GeckoChildProcessHost::GetPathToBinary(FilePath& exePath) { if (ShouldHaveDirectoryService()) { - nsCOMPtr directoryService(do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID)); - NS_ASSERTION(directoryService, "Expected XPCOM to be available"); - if (directoryService) { - nsCOMPtr greDir; - nsresult rv = directoryService->Get(NS_GRE_DIR, NS_GET_IID(nsIFile), getter_AddRefs(greDir)); - if (NS_SUCCEEDED(rv)) { + MOZ_ASSERT(sGreDirCached); + if (sGreDir) { #ifdef OS_WIN - nsString path; - greDir->GetPath(path); + nsString path; + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(sGreDir->GetPath(path))); #else - nsCString path; - greDir->GetNativePath(path); + nsCString path; + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(sGreDir->GetNativePath(path))); #endif - exePath = FilePath(path.get()); + exePath = FilePath(path.get()); #ifdef MOZ_WIDGET_COCOA - // We need to use an App Bundle on OS X so that we can hide - // the dock icon. See Bug 557225. - exePath = exePath.AppendASCII(MOZ_CHILD_PROCESS_BUNDLE); + // We need to use an App Bundle on OS X so that we can hide + // the dock icon. See Bug 557225. + exePath = exePath.AppendASCII(MOZ_CHILD_PROCESS_BUNDLE); #endif - } } } @@ -226,9 +228,11 @@ uint32_t GeckoChildProcessHost::GetSupportedArchitecturesForProcessType(GeckoPro { #ifdef MOZ_WIDGET_COCOA if (type == GeckoProcessType_Plugin) { + // Cache this, it shouldn't ever change. static uint32_t pluginContainerArchs = 0; if (pluginContainerArchs == 0) { + CacheGreDir(); FilePath exePath; GetPathToBinary(exePath); nsresult rv = GetArchitecturesForBinary(exePath.value().c_str(), &pluginContainerArchs); @@ -256,6 +260,42 @@ GeckoChildProcessHost::PrepareLaunch() #ifdef XP_WIN InitWindowsGroupID(); #endif + CacheGreDir(); +} + +//static +void +GeckoChildProcessHost::CacheGreDir() +{ + // PerformAysncLaunchInternal/GetPathToBinary may be called on the IO thread, + // and they want to use the directory service, which needs to happen on the + // main thread (in the event that its implemented in JS). So we grab + // NS_GRE_DIR here and stash it. + +#ifdef MOZ_WIDGET_GONK + // Apparently, this ASSERT should be present on all platforms. Currently, + // this assert causes mochitest failures on the Mac platform if its left in. + + // B2G overrides the directory service in JS, so this needs to be called + // on the main thread. + MOZ_ASSERT(NS_IsMainThread()); +#endif + + if (ShouldHaveDirectoryService()) { + nsCOMPtr directoryService(do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID)); + MOZ_ASSERT(directoryService, "Expected XPCOM to be available"); + if (directoryService) { + // getter_AddRefs doesn't work with StaticRefPtr, so we need to store the + // result in an nsCOMPtr and copy it over. + nsCOMPtr greDir; + nsresult rv = directoryService->Get(NS_GRE_DIR, NS_GET_IID(nsIFile), getter_AddRefs(greDir)); + if (NS_SUCCEEDED(rv)) { + sGreDir = greDir; + mozilla::ClearOnShutdown(&sGreDir); + } + } + } + sGreDirCached = true; } #ifdef XP_WIN @@ -467,7 +507,7 @@ AddAppDirToCommandLine(std::vector& aCmdLine) getter_AddRefs(appDir)); if (NS_SUCCEEDED(rv)) { nsAutoCString path; - appDir->GetNativePath(path); + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(appDir->GetNativePath(path))); #if defined(XP_WIN) aCmdLine.AppendLooseValue(UTF8ToWide("-appdir")); aCmdLine.AppendLooseValue(UTF8ToWide(path.get())); @@ -519,51 +559,46 @@ GeckoChildProcessHost::PerformAsyncLaunchInternal(std::vector& aExt // since LD_LIBRARY_PATH is already set correctly in subprocesses // (meaning that we don't need to set that up in the environment). if (ShouldHaveDirectoryService()) { - nsCOMPtr directoryService(do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID)); - NS_ASSERTION(directoryService, "Expected XPCOM to be available"); - if (directoryService) { - nsCOMPtr greDir; - nsresult rv = directoryService->Get(NS_GRE_DIR, NS_GET_IID(nsIFile), getter_AddRefs(greDir)); - if (NS_SUCCEEDED(rv)) { - nsCString path; - greDir->GetNativePath(path); + MOZ_ASSERT(sGreDirCached); + if (sGreDir) { + nsCString path; + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(sGreDir->GetNativePath(path))); # if defined(OS_LINUX) || defined(OS_BSD) # if defined(MOZ_WIDGET_ANDROID) - path += "/lib"; + path += "/lib"; # endif // MOZ_WIDGET_ANDROID - const char *ld_library_path = PR_GetEnv("LD_LIBRARY_PATH"); - nsCString new_ld_lib_path; - if (ld_library_path && *ld_library_path) { - new_ld_lib_path.Assign(path.get()); - new_ld_lib_path.AppendLiteral(":"); - new_ld_lib_path.Append(ld_library_path); - newEnvVars["LD_LIBRARY_PATH"] = new_ld_lib_path.get(); - } else { - newEnvVars["LD_LIBRARY_PATH"] = path.get(); - } -# elif OS_MACOSX - newEnvVars["DYLD_LIBRARY_PATH"] = path.get(); - // XXX DYLD_INSERT_LIBRARIES should only be set when launching a plugin - // process, and has no effect on other subprocesses (the hooks in - // libplugin_child_interpose.dylib become noops). But currently it - // gets set when launching any kind of subprocess. - // - // Trigger "dyld interposing" for the dylib that contains - // plugin_child_interpose.mm. This allows us to hook OS calls in the - // plugin process (ones that don't work correctly in a background - // process). Don't break any other "dyld interposing" that has already - // been set up by whatever may have launched the browser. - const char* prevInterpose = PR_GetEnv("DYLD_INSERT_LIBRARIES"); - nsCString interpose; - if (prevInterpose) { - interpose.Assign(prevInterpose); - interpose.AppendLiteral(":"); - } - interpose.Append(path.get()); - interpose.AppendLiteral("/libplugin_child_interpose.dylib"); - newEnvVars["DYLD_INSERT_LIBRARIES"] = interpose.get(); -# endif // OS_LINUX + const char *ld_library_path = PR_GetEnv("LD_LIBRARY_PATH"); + nsCString new_ld_lib_path; + if (ld_library_path && *ld_library_path) { + new_ld_lib_path.Assign(path.get()); + new_ld_lib_path.AppendLiteral(":"); + new_ld_lib_path.Append(ld_library_path); + newEnvVars["LD_LIBRARY_PATH"] = new_ld_lib_path.get(); + } else { + newEnvVars["LD_LIBRARY_PATH"] = path.get(); } +# elif OS_MACOSX + newEnvVars["DYLD_LIBRARY_PATH"] = path.get(); + // XXX DYLD_INSERT_LIBRARIES should only be set when launching a plugin + // process, and has no effect on other subprocesses (the hooks in + // libplugin_child_interpose.dylib become noops). But currently it + // gets set when launching any kind of subprocess. + // + // Trigger "dyld interposing" for the dylib that contains + // plugin_child_interpose.mm. This allows us to hook OS calls in the + // plugin process (ones that don't work correctly in a background + // process). Don't break any other "dyld interposing" that has already + // been set up by whatever may have launched the browser. + const char* prevInterpose = PR_GetEnv("DYLD_INSERT_LIBRARIES"); + nsCString interpose; + if (prevInterpose) { + interpose.Assign(prevInterpose); + interpose.AppendLiteral(":"); + } + interpose.Append(path.get()); + interpose.AppendLiteral("/libplugin_child_interpose.dylib"); + newEnvVars["DYLD_INSERT_LIBRARIES"] = interpose.get(); +# endif // OS_LINUX } } #endif // OS_LINUX || OS_MACOSX diff --git a/ipc/glue/GeckoChildProcessHost.h b/ipc/glue/GeckoChildProcessHost.h index 3c87d80fd3f1..dfb5b3f245fd 100644 --- a/ipc/glue/GeckoChildProcessHost.h +++ b/ipc/glue/GeckoChildProcessHost.h @@ -11,12 +11,17 @@ #include "base/waitable_event.h" #include "chrome/common/child_process_host.h" +#include "mozilla/DebugOnly.h" #include "mozilla/ipc/FileDescriptor.h" #include "mozilla/Monitor.h" +#include "mozilla/StaticPtr.h" +#include "nsCOMPtr.h" #include "nsXULAppAPI.h" // for GeckoProcessType #include "nsString.h" +class nsIFile; + namespace mozilla { namespace ipc { @@ -186,6 +191,9 @@ private: bool RunPerformAsyncLaunch(StringVector aExtraOpts=StringVector(), base::ProcessArchitecture aArch=base::GetCurrentProcessArchitecture()); + static void GetPathToBinary(FilePath& exePath); + static void CacheGreDir(); + // In between launching the subprocess and handing off its IPC // channel, there's a small window of time in which *we* might still // be the channel listener, and receive messages. That's bad @@ -194,6 +202,9 @@ private: // // FIXME/cjones: this strongly indicates bad design. Shame on us. std::queue mQueue; + + static StaticRefPtr sGreDir; + static DebugOnly sGreDirCached; }; #ifdef MOZ_NUWA_PROCESS