[tor-commits] [tor-browser] 50/73: Bug 1739348 - Don't open downloads panel after download dialogs. r=NeilDeakin, a=RyanVM

This is an automated email from the git hooks/post-receive script.

richard pushed a commit to branch geckoview-102.3.0esr-12.0-1 in repository tor-browser.

commit 226da410d935623597a0b535222d4386031b1610 Author: Shane Hughes shmediaproductions@gmail.com AuthorDate: Mon Jul 18 20:45:41 2022 +0000

Bug 1739348 - Don't open downloads panel after download dialogs. r=NeilDeakin, a=RyanVM

This is a medium sized patch to legacy download construction. It takes advantage of the new property added in Bug 1762033 to prevent the downloads panel from being automatically shown when a download is added after an interaction with the unknown content type dialog or the file picker dialog. I chose to not do the same for failed transfers since I thought it might serve some use, but that might be wrong. I don't know if there's a way to test the dialog that appears when you download an executable without going through the same path I adjusted with the patch. It seems like it's covered but I could be wrong. Also add a test to cover these changes from the bottom up. Thanks and apologies for my sloppy C++, though I'm sure I'll learn a lot more from the review 😅

Differential Revision: https://phabricator.services.mozilla.com/D145312 --- browser/components/downloads/DownloadsCommon.jsm | 14 +- .../components/downloads/test/browser/browser.ini | 5 + .../test/browser/browser_downloads_panel_opens.js | 236 ++++++++++++++++++++- .../downloads/test/browser/browser_tempfilename.js | 38 +++- toolkit/components/downloads/DownloadLegacy.jsm | 10 +- .../pdfjs/test/browser_pdfjs_download_button.js | 43 +--- toolkit/mozapps/downloads/HelperAppDlg.jsm | 3 +- uriloader/base/nsITransfer.idl | 9 +- .../exthandler/nsExternalHelperAppService.cpp | 27 ++- uriloader/exthandler/nsExternalHelperAppService.h | 8 +- .../exthandler/nsIExternalHelperAppService.idl | 5 +- .../browser_open_internal_choice_persistence.js | 4 +- 12 files changed, 344 insertions(+), 58 deletions(-)

diff --git a/browser/components/downloads/DownloadsCommon.jsm b/browser/components/downloads/DownloadsCommon.jsm index 422be505a3156..11e119a0155ea 100644 --- a/browser/components/downloads/DownloadsCommon.jsm +++ b/browser/components/downloads/DownloadsCommon.jsm @@ -1003,14 +1003,18 @@ DownloadsDataCtor.prototype = { browserWin == Services.focus.activeWindow && gAlwaysOpenPanel;

+ // For new downloads after the first one, don't show the panel + // automatically, but provide a visible notification in the topmost browser + // window, if the status indicator is already visible. Also ensure that if + // openDownloadsListOnStart = false is passed, we always skip opening the + // panel. That's because this will only be passed if the download is started + // without user interaction or if a dialog was previously opened in the + // process of the download (e.g. unknown content type dialog). if ( - this.panelHasShownBefore && aType != "error" && - !shouldOpenDownloadsPanel + ((this.panelHasShownBefore && !shouldOpenDownloadsPanel) || + !openDownloadsListOnStart) ) { - // For new downloads after the first one, don't show the panel - // automatically, but provide a visible notification in the topmost - // browser window, if the status indicator is already visible. DownloadsCommon.log("Showing new download notification."); browserWin.DownloadsIndicatorView.showEventNotification(aType); return; diff --git a/browser/components/downloads/test/browser/browser.ini b/browser/components/downloads/test/browser/browser.ini index f803f34daa988..6098bd3caa7c6 100644 --- a/browser/components/downloads/test/browser/browser.ini +++ b/browser/components/downloads/test/browser/browser.ini @@ -48,6 +48,11 @@ support-files = [browser_downloads_panel_dontshow.js] [browser_downloads_panel_height.js] [browser_downloads_panel_opens.js] +skip-if = + os == "linux" && verify && !debug # For some reason linux opt verify builds time out. +support-files = + foo.txt + foo.txt^headers^ [browser_downloads_autohide.js] [browser_go_to_download_page.js] [browser_pdfjs_preview.js] diff --git a/browser/components/downloads/test/browser/browser_downloads_panel_opens.js b/browser/components/downloads/test/browser/browser_downloads_panel_opens.js index e56b24fd0e3bf..fb1e6f76dabb9 100644 --- a/browser/components/downloads/test/browser/browser_downloads_panel_opens.js +++ b/browser/components/downloads/test/browser/browser_downloads_panel_opens.js @@ -1,6 +1,10 @@ /* Any copyright is dedicated to the Public Domain. * http://creativecommons.org/publicdomain/zero/1.0/ */

+let { MockFilePicker } = SpecialPowers; +MockFilePicker.init(window); +registerCleanupFunction(() => MockFilePicker.cleanup()); + /** * Check that the downloads panel opens when a download is spoofed. */ @@ -85,6 +89,175 @@ function clickCheckbox(checkbox) { checkbox.parentElement.hidePopup(); }

+/** + * Test that the downloads panel correctly opens or doesn't open based on + * whether the download triggered a dialog already. If askWhereToSave is true, + * we should get a file picker dialog. If preferredAction is alwaysAsk, we + * should get an unknown content type dialog. If neither of those is true, we + * should get no dialog at all, and expect the downloads panel to open. + * @param {boolean} [expectPanelToOpen] true - fail if panel doesn't open + * false (default) - fail if it opens + * @param {number} [preferredAction] Default download action: + * 0 (default) - save download to disk + * 1 - open UCT dialog first + * @param {boolean} [askWhereToSave] true - open file picker dialog + * false (default) - use download dir + */ +async function testDownloadsPanelAfterDialog({ + expectPanelToOpen = false, + preferredAction, + askWhereToSave = false, +} = {}) { + const { saveToDisk, alwaysAsk } = Ci.nsIHandlerInfo; + if (![saveToDisk, alwaysAsk].includes(preferredAction)) { + preferredAction = saveToDisk; + } + const openUCT = preferredAction === alwaysAsk; + const TEST_PATH = getRootDirectory(gTestPath).replace( + "chrome://mochitests/content", + "https://example.com" + ); + const MimeSvc = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService); + const HandlerSvc = Cc["@mozilla.org/uriloader/handler-service;1"].getService( + Ci.nsIHandlerService + ); + let publicList = await Downloads.getList(Downloads.PUBLIC); + + for (let download of await publicList.getAll()) { + await publicList.remove(download); + } + + // We need to test the changes from bug 1739348, where the helper app service + // sets a flag based on whether a file picker dialog was opened, and this flag + // determines whether the downloads panel will be opened as the download + // starts. We need to actually hit "Save" for the download to start, but we + // can't interact with the real file picker dialog. So this temporarily + // replaces it with a barebones component that plugs into the helper app + // service and tells it to start saving the file to the default path. + if (askWhereToSave) { + MockFilePicker.returnValue = MockFilePicker.returnOK; + MockFilePicker.showCallback = function(fp) { + // Get the default location from the helper app service. + let testFile = MockFilePicker.displayDirectory.clone(); + testFile.append(fp.defaultString); + info("File picker download path: " + testFile.path); + MockFilePicker.setFiles([testFile]); + MockFilePicker.filterIndex = 0; // kSaveAsType_Complete + MockFilePicker.showCallback = null; + // Confirm that saving should proceed. The helper app service uses this + // value to determine whether to invoke launcher.saveDestinationAvailable + return MockFilePicker.returnOK; + }; + } + + await SpecialPowers.pushPrefEnv({ + set: [ + ["browser.download.useDownloadDir", !askWhereToSave], + ["browser.download.always_ask_before_handling_new_types", openUCT], + ["browser.download.improvements_to_download_panel", true], + ["security.dialog_enable_delay", 0], + ], + }); + + // Configure the handler for the file according to parameters. + let mimeInfo = MimeSvc.getFromTypeAndExtension("text/plain", "txt"); + let existed = HandlerSvc.exists(mimeInfo); + mimeInfo.alwaysAskBeforeHandling = openUCT; + mimeInfo.preferredAction = preferredAction; + HandlerSvc.store(mimeInfo); + registerCleanupFunction(async () => { + // Reset the handler to its original state. + if (existed) { + HandlerSvc.store(mimeInfo); + } else { + HandlerSvc.remove(mimeInfo); + } + await publicList.removeFinished(); + BrowserTestUtils.removeTab(loadingTab); + }); + + let dialogWindowPromise = BrowserTestUtils.domWindowOpenedAndLoaded(); + let downloadFinishedPromise = new Promise(resolve => { + publicList.addView({ + onDownloadChanged(download) { + info("Download changed!"); + if (download.succeeded || download.error) { + info("Download succeeded or failed."); + publicList.removeView(this); + resolve(download); + } + }, + }); + }); + let panelOpenedPromise = expectPanelToOpen ? promisePanelOpened() : null; + + // Open the tab that will trigger the download. + let loadingTab = await BrowserTestUtils.openNewForegroundTab({ + gBrowser, + opening: TEST_PATH + "foo.txt", + waitForLoad: false, + waitForStateStop: true, + }); + + // Wait for a UCT dialog if the handler was set up to open one. + if (openUCT) { + let dialogWindow = await dialogWindowPromise; + is( + dialogWindow.location.href, + "chrome://mozapps/content/downloads/unknownContentType.xhtml", + "Should have seen the unknown content dialogWindow." + ); + let doc = dialogWindow.document; + let dialog = doc.getElementById("unknownContentType"); + let radio = doc.getElementById("save"); + let button = dialog.getButton("accept"); + + await TestUtils.waitForCondition( + () => !button.disabled, + "Waiting for the UCT dialog's Accept button to be enabled." + ); + ok(!radio.hidden, "The Save option should be visible"); + // Make sure we aren't opening the file. + radio.click(); + ok(radio.selected, "The Save option should be selected"); + button.disabled = false; + dialog.acceptDialog(); + } + + info("Waiting for download to finish."); + let download = await downloadFinishedPromise; + ok(!download.error, "There should be no error."); + is( + DownloadsPanel.isPanelShowing, + expectPanelToOpen, + `Panel should${expectPanelToOpen ? " " : " not "}be showing.` + ); + if (DownloadsPanel.isPanelShowing) { + await panelOpenedPromise; + let hiddenPromise = BrowserTestUtils.waitForPopupEvent( + DownloadsPanel.panel, + "hidden" + ); + DownloadsPanel.hidePanel(); + await hiddenPromise; + } + if (download?.target.exists) { + try { + info("Removing test file: " + download.target.path); + if (Services.appinfo.OS === "WINNT") { + await IOUtils.setPermissions(download.target.path, 0o600); + } + await IOUtils.remove(download.target.path); + } catch (ex) { + /* ignore */ + } + } + for (let dl of await publicList.getAll()) { + await publicList.remove(dl); + } + BrowserTestUtils.removeTab(loadingTab); +} + /** * Make sure the downloads panel opens automatically with a new download. */ @@ -93,6 +266,7 @@ add_task(async function test_downloads_panel_opens() { set: [ ["browser.download.improvements_to_download_panel", true], ["browser.download.always_ask_before_handling_new_types", false], + ["browser.download.alwaysOpenPanel", true], ], }); await checkPanelOpens(); @@ -103,6 +277,7 @@ add_task(async function test_customizemode_doesnt_wreck_things() { set: [ ["browser.download.improvements_to_download_panel", true], ["browser.download.always_ask_before_handling_new_types", false], + ["browser.download.alwaysOpenPanel", true], ], });

@@ -114,7 +289,7 @@ add_task(async function test_customizemode_doesnt_wreck_things() { gCustomizeMode.enter(); await customizationReadyPromise;

- info("try to open the panel (will not work, in customize mode)"); + info("Try to open the panel (will not work, in customize mode)"); let promise = promisePanelOpened(); DownloadsCommon.getData(window)._notifyDownloadEvent("start"); await TestUtils.waitForCondition( @@ -134,10 +309,16 @@ add_task(async function test_customizemode_doesnt_wreck_things() { gCustomizeMode.exit(); await afterCustomizationPromise;

+ // Avoid a failure on Linux where the window isn't active for some reason, + // which prevents the window's downloads panel from opening. + if (Services.focus.activeWindow != window) { + info("Main window is not active, trying to focus."); + await SimpleTest.promiseFocus(window); + is(Services.focus.activeWindow, window, "Main window should be active."); + } DownloadsCommon.getData(window)._notifyDownloadEvent("start"); - is( - DownloadsPanel.isPanelShowing, - true, + await TestUtils.waitForCondition( + () => DownloadsPanel.isPanelShowing, "Panel state should indicate a preparation to be opened" ); await promise; @@ -456,3 +637,50 @@ add_task(async function test_alwaysOpenPanel_menuitem() {

await checkPanelOpens(); }); + +/** + * Verify that the downloads panel opens if the download did not open a file + * picker or UCT dialog + */ +add_task(async function test_downloads_panel_after_no_dialogs() { + await testDownloadsPanelAfterDialog({ expectPanelToOpen: true }); + ok(true, "Downloads panel opened because no dialogs were opened."); +}); + +/** + * Verify that the downloads panel doesn't open if the download opened an + * unknown content type dialog (e.g. action = always ask) + */ +add_task(async function test_downloads_panel_after_UCT_dialog() { + await testDownloadsPanelAfterDialog({ + expectPanelToOpen: false, + preferredAction: Ci.nsIHandlerInfo.alwaysAsk, + }); + ok(true, "Downloads panel suppressed after UCT dialog."); +}); + +/** + * Verify that the downloads panel doesn't open if the download opened a file + * picker dialog (e.g. useDownloadDir = false) + */ +add_task(async function test_downloads_panel_after_file_picker_dialog() { + await testDownloadsPanelAfterDialog({ + expectPanelToOpen: false, + preferredAction: Ci.nsIHandlerInfo.saveToDisk, + askWhereToSave: true, + }); + ok(true, "Downloads panel suppressed after file picker dialog."); +}); + +/** + * Verify that the downloads panel doesn't open if the download opened both + * dialogs (e.g. default action = always ask AND useDownloadDir = false) + */ +add_task(async function test_downloads_panel_after_both_dialogs() { + await testDownloadsPanelAfterDialog({ + expectPanelToOpen: false, + preferredAction: Ci.nsIHandlerInfo.alwaysAsk, + askWhereToSave: true, + }); + ok(true, "Downloads panel suppressed after UCT and file picker dialogs."); +}); diff --git a/browser/components/downloads/test/browser/browser_tempfilename.js b/browser/components/downloads/test/browser/browser_tempfilename.js index b995f078895f1..dcef4016a6fbe 100644 --- a/browser/components/downloads/test/browser/browser_tempfilename.js +++ b/browser/components/downloads/test/browser/browser_tempfilename.js @@ -16,6 +16,27 @@ add_task(async function test_tempfilename() { }; list.addView(view); }); + + await SpecialPowers.pushPrefEnv({ + set: [ + ["browser.download.improvements_to_download_panel", true], + ["browser.download.always_ask_before_handling_new_types", false], + ], + }); + + const MimeSvc = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService); + const HandlerSvc = Cc["@mozilla.org/uriloader/handler-service;1"].getService( + Ci.nsIHandlerService + ); + let mimeInfo = MimeSvc.getFromTypeAndExtension( + HandlerSvc.getTypeFromExtension("txt"), + "txt" + ); + let existed = HandlerSvc.exists(mimeInfo); + mimeInfo.alwaysAskBeforeHandling = false; + mimeInfo.preferredAction = Ci.nsIHandlerInfo.saveToDisk; + HandlerSvc.store(mimeInfo); + serveInterruptibleAsDownload(); mustInterruptResponses(); await BrowserTestUtils.withNewTab( @@ -27,7 +48,21 @@ add_task(async function test_tempfilename() { }, async () => { let download = await downloadStarted; - registerCleanupFunction(() => download.finalize()); + registerCleanupFunction(async () => { + if (existed) { + HandlerSvc.store(mimeInfo); + } else { + HandlerSvc.remove(mimeInfo); + } + await download.finalize(true); + if (Services.appinfo.OS === "WINNT") { + // We need to make the file writable to delete it on Windows. + await IOUtils.setPermissions(download.target.path, 0o600); + } + await IOUtils.remove(download.target.path); + await download.finalize(); + await list.removeFinished(); + });

let { partFilePath } = download.target; Assert.stringContains( @@ -51,7 +86,6 @@ add_task(async function test_tempfilename() { !(await IOUtils.exists(download.target.partFilePath)), "Temp file should be gone." ); - await IOUtils.remove(download.target.path); } ); }); diff --git a/toolkit/components/downloads/DownloadLegacy.jsm b/toolkit/components/downloads/DownloadLegacy.jsm index 14b9b8b8780fd..7ff95b53190c6 100644 --- a/toolkit/components/downloads/DownloadLegacy.jsm +++ b/toolkit/components/downloads/DownloadLegacy.jsm @@ -274,7 +274,8 @@ DownloadLegacyTransfer.prototype = { aCancelable, aIsPrivate, aDownloadClassification, - aReferrerInfo + aReferrerInfo, + aOpenDownloadsListOnStart ) { return this._nsITransferInitInternal( aSource, @@ -287,7 +288,8 @@ DownloadLegacyTransfer.prototype = { aCancelable, aIsPrivate, aDownloadClassification, - aReferrerInfo + aReferrerInfo, + aOpenDownloadsListOnStart ); },

@@ -303,6 +305,7 @@ DownloadLegacyTransfer.prototype = { aIsPrivate, aDownloadClassification, aReferrerInfo, + aOpenDownloadsListOnStart, aBrowsingContext, aHandleInternally, aHttpChannel @@ -327,6 +330,7 @@ DownloadLegacyTransfer.prototype = { aIsPrivate, aDownloadClassification, aReferrerInfo, + aOpenDownloadsListOnStart, userContextId, browsingContextId, aHandleInternally, @@ -346,6 +350,7 @@ DownloadLegacyTransfer.prototype = { isPrivate, aDownloadClassification, referrerInfo, + openDownloadsListOnStart = true, userContextId = 0, browsingContextId = 0, handleInternally = false, @@ -397,6 +402,7 @@ DownloadLegacyTransfer.prototype = { contentType, launcherPath, handleInternally, + openDownloadsListOnStart, };

// In case the Download was classified as insecure/dangerous diff --git a/toolkit/components/pdfjs/test/browser_pdfjs_download_button.js b/toolkit/components/pdfjs/test/browser_pdfjs_download_button.js index b0040a71941f6..fa6b6d6c978a9 100644 --- a/toolkit/components/pdfjs/test/browser_pdfjs_download_button.js +++ b/toolkit/components/pdfjs/test/browser_pdfjs_download_button.js @@ -52,22 +52,6 @@ add_setup(async function() { }); });

-async function closeDownloadsPanel() { - if (DownloadsPanel.panel.state !== "closed") { - let hiddenPromise = BrowserTestUtils.waitForEvent( - DownloadsPanel.panel, - "popuphidden" - ); - DownloadsPanel.hidePanel(); - await hiddenPromise; - } - is( - DownloadsPanel.panel.state, - "closed", - "Check that the download panel is closed" - ); -} - /** * Check clicking the download button saves the file and doesn't open a new viewer */ @@ -75,7 +59,10 @@ add_task(async function test_downloading_pdf_nonprivate_window() { const pdfUrl = TESTROOT + "file_pdfjs_test.pdf";

await SpecialPowers.pushPrefEnv({ - set: [["browser.download.improvements_to_download_panel", true]], + set: [ + ["browser.download.improvements_to_download_panel", true], + ["browser.download.always_ask_before_handling_new_types", false], + ], });

await BrowserTestUtils.withNewTab( @@ -87,7 +74,6 @@ add_task(async function test_downloading_pdf_nonprivate_window() { info(`${tabCount} tabs are open at the start of the test`);

let downloadList = await Downloads.getList(Downloads.PUBLIC); - const initialDownloadCount = (await downloadList.getAll()).length;

let filePickerShown = createPromiseForFilePicker();

@@ -100,23 +86,16 @@ add_task(async function test_downloading_pdf_nonprivate_window() { info("Waiting for a filename to be picked from the file picker"); await filePickerShown;

- // check that resulted in a download being added to the list. The - // download panel should not open. await downloadFinishedPromise; - is( - DownloadsPanel.panel.state, - "closed", - "Check the download panel state is 'closed'" - ); - downloadList = await Downloads.getList(Downloads.PUBLIC); - const allDownloads = await downloadList.getAll(); - let currentDownloadCount = allDownloads.length; - is( - currentDownloadCount, - initialDownloadCount + 1, - "A download was added when we clicked download" + ok(true, "A download was added when we clicked download"); + + // See bug 1739348 - don't show panel for downloads that opened dialogs + ok( + !DownloadsPanel.isPanelShowing, + "The download panel did not open, since the file picker was shown already" );

+ const allDownloads = await downloadList.getAll(); const dl = allDownloads.find(dl => dl.source.originalUrl === pdfUrl); ok(!!dl, "The pdf download has the correct url in source.originalUrl");

diff --git a/toolkit/mozapps/downloads/HelperAppDlg.jsm b/toolkit/mozapps/downloads/HelperAppDlg.jsm index 325de51c0c5fe..0a5a4a460bf38 100644 --- a/toolkit/mozapps/downloads/HelperAppDlg.jsm +++ b/toolkit/mozapps/downloads/HelperAppDlg.jsm @@ -409,7 +409,8 @@ nsUnknownContentTypeDialog.prototype = { } } } - aLauncher.saveDestinationAvailable(result); + // Don't pop up the downloads panel redundantly. + aLauncher.saveDestinationAvailable(result, true); }); }); })().catch(Cu.reportError); diff --git a/uriloader/base/nsITransfer.idl b/uriloader/base/nsITransfer.idl index ba701c982d7a0..828e4c4a0f7bc 100644 --- a/uriloader/base/nsITransfer.idl +++ b/uriloader/base/nsITransfer.idl @@ -62,7 +62,12 @@ interface nsITransfer : nsIWebProgressListener2 { * * @param aDownloadClassification Indicates wheter the download is unwanted, * should be considered dangerous or insecure. + * * @param aReferrerInfo The Referrer this download is started with + * + * @param aOpenDownloadsListOnStart true (default) - Open downloads panel. + * false - Only show an icon indicator. + * This parameter is optional. */ void init(in nsIURI aSource, in nsIURI aSourceOriginalURI, @@ -74,7 +79,8 @@ interface nsITransfer : nsIWebProgressListener2 { in nsICancelable aCancelable, in boolean aIsPrivate, in long aDownloadClassification, - in nsIReferrerInfo aReferrerInfo); + in nsIReferrerInfo aReferrerInfo, + [optional] in boolean aOpenDownloadsListOnStart);

/** * Same as init, but allows for passing the browsingContext @@ -97,6 +103,7 @@ interface nsITransfer : nsIWebProgressListener2 { in boolean aIsPrivate, in long aDownloadClassification, in nsIReferrerInfo aReferrerInfo, + [optional] in boolean aOpenDownloadsListOnStart, in BrowsingContext aBrowsingContext, in boolean aHandleInternally, in nsIHttpChannel aHttpChannel); diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/exthandler/nsExternalHelperAppService.cpp index adb05b3c46b85..f5b0e2e5bc95c 100644 --- a/uriloader/exthandler/nsExternalHelperAppService.cpp +++ b/uriloader/exthandler/nsExternalHelperAppService.cpp @@ -1280,6 +1280,7 @@ nsExternalAppHandler::nsExternalAppHandler( mIsFileChannel(false), mShouldCloseWindow(false), mHandleInternally(false), + mDialogShowing(false), mReason(aReason), mTempFileIsExecutable(false), mTimeDownloadStarted(0), @@ -1855,6 +1856,9 @@ NS_IMETHODIMP nsExternalAppHandler::OnStartRequest(nsIRequest* request) { // this will create a reference cycle (the dialog holds a reference to us as // nsIHelperAppLauncher), which will be broken in Cancel or CreateTransfer. nsCOMPtr<nsIInterfaceRequestor> dialogParent = GetDialogParent(); + // Don't pop up the downloads panel since we're already going to pop up the + // UCT dialog for basically the same effect. + mDialogShowing = true; rv = mDialog->Show(this, dialogParent, mReason);

// what do we do if the dialog failed? I guess we should call Cancel and @@ -2351,14 +2355,15 @@ nsresult nsExternalAppHandler::CreateTransfer() { rv = transfer->InitWithBrowsingContext( mSourceUrl, target, u""_ns, mMimeInfo, mTimeDownloadStarted, mTempFile, this, channel && NS_UsePrivateBrowsing(channel), - mDownloadClassification, referrerInfo, mBrowsingContext, - mHandleInternally, nullptr); + mDownloadClassification, referrerInfo, !mDialogShowing, + mBrowsingContext, mHandleInternally, nullptr); } else { rv = transfer->Init(mSourceUrl, nullptr, target, u""_ns, mMimeInfo, mTimeDownloadStarted, mTempFile, this, channel && NS_UsePrivateBrowsing(channel), - mDownloadClassification, referrerInfo); + mDownloadClassification, referrerInfo, !mDialogShowing); } + mDialogShowing = false;

NS_ENSURE_SUCCESS(rv, rv);

@@ -2443,13 +2448,13 @@ nsresult nsExternalAppHandler::CreateFailedTransfer() { rv = transfer->InitWithBrowsingContext( mSourceUrl, pseudoTarget, u""_ns, mMimeInfo, mTimeDownloadStarted, mTempFile, this, channel && NS_UsePrivateBrowsing(channel), - mDownloadClassification, referrerInfo, mBrowsingContext, + mDownloadClassification, referrerInfo, true, mBrowsingContext, mHandleInternally, httpChannel); } else { rv = transfer->Init(mSourceUrl, nullptr, pseudoTarget, u""_ns, mMimeInfo, mTimeDownloadStarted, mTempFile, this, channel && NS_UsePrivateBrowsing(channel), - mDownloadClassification, referrerInfo); + mDownloadClassification, referrerInfo, true); } NS_ENSURE_SUCCESS(rv, rv);

@@ -2459,11 +2464,16 @@ nsresult nsExternalAppHandler::CreateFailedTransfer() { return NS_OK; }

-nsresult nsExternalAppHandler::SaveDestinationAvailable(nsIFile* aFile) { - if (aFile) +nsresult nsExternalAppHandler::SaveDestinationAvailable(nsIFile* aFile, + bool aDialogWasShown) { + if (aFile) { + if (aDialogWasShown) { + mDialogShowing = true; + } ContinueSave(aFile); - else + } else { Cancel(NS_BINDING_ABORTED); + }

return NS_OK; } @@ -2735,6 +2745,7 @@ NS_IMETHODIMP nsExternalAppHandler::Cancel(nsresult aReason) { // Break our reference cycle with the helper app dialog (set up in // OnStartRequest) mDialog = nullptr; + mDialogShowing = false;

mRequest = nullptr;

diff --git a/uriloader/exthandler/nsExternalHelperAppService.h b/uriloader/exthandler/nsExternalHelperAppService.h index ce142a4f5b6b0..439be1645f4fc 100644 --- a/uriloader/exthandler/nsExternalHelperAppService.h +++ b/uriloader/exthandler/nsExternalHelperAppService.h @@ -376,6 +376,12 @@ class nsExternalAppHandler final : public nsIStreamListener, */ bool mHandleInternally;

+ /** + * True if any dialog (e.g. unknown content type or file picker) is shown — + * can stop downloads panel from opening, to avoid redundant interruptions. + */ + bool mDialogShowing; + /** * One of the REASON_ constants from nsIHelperAppLauncherDialog. Indicates the * reason the dialog was shown (unknown content type, server requested it, @@ -522,7 +528,7 @@ class nsExternalAppHandler final : public nsIStreamListener, const nsString& path);

/** - * Set in nsHelperDlgApp.js. This is always null after the user has chosen an + * Set in HelperAppDlg.jsm. This is always null after the user has chosen an * action. */ nsCOMPtr<nsIWebProgressListener2> mDialogProgressListener; diff --git a/uriloader/exthandler/nsIExternalHelperAppService.idl b/uriloader/exthandler/nsIExternalHelperAppService.idl index 3554c69aaced1..307e6196a89df 100644 --- a/uriloader/exthandler/nsIExternalHelperAppService.idl +++ b/uriloader/exthandler/nsIExternalHelperAppService.idl @@ -150,8 +150,11 @@ interface nsIHelperAppLauncher : nsICancelable * Callback invoked by nsIHelperAppLauncherDialog::promptForSaveToFileAsync * after the user has chosen a file through the File Picker (or dismissed it). * @param aFile The file that was chosen by the user (or null if dialog was dismissed). + * @param aDialogWasShown Optional boolean - false by default. Pass true if a + * dialog was opened in the process of reaching this file result. If true, we + * suppress the opening of the downloads panel to avoid redundancy. */ - void saveDestinationAvailable(in nsIFile aFile); + void saveDestinationAvailable(in nsIFile aFile, [optional] in boolean aDialogWasShown);

/** * The following methods are used by the progress dialog to get or set diff --git a/uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js b/uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js index 11f9e24092053..e6dffa8df4ab4 100644 --- a/uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js +++ b/uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js @@ -33,11 +33,13 @@ function waitForAcceptButtonToGetEnabled(doc) { }

add_setup(async function() { - // Remove the security delay for the dialog during the test. await SpecialPowers.pushPrefEnv({ set: [ + // Remove the security delay for the dialog during the test. ["security.dialog_enable_delay", 0], ["browser.helperApps.showOpenOptionForViewableInternally", true], + // Make sure we don't open a file picker dialog somehow. + ["browser.download.useDownloadDir", true], ], });