658544 - Cleanup profiles from old and useless places.sqlite.corrupt files

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

[Marco Bonardo [:mak]]

A bunch of users still have some of these sticking in their profile folder, in some case they may take a non-ignorable disk space.
We should delete all files older than a couple weeks in maintenance code.

Comment 1

[Tiziana Sellitto [:tiziana]]

Marco, do you think this is still a valid issue? 
Perhaps you could add more technical info here that could attract someone for taking care of this.

Comment 2

[Marco Bonardo [:mak]]

yes, it's still a valid issue.

Comment 3

[Donato Sciarra]

Hi there! I would like to take this on. 
@Marco could you expand a bit on what needs to be done?
Thanks

Comment 4

[Nikhil Johny]

Hi, Is Donato working on this bug? If not, can someone please direct me on how to go about fixing this one. How can i reproduce this bug? Thanks.

Comment 5

[Marco Bonardo [:mak]]

let's ask him, I'm sorry but without a needinfo I missed comment 3

Btw, the scope here is to add a task to PlacesDBUtils.jsm that walks the profile directory and removes any places.sqlite.corrupt (we use unique name so it may be places.sqlite-N.corrupt iirc

Comment 6

[Marco Bonardo [:mak]]

well, not any corrupt files but only the ones older than N days

Comment 7

[Donato Sciarra]

Thanks Marco for following-up ;) I forgot the needinfo flag. 

I would like to take this on. I had a look at PlacesDBUtils.jsm and it should not be difficult to implement. Are there utility functions I could use? (list files in a directory, remove file, etc.)

Comment 8

[Marco Bonardo [:mak]]

(In reply to Donato Sciarra from comment #7)
> Thanks Marco for following-up ;) I forgot the needinfo flag. 
> 
> I would like to take this on. I had a look at PlacesDBUtils.jsm and it
> should not be difficult to implement. Are there utility functions I could
> use? (list files in a directory, remove file, etc.)

you should use OS.File to walk the directory and do the removals, so that everything happens off the main-thread
https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread
https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File.DirectoryIterator_for_the_main_thread
https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.Path

Comment 9

[Donato Sciarra]

Attachment: bug658544.patch

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8384562 [details] [diff] [review]
bug658544.patch

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

the approach looks ok, though this needs an xpcshell test. The test should create some .corrupt files in the profile folder, set their modified dates back in the past, then execute this task and check the files have been removed.
You can use add_task in the test (see other recent tests in toolkit/components/places/tests/unit/)

Also, this new task should be added to the maintenanceOnIdle tasks list

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +1026,5 @@
>    },
>  
>    /**
> +   * Cleanup profile from old and useless places.sqlite.corrupt files.
> +   * The file is deleted is older than N days.

typo: s/"is older"/"if older"

@@ +1032,5 @@
> +   * @param aTasks
> +   *        Array of tasks to be executed, in form of pointers to methods in
> +   *        this module.
> +   * @param [optional] aNDays
> +   *        Delete if older than aNDays days.

s/aNDays/aNumDays/ and here just "Delete if older than aNumDays"

@@ +1037,5 @@
> +   */
> +  cleanupDBProfile: function PDBU_cleanupDBProfile(aTasks, aNDays)
> +  {
> +    let tasks = new Tasks(aTasks);
> +    tasks.log("> Cleanup profile from places.sqlite.corrupt");

"from old places.sqlite.corrupt files"

@@ +1040,5 @@
> +    let tasks = new Tasks(aTasks);
> +    tasks.log("> Cleanup profile from places.sqlite.corrupt");
> +
> +    let iterator = new OS.File.DirectoryIterator(OS.Constants.Path.profileDir);
> +    let pattern = /places\.sqlite(-\d*)?\.corrupt$/;

for readability, please use new RegExp

@@ +1047,5 @@
> +
> +    // Iterate through the directory
> +    let promise = iterator.forEach(
> +       function onEntry(entry) {
> +         if (!entry.isDir && !entry.isSymLink && pattern.test(entry)){

add space between ) and {

@@ +1049,5 @@
> +    let promise = iterator.forEach(
> +       function onEntry(entry) {
> +         if (!entry.isDir && !entry.isSymLink && pattern.test(entry)){
> +           //convert milliseconds in days
> +           let days = (currentTime - entry.lastModificationDate.getTime())*0.011574074074074075;

I prefer if you define a const MS_PER_DAY at the top of the module and here divide by it

There's an issue here though, to use lastModificationDate you need to first call OS.File.stat and get a file info object.

I suggest, instead of getting a promise and such, you may lazy import module Task.jsm and here do something like this:

Task.spawn(function* () {
  let entries = yield new OS.File.DirectoryIterator(OS.Constants.Path.profileDir).nextBatch();
  for (let entry of entries) {
    let info = yield OS.File.stat(entry.path);
    // do whatever you should here
    console.log(entry.path + " " + info.lastModificationDate);
  }
}).then(log_success, log_failure)
  .then(() => this._executeTasks(tasks));

eventually you may even further improve that by checking "winLastWriteDate" in OS.File.DirectoryIterator.Entry.prototype, if it exists you can avoid a stat() call and use its value instead of lastModificationDate

@@ +1050,5 @@
> +       function onEntry(entry) {
> +         if (!entry.isDir && !entry.isSymLink && pattern.test(entry)){
> +           //convert milliseconds in days
> +           let days = (currentTime - entry.lastModificationDate.getTime())*0.011574074074074075;
> +           if(days >= aNDays){

please space as "if (condition) {"

Comment 11

[Marco Bonardo [:mak]]

PS: I figure there's some name conflict here between maintenance Task and Task that is a promises helper... I think in future we'll make maintenance tasks be actual Task as defined by Task.jsm. In case you have issues with my comments above due to this conflict please just ask.

Comment 12

[Donato Sciarra]

Attachment: bug658544.patch

Comment 13

[Donato Sciarra]

Attachment: test_658544.js

Comment 14

[Donato Sciarra]

I have made some progress but there are some subtle Javascript problems with the test. The test fails with a long list of coding exception. I have also tried to ./mach --sequential but nothing changed. Help is much appreciated! Thanks!

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8395604 [details] [diff] [review]
bug658544.patch

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

I wonder if the mistake I found here may be enough to solve your test problems

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +27,5 @@
>  const BYTES_PER_MEBIBYTE = 1048576;
>  
> +const MS_PER_DAY = 86400000;
> +
> +const DAYS_CORRUPTED_DB_IS_OLD = 14;

I'd suggest CORRUPTED_DB_RETAIN_DAYS

@@ +1041,5 @@
> +   * @param [optional] aTasks
> +   *        Array of tasks to be executed, in form of pointers to methods in
> +   *        this module.
> +   */
> +  cleanupDBProfile: function PDBU_cleanupDBProfile(aTasks)

I'd suggest to rename to removeOldCorruptDBs

nit: there's no more need to define both a property name and a function name, it was needed time ago to have proper stacks, but now js can properly unwind the stacks. so now you can just do
removeOldCorruptDBs : function () {

@@ +1043,5 @@
> +   *        this module.
> +   */
> +  cleanupDBProfile: function PDBU_cleanupDBProfile(aTasks)
> +  {
> +    this.cleanupDBProfileDays(DAYS_CORRUPTED_DB_IS_OLD, aTasks);

I think we don't need to abstract away to a separate method for now, just inline the code here. Generally we try to not code for "future changes" since very often our predictions failed :)

@@ +1062,5 @@
> +    let tasks = new Tasks(aTasks);
> +    tasks.log("> Cleanup profile from places.sqlite.corrupt files older than " + aNumDays + " days.");
> +
> +    Task.spawn(function* () {
> +        let regExp = new RegExp("places\.sqlite(-\d*)?\.corrupt$");

looks like your text editor is indenting too much here, just 2 spaces please.

@@ +1069,5 @@
> +        let modificationTime = 0;
> +        let iterator = new OS.File.DirectoryIterator(OS.Constants.Path.profileDir);
> +        let entries = yield iterator.nextBatch();
> +        for (let entry of entries) {
> +          if (!entry.isDir && !entry.isSymLink && regExp.test(entry)) {

I think you may test against Os.Path.basename(entry) (so just the filename)

@@ +1070,5 @@
> +        let iterator = new OS.File.DirectoryIterator(OS.Constants.Path.profileDir);
> +        let entries = yield iterator.nextBatch();
> +        for (let entry of entries) {
> +          if (!entry.isDir && !entry.isSymLink && regExp.test(entry)) {
> +            if("winLastWriteDate" in entry) {

missing space after if

@@ +1074,5 @@
> +            if("winLastWriteDate" in entry) {
> +              modificationTime = entry.winLastWriteDate;
> +            } else {
> +              let info = yield OS.File.stat(entry.path);
> +              modificationTime = info.lastModificationDate.getTime();

I think both winLastWriteDate and lastModificationDate are Date objects, so why getTime on one and not the other one? (fwiw later an implicit conversion will happen, making both work, so I'm fine being explicit)

@@ +1076,5 @@
> +            } else {
> +              let info = yield OS.File.stat(entry.path);
> +              modificationTime = info.lastModificationDate.getTime();
> +            }
> +            //convert milliseconds in days

missing space after //
s/in/to/
nit: start comments with uppercase and end with a period.

@@ +1077,5 @@
> +              let info = yield OS.File.stat(entry.path);
> +              modificationTime = info.lastModificationDate.getTime();
> +            }
> +            //convert milliseconds in days
> +            let days = (currentTime - modificationTime)/MS_PER_DAY;

please add spaces around /

@@ +1078,5 @@
> +              modificationTime = info.lastModificationDate.getTime();
> +            }
> +            //convert milliseconds in days
> +            let days = (currentTime - modificationTime)/MS_PER_DAY;
> +            if (days >= aNumDays){

missing space between ) and {

I think we should add a further OR condition here, to protect against files with future date.
in that case currentTime = modificationTime is negative, we could say
// Remove old files, or files with future date.
if (days >= aNumDays || days < -1) {

here I'm just using 1 day since there may be a skew between js time and system time and we don't want to wrongly remove just created corrupt files just case they are 10ms in the future.

@@ +1088,5 @@
> +            }
> +          }
> +        }
> +      }).then(
> +        //log success

I'd say to remove this comment, the function name is pretty much self-documenting

@@ +1090,5 @@
> +        }
> +      }).then(
> +        //log success
> +        function success(value) {
> +          iterator.close();

If I read this correctly, iterator is defined inside the task, so I don't think this is working.

@@ +1092,5 @@
> +        //log success
> +        function success(value) {
> +          iterator.close();
> +          tasks.log(counter>0 ? "> Removed " + counter + "corrupted file(s)."
> +                    : "> No corrupted files to remove.");

and ditto for counter
inside task you can use try/finally, so just move iterator.close() into a finally.

You are not seeing these errors reported cause you didn't add a failure handler in the later then...

@@ +1096,5 @@
> +                    : "> No corrupted files to remove.");
> +        }, 
> +        //log failure
> +        function failure(reason) {
> +          iterator.close();

ditto

@@ +1100,5 @@
> +          iterator.close();
> +          tasks.log("> " + reason);
> +        }       
> +    )
> +    .then(() => this._executeTasks(tasks));

...you can add a ", Cu.reportError)" here to handle exceptions thrown by the handlers above.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8395605 [details]
test_658544.js

next version of the patch please hg add the test file.
Also, please rename the test so that it contains a description of it, rather than just a bug number, in the past we used just bug# in tests names and now we must open them to figure what they do :)
something like test_PlacesDBUtils_removeCorruptDBs.js would work

>function printDirectory() {
>  Task.spawn(function() {
>      let iterator = new OS.File.DirectoryIterator(OS.Constants.Path.profileDir);
>      let entries = yield iterator.nextBatch();
>      for (let entry of entries) {
>        do_print(entry);

here you can probably use .forEach(do_print)

>function deleteCorruptedFiles(){
>  for(let i=0; i<TEST_FILES_TO_CREATE; i++){

our coding style loves spacing, so "for (let i = 0; i < TEST...; i++) {"
this is valid for any construct basically (you can find more here https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style)
also indentation is always 2 spaces.

>              let outfile = yield OS.File.open(filePath, {write: true});
>              let setTime = TEST_CURRENT_TIME - MS_PER_DAY*i;
>              // do_print('createCorruptedFiles, file i: ' + filePath);
>              yield outfile.write(fileName);
>              yield outfile.setDates(setTime, setTime);
>              outfile.close();

close() returns a promise, considered Windows has the bad habit to keep handles locked I'd probably yield here


>  return Math.max(0, TEST_FILES_TO_CREATE-DAYS_CORRUPTED_DB_IS_OLD);

I'd have nothing against you making the const an actual value exported by PlacesDBUtils, like PlacesDBUtils.CORRUPT_DB_RETAIN_DAYS

>add_task(test_cleanupDBProfile);

just inline the test like

add_task(function* () {
  // your test
});

Comment 17

[Donato Sciarra]

Attachment: bug658544.patch

So here is my new tentative. I am still not able to run the test though. I think there is something fishy in the test or in the code which I cannot spot.

Comment 18

[Donato Sciarra]

Here the output of the test: http://pastebin.com/aUYn0zTc

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8442792 [details] [diff] [review]
bug658544.patch

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

=

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +1045,5 @@
> +   */
> +  removeOldCorruptDBs: function (aTasks) {
> +    let tasks = new Tasks(aTasks);
> +    tasks.log("> Cleanup profile from places.sqlite.corrupt files older than " + CORRUPTED_DB_RETAIN_DAYS + " days.");
> +    Task.spawn( function* () {

As a rule of thumb, you should generally return the promise.

@@ +1081,5 @@
> +      function failure(reason) {
> +        yield iterator.close();
> +        tasks.log("> " + reason);
> +      }
> +    ).then( () => this._executeTasks(tasks), Cu.reportError);

Since you are using `Task`, you should replace these `then` with `try/catch/finally`.

Comment 20

[Donato Sciarra]

Attachment: bug658544.patch

I have modified the function as David suggested but the test still fails. This is the output: http://pastebin.com/mDjKV4eU

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8443047 [details] [diff] [review]
bug658544.patch

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

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +11,5 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/PlacesUtils.jsm");
> +Cu.import("resource://gre/modules/osfile.jsm");

Since you are generally not using OS.File, you should make this a lazy module getter.

@@ +1031,5 @@
>  
>      PlacesDBUtils._executeTasks(tasks);
>    },
>  
> +  // It controls the behavior of removeOldCorruptDBs.

I don't understand this comment.

@@ +1046,5 @@
> +  removeOldCorruptDBs: function (aTasks) {
> +    let tasks = new Tasks(aTasks);
> +    tasks.log("> Cleanup profile from places.sqlite.corrupt files older than " + this.CORRUPTED_DB_RETAIN_DAYS + " days.");
> +    return Task.spawn( function* () {
> +      let regExp = new RegExp("places\.sqlite(-\d*)?\.corrupt$");

Generally speaking, I believe that we prefer /stuff/ syntax for regexps.

@@ +1047,5 @@
> +    let tasks = new Tasks(aTasks);
> +    tasks.log("> Cleanup profile from places.sqlite.corrupt files older than " + this.CORRUPTED_DB_RETAIN_DAYS + " days.");
> +    return Task.spawn( function* () {
> +      let regExp = new RegExp("places\.sqlite(-\d*)?\.corrupt$");
> +      //      let counter = 0;

If this code is useless, please remove it from the patch.

@@ +1054,5 @@
> +      let iterator = new OS.File.DirectoryIterator(OS.Constants.Path.profileDir);
> +      let entries = yield iterator.nextBatch();
> +      try {
> +        for (let entry of entries) {
> +          if (!entry.isDir && !entry.isSymLink && regExp.test(entry)) {

`entry` is not a string, so I doubt that regExp.test will do anything useful.
Also, as a matter of style, we prefer nesting less, e.g.

if (entry.isDir || entry.isSymLink || !regExp.test(entry.name)) {
  continue;
}

@@ +1063,5 @@
> +              modificationTime = info.lastModificationDate.getTime();
> +            }
> +            // Convert milliseconds to days.
> +            let days = (currentTime - modificationTime) / MS_PER_DAY;
> +            if (days > this.CORRUPTED_DB_RETAIN_DAYS || days < -1) {

What's this `days < -1`?

@@ +1065,5 @@
> +            // Convert milliseconds to days.
> +            let days = (currentTime - modificationTime) / MS_PER_DAY;
> +            if (days > this.CORRUPTED_DB_RETAIN_DAYS || days < -1) {
> +              yield OS.File.remove(entry.path);
> +              //counter++;

Dead code.

@@ +1073,5 @@
> +            }
> +          }
> +        }
> +      } catch (ex) {
> +        Cu.reportError ("Exception:" + ex);

This error message is useless. Could you replace it with something more useful?

::: toolkit/components/places/tests/unit/test_PlacesDBUtils_removeOldCorruptDBs.js
@@ +26,5 @@
> +//      for (let entry of entries) {
> +//        do_print(entry);
> +//      }
> +//    });
> +//}

Again, pleaase remove the dead code.

@@ +40,5 @@
> +      try{
> +        let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);
> +        if (yield OS.File.exists(filePath)) {
> +          yield OS.File.remove(filePath);
> +        }

You don't need the call to `exists`. If the file doesn't exist, `remove` will still succeed.

@@ +42,5 @@
> +        if (yield OS.File.exists(filePath)) {
> +          yield OS.File.remove(filePath);
> +        }
> +      }
> +      catch (ex) { }

Please don't eat exceptions.

@@ +62,5 @@
> +    yield Task.spawn( function*() {
> +        let fileName = i > 0 ? 'places.sqlite-' + i + '.corrupt' : 'places.sqlite.corrupt';
> +        let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);
> +        try{
> +          let outfile = yield OS.File.open(filePath, {write: true});

I'd suggest
yield OS.File.writeAtomic(filePath, "some data")
yield OS.File.setDates(filePath, setTime, setTime);

This way, you'll be sure that closing the file won't change a date.

@@ +63,5 @@
> +        let fileName = i > 0 ? 'places.sqlite-' + i + '.corrupt' : 'places.sqlite.corrupt';
> +        let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);
> +        try{
> +          let outfile = yield OS.File.open(filePath, {write: true});
> +          let setTime = TEST_CURRENT_TIME - MS_PER_DAY*i;

Make sure that you don't rely on the file system having an exact precision. It can err by 10 seconds on some systems.

@@ +66,5 @@
> +          let outfile = yield OS.File.open(filePath, {write: true});
> +          let setTime = TEST_CURRENT_TIME - MS_PER_DAY*i;
> +          yield outfile.setDates(setTime, setTime);
> +        }
> +        catch (ex) { }

Please don't eat exceptions.

@@ +86,5 @@
> + * - set their modified dates back in the past
> + * - execute cleanupDBProfile
> + * - assert correct behavior
> + */
> +add_task( function* () {

Please give names to your tests. It makes it easier to read stacks.

@@ +87,5 @@
> + * - execute cleanupDBProfile
> + * - assert correct behavior
> + */
> +add_task( function* () {
> +    //create .corrupt files

This comment is not very useful.

@@ +90,5 @@
> +add_task( function* () {
> +    //create .corrupt files
> +    createCorruptedFiles();
> +  
> +    //Run cleanupDBProfile()

Not very uesful either.

@@ +107,5 @@
> +        try{
> +          if (i < deletedFiles) {
> +            // The file does not exist anymore.
> +            do_check_false( yield OS.File.exists(filePath) );
> +            do_print("<<<<"+i);

I assume that this `do_print` is supposed to disappear?

@@ +117,5 @@
> +            do_check_true( (TEST_CURRENT_TIME - modificationTime) < MS_PER_DAY * RETAIN_DAYS );
> +            do_print(">>>>"+i);
> +          }
> +        }
> +        catch (ex) { do_print(ex) }

I don't think that this try/catch is useful.

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 8443047 [details] [diff] [review]
bug658544.patch

please address David's feedback first, I'll go through the final patch.

Comment 23

[Marco Bonardo [:mak]]

Hi, are you still working on this bug?

Comment 24

[Donato Sciarra]

Yes. I will try to come up in the following days with an hopefully final patch...

Comment 25

[Donato Sciarra]

Attachment: bug658544.patch

I am sending this latest patch. I have included the comments from David but I am still having issues to make the test green. Looking forward to your suggestions...

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 8561098 [details] [diff] [review]
bug658544.patch

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

Let's start fixing the code, then we'll look into the test failure.
Regarding that, please attach the test output to the bug as a txt file if it's long, or just as a comment if it's short. Unfortunately previous pastebins expired.

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +1078,5 @@
>    },
>  
> +   // Threshold value for removeOldCorruptDBs. 
> +   // Corrupted DBs older than this value are removed.
> +   CORRUPTED_DB_RETAIN_DAYS: 14,

let's make this read-only by converting it to a getter

get CORRUPTED_DB_RETAIN_DAYS() {
  return 14;
},

@@ +1092,5 @@
> +   removeOldCorruptDBs: function (aTasks) {
> +     let tasks = new Tasks(aTasks);
> +     tasks.log("> Cleanup profile from places.sqlite.corrupt files older than " + this.CORRUPTED_DB_RETAIN_DAYS + " days.");
> +     return Task.spawn( function* () {
> +       let regExp = new RegExp("places/.sqlite(-/d*)?/.corrupt$");

let's use the // form, so escaping is more readable

let re = /places\.sqlite(-\d)?\.corrupt$/;

@@ +1094,5 @@
> +     tasks.log("> Cleanup profile from places.sqlite.corrupt files older than " + this.CORRUPTED_DB_RETAIN_DAYS + " days.");
> +     return Task.spawn( function* () {
> +       let regExp = new RegExp("places/.sqlite(-/d*)?/.corrupt$");
> +       let currentTime = Date.now();
> +       let modificationTime = 0;

modificationTime should be defined inside the for loop, just before its use.

@@ +1110,5 @@
> +             modificationTime = info.lastModificationDate.getTime();
> +           }
> +           // Convert milliseconds to days.
> +           let days = (currentTime - modificationTime) / MS_PER_DAY;
> +           if (days > this.CORRUPTED_DB_RETAIN_DAYS || days < -1) {

is -1 acceptable? did you mean < 0?

@@ +1115,5 @@
> +             yield OS.File.remove(entry.path);
> +             tasks.log("> " + entry.path + " [Removed]");
> +           } else {
> +             tasks.log("> " + entry.path + " [" + days + " days old]");
> +           }

I'd not log anything in the else case. just remove it.

@@ +1120,5 @@
> +         }
> +       } catch (ex) {
> +         Cu.reportError ("Exception:" + ex);
> +       } finally {
> +         iterator.close();

I think after invoking iterator.nextBatch() you can immediately invoke .close()? It should have returned all the entries already.

I'd rather put a try catch around the single loop, so that even if one file is locked or bad, we proceed with the others.

@@ +1122,5 @@
> +         Cu.reportError ("Exception:" + ex);
> +       } finally {
> +         iterator.close();
> +       }
> +     });

So, here once the Task is complete, you should invoke PlacesDBUtils._executeTasks(tasks); both in case of success or failure.

This should become something like
return Task.spawn(...).catch(Cu.reportError).then(() => PlacesDBUtils._executeTasks(tasks));

This is likely the reason your test fails. The problem is that the "tasks" as defined here are a very fancy thing that has nothing to do with Task. To know whether everything completed you need to listen to the finished notification, and that is fired when all the tasks are complete. Thus you need to chain with PlacesDBUtils._executeTasks(tasks);

There is a separate bug to fix this mess.

::: toolkit/components/places/tests/unit/test_PlacesDBUtils_removeOldCorruptDBs.js
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * vim: sw=2 ts=2 sts=2 expandtab filetype=javascript
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

you can remove all of this, it's no more needed.

@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include PlacesDBUtils
> +////////////////////////////////////////////////////////////////////////////////

please remove this comment, the code is pretty much self documenting here

@@ +7,5 @@
> +// Include PlacesDBUtils
> +////////////////////////////////////////////////////////////////////////////////
> +Components.utils.import("resource://gre/modules/PlacesDBUtils.jsm");
> +
> +const TEMP_FILES_TO_CREATE = 100;

please reduce this to 10 or less, the tests should not take too much time.

@@ +18,5 @@
> + * Delete the temporary files from the profile.
> + *
> + */
> +function* deleteAllTempFiles() {
> +  for(let i=0; i < TEMP_FILES_TO_CREATE; i++) {

add space after for and around =

@@ +19,5 @@
> + *
> + */
> +function* deleteAllTempFiles() {
> +  for(let i=0; i < TEMP_FILES_TO_CREATE; i++) {
> +    yield Task.spawn( function*() {

you don't need to Task.spawn, you are already inside a generator function, just yield what you need to

@@ +21,5 @@
> +function* deleteAllTempFiles() {
> +  for(let i=0; i < TEMP_FILES_TO_CREATE; i++) {
> +    yield Task.spawn( function*() {
> +      let fileName = i > 0 ? 'places.sqlite-' + i + '.corrupt' : 'places.sqlite.corrupt';
> +      try{

add space after try

@@ +22,5 @@
> +  for(let i=0; i < TEMP_FILES_TO_CREATE; i++) {
> +    yield Task.spawn( function*() {
> +      let fileName = i > 0 ? 'places.sqlite-' + i + '.corrupt' : 'places.sqlite.corrupt';
> +      try{
> +        let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);

add space after comma

@@ +32,5 @@
> +}
> +
> +/**
> + *
> + * The function creates a number of temporary files in the profile folder.

remove empty line

@@ +40,5 @@
> + * files, the test will delete:
> + *    Math.max(TEST_FILES_TO_CREATE - RETAIN_DAYS, 0)
> + */
> +function* createCorruptedFiles() {
> +  for(let i = TEMP_FILES_TO_CREATE-1; i >= 0; i--) {

add space after for

@@ +41,5 @@
> + *    Math.max(TEST_FILES_TO_CREATE - RETAIN_DAYS, 0)
> + */
> +function* createCorruptedFiles() {
> +  for(let i = TEMP_FILES_TO_CREATE-1; i >= 0; i--) {
> +    yield Task.spawn( function*() {

no need to Task.spawn

@@ +44,5 @@
> +  for(let i = TEMP_FILES_TO_CREATE-1; i >= 0; i--) {
> +    yield Task.spawn( function*() {
> +        let fileName = i > 0 ? 'places.sqlite-' + i + '.corrupt' : 'places.sqlite.corrupt';
> +        let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);
> +        try { 

trailing space

@@ +45,5 @@
> +    yield Task.spawn( function*() {
> +        let fileName = i > 0 ? 'places.sqlite-' + i + '.corrupt' : 'places.sqlite.corrupt';
> +        let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);
> +        try { 
> +          let setTime = TEST_CURRENT_TIME - MS_PER_DAY*i;

add spaces around *

@@ +50,5 @@
> +          yield OS.File.writeAtomic(filePath, "some data")
> +          yield OS.File.setDates(filePath, setTime, setTime);
> +        }
> +        catch (ex) { 
> +          Cu.reportError ("Exception:" + ex);

what is this catching? I think the test should fail if anything here fails? This catch will cover errors.

@@ +52,5 @@
> +        }
> +        catch (ex) { 
> +          Cu.reportError ("Exception:" + ex);
> +        }
> +        finally { outfile.close(); }

what is outfile? it's not defined anywhere

@@ +72,5 @@
> + * - execute cleanupDBProfile
> + * - assert correct behavior
> + * - cleanup profile folder
> + */
> +add_task( function* test_removeOldCorruptDBs() {

remove space after add_task(

@@ +74,5 @@
> + * - cleanup profile folder
> + */
> +add_task( function* test_removeOldCorruptDBs() {
> +    createCorruptedFiles();
> +  

trailing space

@@ +75,5 @@
> + */
> +add_task( function* test_removeOldCorruptDBs() {
> +    createCorruptedFiles();
> +  
> +    PlacesDBUtils.removeOldCorruptDBs();

you should at a minimum yield on this...

But for how this module is build, what you should do is
return new Promise(resolve => {
  PlacesDBUtils.runTasks([PlacesDBUtils.removeOldCorruptDBs], function(aLog) {

    // Check the expected conditions HERE

    // Clean-up
    deleteAllTempFiles();

    resolve();
  });
});

@@ +79,5 @@
> +    PlacesDBUtils.removeOldCorruptDBs();
> +
> +    // We know that the first numOfFilesToDelete() will not be there, the rest must be there.
> +    let deletedFiles = numOfFilesToDelete();
> +    for(let i=0; i < TEMP_FILES_TO_CREATE; i++){

add missing spaces

@@ +80,5 @@
> +
> +    // We know that the first numOfFilesToDelete() will not be there, the rest must be there.
> +    let deletedFiles = numOfFilesToDelete();
> +    for(let i=0; i < TEMP_FILES_TO_CREATE; i++){
> +      yield Task.spawn( function*() {

no need to.

@@ +83,5 @@
> +    for(let i=0; i < TEMP_FILES_TO_CREATE; i++){
> +      yield Task.spawn( function*() {
> +        let fileName = i > 0 ? 'places.sqlite-' + i + '.corrupt' : 'places.sqlite.corrupt';
> +        let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);
> +        try{

add space after try

@@ +86,5 @@
> +        let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);
> +        try{
> +          if (i < deletedFiles) {
> +            // The file does not exist anymore.
> +            do_check_false( yield OS.File.exists(filePath) );

Instead of do_check_false please use Assert.ok(!condition);

@@ +89,5 @@
> +            // The file does not exist anymore.
> +            do_check_false( yield OS.File.exists(filePath) );
> +          } else {
> +            // The file must be present in the folder.
> +            do_check_true( yield OS.File.exists(filePath) );

Assert.ok(

@@ +104,5 @@
> +    deleteAllTempFiles();
> +  });
> +
> +// Execute tests
> +////////////////////////////////////////////////////////////////////////////////

please remove comment

::: toolkit/components/places/tests/unit/xpcshell.ini
@@ +145,5 @@
>  skip-if = os == "android"
>  [test_utils_backups_create.js]
>  [test_utils_getURLsForContainerNode.js]
>  [test_utils_setAnnotationsFor.js]
> +[test_PlacesDBUtils_removeOldCorruptDBs.js]

please keep the list in alphabetical order

Comment 27

[Donato Sciarra]

Attachment: bug658544.patch

Adding suggestions from mak. Regarding the condition on when the .corrupt file should be remove (namely days < -1), please see Comment 15.

Comment 28

[Donato Sciarra]

Attachment: test.output

Tests now hang after creating temporary files. See attached log.

Comment 29

[Marco Bonardo [:mak]]

Comment on attachment 8568170 [details] [diff] [review]
bug658544.patch

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

yes, sounds like we are almost there
Sorry for the delay, I spent many days blocked in a bed...

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +14,5 @@
>  Cu.import("resource://gre/modules/PlacesUtils.jsm");
>  
>  this.EXPORTED_SYMBOLS = [ "PlacesDBUtils" ];
>  
> +////////////////////////////////////////////////////////////////////////////////0

looks like an unwanted change

@@ +33,5 @@
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +                                  "resource://gre/modules/Task.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "OS",

nit: can remove the empty newline in the middle

@@ +1081,5 @@
> +   // Corrupted DBs older than this value are removed.
> +   _get_CORRUPTED_DB_RETAIN_DAYS(){ 
> +       return 14;
> +   },
> + 

there are some trailing spaced on the previous lines
also, I assume this should be

get CORRUPTED_DB_RETAIN_DAYS() {
  return 14;
},

so you can use it as this.CORRUPTED_DB_RETAIN_DAYS

(while here I'd use just CORRUPT rather than CORRUPTED)

@@ +1095,5 @@
> +     tasks.log("> Cleanup profile from places.sqlite.corrupt files older than " + this._get_CORRUPTED_DB_RETAIN_DAYS() + " days.");
> +     let re = /places\.sqlite(-\d)?\.corrupt$/;
> +     let currentTime = Date.now();
> +     let iterator = new OS.File.DirectoryIterator(OS.Constants.Path.profileDir);
> +     return Task.spawn( function* () {

nit: please remove the space after Task.spawn(

@@ +1120,5 @@
> +         } catch (ex) {
> +           Cu.reportError ("Exception:" + ex);
> +         }
> +       }
> +     }).catch(Cu.reportError).then(() => PlacesDBUtils._executeTasks(tasks));

here you should ...}.bind(this)).catch... otherwise the generator inside the Task.spawn won't see the correct "this".

::: toolkit/components/places/tests/unit/test_PlacesDBUtils_removeOldCorruptDBs.js
@@ +10,5 @@
> + * Helper function to create a number of temporary files in the profile folder.
> + * The files have a name in the form places-sqlite(-N).corrupt where N is a number
> + * that varies between TEST_FILES_TO_CREATE-1 and 0.
> + */
> +function* createCorruptedFiles() {

s/Corrupted/Corrupt/

@@ +12,5 @@
> + * that varies between TEST_FILES_TO_CREATE-1 and 0.
> + */
> +function* createCorruptedFiles() {
> +  let setTime = TEST_CURRENT_TIME;
> +  for( let i = 0; i < TEMP_FILES_TO_CREATE; ++i ) {

the usual coding style is:

for (let....++i) {

@@ +14,5 @@
> +function* createCorruptedFiles() {
> +  let setTime = TEST_CURRENT_TIME;
> +  for( let i = 0; i < TEMP_FILES_TO_CREATE; ++i ) {
> +    let fileName = 'places.sqlite' + (i > 0 ? '-' + i : '' ) + '.corrupt';
> +    let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);

nit: add space after comma

@@ +15,5 @@
> +  let setTime = TEST_CURRENT_TIME;
> +  for( let i = 0; i < TEMP_FILES_TO_CREATE; ++i ) {
> +    let fileName = 'places.sqlite' + (i > 0 ? '-' + i : '' ) + '.corrupt';
> +    let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);
> +    yield OS.File.writeAtomic(filePath, "test-file-delete-me", {tmpPath: fileName+".tmp"});

nit: add spaces around +

@@ +16,5 @@
> +  for( let i = 0; i < TEMP_FILES_TO_CREATE; ++i ) {
> +    let fileName = 'places.sqlite' + (i > 0 ? '-' + i : '' ) + '.corrupt';
> +    let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);
> +    yield OS.File.writeAtomic(filePath, "test-file-delete-me", {tmpPath: fileName+".tmp"});
> +    Assert.ok( (yield OS.File.exists(filePath)), filePath);

nit: remove space after ok(

I also think you maybe want a better message than just filePath, something like `File exists: ${filePath}`

@@ +34,5 @@
> + * - assert correct behavior
> + */
> +add_task( function* test_removeOldCorruptDBs() {
> +  yield createCorruptedFiles();
> +  

trailing spaces here

@@ +36,5 @@
> +add_task( function* test_removeOldCorruptDBs() {
> +  yield createCorruptedFiles();
> +  
> +  yield new Promise(resolve => {
> +  PlacesDBUtils.runTasks([PlacesDBUtils.removeOldCorruptDBs], function(aLog) {

I think the problem is that removeOldCorruptDBs is throwing and your callback is not invoked...

you should also fix indentation here...

@@ +39,5 @@
> +  yield new Promise(resolve => {
> +  PlacesDBUtils.runTasks([PlacesDBUtils.removeOldCorruptDBs], function(aLog) {
> +    // Check the expected conditions HERE
> +    // We know that the first numOfFilesToDelete() will not be there, the rest must be there.
> +    for( let i = 0; i < TEMP_FILES_TO_CREATE; ++i ){

the usual coding style is:

for (let....++i) {

@@ +42,5 @@
> +    // We know that the first numOfFilesToDelete() will not be there, the rest must be there.
> +    for( let i = 0; i < TEMP_FILES_TO_CREATE; ++i ){
> +      try{
> +	let fileName = i > 0 ? 'places.sqlite-' + i + '.corrupt' : 'places.sqlite.corrupt';
> +        let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);

add space after comma

@@ +43,5 @@
> +    for( let i = 0; i < TEMP_FILES_TO_CREATE; ++i ){
> +      try{
> +	let fileName = i > 0 ? 'places.sqlite-' + i + '.corrupt' : 'places.sqlite.corrupt';
> +        let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);
> +        if ( i > RETAIN_DAYS ) {

common style is

if (condition) {

@@ +45,5 @@
> +	let fileName = i > 0 ? 'places.sqlite-' + i + '.corrupt' : 'places.sqlite.corrupt';
> +        let filePath = OS.Path.join(OS.Constants.Path.profileDir,fileName);
> +        if ( i > RETAIN_DAYS ) {
> +          // The file does not exist anymore.
> +          Assert.ok( !( yield OS.File.exists(filePath) ), filePath );

nit: remove space after ok(
and better message as above

@@ +48,5 @@
> +          // The file does not exist anymore.
> +          Assert.ok( !( yield OS.File.exists(filePath) ), filePath );
> +        } else {
> +          // The file must be present in the folder.
> +          Assert.ok( (yield OS.File.exists(filePath) ), filePath );

ditto

@@ +55,5 @@
> +          Assert.ok( (TEST_CURRENT_TIME - modificationTime) < MS_PER_DAY * RETAIN_DAYS );
> +        }
> +      } catch(ex) {
> +        
> +      }

You should not try/catch everything here, that's going to hide errors...

@@ +59,5 @@
> +      }
> +    }
> +
> +    resolve();
> +    });

some problem with indentation

Comment 30

[Marco Bonardo [:mak]]

This needs an unbitrot, moving to async/await and fixing my review comments.
I don't think Donato is still looking into this

Comment 31

[Marco Bonardo [:mak]]

Maybe someone could be interesting in volunteer to finish this patch and make it landable.

Comment 32

[Jayati Shrivastava]

Hi, may I work on this?

Comment 33

[Mark Hammond [:markh] [:mhammond]]

(In reply to Jayati Shrivastava from comment #32)

Hi, may I work on this?

Yes - please be sure to read all the comments above to get a good understanding of what needs to be done, and feel free to ask any specific questions you might have.

Comment 34

[SSH]

(In reply to Marco Bonardo [:mak] from comment #30)

This needs an unbitrot, moving to async/await and fixing my review comments.
I don't think Donato is still looking into this

Hey,
May I work on this?

Comment 35

[Lina Butler (ex-Mozilla)]

Hi! Jayati is already working on this, so let's give her time to put up a patch.

Comment 36

[SSH]

Sure

Comment 37

[Jayati Shrivastava]

Attachment: Bug 658544 - Add support for cleanup of profile folder. r=mak,lina

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/130dadf64e30
Add support for cleanup of profile folder. r=lina

Comment 39

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on test_PlacesDBUtils_removeOldCorruptDBs.js

backout: https://hg.mozilla.org/integration/autoland/rev/e4ee3de5124eb00ec2e5d11606c50c9fa6fe81b9

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=130dadf64e30ded702c2eac8da3eff993ccc7611&group_state=expanded&selectedJob=297813267

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297813267&repo=autoland&lineNumber=2901

[task 2020-04-15T21:26:07.659Z] 21:26:07 INFO - TEST-START | toolkit/components/places/tests/unit/test_PlacesDBUtils_removeOldCorruptDBs.js
[task 2020-04-15T21:26:08.379Z] 21:26:08 WARNING - TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/unit/test_PlacesDBUtils_removeOldCorruptDBs.js | xpcshell return code: 0
[task 2020-04-15T21:26:08.379Z] 21:26:08 INFO - TEST-INFO took 724ms
[task 2020-04-15T21:26:08.380Z] 21:26:08 INFO - >>>>>>>
[task 2020-04-15T21:26:08.381Z] 21:26:08 INFO - PID 16076 | [16076, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/checkouts/gecko/toolkit/crashreporter/nsExceptionHandler.cpp, line 2895
[task 2020-04-15T21:26:08.381Z] 21:26:08 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2020-04-15T21:26:08.381Z] 21:26:08 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2020-04-15T21:26:08.381Z] 21:26:08 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2020-04-15T21:26:08.382Z] 21:26:08 INFO - running event loop
[task 2020-04-15T21:26:08.382Z] 21:26:08 INFO - toolkit/components/places/tests/unit/test_PlacesDBUtils_removeOldCorruptDBs.js | Starting setup
[task 2020-04-15T21:26:08.382Z] 21:26:08 INFO - (xpcshell/head.js) | test setup pending (2)
[task 2020-04-15T21:26:08.382Z] 21:26:08 INFO - PID 16076 | [16076, Main Thread] WARNING: Workers don't support the 'mem.mem.' preference!: file /builds/worker/checkouts/gecko/dom/workers/RuntimeService.cpp, line 538
[task 2020-04-15T21:26:08.383Z] 21:26:08 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2020-04-15T21:26:08.383Z] 21:26:08 INFO - (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2020-04-15T21:26:08.383Z] 21:26:08 INFO - (xpcshell/head.js) | test setup finished (2)
[task 2020-04-15T21:26:08.384Z] 21:26:08 INFO - toolkit/components/places/tests/unit/test_PlacesDBUtils_removeOldCorruptDBs.js | Starting removefiles

Comment 40

[Jayati Shrivastava]

It worked fine locally...How do I fix this?

Comment 41

[Lina Butler (ex-Mozilla)]

I've no idea, but let's see if we can figure it out! That is a profoundly unhelpful error message:

[task 2020-04-15T21:26:08.384Z] 21:26:08 INFO - toolkit/components/places/tests/unit/test_PlacesDBUtils_removeOldCorruptDBs.js | Starting removefiles
[task 2020-04-15T21:26:08.384Z] 21:26:08 INFO - (xpcshell/head.js) | test removefiles pending (2)
[task 2020-04-15T21:26:08.384Z] 21:26:08 INFO - (xpcshell/head.js) | test run_next_test 1 finished (2)
[task 2020-04-15T21:26:08.384Z] 21:26:08 INFO - Unexpected exception Error:

Buuut...it fails right away (and only on Linux—it passes locally for me too, as well as the xpcshell tests on Windows and macOS), without printing any file created: messages, so let's look closer at that OS.File.writeAtomic call.

await OS.File.writeAtomic(filePath, "test-file-delete-me", {
  tmpPath: fileName + ".tmp",
});

I wonder if the problem is that we aren't specifying a full path for tmpPath. Here's where we pass tmpPath to the native system call, and it doesn't look like we do any kind of resolution on it...so it'll get created in the current directory, and that might fail if we can't write to it.

Can you try changing it to tmpPath: filePath + ".tmp", so that we use the full path, and pushing to Try? If it still fails, we'll have to figure out what else could be causing this, but that's the first thing that stood out to me.

Comment 42

[Jayati Shrivastava]

Hi Lina. I changed it to filePath. It works. Thanks a lot!
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=297925996&revision=7996b844c23667a8a2f89061867037459ce916f6

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d5bdfad257f
Add support for cleanup of profile folder. r=lina

Comment 44

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/3d5bdfad257f

Comment 45

[Lina Butler (ex-Mozilla)]

Yessss! 🎉

Comment 46

[James May [:fowl2]]

should this go in release notes?

[Why is this notable]:

• (very?) unlikely data loss. Wording would have to be careful.
• space saving
• someone would need to confirm this - but there's a privacy improvement as there might have still been recoverable data in the corrupt files that would be missed by clear feature(s).

Comment 47

[Marco Bonardo [:mak]]

I don't think this is worth a relnote, very few users are likely to hit this problem and it would be asymptomatic