From 588bf8226f30c10e8bc99d3c13861a0eaee64b03 Mon Sep 17 00:00:00 2001 From: Ben Hearsum Date: Thu, 23 Jun 2022 15:15:42 +0000 Subject: [PATCH] Bug 1762994: ensure a Private Browsing shortcut exists as early as possible r=mhowell,fluent-reviewers This fixes a bug where pinning a Private Browsing window to Taskbar with the windows context menu item pins regular Firefox instead. This happens because Windows cannot find an appropriate shortcut (presumably by looking for one with the right AUMID), and ends up creating its own instead -- with almost entirely incorrect metadata. With this, we'll be creating one at a few points (if it doesn't already exist): * Installer - for new installs * Post-Update - to make sure we get one when people update to the first version where we pref this on * Startup idle - for zip installs, and to handle any other possible case where the shortcut doesn't exist Until we enable separation of Private Windows by default I've pref'ed off this behaviour (otherwise a user may pin the shortcut to the Taskbar, but the app will launch into a different icon). Differential Revision: https://phabricator.services.mozilla.com/D147702 --- browser/components/BrowserGlue.jsm | 76 +++++++++++++++++++ .../shell/nsIWindowsShellService.idl | 2 + .../shell/nsWindowsShellService.cpp | 71 ++++++++++++----- browser/installer/windows/nsis/defines.nsi.in | 1 + browser/installer/windows/nsis/installer.nsi | 15 ++++ browser/installer/windows/nsis/shared.nsh | 11 +++ browser/locales/en-US/browser/browser.ftl | 2 + .../locales/en-US/installer/custom.properties | 1 + 8 files changed, 159 insertions(+), 20 deletions(-) diff --git a/browser/components/BrowserGlue.jsm b/browser/components/BrowserGlue.jsm index 47fbe51a6d86..64ac529d5427 100644 --- a/browser/components/BrowserGlue.jsm +++ b/browser/components/BrowserGlue.jsm @@ -137,6 +137,13 @@ const PREF_PDFJS_ISDEFAULT_CACHE_STATE = "pdfjs.enabledCache.state"; const PREF_DFPI_ENABLED_BY_DEFAULT = "privacy.restrict3rdpartystorage.rollout.enabledByDefault"; +// Index of Private Browsing icon in firefox.exe +// Must line up with the one in nsNativeAppSupportWin.h. +const PRIVATE_BROWSING_ICON_INDEX = 5; +const PREF_PRIVACY_SEGMENTATION = "browser.privacySegmentation.enabled"; +const PREF_PRIVATE_BROWSING_SHORTCUT_CREATED = + "browser.privacySegmentation.createdShortcut"; + /** * Fission-compatible JSProcess implementations. * Each actor options object takes the form of a ProcessActorOptions dictionary. @@ -2510,6 +2517,75 @@ BrowserGlue.prototype = { }, }, + // Ensure a Private Browsing Shortcut exists. This is needed in case + // a user tries to use Windows functionality to pin our Private Browsing + // mode icon to the Taskbar (eg: the "Pin to Taskbar" context menu item). + // This is also created by the installer, but it's possible that a user + // has removed it, or is running out of a zip build. The consequences of not + // having a Shortcut for this are that regular Firefox will be pinned instead + // of the Private Browsing version -- so it's quite important we do our best + // to make sure one is available. + // See https://bugzilla.mozilla.org/show_bug.cgi?id=1762994 for additional + // background. + { + condition: + AppConstants.platform == "win" && + // Pref'ed off until Private Browsing window separation is enabled by default + // to avoid a situation where a user pins the Private Browsing shortcut to + // the Taskbar, which will end up launching into a different Taskbar icon. + Services.prefs.getBoolPref(PREF_PRIVACY_SEGMENTATION, false) && + // Private Browsing shortcuts for packaged builds come with the package, + // if they exist at all. We shouldn't try to create our own. + !Services.sysinfo.getProperty("hasWinPackageId") && + // If we've ever done this successfully before, don't try again. The + // user may have deleted the shortcut, and we don't want to force it + // on them. + !Services.prefs.getBoolPref( + PREF_PRIVATE_BROWSING_SHORTCUT_CREATED, + false + ), + task: async () => { + let shellService = Cc[ + "@mozilla.org/browser/shell-service;1" + ].getService(Ci.nsIWindowsShellService); + let winTaskbar = Cc["@mozilla.org/windows-taskbar;1"].getService( + Ci.nsIWinTaskbar + ); + + if ( + !shellService.hasMatchingShortcut( + winTaskbar.defaultPrivateGroupId, + true + ) + ) { + let exe = Services.dirsvc.get("XREExeF", Ci.nsIFile); + let strings = new Localization( + ["branding/brand.ftl", "browser/browser.ftl"], + true + ); + let [desc] = await strings.formatValues([ + "private-browsing-shortcut-text", + ]); + shellService.createShortcut( + exe, + ["-private-window"], + desc, + exe, + // The code we're calling indexes from 0 instead of 1 + PRIVATE_BROWSING_ICON_INDEX - 1, + winTaskbar.defaultPrivateGroupId, + "Programs", + desc + ".lnk", + Services.dirsvc.get("GreD", Ci.nsIFile) + ); + Services.prefs.setBoolPref( + PREF_PRIVATE_BROWSING_SHORTCUT_CREATED, + true + ); + } + }, + }, + // Report whether Firefox is the default handler for various files types, // in particular, ".pdf". { diff --git a/browser/components/shell/nsIWindowsShellService.idl b/browser/components/shell/nsIWindowsShellService.idl index 2fc88f8e2502..380449dbabd0 100644 --- a/browser/components/shell/nsIWindowsShellService.idl +++ b/browser/components/shell/nsIWindowsShellService.idl @@ -144,6 +144,8 @@ interface nsIWindowsShellService : nsISupports */ AString classifyShortcut(in AString aPath); + bool hasMatchingShortcut(in AString aAUMID, in bool aPrivateBrowsing); + /* * Check if setDefaultBrowserUserChoice() is expected to succeed. * diff --git a/browser/components/shell/nsWindowsShellService.cpp b/browser/components/shell/nsWindowsShellService.cpp index 3c2781111bf8..5c846da6fe5f 100644 --- a/browser/components/shell/nsWindowsShellService.cpp +++ b/browser/components/shell/nsWindowsShellService.cpp @@ -1048,6 +1048,48 @@ static nsresult GetMatchingShortcut(int aCSIDL, const nsAString& aAUMID, return result; } + +static nsresult FindMatchingShortcut(const nsAString& aAppUserModelId, + const bool aPrivateBrowsing, + nsAutoString& aShortcutPath) { + wchar_t exePath[MAXPATHLEN] = {}; + if (NS_WARN_IF(NS_FAILED(BinaryPath::GetLong(exePath)))) { + return NS_ERROR_FAILURE; + } + + int shortcutCSIDLs[] = {CSIDL_COMMON_PROGRAMS, CSIDL_PROGRAMS, + CSIDL_COMMON_DESKTOPDIRECTORY, + CSIDL_DESKTOPDIRECTORY}; + for (int shortcutCSIDL : shortcutCSIDLs) { + // GetMatchingShortcut may fail when the exe path doesn't match, even + // if it refers to the same file. This should be rare, and the worst + // outcome would be failure to pin, so the risk is acceptable. + nsresult rv = GetMatchingShortcut(shortcutCSIDL, aAppUserModelId, exePath, + aPrivateBrowsing, aShortcutPath); + if (NS_SUCCEEDED(rv)) { + return NS_OK; + } + } + + return NS_ERROR_FILE_NOT_FOUND; +} + +NS_IMETHODIMP +nsWindowsShellService::HasMatchingShortcut(const nsAString& aAppUserModelId, + const bool aPrivateBrowsing, + bool* aHasMatch) { + nsAutoString shortcutPath; + nsresult rv = + FindMatchingShortcut(aAppUserModelId, aPrivateBrowsing, shortcutPath); + if (SUCCEEDED(rv)) { + *aHasMatch = true; + } else { + *aHasMatch = false; + } + + return NS_OK; +} + static nsresult PinCurrentAppToTaskbarWin7(bool aCheckOnly, nsAutoString aShortcutPath) { nsModuleHandle shellInst(LoadLibraryW(L"shell32.dll")); @@ -1256,27 +1298,11 @@ static nsresult PinCurrentAppToTaskbarImpl(bool aCheckOnly, !NS_IsMainThread(), "PinCurrentAppToTaskbarImpl should be called off main thread only"); - wchar_t exePath[MAXPATHLEN] = {}; - if (NS_WARN_IF(NS_FAILED(BinaryPath::GetLong(exePath)))) { - return NS_ERROR_FAILURE; - } - - // Try to find a shortcut matching the running app nsAutoString shortcutPath; - int shortcutCSIDLs[] = {CSIDL_COMMON_PROGRAMS, CSIDL_PROGRAMS, - CSIDL_COMMON_DESKTOPDIRECTORY, - CSIDL_DESKTOPDIRECTORY}; - for (size_t i = 0; i < ArrayLength(shortcutCSIDLs); ++i) { - // GetMatchingShortcut may fail when the exe path doesn't match, even - // if it refers to the same file. This should be rare, and the worst - // outcome would be failure to pin, so the risk is acceptable. - nsresult rv = GetMatchingShortcut(shortcutCSIDLs[i], aAppUserModelId, - exePath, aPrivateBrowsing, shortcutPath); - if (NS_SUCCEEDED(rv)) { - break; - } else { - shortcutPath.Truncate(); - } + nsresult rv = + FindMatchingShortcut(aAppUserModelId, aPrivateBrowsing, shortcutPath); + if (NS_FAILED(rv)) { + shortcutPath.Truncate(); } if (shortcutPath.IsEmpty()) { if (aCheckOnly) { @@ -1314,6 +1340,11 @@ static nsresult PinCurrentAppToTaskbarImpl(bool aCheckOnly, nsAutoString linkName(desc); linkName.AppendLiteral(".lnk"); + wchar_t exePath[MAXPATHLEN] = {}; + if (NS_WARN_IF(NS_FAILED(BinaryPath::GetLong(exePath)))) { + return NS_ERROR_FAILURE; + } + nsAutoString exeStr; nsCOMPtr exeFile; exeStr.Assign(exePath); diff --git a/browser/installer/windows/nsis/defines.nsi.in b/browser/installer/windows/nsis/defines.nsi.in index 0056bdbac2ca..079d08640537 100644 --- a/browser/installer/windows/nsis/defines.nsi.in +++ b/browser/installer/windows/nsis/defines.nsi.in @@ -44,6 +44,7 @@ ; resource IDs in the registry are 0-based. !define IDI_APPICON_ZERO_BASED "0" !define IDI_DOCUMENT_ZERO_BASED "1" +!define IDI_PBICON_ZERO_BASED "4" !define IDI_DOCUMENT_PDF_ZERO_BASED "5" !define CERTIFICATE_NAME "Mozilla Corporation" diff --git a/browser/installer/windows/nsis/installer.nsi b/browser/installer/windows/nsis/installer.nsi index e5942041f91b..5914ca7d74a8 100755 --- a/browser/installer/windows/nsis/installer.nsi +++ b/browser/installer/windows/nsis/installer.nsi @@ -611,6 +611,21 @@ Section "-Application" APP_IDX ${EndIf} ${EndIf} + ; This is always added if it doesn't already exist to ensure that Windows' + ; native "Pin to Taskbar" functionality can find an appropriate shortcut. + ; See https://bugzilla.mozilla.org/show_bug.cgi?id=1762994 for additional + ; background. + ; Pref'ed off until Private Browsing window separation is enabled by default + ; to avoid a situation where a user pins the Private Browsing shortcut to + ; the Taskbar, which will end up launching into a different Taskbar icon. + ClearErrors + ReadRegDWORD $2 HKCU \ + "Software\Mozilla\${AppName}\Installer\$AppUserModelID" \ + "CreatePrivateBrowsingShortcut" + ${IfNot} ${Errors} + ${AddPrivateBrowsingShortcut} + ${EndIf} + ; Update lastwritetime of the Start Menu shortcut to clear the tile cache. ; Do this for both shell contexts in case the user has shortcuts in multiple ; locations, then restore the previous context at the end. diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh index 893e8dded1eb..101942da9d9f 100755 --- a/browser/installer/windows/nsis/shared.nsh +++ b/browser/installer/windows/nsis/shared.nsh @@ -208,6 +208,17 @@ ${RemoveDefaultBrowserAgentShortcut} !macroend !define TouchStartMenuShortcut "!insertmacro TouchStartMenuShortcut" +!macro AddPrivateBrowsingShortcut + ${IfNot} ${FileExists} "$SMPROGRAMS\$(PRIVATE_BROWSING_SHORTCUT_TITLE).lnk" + CreateShortcut "$SMPROGRAMS\$(PRIVATE_BROWSING_SHORTCUT_TITLE).lnk" "$INSTDIR\${FileMainEXE}" "-private-window" "$INSTDIR\${FileMainEXE}" ${IDI_PBICON_ZERO_BASED} + ShellLink::SetShortcutWorkingDirectory "$SMPROGRAMS\$(PRIVATE_BROWSING_SHORTCUT_TITLE).lnk" "$INSTDIR" + ShellLink::SetShortcutDescription "$SMPROGRAMS\$(PRIVATE_BROWSING_SHORTCUT_TITLE).lnk" "$(PRIVATE_BROWSING_SHORTCUT_TITLE)" + ApplicationID::Set "$SMPROGRAMS\$(PRIVATE_BROWSING_SHORTCUT_TITLE).lnk" "$AppUserModelID;PrivateBrowsingAUMID" "true" + ${LogStartMenuShortcut} "$(PRIVATE_BROWSING_SHORTCUT_TITLE).lnk" + ${EndIf} +!macroend +!define AddPrivateBrowsingShortcut "!insertmacro AddPrivateBrowsingShortcut" + !macro SetAsDefaultAppGlobal ${RemoveDeprecatedKeys} ; Does not use SHCTX diff --git a/browser/locales/en-US/browser/browser.ftl b/browser/locales/en-US/browser/browser.ftl index fbbac29b6a73..aa2deb1fdd63 100644 --- a/browser/locales/en-US/browser/browser.ftl +++ b/browser/locales/en-US/browser/browser.ftl @@ -45,6 +45,8 @@ browser-main-window-mac = # `browser-main-window` and `browser-main-window-mac`. browser-main-window-title = { -brand-full-name } +private-browsing-shortcut-text = { -brand-short-name } Private Browsing + ## urlbar-identity-button = diff --git a/browser/locales/en-US/installer/custom.properties b/browser/locales/en-US/installer/custom.properties index 9127b8719971..37f676ef6f51 100644 --- a/browser/locales/en-US/installer/custom.properties +++ b/browser/locales/en-US/installer/custom.properties @@ -19,6 +19,7 @@ # from en-US contains a \n. REG_APP_DESC=$BrandShortName delivers safe, easy web browsing. A familiar user interface, enhanced security features including protection from online identity theft, and integrated search let you get the most out of the web. +PRIVATE_BROWSING_SHORTCUT_TITLE=$BrandShortName Private Browsing CONTEXT_OPTIONS=$BrandShortName &Options CONTEXT_SAFE_MODE=$BrandShortName &Safe Mode OPTIONS_PAGE_TITLE=Setup Type