908244 - When a download is launched automatically, the target file should be deleted when the browser is closed

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

nsDownloadManager.cpp queues downloaded files for deletion, in case the target
file is executed automatically:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#3347

We should probably do the same in the launchWhenSucceeded implementation (but
only once on completion, and not for target files launched by other code).

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[:Paolo Amadini]

Comment on attachment 800006 [details] [diff] [review]
v1

Review of attachment 800006 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +435,5 @@
> +              if (this.source.isPrivate) {
> +                gExternalAppLauncher.deleteTemporaryPrivateFileWhenPossible(
> +                                     new FileUtils.File(this.target.path))
> +
> +              } else if (deleteTempFileOnExit) {

We should just read the "browser.helperApps.deleteTempFileOnExit" preference here. We can define it in "modules/libpref/src/init/all.js" so that we don't need to check its existence nor set a default value in code (see also bug 909022).

It looks like its preferred default value in "all.js" should be false:

http://mxr.mozilla.org/mozilla-central/search?string=deleteTempFileOnExit

In "browser/app/profile/firefox.js", we can override the preference to true in a "#ifndef XP_MACOSX" block.

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ +1618,5 @@
> + * Tests that the temp download files are removed on exit and existing private
> + * mode after they have been launched.
> + */
> +add_task(function test_deleteTempFile() {
> +  if (!("nsILocalFileMac" in Ci)) {

We should just test both values of the preference on all platforms.

@@ +1621,5 @@
> +add_task(function test_deleteTempFile() {
> +  if (!("nsILocalFileMac" in Ci)) {
> +    let customLauncher = getTempFile("app-launcher");
> +    let privateTarget = getTempFile(TEST_TARGET_FILE_NAME);
> +    let target = getTempFile(TEST_TARGET_FILE_NAME);

Plaase use path strings and yield OS.File.exists() like in other similar tests, for consistency.

@@ +1642,5 @@
> +
> +    do_check_true(privateTarget.exists());
> +    do_check_true(target.exists());
> +
> +    yield Task.spawn(function() {

No need to use a nested task here.

@@ +1679,5 @@
> +      Services.obs.notifyObservers(null, "last-pb-context-exited", null);
> +      // Simulate browser shutdown
> +      Services.obs.notifyObservers(null, "profile-before-change", null);
> +
> +      return deferred.promise;

If you need to wait on the topics, you can write the patch on top of bug 908240 and use promiseTopicObserved.

In any case, the notifyObservers calls are synchronous, so after they return you can already check the existence of the files (with the promiseExecuteSoon, only in case it is still necessary) without setting up your own observers.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v2

Paolo: I have fixed the patch based on your comment.  However, I've just noticed that if we notify the "profile-before-change", the following tests wouldn't perform correctly, probably because some of the code thinks the browser is going to shutdown and get executed.

What's your suggestion?

Comment 4

[:Paolo Amadini]

Comment on attachment 800601 [details] [diff] [review]
v2

Review of attachment 800601 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Raymond Lee [:raymondlee] from comment #3)
> if we notify the "profile-before-change", the following tests
> wouldn't perform correctly, probably because some of the code thinks the
> browser is going to shutdown and get executed.

If you can get a reference to the nsIObserver interface of the helper app service, you can probably invoke its "observe" method with the topic directly, like we do for "places-debug-start-expiration".

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ +1771,5 @@
> +
> +  // Simulate leaving private browsing mode
> +  let promiseObserved = promiseTopicObserved("last-pb-context-exited");
> +  Services.obs.notifyObservers(null, "last-pb-context-exited", null);
> +  yield promiseObserved;

I think you don't need to wait for the topic being observed, as the file deletion is synchronous and Services.obs.notifyObservers only returns after the notification was processed by all observers, including this one:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1064

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v3

The patch requires a method in the patch of bug 908240

Comment 6

[:Paolo Amadini]

Comment on attachment 801416 [details] [diff] [review]
v3

Review of attachment 801416 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +430,5 @@
> +              // Always schedule files to be deleted at the end of the private browsing
> +              // mode, regardless of the value of the pref.
> +              if (this.source.isPrivate) {
> +                gExternalAppLauncher.deleteTemporaryPrivateFileWhenPossible(
> +                                     new FileUtils.File(this.target.path))

I just noticed the ";" is missing (also below).

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ +1725,5 @@
>    yield promiseDownloadStopped(download);
>  });
> +
> +/**
> + * Tests that the temp download files are removed on exit and existing private

"exiting"

@@ +1728,5 @@
> +/**
> + * Tests that the temp download files are removed on exit and existing private
> + * mode after they have been launched.
> + */
> +add_task(function test_deleteTempFile() {

nit: test_launchWhenSucceeded_deleteTempFileOnExit

@@ +1734,5 @@
> +
> +  let customLauncherPath = getTempFile("app-launcher").path;
> +  let privateTargetPath = getTempFile(TEST_TARGET_FILE_NAME).path;
> +  let targetPath = getTempFile(TEST_TARGET_FILE_NAME).path;
> +  let anotherTargetPath = getTempFile(TEST_TARGET_FILE_NAME).path;

nit: for clarity, rename to "autoDeleteTargetPath" / "noAutoDeleteTargetPath" (and "autoDeleteDownload" / "noAutoDeleteDownload").

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Fixed the patch based on comment 6

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=7aa707752360

Comment 9

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #8)
> Created attachment 802740 [details] [diff] [review]
> Patch for checkin
> 
> Pushed to try and waiting for results
> https://tbpl.mozilla.org/?tree=Try&rev=7aa707752360

Passed try!

Comment 10

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/ef225215ef97

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef225215ef97

Comment 12

[Mihai Morar, (:MihaiMorar)]

Marking as [qa-] as long as there is a mochi test in the patch.

Comment 13

[Mihai Morar, (:MihaiMorar)]

In addition I saw this patch had a big impact on the FF builds and I did a couple of verifications on Windows 7x64 and Ubuntu 13.04 using Latest Nightly 27 and I can confirm this functionality of Download Manager is working as expected.

Marking as Verified Fixed based on these verifications.