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

VERIFIED FIXED in mozilla26

Status

()

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Paolo, Assigned: raymondlee)

Tracking

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 4 obsolete attachments)

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).
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Posted patch v1 (obsolete) — Splinter Review
Attachment #800006 - Flags: review?(paolo.mozmail)
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.
Attachment #800006 - Flags: review?(paolo.mozmail)
Posted patch v2 (obsolete) — Splinter Review
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?
Attachment #800006 - Attachment is obsolete: true
Attachment #800601 - Flags: feedback?(paolo.mozmail)
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
Attachment #800601 - Flags: feedback?(paolo.mozmail) → feedback+
Posted patch v3 (obsolete) — Splinter Review
The patch requires a method in the patch of bug 908240
Attachment #800601 - Attachment is obsolete: true
Attachment #801416 - Flags: review?(paolo.mozmail)
Depends on: 908240
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").
Attachment #801416 - Flags: review?(paolo.mozmail) → review+
Posted patch Patch for check-in (obsolete) — Splinter Review
Fixed the patch based on comment 6
Attachment #801416 - Attachment is obsolete: true
Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=7aa707752360
Attachment #802000 - Attachment is obsolete: true
(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!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ef225215ef97
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Marking as [qa-] as long as there is a mochi test in the patch.
Whiteboard: [qa-]
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.