From 608f3e7539be01634c6fdaae71349ff6111414e1 Mon Sep 17 00:00:00 2001 From: Gijs Kruitbosch Date: Sat, 2 Nov 2019 22:33:42 +0000 Subject: [PATCH] Bug 1545123 - simplify how we get directory information for plugins, r=handyman,mconley In this change we: - stop treating the nsPluginDirServiceProvider as a directory provider, as its GetFile implementation was a no-op anyway - registering it didn't make any difference. - stop treating it as a class entirely, because the PLID getters were already static, so instantiating it also didn't do anything. - move IO from the plugin directory list provider and the Windows-only PLID getters into nsPluginHost. This enables us to move it off of the main thread later - the directory getting has to happen on the main thread, but we can postpone further checks on the nsIFile instances. - in the process, stop doing exists() calls on files because we can fail more lazily. This allows us to remove more allowlist entries from browser_startup_mainthreadio, though the `isDirectory` calls will actually still cause IO - they don't seem to create IO markers in the profiler. We will move this IO away from the main thread in subsequent commits. Depends on D48328 Differential Revision: https://phabricator.services.mozilla.com/D48329 --HG-- extra : moz-landing-system : lando --- .../browser_startup_mainthreadio.js | 12 - .../base/nsPluginDirServiceProvider.cpp | 89 ++----- dom/plugins/base/nsPluginDirServiceProvider.h | 33 +-- dom/plugins/base/nsPluginHost.cpp | 219 ++++++++---------- dom/plugins/base/nsPluginHost.h | 10 +- xpcom/io/nsAppFileLocationProvider.cpp | 6 +- 6 files changed, 119 insertions(+), 250 deletions(-) diff --git a/browser/base/content/test/performance/browser_startup_mainthreadio.js b/browser/base/content/test/performance/browser_startup_mainthreadio.js index dd4d643275b5..06f1623ddd4b 100644 --- a/browser/base/content/test/performance/browser_startup_mainthreadio.js +++ b/browser/base/content/test/performance/browser_startup_mainthreadio.js @@ -332,12 +332,6 @@ const startupPhases = { // We reach this phase right after showing the first browser window. // This means that any I/O at this point delayed first paint. "before first paint": [ - { - // bug 1541226 - path: "ProfD:", - condition: WIN, - stat: 1, - }, { // bug 1545119 path: "OldUpdRootD:", @@ -369,12 +363,6 @@ const startupPhases = { condition: WIN, stat: 1, }, - { - // bug 1545123 - path: "UserPlugins:", - condition: WIN, - stat: 1, - }, { // bug 1545123 path: "ProfD:plugins/nptest.dll", diff --git a/dom/plugins/base/nsPluginDirServiceProvider.cpp b/dom/plugins/base/nsPluginDirServiceProvider.cpp index 092e1fb9f0a0..87236548027e 100644 --- a/dom/plugins/base/nsPluginDirServiceProvider.cpp +++ b/dom/plugins/base/nsPluginDirServiceProvider.cpp @@ -7,59 +7,21 @@ #include "nsCRT.h" #include "nsIFile.h" -#include "nsDependentString.h" -#include "nsArrayEnumerator.h" -#include "mozilla/Preferences.h" #include #include "nsIWindowsRegKey.h" using namespace mozilla; -//***************************************************************************** -// nsPluginDirServiceProvider::Constructor/Destructor -//***************************************************************************** - -nsPluginDirServiceProvider::nsPluginDirServiceProvider() {} - -nsPluginDirServiceProvider::~nsPluginDirServiceProvider() {} - -//***************************************************************************** -// nsPluginDirServiceProvider::nsISupports -//***************************************************************************** - -NS_IMPL_ISUPPORTS(nsPluginDirServiceProvider, nsIDirectoryServiceProvider) - -//***************************************************************************** -// nsPluginDirServiceProvider::nsIDirectoryServiceProvider -//***************************************************************************** - -NS_IMETHODIMP -nsPluginDirServiceProvider::GetFile(const char* charProp, bool* persistant, - nsIFile** _retval) { - NS_ENSURE_ARG(charProp); - - *_retval = nullptr; - *persistant = false; - - return NS_ERROR_FAILURE; +/* static */ nsresult GetPLIDDirectories(nsTArray>& aDirs) { + GetPLIDDirectoriesWithRootKey(nsIWindowsRegKey::ROOT_KEY_CURRENT_USER, aDirs); + GetPLIDDirectoriesWithRootKey(nsIWindowsRegKey::ROOT_KEY_LOCAL_MACHINE, + aDirs); + return NS_OK; } -nsresult nsPluginDirServiceProvider::GetPLIDDirectories( - nsISimpleEnumerator** aEnumerator) { - NS_ENSURE_ARG_POINTER(aEnumerator); - *aEnumerator = nullptr; - - nsCOMArray dirs; - - GetPLIDDirectoriesWithRootKey(nsIWindowsRegKey::ROOT_KEY_CURRENT_USER, dirs); - GetPLIDDirectoriesWithRootKey(nsIWindowsRegKey::ROOT_KEY_LOCAL_MACHINE, dirs); - - return NS_NewArrayEnumerator(aEnumerator, dirs, NS_GET_IID(nsIFile)); -} - -nsresult nsPluginDirServiceProvider::GetPLIDDirectoriesWithRootKey( - uint32_t aKey, nsCOMArray& aDirs) { +/* static */ nsresult GetPLIDDirectoriesWithRootKey( + uint32_t aKey, nsTArray>& aDirs) { nsCOMPtr regKey = do_CreateInstance("@mozilla.org/windows-registry-key;1"); NS_ENSURE_TRUE(regKey, NS_ERROR_FAILURE); @@ -85,37 +47,14 @@ nsresult nsPluginDirServiceProvider::GetPLIDDirectoriesWithRootKey( nsAutoString path; rv = childKey->ReadStringValue(NS_LITERAL_STRING("Path"), path); if (NS_SUCCEEDED(rv)) { + // We deliberately do not do any further checks here on whether + // these are actually directories, whether they even exist, or + // whether they are duplicates. The pluginhost code will do them. + // This allows the whole process to avoid mainthread IO. nsCOMPtr localFile; - if (NS_SUCCEEDED( - NS_NewLocalFile(path, true, getter_AddRefs(localFile))) && - localFile) { - // Some vendors use a path directly to the DLL so chop off - // the filename - bool isDir = false; - if (NS_SUCCEEDED(localFile->IsDirectory(&isDir)) && !isDir) { - nsCOMPtr temp; - localFile->GetParent(getter_AddRefs(temp)); - if (temp) localFile = temp; - } - - // Now we check to make sure it's actually on disk and - // To see if we already have this directory in the array - bool isFileThere = false; - bool isDupEntry = false; - if (NS_SUCCEEDED(localFile->Exists(&isFileThere)) && isFileThere) { - int32_t c = aDirs.Count(); - for (int32_t i = 0; i < c; i++) { - nsIFile* dup = static_cast(aDirs[i]); - if (dup && NS_SUCCEEDED(dup->Equals(localFile, &isDupEntry)) && - isDupEntry) { - break; - } - } - - if (!isDupEntry) { - aDirs.AppendObject(localFile); - } - } + rv = NS_NewLocalFile(path, true, getter_AddRefs(localFile)); + if (NS_SUCCEEDED(rv) && localFile) { + aDirs.AppendElement(localFile); } } } diff --git a/dom/plugins/base/nsPluginDirServiceProvider.h b/dom/plugins/base/nsPluginDirServiceProvider.h index bca57e69cb26..65571a473be4 100644 --- a/dom/plugins/base/nsPluginDirServiceProvider.h +++ b/dom/plugins/base/nsPluginDirServiceProvider.h @@ -6,35 +6,14 @@ #ifndef nsPluginDirServiceProvider_h_ #define nsPluginDirServiceProvider_h_ -#include "nsIDirectoryService.h" - -#if defined(XP_WIN) -# include "nsCOMArray.h" -#endif - -class nsISimpleEnumerator; - -//***************************************************************************** -// class nsPluginDirServiceProvider -//***************************************************************************** - -class nsPluginDirServiceProvider : public nsIDirectoryServiceProvider { - public: - nsPluginDirServiceProvider(); - - NS_DECL_THREADSAFE_ISUPPORTS - NS_DECL_NSIDIRECTORYSERVICEPROVIDER - #ifdef XP_WIN - static nsresult GetPLIDDirectories(nsISimpleEnumerator** aEnumerator); +# include "nsCOMPtr.h" +# include "nsTArray.h" - private: - static nsresult GetPLIDDirectoriesWithRootKey(uint32_t aKey, - nsCOMArray& aDirs); -#endif +static nsresult GetPLIDDirectories(nsTArray>& aDirs); - protected: - virtual ~nsPluginDirServiceProvider(); -}; +static nsresult GetPLIDDirectoriesWithRootKey( + uint32_t aKey, nsTArray>& aDirs); +#endif // XP_WIN #endif // nsPluginDirServiceProvider_h_ diff --git a/dom/plugins/base/nsPluginHost.cpp b/dom/plugins/base/nsPluginHost.cpp index cbccb872d090..8f40f459dbc5 100644 --- a/dom/plugins/base/nsPluginHost.cpp +++ b/dom/plugins/base/nsPluginHost.cpp @@ -99,6 +99,8 @@ #include "mozilla/dom/Promise.h" +#include "mozilla/ResultExtensions.h" + #if defined(XP_WIN) # include "nsIWindowMediator.h" # include "nsIBaseWindow.h" @@ -145,9 +147,6 @@ static const uint32_t kDefaultPluginUnloadingTimeout = 30; static const char* kPluginRegistryVersion = "0.19"; -static const char kDirectoryServiceContractID[] = - "@mozilla.org/file/directory_service;1"; - #define kPluginRegistryFilename NS_LITERAL_CSTRING("pluginreg.dat") LazyLogModule nsPluginLogging::gNPNLog(NPN_LOG_NAME); @@ -837,15 +836,6 @@ nsresult nsPluginHost::UnloadPlugins() { NS_RELEASE(sPluginTempDir); } -#ifdef XP_WIN - if (mPrivateDirServiceProvider) { - nsCOMPtr dirService = - do_GetService(kDirectoryServiceContractID); - if (dirService) dirService->UnregisterProvider(mPrivateDirServiceProvider); - mPrivateDirServiceProvider = nullptr; - } -#endif /* XP_WIN */ - mPluginsLoaded = false; return NS_OK; @@ -2280,31 +2270,6 @@ void nsPluginHost::UpdatePluginBlocklistState(nsPluginTag* aPluginTag, } } -nsresult nsPluginHost::ScanPluginsDirectoryList(nsISimpleEnumerator* dirEnum, - bool aCreatePluginList, - bool* aPluginsChanged) { - MOZ_ASSERT(XRE_IsParentProcess()); - - bool hasMore; - while (NS_SUCCEEDED(dirEnum->HasMoreElements(&hasMore)) && hasMore) { - nsCOMPtr supports; - nsresult rv = dirEnum->GetNext(getter_AddRefs(supports)); - if (NS_FAILED(rv)) continue; - nsCOMPtr nextDir(do_QueryInterface(supports, &rv)); - if (NS_FAILED(rv)) continue; - - // don't pass aPluginsChanged directly to prevent it from been reset - bool pluginschanged = false; - ScanPluginsDirectory(nextDir, aCreatePluginList, &pluginschanged); - - if (pluginschanged) *aPluginsChanged = true; - - // if changes are detected and we are not creating the list, do not proceed - if (!aCreatePluginList && *aPluginsChanged) break; - } - return NS_OK; -} - void nsPluginHost::IncrementChromeEpoch() { MOZ_ASSERT(XRE_IsParentProcess()); mPluginEpoch++; @@ -2465,88 +2430,63 @@ nsresult nsPluginHost::FindPlugins(bool aCreatePluginList, *aPluginsChanged = false; - nsresult rv; + nsresult rv = EnsurePluginReg(); + if (NS_FAILED(rv)) { + // If the profile isn't yet available then don't scan for plugins + return rv == NS_ERROR_NOT_AVAILABLE ? NS_OK : rv; + } - // Read cached plugins info. If the profile isn't yet available then don't - // scan for plugins - if (ReadPluginInfo() == NS_ERROR_NOT_AVAILABLE) return NS_OK; + // Read cached plugins info. + ReadPluginInfo(); -#ifdef XP_WIN - // Failure here is not a show-stopper so just warn. - rv = EnsurePrivateDirServiceProvider(); - NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to register dir service provider."); -#endif /* XP_WIN */ + // Scan plugin directories: + nsTArray> pluginDirs; + MOZ_TRY(DeterminePluginDirs(pluginDirs)); - nsCOMPtr dirService( - do_GetService(kDirectoryServiceContractID, &rv)); - if (NS_FAILED(rv)) return rv; - - nsCOMPtr dirList; - - // Scan plugins directories; - // don't pass aPluginsChanged directly, to prevent its - // possible reset in subsequent ScanPluginsDirectory calls - bool pluginschanged = false; - - // Scan the app-defined list of plugin dirs. - rv = dirService->Get(NS_APP_PLUGINS_DIR_LIST, NS_GET_IID(nsISimpleEnumerator), - getter_AddRefs(dirList)); - if (NS_SUCCEEDED(rv)) { - ScanPluginsDirectoryList(dirList, aCreatePluginList, &pluginschanged); - - if (pluginschanged) *aPluginsChanged = true; - - // In tests, load plugins from the profile. - if (xpc::IsInAutomation()) { - nsCOMPtr profDir; - rv = dirService->Get(NS_APP_USER_PROFILE_50_DIR, NS_GET_IID(nsIFile), - getter_AddRefs(profDir)); - if (NS_SUCCEEDED(rv)) { - profDir->Append(NS_LITERAL_STRING("plugins")); - ScanPluginsDirectory(profDir, aCreatePluginList, &pluginschanged); - if (pluginschanged) { - *aPluginsChanged = true; - } + for (nsIFile* pluginDir : pluginDirs) { + if (!pluginDir) { + continue; + } + // Ensure we have a directory; if this isn't a directory, try the parent. + // We do this because in some cases items in this list of supposed plugin + // directories can be individual plugin files instead of directories. + bool isDir = false; + nsCOMPtr parent; + if (NS_FAILED(pluginDir->IsDirectory(&isDir)) || !isDir) { + pluginDir->GetParent(getter_AddRefs(parent)); + pluginDir = parent; + if (!pluginDir) { + continue; } } + // pluginDir not existing will be handled transparently in + // ScanPluginsDirectory - // if we are just looking for possible changes, - // no need to proceed if changes are detected - if (!aCreatePluginList && *aPluginsChanged) { - NS_ITERATIVE_UNREF_LIST(RefPtr, mCachedPlugins, mNext); - NS_ITERATIVE_UNREF_LIST(RefPtr, mInvalidPlugins, - mNext); - return NS_OK; + // don't pass aPluginsChanged directly, to prevent its + // possible reset in subsequent ScanPluginsDirectory calls + bool pluginschanged = false; + ScanPluginsDirectory(pluginDir, aCreatePluginList, &pluginschanged); + + if (pluginschanged) { + *aPluginsChanged = true; + if (!aCreatePluginList) { + // We're just looking for changes, so we can stop looking. + break; + } } } + // if we are just looking for possible changes, + // no need to proceed if changes are detected + if (!aCreatePluginList && *aPluginsChanged) { + NS_ITERATIVE_UNREF_LIST(RefPtr, mCachedPlugins, mNext); + NS_ITERATIVE_UNREF_LIST(RefPtr, mInvalidPlugins, mNext); + return NS_OK; + } + mPluginsLoaded = true; // at this point 'some' plugins have been loaded, // the rest is optional -#ifdef XP_WIN - bool bScanPLIDs = Preferences::GetBool("plugin.scan.plid.all", false); - - // Now lets scan any PLID directories - if (bScanPLIDs && mPrivateDirServiceProvider) { - rv = - mPrivateDirServiceProvider->GetPLIDDirectories(getter_AddRefs(dirList)); - if (NS_SUCCEEDED(rv)) { - ScanPluginsDirectoryList(dirList, aCreatePluginList, &pluginschanged); - - if (pluginschanged) *aPluginsChanged = true; - - // if we are just looking for possible changes, - // no need to proceed if changes are detected - if (!aCreatePluginList && *aPluginsChanged) { - NS_ITERATIVE_UNREF_LIST(RefPtr, mCachedPlugins, mNext); - NS_ITERATIVE_UNREF_LIST(RefPtr, mInvalidPlugins, - mNext); - return NS_OK; - } - } - } -#endif - // We should also consider plugins to have changed if any plugins have been // removed. We'll know if any were removed if they weren't taken out of the // cached plugins list during our scan, thus we can assume something was @@ -2882,7 +2822,6 @@ nsresult nsPluginHost::EnsurePluginReg() { } nsresult rv; - nsCOMPtr directoryService( do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv)); if (NS_FAILED(rv)) return rv; @@ -2905,6 +2844,55 @@ nsresult nsPluginHost::EnsurePluginReg() { return mPluginRegFile->AppendNative(kPluginRegistryFilename); } +nsresult nsPluginHost::DeterminePluginDirs( + nsTArray>& pluginDirs) { + nsresult rv; + nsCOMPtr dirService( + do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv)); + if (NS_FAILED(rv)) { + return rv; + } + + // Get the app-defined list of plugin dirs. + nsCOMPtr dirEnum; + MOZ_TRY(dirService->Get(NS_APP_PLUGINS_DIR_LIST, + NS_GET_IID(nsISimpleEnumerator), + getter_AddRefs(dirEnum))); + + bool hasMore = false; + while (NS_SUCCEEDED(dirEnum->HasMoreElements(&hasMore)) && hasMore) { + nsCOMPtr supports; + nsresult rv = dirEnum->GetNext(getter_AddRefs(supports)); + if (NS_SUCCEEDED(rv)) { + nsCOMPtr nextDir(do_QueryInterface(supports, &rv)); + if (NS_SUCCEEDED(rv)) { + pluginDirs.AppendElement(nextDir); + } + } + } + + // In tests, load plugins from the profile. + if (xpc::IsInAutomation()) { + nsCOMPtr profDir; + rv = dirService->Get(NS_APP_USER_PROFILE_50_DIR, NS_GET_IID(nsIFile), + getter_AddRefs(profDir)); + if (NS_SUCCEEDED(rv)) { + profDir->Append(NS_LITERAL_STRING("plugins")); + pluginDirs.AppendElement(profDir); + } + } + + // Get the directories from the windows registry. Note: these can + // be files instead of directories. We'll have to filter them later. +#ifdef XP_WIN + bool bScanPLIDs = Preferences::GetBool("plugin.scan.plid.all", false); + if (bScanPLIDs) { + GetPLIDDirectories(pluginDirs); + } +#endif /* XP_WIN */ + return NS_OK; +} + nsresult nsPluginHost::ReadPluginInfo() { MOZ_ASSERT(XRE_IsParentProcess()); @@ -3160,21 +3148,6 @@ void nsPluginHost::RemoveCachedPluginsInfo(const char* filePath, } } -#ifdef XP_WIN -nsresult nsPluginHost::EnsurePrivateDirServiceProvider() { - if (!mPrivateDirServiceProvider) { - nsresult rv; - mPrivateDirServiceProvider = new nsPluginDirServiceProvider(); - nsCOMPtr dirService( - do_GetService(kDirectoryServiceContractID, &rv)); - if (NS_FAILED(rv)) return rv; - rv = dirService->RegisterProvider(mPrivateDirServiceProvider); - if (NS_FAILED(rv)) return rv; - } - return NS_OK; -} -#endif /* XP_WIN */ - nsresult nsPluginHost::NewPluginURLStream( const nsString& aURL, nsNPAPIPluginInstance* aInstance, nsNPAPIPluginStreamListener* aListener, nsIInputStream* aPostStream, diff --git a/dom/plugins/base/nsPluginHost.h b/dom/plugins/base/nsPluginHost.h index 4e22d35925b1..bb183bd9664a 100644 --- a/dom/plugins/base/nsPluginHost.h +++ b/dom/plugins/base/nsPluginHost.h @@ -310,16 +310,14 @@ class nsPluginHost final : public nsIPluginHost, nsresult ScanPluginsDirectory(nsIFile* pluginsDir, bool aCreatePluginList, bool* aPluginsChanged); - nsresult ScanPluginsDirectoryList(nsISimpleEnumerator* dirEnum, - bool aCreatePluginList, - bool* aPluginsChanged); - nsresult EnsurePluginLoaded(nsPluginTag* aPluginTag); bool IsRunningPlugin(nsPluginTag* aPluginTag); nsresult EnsurePluginReg(); + nsresult DeterminePluginDirs(nsTArray>& pluginDirs); + // Read plugin info (either from prefs or disk) nsresult ReadPluginInfo(); @@ -346,8 +344,6 @@ class nsPluginHost final : public nsIPluginHost, // Returns the first plugin at |path| nsPluginTag* FirstPluginWithPath(const nsCString& path); - nsresult EnsurePrivateDirServiceProvider(); - void OnPluginInstanceDestroyed(nsPluginTag* aPluginTag); // To be used by the chrome process whenever the set of plugins changes. @@ -390,8 +386,6 @@ class nsPluginHost final : public nsIPluginHost, // An nsIFile for the pluginreg.dat file in the profile. nsCOMPtr mPluginRegFile; #ifdef XP_WIN - RefPtr mPrivateDirServiceProvider; - // In order to reload plugins when they change, we watch the registry via // this object. nsCOMPtr mRegKeyHKLM; diff --git a/xpcom/io/nsAppFileLocationProvider.cpp b/xpcom/io/nsAppFileLocationProvider.cpp index 9f6a3fa5b204..d180d7c7d039 100644 --- a/xpcom/io/nsAppFileLocationProvider.cpp +++ b/xpcom/io/nsAppFileLocationProvider.cpp @@ -370,11 +370,7 @@ class nsAppDirectoryEnumerator : public nsSimpleEnumerator { nsCOMPtr testFile; (void)mProvider->GetFile(*mCurrentKey++, &dontCare, getter_AddRefs(testFile)); - // Don't return a file which does not exist. - bool exists; - if (testFile && NS_SUCCEEDED(testFile->Exists(&exists)) && exists) { - mNext = testFile; - } + mNext = testFile; } *aResult = mNext != nullptr; return NS_OK;