852041 - Apply use of BookmarkJSONUtils.exportToFile

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Andres Hernandez [:andreshm]]

Replace sync calls to PlacesUtils.saveBookmarksToJSONFile with BookmarkJSONUtils.exportToFile async calls.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

I have not touched the placesUtils.jsm because Bug 852032 is being worked on to move the backup code to PlacesBackups.jsm

Comment 2

[Raymond Lee [:raymondlee]]

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

Updating the patch

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v2

This should be reviewed and landed before we try to move code from PlacesUtils to PlacesBackups bug 852032

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v3

Comment 5

[Marco Bonardo [:mak]]

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

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

Due to lack of time I just skimmed through the patch quickly, a more complete review is still needed, so forwarding that to Mano

::: browser/components/nsBrowserGlue.js
@@ +1167,5 @@
> +    );
> +    let thread = Services.tm.currentThread;
> +    while (!shutdownComplete) {
> +      thread.processNextEvent(true);
> +    }

This is a bit problematic, we accepted to introduce this looping just under the "browser.bookmarks.autoExportHTML" pref, but this patch makes so we always hit it.

It should not happen if there is no need to backup and no "browser.bookmarks.autoExportHTML" is set, and a bug should be filed (provided there isn't one already filed) to stop backing up on shutdown (once we have better async backups).
But surely we should not do this at every shutdown.

::: browser/components/places/content/places.js
@@ +466,5 @@
>      let fpCallback = function fpCallback_done(aResult) {
>        if (aResult != Ci.nsIFilePicker.returnCancel) {
> +        Task.spawn(function() {
> +          yield BookmarkJSONUtils.exportToFile(fp.file);
> +        });

what's the point of this Task.spawn/yield? doesn't look useful.

::: browser/components/places/content/placesOverlay.xul
@@ +26,5 @@
>  
>      Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
>      Components.utils.import("resource:///modules/PlacesUIUtils.jsm");
> +    Components.utils.import("resource://gre/modules/BookmarkJSONUtils.jsm");
> +    Components.utils.import("resource://gre/modules/Task.jsm");

We should probably not do this, it's going to import those modules everywhere and we don't want that, we have enough issues with Cc, Ci and Cr already :)

::: services/sync/modules/engines/bookmarks.js
@@ +371,3 @@
>  
> +      this._store._childrenToOrder = {};
> +    }.bind(this));

The sync changes should be reviewed by :rnewma, please split them to a separate part.

::: toolkit/components/places/PlacesUtils.jsm
@@ -1703,5 @@
> -   * @see backups.saveBookmarksToJSONFile(aFile)
> -   */
> -  backupBookmarksToFile: function PU_backupBookmarksToFile(aFile) {
> -    this.backups.saveBookmarksToJSONFile(aFile);
> -  },

Need to check add-ons usage before removing this pseudo-api from PU, we may have to mark it with Deprecated.warning if it's used by popular add-ons (anyone in the ff team may help you with the research in add-ons if you don't have ldap access to addons mxr)

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: Part 1 browser and toolkit components - v4

(In reply to Marco Bonardo [:mak] (Away Mar 27 - Apr 2) from comment #5)
> Comment on attachment 728954 [details] [diff] [review]
> v3
> 
> Review of attachment 728954 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Due to lack of time I just skimmed through the patch quickly, a more
> complete review is still needed, so forwarding that to Mano
> 
> ::: browser/components/nsBrowserGlue.js
> @@ +1167,5 @@
> > +    );
> > +    let thread = Services.tm.currentThread;
> > +    while (!shutdownComplete) {
> > +      thread.processNextEvent(true);
> > +    }
> 
> This is a bit problematic, we accepted to introduce this looping just under
> the "browser.bookmarks.autoExportHTML" pref, but this patch makes so we
> always hit it.
> 
> It should not happen if there is no need to backup and no
> "browser.bookmarks.autoExportHTML" is set, and a bug should be filed
> (provided there isn't one already filed) to stop backing up on shutdown
> (once we have better async backups).
> But surely we should not do this at every shutdown.

Filed 855226 to stop backup up on shutdown.

I've updated the code to do the looping if we need to backup bookmarks and/or "browser.bookmarks.autoExportHTML" is set

> 
> ::: browser/components/places/content/places.js
> @@ +466,5 @@
> >      let fpCallback = function fpCallback_done(aResult) {
> >        if (aResult != Ci.nsIFilePicker.returnCancel) {
> > +        Task.spawn(function() {
> > +          yield BookmarkJSONUtils.exportToFile(fp.file);
> > +        });
> 
> what's the point of this Task.spawn/yield? doesn't look useful.

Removed Task.spawn/yield

> 
> ::: browser/components/places/content/placesOverlay.xul
> @@ +26,5 @@
> >  
> >      Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
> >      Components.utils.import("resource:///modules/PlacesUIUtils.jsm");
> > +    Components.utils.import("resource://gre/modules/BookmarkJSONUtils.jsm");
> > +    Components.utils.import("resource://gre/modules/Task.jsm");
> 
> We should probably not do this, it's going to import those modules
> everywhere and we don't want that, we have enough issues with Cc, Ci and Cr
> already :)

Removed

> 
> ::: toolkit/components/places/PlacesUtils.jsm
> @@ -1703,5 @@
> > -   * @see backups.saveBookmarksToJSONFile(aFile)
> > -   */
> > -  backupBookmarksToFile: function PU_backupBookmarksToFile(aFile) {
> > -    this.backups.saveBookmarksToJSONFile(aFile);
> > -  },
> 
> Need to check add-ons usage before removing this pseudo-api from PU, we may
> have to mark it with Deprecated.warning if it's used by popular add-ons
> (anyone in the ff team may help you with the research in add-ons if you
> don't have ldap access to addons mxr)

There are some add-ons using it e.g. stumbleUpon so I've added Deprecated.warning statement to that method.

Filed bug 855218 to remove it in the future.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: Part 2 services/sync - v4

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 730063 [details] [diff] [review]
Part 2 services/sync - v4

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

All of Sync's calls are synchronous. Whenever it uses an async API, it must spin the event loop.

Task.spawn returns a promise; you need to prevent wipe() and _syncStartup() from returning until that promise is resolved. Right now you're allowing a bookmark sync to proceed before _guidMap has been built, or similar potential horrors.

There are utilities in the Async namespace (see elsewhere in Sync) that you can use to do so.

::: services/sync/modules/engines/bookmarks.js
@@ +17,5 @@
>  Cu.import("resource://services-sync/engines.js");
>  Cu.import("resource://services-sync/record.js");
>  Cu.import("resource://services-sync/util.js");
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");

I don't believe you need the promise import here.

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: Part 2 services/sync - v5

(In reply to Richard Newman [:rnewman] from comment #8)
> Comment on attachment 730063 [details] [diff] [review]
> Part 2 services/sync - v4
> 
> Review of attachment 730063 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All of Sync's calls are synchronous. Whenever it uses an async API, it must
> spin the event loop.
> 
> Task.spawn returns a promise; you need to prevent wipe() and _syncStartup()
> from returning until that promise is resolved. Right now you're allowing a
> bookmark sync to proceed before _guidMap has been built, or similar
> potential horrors.
> 
> There are utilities in the Async namespace (see elsewhere in Sync) that you
> can use to do so.

Applied Async.makeSpinningCallback. Please check.

> 
> ::: services/sync/modules/engines/bookmarks.js
> @@ +17,5 @@
> >  Cu.import("resource://services-sync/engines.js");
> >  Cu.import("resource://services-sync/record.js");
> >  Cu.import("resource://services-sync/util.js");
> > +Cu.import("resource://gre/modules/Task.jsm");
> > +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
> 
> I don't believe you need the promise import here.

You are right. Removed it.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 730569 [details] [diff] [review]
Part 2 services/sync - v5

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

This looks good, thanks.

Please make sure that TPS tests pass before landing. It's pretty straightforward:

  testing/tps/README

Comment 11

[Asaf Romano (gone)]

Comment on attachment 730060 [details] [diff] [review]
Part 1 browser and toolkit components - v4

[Note I was reviewing a -w diff]

> diff -r de4f34069146 browser/components/nsBrowserGlue.js
> --- a/browser/components/nsBrowserGlue.js	Sun Apr 07 22:59:33 2013 -0700
> +++ b/browser/components/nsBrowserGlue.js	Mon Apr 08 13:16:34 2013 +0300

> -    this._backupBookmarks();
> +    let backupComplete = true;
> +    if (this._shouldBackupBookmarks()) {
> +      backupComplete = false;
> +      this._backupBookmarks().then(
> +        function onSuccess() {
> +          backupComplete = true;
> +        },
> +        function onFailure() {
> +          // There is no point in reporting errors since we are shutting down.

I would report anyway. It's useful in debug builds.

> +          backupComplete = true;
> +        }
> +      );
> +    }
>  
>      // Backup bookmarks to bookmarks.html to support apps that depend
>      // on the legacy format.
> +    let autoExportHTMLComplete = true;
>      try {

Let's inverse-rename this to waitingForHTMLExportToComplete (and then you may also want to rename backupComplete for consistency).

Alos, while you're there, plase remove the try-catch blocks. They're not necessary (as the preference is always set) and this code is complicated enough without them.


>    /**
> -   * Backup bookmarks if needed.
> +   * Determine whether to backup bookmarks or not.

@return


> diff -r de4f34069146 browser/components/places/content/places.js
> --- a/browser/components/places/content/places.js	Sun Apr 07 22:59:33 2013 -0700
> +++ b/browser/components/places/content/places.js	Mon Apr 08 13:16:34 2013 +0300
> @@ -465,17 +465,17 @@
>     */
>    backupBookmarks: function PO_backupBookmarks() {
>      let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
>                   getService(Ci.nsIProperties);
>      let backupsDir = dirSvc.get("Desk", Ci.nsILocalFile);
>      let fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
>      let fpCallback = function fpCallback_done(aResult) {
>        if (aResult != Ci.nsIFilePicker.returnCancel) {
> -        PlacesUtils.backups.saveBookmarksToJSONFile(fp.file);
> +        BookmarkJSONUtils.exportToFile(fp.file);

Given the backup-and-exit is an expected use case, please file a follow up for some progress indicator UI. As it is, one could hit backup, close the browser and expect the backup to be in place.


> diff -r de4f34069146 toolkit/components/places/PlacesUtils.jsm
> --- a/toolkit/components/places/PlacesUtils.jsm	Sun Apr 07 22:59:33 2013 -0700
> +++ b/toolkit/components/places/PlacesUtils.jsm	Mon Apr 08 13:16:34 2013 +0300

>       */
>      saveBookmarksToJSONFile:
>      function PU_B_saveBookmarksToFile(aFile) {
> +      return Task.spawn(function() {
>        if (!aFile.exists())
>          aFile.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0600);
>        if (!aFile.exists() || !aFile.isWritable()) {
>          Cu.reportError("Unable to create bookmarks backup file: " + aFile.leafName);
> -        return;
> +          throw new Task.Result();

As it doesn't break callers now, we can throw rather than report the error (throw new Error("unable...")).

Beside that, unless you wish to return some info, why did you replace return with Task.result?

>      create:
>      function PU_B_create(aMaxBackups, aForceBackup) {
> +      return Task.spawn(function() {
>        // Construct the new leafname.
>        let newBackupFilename = this.getFilenameForDate();
>        let mostRecentBackupFile = this.getMostRecent();
>  
>        if (!aForceBackup) {
>          let numberOfBackupsToDelete = 0;
>          if (aMaxBackups !== undefined && aMaxBackups > -1)
>            numberOfBackupsToDelete = this.entries.length - aMaxBackups;
> @@ -1972,31 +1944,31 @@
>              oldestBackup.remove(false);

In a follow up, OS.File should also be adopted (here and elsewhere).


>          // Do nothing if we already have this backup or we don't want backups.
>          if (aMaxBackups === 0 ||
>              (mostRecentBackupFile &&
>               mostRecentBackupFile.leafName == newBackupFilename))
> -          return;
> +            throw new Task.Result();

Same question as above. There's no reason to use Task.Result if you're not returning some special value. That's  the code from Task.jsm:

214     } catch (ex if ex instanceof Task.Result) {
215       // The generator function threw the special exception that allows it to
216       // return a specific value on resolution.
217       this.deferred.resolve(ex.value);
218     } catch (ex if ex instanceof StopIteration) {
219       // The generator function terminated with no specific result.
220       this.deferred.resolve();

>        if (newBackupFile.exists())
> -        return;
> -
> -      this.saveBookmarksToJSONFile(newBackupFile);
> +          throw new Task.Result();
> +

ditto.

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: Part 1 browser and toolkit components - v5

(In reply to Mano from comment #11)
> Comment on attachment 730060 [details] [diff] [review]
> Part 1 browser and toolkit components - v4
> 
> [Note I was reviewing a -w diff]
> 
> > diff -r de4f34069146 browser/components/nsBrowserGlue.js
> > --- a/browser/components/nsBrowserGlue.js	Sun Apr 07 22:59:33 2013 -0700
> > +++ b/browser/components/nsBrowserGlue.js	Mon Apr 08 13:16:34 2013 +0300
> 
> > -    this._backupBookmarks();
> > +    let backupComplete = true;
> > +    if (this._shouldBackupBookmarks()) {
> > +      backupComplete = false;
> > +      this._backupBookmarks().then(
> > +        function onSuccess() {
> > +          backupComplete = true;
> > +        },
> > +        function onFailure() {
> > +          // There is no point in reporting errors since we are shutting down.
> 
> I would report anyway. It's useful in debug builds.
Done

> 
> > +          backupComplete = true;
> > +        }
> > +      );
> > +    }
> >  
> >      // Backup bookmarks to bookmarks.html to support apps that depend
> >      // on the legacy format.
> > +    let autoExportHTMLComplete = true;
> >      try {
> 
> Let's inverse-rename this to waitingForHTMLExportToComplete (and then you
> may also want to rename backupComplete for consistency).
> 

Updated

> Alos, while you're there, plase remove the try-catch blocks. They're not
> necessary (as the preference is always set) and this code is complicated
> enough without them.
> 
> 
> >    /**
> > -   * Backup bookmarks if needed.
> > +   * Determine whether to backup bookmarks or not.
> 
> @return
> 

Updated

> 
> > diff -r de4f34069146 browser/components/places/content/places.js
> > --- a/browser/components/places/content/places.js	Sun Apr 07 22:59:33 2013 -0700
> > +++ b/browser/components/places/content/places.js	Mon Apr 08 13:16:34 2013 +0300
> > @@ -465,17 +465,17 @@
> >     */
> >    backupBookmarks: function PO_backupBookmarks() {
> >      let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
> >                   getService(Ci.nsIProperties);
> >      let backupsDir = dirSvc.get("Desk", Ci.nsILocalFile);
> >      let fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
> >      let fpCallback = function fpCallback_done(aResult) {
> >        if (aResult != Ci.nsIFilePicker.returnCancel) {
> > -        PlacesUtils.backups.saveBookmarksToJSONFile(fp.file);
> > +        BookmarkJSONUtils.exportToFile(fp.file);
> 
> Given the backup-and-exit is an expected use case, please file a follow up
> for some progress indicator UI. As it is, one could hit backup, close the
> browser and expect the backup to be in place.

Filed bug 859692

> 
> 
> > diff -r de4f34069146 toolkit/components/places/PlacesUtils.jsm
> > --- a/toolkit/components/places/PlacesUtils.jsm	Sun Apr 07 22:59:33 2013 -0700
> > +++ b/toolkit/components/places/PlacesUtils.jsm	Mon Apr 08 13:16:34 2013 +0300
> 
> >       */
> >      saveBookmarksToJSONFile:
> >      function PU_B_saveBookmarksToFile(aFile) {
> > +      return Task.spawn(function() {
> >        if (!aFile.exists())
> >          aFile.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0600);
> >        if (!aFile.exists() || !aFile.isWritable()) {
> >          Cu.reportError("Unable to create bookmarks backup file: " + aFile.leafName);
> > -        return;
> > +          throw new Task.Result();
> 
> As it doesn't break callers now, we can throw rather than report the error
> (throw new Error("unable...")).
> 
> Beside that, unless you wish to return some info, why did you replace return
> with Task.result?

Fixed.  I thought we can't have "return" inside Task.spawn.

> 
> >      create:
> >      function PU_B_create(aMaxBackups, aForceBackup) {
> > +      return Task.spawn(function() {
> >        // Construct the new leafname.
> >        let newBackupFilename = this.getFilenameForDate();
> >        let mostRecentBackupFile = this.getMostRecent();
> >  
> >        if (!aForceBackup) {
> >          let numberOfBackupsToDelete = 0;
> >          if (aMaxBackups !== undefined && aMaxBackups > -1)
> >            numberOfBackupsToDelete = this.entries.length - aMaxBackups;
> > @@ -1972,31 +1944,31 @@
> >              oldestBackup.remove(false);
> 
> In a follow up, OS.File should also be adopted (here and elsewhere).

Filed bug 859695

> 
> 
> >          // Do nothing if we already have this backup or we don't want backups.
> >          if (aMaxBackups === 0 ||
> >              (mostRecentBackupFile &&
> >               mostRecentBackupFile.leafName == newBackupFilename))
> > -          return;
> > +            throw new Task.Result();

Fixed

> 
> Same question as above. There's no reason to use Task.Result if you're not
> returning some special value. That's  the code from Task.jsm:
> 
> 214     } catch (ex if ex instanceof Task.Result) {
> 215       // The generator function threw the special exception that allows
> it to
> 216       // return a specific value on resolution.
> 217       this.deferred.resolve(ex.value);
> 218     } catch (ex if ex instanceof StopIteration) {
> 219       // The generator function terminated with no specific result.
> 220       this.deferred.resolve();
> 
> >        if (newBackupFile.exists())
> > -        return;
> > -
> > -      this.saveBookmarksToJSONFile(newBackupFile);
> > +          throw new Task.Result();
> > +
> 
> ditto.

Fixed

Comment 13

[Richard Newman [:rnewman]]

(In reply to Mano from comment #11)

> > +        function onFailure() {
> > +          // There is no point in reporting errors since we are shutting down.
> 
> I would report anyway. It's useful in debug builds.

I'd also encourage you to add this to your mental list of "Places problems that we'd like to include in FHR" -- I could imagine it being very handy to report to the user "your bookmark roots got corrupted twice (bad addon?), and your backups are failing".

Comment 14

[Raymond Lee [:raymondlee]]

(In reply to Richard Newman [:rnewman] from comment #10)
> Comment on attachment 730569 [details] [diff] [review]
> Part 2 services/sync - v5
> 
> Review of attachment 730569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, thanks.
> 
> Please make sure that TPS tests pass before landing. It's pretty
> straightforward:
> 
>   testing/tps/README

When I ran the TPS tests, some tests failed.  However, the same tests also failed without applying any patches.

I have filed bug 859920.

Comment 15

[Richard Newman [:rnewman]]

Thanks Raymond. That's good enough for me!

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: Part 2 services/sync - v6

Comment 17

[Raymond Lee [:raymondlee]]

Passed try
https://tbpl.mozilla.org/?tree=Try&rev=f0759c74f681

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d946c9ecdea6
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b3b57c68f99

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d946c9ecdea6
https://hg.mozilla.org/mozilla-central/rev/7b3b57c68f99