Apply use of BookmarkJSONUtils.exportToFile

RESOLVED FIXED in mozilla23

Status

()

Toolkit
Places
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: andreshm, Assigned: raymondlee)

Tracking

(Blocks: 1 bug)

unspecified
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Replace sync calls to PlacesUtils.saveBookmarksToJSONFile with BookmarkJSONUtils.exportToFile async calls.
(Reporter)

Updated

5 years ago
Depends on: 822200
(Assignee)

Updated

5 years ago
Assignee: nobody → raymond
(Assignee)

Comment 1

5 years ago
Created attachment 727107 [details] [diff] [review]
v1

I have not touched the placesUtils.jsm because Bug 852032 is being worked on to move the backup code to PlacesBackups.jsm
Attachment #727107 - Flags: review?(mak77)
(Assignee)

Updated

5 years ago
Blocks: 852034
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Comment on attachment 727107 [details] [diff] [review]
v1

Updating the patch
Attachment #727107 - Flags: review?(mak77)
(Assignee)

Comment 3

5 years ago
Created attachment 728945 [details] [diff] [review]
v2

This should be reviewed and landed before we try to move code from PlacesUtils to PlacesBackups bug 852032
Attachment #727107 - Attachment is obsolete: true
Attachment #728945 - Flags: review?(mak77)
(Assignee)

Updated

5 years ago
Blocks: 852032
(Assignee)

Comment 4

5 years ago
Created attachment 728954 [details] [diff] [review]
v3
Attachment #728945 - Attachment is obsolete: true
Attachment #728945 - Flags: review?(mak77)
Attachment #728954 - Flags: review?(mak77)
(Reporter)

Updated

5 years ago
Blocks: 854761
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)
Attachment #728954 - Flags: review?(mak77) → review?(mano)
(Assignee)

Comment 6

5 years ago
Created attachment 730060 [details] [diff] [review]
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.
Attachment #728954 - Attachment is obsolete: true
Attachment #728954 - Flags: review?(mano)
Attachment #730060 - Flags: review?(mano)
(Assignee)

Updated

5 years ago
Attachment #730060 - Attachment description: v4 → Part 1 browser and toolkit components - v4
(Assignee)

Comment 7

5 years ago
Created attachment 730063 [details] [diff] [review]
Part 2 services/sync - v4
Attachment #730063 - Flags: review?(rnewman)
(Assignee)

Updated

5 years ago
Attachment #730063 - Attachment description: Part 2 services/sync -v4 → Part 2 services/sync - v4
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.
Attachment #730063 - Flags: review?(rnewman) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 730569 [details] [diff] [review]
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.
Attachment #730063 - Attachment is obsolete: true
Attachment #730569 - Flags: review?(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
Attachment #730569 - Flags: review?(rnewman) → review+
(Assignee)

Updated

5 years ago
Blocks: 855218
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.
Attachment #730060 - Flags: review?(mano) → review+
(Assignee)

Updated

5 years ago
Blocks: 859692
(Assignee)

Comment 12

5 years ago
Created attachment 735057 [details] [diff] [review]
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
Attachment #730060 - Attachment is obsolete: true
(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".
(Assignee)

Comment 14

5 years ago
(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.
Depends on: 859920
Thanks Raymond. That's good enough for me!
(Assignee)

Comment 16

5 years ago
Created attachment 735320 [details] [diff] [review]
Part 2 services/sync - v6
Attachment #730569 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Passed try
https://tbpl.mozilla.org/?tree=Try&rev=f0759c74f681
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d946c9ecdea6
https://hg.mozilla.org/mozilla-central/rev/7b3b57c68f99
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Updated

5 years ago
Blocks: 867068
You need to log in before you can comment on or make changes to this bug.