bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

NEW
Unassigned

Status

()

Toolkit
Places
P3
normal
7 years ago
11 months ago

People

(Reporter: mak, Unassigned, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Summary: Cleanup profiles from old old places.sqlite.corrupt files → Cleanup profiles from old and useless places.sqlite.corrupt files
(Reporter)

Updated

7 years ago
Whiteboard: [places-next-wanted] → [places-next-wanted][good first bug]
(Reporter)

Updated

6 years ago
Whiteboard: [places-next-wanted][good first bug] → [places-next-wanted][good first bug][mentor=mak]
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.
Flags: needinfo?(mak77)
(Reporter)

Comment 2

5 years ago
yes, it's still a valid issue.
Flags: needinfo?(mak77)
Whiteboard: [places-next-wanted][good first bug][mentor=mak] → [good-first-bug][mentor=mak][lang=js]

Comment 3

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

Updated

5 years ago
Whiteboard: [good-first-bug][mentor=mak][lang=js] → [good first bug][mentor=mak][lang=js]

Comment 4

4 years ago
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.
(Reporter)

Comment 5

4 years ago
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
Flags: needinfo?(sciarp)
(Reporter)

Comment 6

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

Comment 7

4 years ago
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.)
Flags: needinfo?(sciarp) → needinfo?(mak77)
(Reporter)

Comment 8

4 years ago
(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
Assignee: nobody → sciarp
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)

Updated

4 years ago
Attachment #8384562 - Flags: review?(mak77)
(Reporter)

Comment 10

4 years ago
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) {"
Attachment #8384562 - Flags: review?(mak77) → feedback+
(Reporter)

Comment 11

4 years ago
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

4 years ago
Created attachment 8395604 [details] [diff] [review]
bug658544.patch
Attachment #8384562 - Attachment is obsolete: true
Attachment #8395604 - Flags: review?(mak77)

Comment 13

4 years ago
Created attachment 8395605 [details]
test_658544.js
Attachment #8395605 - Flags: review?(mak77)

Comment 14

4 years ago
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!
(Reporter)

Updated

4 years ago
Attachment #8395605 - Attachment mime type: application/javascript → text/plain
(Reporter)

Comment 15

4 years ago
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.
Attachment #8395604 - Flags: review?(mak77)
(Reporter)

Comment 16

4 years ago
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
});
Attachment #8395605 - Flags: review?(mak77)
(Assignee)

Updated

4 years ago
Mentor: mak77
Whiteboard: [good first bug][mentor=mak][lang=js] → [good first bug][lang=js]

Comment 17

4 years ago
Created attachment 8442792 [details] [diff] [review]
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.
Attachment #8395604 - Attachment is obsolete: true
Attachment #8395605 - Attachment is obsolete: true
Attachment #8442792 - Flags: review?(mak77)

Comment 18

4 years ago
Here the output of the test: http://pastebin.com/aUYn0zTc
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

4 years ago
Created attachment 8443047 [details] [diff] [review]
bug658544.patch

I have modified the function as David suggested but the test still fails. This is the output: http://pastebin.com/mDjKV4eU
Attachment #8442792 - Attachment is obsolete: true
Attachment #8442792 - Flags: review?(mak77)
Attachment #8443047 - Flags: review?(mak77)
Attachment #8443047 - Flags: review?(dteller)
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.
Attachment #8443047 - Flags: review?(dteller) → feedback+
(Reporter)

Comment 22

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

please address David's feedback first, I'll go through the final patch.
Attachment #8443047 - Flags: review?(mak77)
(Reporter)

Comment 23

4 years ago
Hi, are you still working on this bug?
Flags: needinfo?(sciarp)

Comment 24

4 years ago
Yes. I will try to come up in the following days with an hopefully final patch...
Flags: needinfo?(sciarp)

Comment 25

4 years ago
Created attachment 8561098 [details] [diff] [review]
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...
Attachment #8443047 - Attachment is obsolete: true
Flags: needinfo?(mak77)
(Reporter)

Comment 26

4 years ago
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
Attachment #8561098 - Flags: review-
(Reporter)

Updated

4 years ago
Flags: needinfo?(mak77)

Comment 27

3 years ago
Created attachment 8568170 [details] [diff] [review]
bug658544.patch

Adding suggestions from mak. Regarding the condition on when the .corrupt file should be remove (namely days < -1), please see Comment 15.
Attachment #8561098 - Attachment is obsolete: true
Attachment #8568170 - Flags: review?(mak77)

Comment 28

3 years ago
Created attachment 8568171 [details]
test.output

Tests now hang after creating temporary files. See attached log.
(Reporter)

Updated

3 years ago
Attachment #8568171 - Attachment mime type: application/mbox → text/plain
(Reporter)

Comment 29

3 years ago
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
Attachment #8568170 - Flags: review?(mak77)
(Reporter)

Updated

3 years ago
Priority: -- → P3
(Reporter)

Updated

3 years ago
Flags: needinfo?(mak77)
(Reporter)

Comment 30

a year ago
This needs an unbitrot, moving to async/await and fixing my review comments.
I don't think Donato is still looking into this
Assignee: sciarp → nobody
Status: ASSIGNED → NEW
(Reporter)

Comment 31

11 months ago
Maybe someone could be interesting in volunteer to finish this patch and make it landable.
Flags: needinfo?(mak77)
You need to log in before you can comment on or make changes to this bug.