859695 - OS.File should be adopted in PlacesBackups.jsm and PlacesUtils.jsm

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Raymond Lee [:raymondlee]]

See bug 852041 comment 11

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: WIP

I have some questions regarding several methods.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1596

I think we can replace the code in entries getter with OS.File.DirectoryIterator  but OS.File.DirectoryIterator.Entry doesn't return nsIFile so shall we create nsIFile object using the path returned by OS.File.DirectoryIterator.Entry?

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1689

Since the BookmarkJSONUtils.exportToFile(aFile) takes a nsIFile object, do we still want to replace the below with OS.File.exists and OS.File.open to create the file if needed?

1689         if (!aFile.exists())
1690           aFile.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0600);
1691         if (!aFile.exists() || !aFile.isWritable()) {
1692           throw new Error("Unable to create bookmarks backup file: " + aFile.leafName);
1693         }

Comment 2

[Marco Bonardo [:mak]]

(In reply to Raymond Lee [:raymondlee] from comment #1)
> I think we can replace the code in entries getter with
> OS.File.DirectoryIterator  but OS.File.DirectoryIterator.Entry doesn't
> return nsIFile so shall we create nsIFile object using the path returned by
> OS.File.DirectoryIterator.Entry?

So, first of all, there are various problems with making entries async:
1. it's used by getMostRecent(), that is used in nsBrowserGlue. though shouldBackupBookmarks may be merged into backupBookmarks() that is already async, and the other entry is already async, so it should be feasible.
2. in places.js it's unfortunately used to build the backups menu on popupshowing, you should verify if on Mac the popup is properly live updating since iirc they don't live-update there (though I don't recall if it's just an issue of the native menubar). Or I may verify that if you don't have a Mac.

We should keep the old entries getter (and deprecate it), though we may introduce a new async one. Since we will change the signature for this async version we may return the OS.File format, it's easy to build an nsIFile from it.

> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> PlacesUtils.jsm#1689
> 
> Since the BookmarkJSONUtils.exportToFile(aFile) takes a nsIFile object, do
> we still want to replace the below with OS.File.exists and OS.File.open to
> create the file if needed?

yes, I think so. Wherever we can avoid mainthread access we should try to.

Comment 3

[Raymond Lee [:raymondlee]]

(In reply to Marco Bonardo [:mak] from comment #2)
> 2. in places.js it's unfortunately used to build the backups menu on
> popupshowing, you should verify if on Mac the popup is properly live
> updating since iirc they don't live-update there (though I don't recall if
> it's just an issue of the native menubar). Or I may verify that if you don't
> have a Mac.

I use Mac.  I've tried with a setTimeout to add a separator to the popup after few seconds on popupshowing, and it works.  Therefore, the popup is live updating.

Comment 4

[Marco Bonardo [:mak]]

ok, it is just the native menubar that doesn't live update then.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v2 diff -w patch

This patch adds the following things
1) two new async getter asyncEntries and asyncFolder and add deprecated.warning to the sync version.
2) update the other callers to use asyncEntries and asyncFolder
3) replace the rest of I/O with OS.File in PlacesBackups.jsm
4) fix some tests

I have a question regarding the following test.  After apply the patch, the order of expected notifications is different but I am not sure what causes that. Is that a big concern that "places-expiration-finished" is fired after "places-will-close-connection"?

+++ b/browser/components/places/tests/unit/test_clearHistory_shutdown.js
> @@ -14,18 +14,18 @@ const URIS = [
> , "http://b.example2.com/"
> , "http://c.example3.com/"
> ];
> 
> const TOPIC_CONNECTION_CLOSED = "places-connection-closed";
> 
> let EXPECTED_NOTIFICATIONS = [
>   "places-shutdown"
>+, "places-expiration-finished"
> , "places-will-close-connection"
>-, "places-expiration-finished"
> , "places-connection-closed"
> ];

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 741728 [details] [diff] [review]
v2 diff -w patch

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

::: browser/components/nsBrowserGlue.js
@@ +1060,3 @@
>        this._backupBookmarks().then(
>          function onSuccess() {
>            waitingForBackupToComplete = true;

unfortunately this doesn't work cause we'd decide to wait for the backup after the backup has been made, that means we won't spin and the bacup won't be made.
instead we need to spin only if the backup procedure started...

Unfortunately we can't figure that synchronously with the new code, that means we have to spin regardless below (please add a comment about why we spin), and immediately start working on a patch to reduce the likelyhood for backups to happen on shutdown (by reducing idle time and increasing acceptable timeframe without backups).

::: browser/components/places/content/places.js
@@ +367,5 @@
>      while (restorePopup.childNodes.length > 1)
>        restorePopup.removeChild(restorePopup.firstChild);
>  
> +    Task.spawn(function() {
> +      let backupFiles = yield PlacesBackups.asyncEntries;

I don't like much async properties, please use a method, even better, while we are renaming it, please rename to something better like getBackupFiles()

@@ +404,1 @@
>      for (let i = 0; i < backupFiles.length; i++) {

for...of

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js
@@ +24,1 @@
>  , "places-connection-closed"

does this still happen if you spin as said above?

it's quite strange, there should be a places-expiration-finished after will-close, cause at will-close nsPlacesExpiration.js fires the clear on shutdown task that should generate that expiration-finished notification.
So yes, there's something unexpected if it happens earlier

::: toolkit/components/places/PlacesBackups.jsm
@@ +45,5 @@
>      delete this.folder;
>      return this.folder = bookmarksBackupDir;
>    },
>  
> +  get asyncFolder() {

please getBackupFolder() (to cope with getBackupFiles) and use a _folder property as cache, don't replace the getter

@@ +101,5 @@
> +   * Cache current backups in a sorted (by date DESC) array.
> +   */
> +  get asyncEntries() {
> +    delete this.asyncEntries;
> +    this.asyncEntries = [];

use a private _backupFiles property for cache

@@ +157,5 @@
>     * @return A Date object for the backup's creation time.
>     */
>    getDateForFile: function PB_getDateForFile(aBackupFile) {
> +    let filename = (aBackupFile instanceof Ci.nsIFile) ? aBackupFile.leafName :
> +                    aBackupFile.name;

is this needed? looks like you always create nsIFile

@@ +179,3 @@
>      let fileExt = aFileExt || "(json|html)";
> +      let entries = yield this.asyncEntries;
> +      for (let i = 0; i < entries.length; i++) {

for...of

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v3 -w diff

(In reply to Marco Bonardo [:mak] from comment #6)
> Comment on attachment 741728 [details] [diff] [review]
> v2 diff -w patch
> 
> Review of attachment 741728 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/nsBrowserGlue.js
> @@ +1060,3 @@
> >        this._backupBookmarks().then(
> >          function onSuccess() {
> >            waitingForBackupToComplete = true;
> 
> unfortunately this doesn't work cause we'd decide to wait for the backup
> after the backup has been made, that means we won't spin and the bacup won't
> be made.
> instead we need to spin only if the backup procedure started...
> 
> Unfortunately we can't figure that synchronously with the new code, that
> means we have to spin regardless below (please add a comment about why we
> spin), and immediately start working on a patch to reduce the likelyhood for
> backups to happen on shutdown (by reducing idle time and increasing
> acceptable timeframe without backups).

Do you mean by wrapping the this._backupBookmarks() with |Task.spawn| and |yield|. Please see the attached patch.

> 
> ::: browser/components/places/tests/unit/test_clearHistory_shutdown.js
> @@ +24,1 @@
> >  , "places-connection-closed"
> 
> does this still happen if you spin as said above?
> 
> it's quite strange, there should be a places-expiration-finished after
> will-close, cause at will-close nsPlacesExpiration.js fires the clear on
> shutdown task that should generate that expiration-finished notification.
> So yes, there's something unexpected if it happens earlier
> 

Still got the issue with this patch.


> ::: toolkit/components/places/PlacesBackups.jsm
> @@ +45,5 @@
> >      delete this.folder;
> >      return this.folder = bookmarksBackupDir;
> >    },
> >  
> > +  get asyncFolder() {
> 
> please getBackupFolder() (to cope with getBackupFiles) and use a _folder
> property as cache, don't replace the getter

Do you want to keep the asyncFolder getter?

> 
> @@ +157,5 @@
> >     * @return A Date object for the backup's creation time.
> >     */
> >    getDateForFile: function PB_getDateForFile(aBackupFile) {
> > +    let filename = (aBackupFile instanceof Ci.nsIFile) ? aBackupFile.leafName :
> > +                    aBackupFile.name;
> 
> is this needed? looks like you always create nsIFile
> 

This is one place in PB_getBackupFiles() we pass the OS.File.entry into getDateForFile to check the date before removing bogus backups in future dates.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 748704 [details] [diff] [review]
v3 -w diff

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

I think you attached the wrong patch? this looks like getShortcutOrURI patch that Mano was reviewing elsewhere...

Comment 9

[Marco Bonardo [:mak]]

(In reply to Raymond Lee [:raymondlee] from comment #7)
> Created attachment 748704 [details] [diff] [review]
> v3 -w diff
> 
> (In reply to Marco Bonardo [:mak] from comment #6)
> > Comment on attachment 741728 [details] [diff] [review]
> > v2 diff -w patch
> > 
> > Review of attachment 741728 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/components/nsBrowserGlue.js
> > @@ +1060,3 @@
> > >        this._backupBookmarks().then(
> > >          function onSuccess() {
> > >            waitingForBackupToComplete = true;
> > 
> > unfortunately this doesn't work cause we'd decide to wait for the backup
> > after the backup has been made, that means we won't spin and the bacup won't
> > be made.
> > instead we need to spin only if the backup procedure started...
> > 
> > Unfortunately we can't figure that synchronously with the new code, that
> > means we have to spin regardless below (please add a comment about why we
> > spin), and immediately start working on a patch to reduce the likelyhood for
> > backups to happen on shutdown (by reducing idle time and increasing
> > acceptable timeframe without backups).
> 
> Do you mean by wrapping the this._backupBookmarks() with |Task.spawn| and
> |yield|. Please see the attached patch.


I mean we should spin the events loop regardless, while currently we just spin it if we should export to html or backup.

> > ::: browser/components/places/tests/unit/test_clearHistory_shutdown.js
> > @@ +24,1 @@
> > >  , "places-connection-closed"
> > 
> > does this still happen if you spin as said above?
> > 
> > it's quite strange, there should be a places-expiration-finished after
> > will-close, cause at will-close nsPlacesExpiration.js fires the clear on
> > shutdown task that should generate that expiration-finished notification.
> > So yes, there's something unexpected if it happens earlier
> > 
> 
> Still got the issue with this patch.

That's very strange, I wil try to reproduce with the next version of the patch.

> > ::: toolkit/components/places/PlacesBackups.jsm
> > @@ +45,5 @@
> > >      delete this.folder;
> > >      return this.folder = bookmarksBackupDir;
> > >    },
> > >  
> > > +  get asyncFolder() {
> > 
> > please getBackupFolder() (to cope with getBackupFiles) and use a _folder
> > property as cache, don't replace the getter
> 
> Do you want to keep the asyncFolder getter?

no, convert it to a method as expressed.
I was unclear, my second comment was generic, just to say to never replace an async getter with a synchronous value.

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v4 -w

Oops, this should be the correct patch.

(In reply to Marco Bonardo [:mak] from comment #9)
> I mean we should spin the events loop regardless, while currently we just
> spin it if we should export to html or backup.
> 

The events loop should spin at least once because waitingForBackupToComplete is true before this._backupBookmarks().then() resets the value.

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: v4

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 753213 [details] [diff] [review]
v4 -w

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

I will verify what's up with that topic firing at a different time, I suspect the events loop spinning is involved but have to verify that locally.
some comments in the meanwhile.

::: toolkit/components/places/PlacesBackups.jsm
@@ +49,5 @@
> +  /**
> +   * Gets backup folder asynchronously.
> +   */
> +  getBackupFolder: function PB_getBackupFolder() {
> +    delete this._folder;

doesn't look like you are using the cache, you should return it if it's defined.

@@ +103,5 @@
> +   * Cache current backups in a sorted (by date DESC) array.
> +   */
> +  getBackupFiles: function PB_getBackupFiles() {
> +    delete this._backupFiles;
> +    this._backupFiles = [];

again, looks like you are not using the cache

@@ +159,5 @@
>     * @return A Date object for the backup's creation time.
>     */
>    getDateForFile: function PB_getDateForFile(aBackupFile) {
> +    let filename = (aBackupFile instanceof Ci.nsIFile) ? aBackupFile.leafName :
> +                    aBackupFile.name;

nit:  prefer indentation like:

let filename = aBackupFile instanceof Ci.nsIFile ? aBackupFile.leafName
                                                 : aBackupFile.name;

@@ +178,3 @@
>     */
>    getMostRecent: function PB_getMostRecent(aFileExt) {
> +    return Task.spawn(function() {

probably we should deprecate this and make a separate async version...
please verify if add-ons are using this method or if it has only internal usage.

@@ +213,3 @@
>          // Update internal cache.
>          this.entries.push(aFile);
> +        this._backupFiles.push(aFile);

are you sure that _backupFiles array exists at this point?

@@ +229,4 @@
>              // Update internal cache if we are not replacing an existing
>              // backup file.
>              this.entries.push(file);
> +            this._backupFiles.push(file);

ditto

@@ +258,5 @@
>  
>        if (!aForceBackup) {
>          let numberOfBackupsToDelete = 0;
>          if (aMaxBackups !== undefined && aMaxBackups > -1)
> +          numberOfBackupsToDelete = this._backupFiles.length - aMaxBackups;

ditto

@@ +270,5 @@
>              numberOfBackupsToDelete++;
>  
>            while (numberOfBackupsToDelete--) {
> +            this.entries.pop();
> +            let oldestBackup = this._backupFiles.pop();

ditto

@@ +282,5 @@
>               mostRecentBackupFile.leafName == newBackupFilename))
>            return;
>        }
>  
> +      let newBackupFile = this._folder.clone();

ditto

more generically, you should never access a private cache, unless it's to update it, if it doesn't exist it doesn't even need to be updated. To get the value it's always safer to use the external method.

Comment 13

[Marco Bonardo [:mak]]

Note bug 873293 may be useful here, we have a large object that we want to serialize to a json file.

Comment 14

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

(In reply to Marco Bonardo [:mak] from comment #13)
> Note bug 873293 may be useful here, we have a large object that we want to
> serialize to a json file.

Unfortunately, there is no ETA for that.

Comment 15

[Marco Bonardo [:mak]]

(In reply to David Rajchenbach Teller [:Yoric] <on workweek, will not follow bugzilla until July 1st> from comment #14)
> (In reply to Marco Bonardo [:mak] from comment #13)
> > Note bug 873293 may be useful here, we have a large object that we want to
> > serialize to a json file.
> 
> Unfortunately, there is no ETA for that.

it's ok, we can start with the possible behavior, add some telemetry, and re-prioritize further changes if needed.

Comment 16

[Marco Bonardo [:mak]]

when doing this, we should  reduce BOOKMARKS_BACKUP_IDLE_TIME to 10 * 60. we want to reduce the possibility to hit the backup on shutdown path.
The alternative is to first change backups to happen in background, but for that we first need bug 887043 and this one (so that everything is off main thread)

Comment 17

[Raymond Lee [:raymondlee]]

Also see bug 818593 comment 36 to ensure that we don't hit main-thread IO for the menu.

Comment 18

[Raymond Lee [:raymondlee]]

Attachment: v5 -w

An updated version after the

Comment 19

[Raymond Lee [:raymondlee]]

Attachment: v5

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 795332 [details] [diff] [review]
v5 -w

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

::: browser/components/nsBrowserGlue.js
@@ +78,5 @@
>  const PREF_PLUGINS_UPDATEURL  = "plugins.update.url";
>  
>  // We try to backup bookmarks at idle times, to avoid doing that at shutdown.
>  // Number of idle seconds before trying to backup bookmarks.  15 minutes.
> +const BOOKMARKS_BACKUP_IDLE_TIME = 10 * 60;

please also update the comment above to "10 minutes"

::: toolkit/components/places/PlacesBackups.jsm
@@ +53,5 @@
>    },
>  
> +  /**
> +   * Gets backup folder asynchronously.
> +   */

please add the fact it @return a {Promise} that resolves to the folder (nsIFile)

@@ +111,5 @@
>    },
>  
>    /**
> +   * Cache current backups in a sorted (by date DESC) array.
> +   */

ditto for the @return

@@ +324,5 @@
> +            if (!this._backupFiles) {
> +              yield this.getBackupFiles();
> +            }
> +            let oldestBackup = this._backupFiles.pop();
> +            yield OS.File.remove(oldestBackup.path);

why do you have to generate _backupFiles and pop from it, won't it just be generated later with the proper entries after you remove? Is this to avoid a race condition? If so, a comment would be appreciated.

@@ +347,1 @@
>            return;

please always brace both sides of an if/else if one side is braced

Comment 21

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

Comment on attachment 795332 [details] [diff] [review]
v5 -w

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

How hard would it be to completely get rid of nsIFile? Mixing nsIFile and OS.File is neither very nice, nor a very good idea, performance-wise.

::: browser/components/nsBrowserGlue.js
@@ +1201,1 @@
>        BookmarkHTMLUtils.exportToFile(FileUtils.getFile("BMarks", [])).then(

Would it be possible to get rid of this instance of |FileUtils.getFile|? This method is much more expensive than it looks at first (see bug 920187, bug 898315).

@@ +1216,1 @@
>        thread.processNextEvent(true);

Looks like something that should be plugged into AsyncShutdown. I'll leave mak to determine whether this should be done now or in a followup bug.

::: toolkit/components/places/PlacesBackups.jsm
@@ +53,5 @@
>    },
>  
> +  /**
> +   * Gets backup folder asynchronously.
> +   */

Do we actually need this to be a nsIFile or could we return a string path, for use with OS.Path?

@@ +171,5 @@
>     * @return A Date object for the backup's creation time.
>     */
>    getDateForFile: function PB_getDateForFile(aBackupFile) {
> +    let filename = (aBackupFile instanceof Ci.nsIFile) ? aBackupFile.leafName
> +                                                       : aBackupFile.name;

This line seems to indicate that the documentation above is now false.

@@ +220,5 @@
> +           throw new Task.Result(entry);
> +         }
> +       }
> +       throw new Task.Result(null);
> +    }.bind(this));

That looks expensive. Is this method called often?

@@ +238,5 @@
>      return Task.spawn(function() {
> +      try {
> +        let file = yield OS.File.open(aFile.path, { create: true });
> +        yield file.close();
> +      } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) {}

So what exactly is the point of this block? It instructs the code to fail if the file exists but then catches that specific error and does nothing with it.

@@ +263,5 @@
>          let backupFile = yield this._getBackupFileForSameDate(name);
>          if (backupFile) {
> +          try {
> +            yield OS.File.remove(backupFile.path);
> +          } catch(ex if ex instanceof OS.File.Error && ex.becauseNoSuchFile) {}

Just use
  yield OS.File.remove(backupFile.path, { ignoreAbsent: true })
without the try/catch

@@ +341,5 @@
>        if (backupFile) {
> +        if (aForceBackup) {
> +          try {
> +            yield OS.File.remove(backupFile.path);
> +          } catch(ex if ex instanceof OS.File.Error && ex.becauseNoSuchFile) {}

ditto

Comment 22

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #21)
> ::: browser/components/nsBrowserGlue.js
> @@ +1201,1 @@
> >        BookmarkHTMLUtils.exportToFile(FileUtils.getFile("BMarks", [])).then(
> 
> Would it be possible to get rid of this instance of |FileUtils.getFile|?
> This method is much more expensive than it looks at first (see bug 920187,
> bug 898315).

This will go away in bug 919506.

Comment 23

[Raymond Lee [:raymondlee]]

(In reply to Marco Bonardo [:mak] from comment #20)
> @@ +324,5 @@
> > +            if (!this._backupFiles) {
> > +              yield this.getBackupFiles();
> > +            }
> > +            let oldestBackup = this._backupFiles.pop();
> > +            yield OS.File.remove(oldestBackup.path);
> 
> why do you have to generate _backupFiles and pop from it, won't it just be
> generated later with the proper entries after you remove? Is this to avoid a
> race condition? If so, a comment would be appreciated.
> 

this._backupFiles is the cache for getBackupFiles(). If we remove a file, the cache this._backupFiles should not be updated unless we remove it from this._backupFiles.  Alternatively, we can do delete this._backupFiles. What do you think?


(In reply to David Rajchenbach Teller [:Yoric] from comment #21)
> Comment on attachment 795332 [details] [diff] [review]
> v5 -w
> 
> Review of attachment 795332 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How hard would it be to completely get rid of nsIFile? Mixing nsIFile and
> OS.File is neither very nice, nor a very good idea, performance-wise.
> 
> ::: browser/components/nsBrowserGlue.js
> @@ +1201,1 @@
> >        BookmarkHTMLUtils.exportToFile(FileUtils.getFile("BMarks", [])).then(
> 
> Would it be possible to get rid of this instance of |FileUtils.getFile|?
> This method is much more expensive than it looks at first (see bug 920187,
> bug 898315).
> 
> @@ +1216,1 @@
> >        thread.processNextEvent(true);
> 
> Looks like something that should be plugged into AsyncShutdown. I'll leave
> mak to determine whether this should be done now or in a followup bug.
> 

What do you think?

> ::: toolkit/components/places/PlacesBackups.jsm
> @@ +53,5 @@
> >    },
> >  
> > +  /**
> > +   * Gets backup folder asynchronously.
> > +   */
> 
> Do we actually need this to be a nsIFile or could we return a string path,
> for use with OS.Path?
> 

Shall I make getBackupFolder() to return a string path? Do we need to change others like entries(), getBackupFiles()

Comment 24

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

(In reply to Raymond Lee [:raymondlee] from comment #23)
> > @@ +1216,1 @@
> > >        thread.processNextEvent(true);
> > 
> > Looks like something that should be plugged into AsyncShutdown. I'll leave
> > mak to determine whether this should be done now or in a followup bug.
> > 
> 
> What do you think?

If you can do it in this bug, that's great.

> > ::: toolkit/components/places/PlacesBackups.jsm
> > Do we actually need this to be a nsIFile or could we return a string path,
> > for use with OS.Path?
> > 
> 
> Shall I make getBackupFolder() to return a string path? Do we need to change
> others like entries(), getBackupFiles()

That would be better, indeed. If there is a place in which a nsIFile is strongly needed, you can live if for the moment, but it is my understanding that that's not the case.

Comment 25

[Marco Bonardo [:mak]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #21)
> How hard would it be to completely get rid of nsIFile? Mixing nsIFile and
> OS.File is neither very nice, nor a very good idea, performance-wise.

I think it's a good next step, though I'm not sure the other code is ready to cope with something else then nsIFile atm

> ::: browser/components/nsBrowserGlue.js
> @@ +1201,1 @@
> >        BookmarkHTMLUtils.exportToFile(FileUtils.getFile("BMarks", [])).then(
> 
> Would it be possible to get rid of this instance of |FileUtils.getFile|?
> This method is much more expensive than it looks at first (see bug 920187,
> bug 898315).

we should not use getFile("Bmarks" here regardless, since it's bogus (see bug 919506), but the replacement is not any better from a "jank" point of view.

> 
> @@ +1216,1 @@
> >        thread.processNextEvent(true);
> 
> Looks like something that should be plugged into AsyncShutdown. I'll leave
> mak to determine whether this should be done now or in a followup bug.

follow-up, definitely. let's avoid scope creep.

Comment 26

[Marco Bonardo [:mak]]

(In reply to Raymond Lee [:raymondlee] from comment #23)
> (In reply to Marco Bonardo [:mak] from comment #20)
> > @@ +324,5 @@
> > > +            if (!this._backupFiles) {
> > > +              yield this.getBackupFiles();
> > > +            }
> > > +            let oldestBackup = this._backupFiles.pop();
> > > +            yield OS.File.remove(oldestBackup.path);
> > 
> > why do you have to generate _backupFiles and pop from it, won't it just be
> > generated later with the proper entries after you remove? Is this to avoid a
> > race condition? If so, a comment would be appreciated.
> > 
> 
> this._backupFiles is the cache for getBackupFiles(). If we remove a file,
> the cache this._backupFiles should not be updated unless we remove it from
> this._backupFiles.  Alternatively, we can do delete this._backupFiles. What
> do you think?

I think we can leave it as-is for now, I think the race condition may exist so better staying on the safe side, as you already did.

I don't remember which places are currently in need of nsIFile rather than a string path, surely we use a lot .leafName. Though I was looking comment 2 now, and looks like I indeed suggested to return string paths since it's easy to make a nsIFile out of them for consumers.

Comment 27

[Raymond Lee [:raymondlee]]

Attachment: v6 -w

Update several methods to return string path(s), and also fixed other stuff based on comments

Comment 28

[Raymond Lee [:raymondlee]]

Attachment: v6

Comment 29

[Raymond Lee [:raymondlee]]

Attachment: v6 -w

Comment 30

[Raymond Lee [:raymondlee]]

(In reply to Marco Bonardo [:mak] from comment #25)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #21)
> > 
> > @@ +1216,1 @@
> > >        thread.processNextEvent(true);
> > 
> > Looks like something that should be plugged into AsyncShutdown. I'll leave
> > mak to determine whether this should be done now or in a followup bug.
> 
> follow-up, definitely. let's avoid scope creep.

Filed bug 923604

Comment 31

[Marco Bonardo [:mak]]

Comment on attachment 811942 [details] [diff] [review]
v6 -w

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

::: browser/components/places/content/places.js
@@ +459,5 @@
>      let backupName = aMenuItem.getAttribute("value");
> +      let backupFilePaths = yield PlacesBackups.getBackupFiles();
> +      for (let backupFilePath of backupFilePaths) {
> +        let backupFile = new FileUtils.File(backupFilePath);
> +        if (backupFile.leafName == backupName) {

I think you can use OS.Path.basename(backupFilePath) and create the nsIFile inside the if, for now.

::: toolkit/components/places/PlacesBackups.jsm
@@ +208,5 @@
> +    * @param [optional] aFileExt
> +    *                   Force file extension.  Either "html" or "json".
> +    *                   Will check for both if not defined.
> +    * @return {Promise}
> +    * @result OS.File backup file.

the path to the file

Comment 32

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

(In reply to Marco Bonardo [:mak] from comment #31)
> Comment on attachment 811942 [details] [diff] [review]
> v6 -w
> 
> Review of attachment 811942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/places/content/places.js
> @@ +459,5 @@
> >      let backupName = aMenuItem.getAttribute("value");
> > +      let backupFilePaths = yield PlacesBackups.getBackupFiles();
> > +      for (let backupFilePath of backupFilePaths) {
> > +        let backupFile = new FileUtils.File(backupFilePath);
> > +        if (backupFile.leafName == backupName) {
> 
> I think you can use OS.Path.basename(backupFilePath) and create the nsIFile
> inside the if, for now.
> 

Done.

> ::: toolkit/components/places/PlacesBackups.jsm
> @@ +208,5 @@
> > +    * @param [optional] aFileExt
> > +    *                   Force file extension.  Either "html" or "json".
> > +    *                   Will check for both if not defined.
> > +    * @return {Promise}
> > +    * @result OS.File backup file.
> 
> the path to the file

Updated

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

Comment 33

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #32)
> Pushed to try and waiting for results.
> https://tbpl.mozilla.org/?tree=Try&rev=be546211d377

Passed Try

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f82ddd5f63a3

Comment 35

[Phil Ringnalda (:philor)]

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

Comment 36

[Byron Campen [:bwc]]

Attachment: OS.File should be adopted in PlacesBackups.jsm and PlacesUtils.jsm.

Handle ekr's review comments, plus a few other things I noticed.

Comment 37

[Byron Campen [:bwc]]

Wow. Not sure why bzexport decided to put someone else's patch on someone else's bug, but here we are. Give me a moment to fix and collect info for a bug report.

Comment 38

[Byron Campen [:bwc]]

Comment on attachment 818148 [details] [diff] [review]
OS.File should be adopted in PlacesBackups.jsm and PlacesUtils.jsm.

Cleaning up after bzexport's goof.