1432349 - Cloud Storage Downloads Sync API

Status: NEW

(Toolkit :: Downloads API, enhancement, P3)

Description

[Punam Dahiya [:pdahiya]]

Implement Cloud Storage Downloads Sync module that facilitates

a) Creating a config JSON file in respective storage provider app folder after the user has given permission to ‘save download to provider folder and show it in download history on other synced devices’.

This config file will contain minimal data that duplicates whats in download manager, enough to reconstruct download history on other devices.

b) Check existence of this config file on other synced devices and display provider downloads in download history panel.

Comment 1

[Punam Dahiya [:pdahiya]]

Attachment: WIP Cloud Storage Download Sync API Patch

Comment 2

[Punam Dahiya [:pdahiya]]

Attachment: Cloud Storage Download Sync API Patch

Hi Gijs

Attaching cloud storage download sync API WIP patch for your feedback.

Thanks!

Comment 3

[Punam Dahiya [:pdahiya]]

Sync cloud storage downloads across desktop devices with the more seamless UX is the focus for next release and experiments.

https://mozilla.invisionapp.com/share/U5FN57H2RTX#/screens/276953845_Sync_To_Cloud

Comment 4

[:Gijs (he/him)]

Comment on attachment 8950139 [details] [diff] [review]
Cloud Storage Download Sync API Patch

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

I'm sorry, but I'm going to punt this review to Mark. Mark knows sync much better and is therefore probably a better reviewer for this patch anyway, and my queue hasn't really gone below 3-5 patches a day to review for a week or two now, so I'd struggle to dedicate the time required to do a good review of this work. Mark's queue looks smaller, from a quick glance... Mark, feel free to send this back to me (or fwd to Kit or :tcsc) if you don't have time.

Also, if this code is also meant to run on fennec, you probably want additional review from a mobile peer.

::: toolkit/components/cloudstorage/CloudStorage.jsm
@@ +19,5 @@
>  ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> +ChromeUtils.defineModuleGetter(this, "CloudStorageDwnldSync",

Nit: please don't abbreviate the 'download' bit like this. We could just use the full name, or if you think it really needs to be shorter, omit 'Storage' from the name or use a 'cloudstorage' directory under 'modules' and name the module 'DownloadSync.jsm'.

@@ +298,5 @@
>        this.resetFolderListPref().catch((err) => {
>          Cu.reportError("CloudStorage: Failed to reset folderList pref " + err);
>        });
> +
> +      // Call unitialize method of cloud storage download sync

This comment doesn't really add anything.

@@ +300,5 @@
>        });
> +
> +      // Call unitialize method of cloud storage download sync
> +      await CloudStorageDwnldSync.uninit(this.providersMetaData).catch((err) => {
> +        Cu.reportError("CloudStorage: Failed to uninit cloud storage dwnld sync api " + err);

Don't abbreviate in the message for sure. You probably don't need to repeat 'cloud storage' in here either.

@@ +313,5 @@
>      if (providersCount > 0) {
>        // Array of boolean results for each provider handled for custom downloadpath
>        let handledProviders = await this.initDownloadPathIfProvidersExist();
>        if (handledProviders.length === providersCount) {
> +        // Initialize download sync module by passing provider metadata

Same thing here. If you think we need a comment, maybe:

// We have at least 1 provider with a usable download path. Ensure we sync downloads.

or something.

::: toolkit/components/cloudstorage/tests/unit/test_cloudstoragedwnldsync.js
@@ +17,5 @@
> +                                  "resource://gre/modules/FileUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "OS",
> +                                  "resource://gre/modules/osfile.jsm");
> +ChromeUtils.defineModuleGetter(this, "setTimeout",
> +                               "resource://gre/modules/Timer.jsm");

You don't use this (and you can also remove the `no-arbitrary-setTimeout` eslint line).

Comment 5

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

Comment on attachment 8950139 [details] [diff] [review]
Cloud Storage Download Sync API Patch

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

This looks OK to me as a first pass. However, I thought this feature was going to be rolled out as a system addon so you didn't need to ride the trains - which was one of the key advantages of the "hack" to use a json file rather than a real sync engine?

I think the error handling might need a little more work, and that you should consider a better logging strategy - Log.jsm might make sense for stuff that assumes Sync is configured (so your log entries will end up in the sync log files), or maybe console.jsm or similar, so you can have debug logging and a concept of "warnings" - there are a number of Cu.reportError() calls that appear to be more for debugging than for errors.

::: toolkit/components/cloudstorage/CloudStorage.jsm
@@ +299,5 @@
>          Cu.reportError("CloudStorage: Failed to reset folderList pref " + err);
>        });
> +
> +      // Call unitialize method of cloud storage download sync
> +      await CloudStorageDwnldSync.uninit(this.providersMetaData).catch((err) => {

|await somePromise.catch(...)| is quite unusual - it's typically written as:

try
  await somePromise;
catch (err) {
  ...
}

alternatively, maybe it could continue in the background (ie, without an await) like the call to resetFolderListPref above?

::: toolkit/components/cloudstorage/CloudStorageDwnldSync.jsm
@@ +11,5 @@
> +ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +ChromeUtils.defineModuleGetter(this, "CloudStorage",

I suspect at least some of these should be using defineLazyModuleGetter?

@@ +21,5 @@
> +ChromeUtils.defineModuleGetter(this, "DownloadPaths",
> +                                  "resource://gre/modules/DownloadPaths.jsm");
> +ChromeUtils.defineModuleGetter(this, "FileUtils",
> +                                  "resource://gre/modules/FileUtils.jsm");
> +ChromeUtils.defineModuleGetter(this, "NetUtil",

You import this only for newURI - if you use Services.io.newURI you could avoid this import.

@@ +30,5 @@
> +                                  "resource://gre/modules/PlacesUtils.jsm");
> +XPCOMUtils.defineLazyServiceGetter(this, "gDownloadHistory",
> +           "@mozilla.org/browser/download-history;1",
> +           Ci.nsIDownloadHistory);
> +

You could consider using Log.jsm and a log that starts with the name "Sync." - then logging info will end up in Sync log files (and possibly also to the console or to dump, depending on sync prefs)

@@ +139,5 @@
> +      isInitialized = await CloudStorageDwnldSyncInternal.syncDownloadHistory(metadata);
> +    } catch (err) {
> +      Cu.reportError(err);
> +    }
> +    return isInitialized;

It appears this result value isn't used?

@@ +225,5 @@
> +   * @return {Promise} which resolves to File.info of asset, null otherwise.
> +   */
> +  async _getAssetInfo(path) {
> +    return OS.File.stat(path).catch(err => {
> +      Cu.reportError(`Failed to get info of ${path}`, err);

Cu.reportError doesn't take 2 args (but the logging module's functions typically do)

@@ +273,5 @@
> +      return false;
> +    }
> +
> +    let keys = this.providersInUse.split(",");
> +    let configHandleTime = Math.floor(Date.now() / 1000);

It appears you are storing the local time here, but later you compare it with the lastModifiedTime as reported by the provider. This is going to be an issue with clock skew. Is it possible for you to store the lastModified time of each file (ie, avoid the use of Date.now completely)?

@@ +287,5 @@
> +      let publicList = await Downloads.getList(Downloads.ALL);
> +      for (let key of keys) {
> +        // Add downloads history item of a provider to history panel.
> +        for (let historyItem of this.downloadsHistoryMetaData[key].history) {
> +          await this.addDownloadHistoryItem(historyItem, this.downloadsHistoryMetaData[key].downloadPath, publicList);

It might be wise to catch and log errors here to ensure that one strange item doesn't kill the entire feature.

@@ +604,5 @@
> +
> +    try {
> +      await OS.File.remove(path);
> +    } catch (ex) {
> +      // failure to delete it isn't an error.

Is it possible that failure to delete it due to a transient error might bite you later if the user reconfigures the feature and this code finds an old, stale file left behind?

Comment 6

[Punam Dahiya [:pdahiya]]

Attachment: Cloud Storage Download Sync API Patch

Comment 7

[Punam Dahiya [:pdahiya]]

Attachment: Cloud Storage Download Sync API Patch

Comment 8

[Punam Dahiya [:pdahiya]]

(In reply to Mark Hammond [:markh] from comment #5)
> Comment on attachment 8950139 [details] [diff] [review]
> Cloud Storage Download Sync API Patch
> 
> Review of attachment 8950139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks OK to me as a first pass. However, I thought this feature was
> going to be rolled out as a system addon so you didn't need to ride the
> trains - which was one of the key advantages of the "hack" to use a json
> file rather than a real sync engine?
> 

At first, plan was to get it out as a shield add-on but that has sync limitation and in both system add-on and back-end API we loose flexibility to land code outside of trains. I agree it's a faster solution to validate this experiment, until we are ready to roll limited cloud download sync in real sync engine. 


> @@ +30,5 @@
> > +                                  "resource://gre/modules/PlacesUtils.jsm");
> > +XPCOMUtils.defineLazyServiceGetter(this, "gDownloadHistory",
> > +           "@mozilla.org/browser/download-history;1",
> > +           Ci.nsIDownloadHistory);
> > +
> 
> You could consider using Log.jsm and a log that starts with the name "Sync."
> - then logging info will end up in Sync log files (and possibly also to the
> console or to dump, depending on sync prefs)
> 

Working on updating logging using Log.jsm

> @@ +139,5 @@
> > +      isInitialized = await CloudStorageDwnldSyncInternal.syncDownloadHistory(metadata);
> > +    } catch (err) {
> > +      Cu.reportError(err);
> > +    }
> > +    return isInitialized;
> 
> It appears this result value isn't used?
> 

Updated to use result value to uninitialize only if API is previously initialized
> @@ +225,5 @@
> > +   * @return {Promise} which resolves to File.info of asset, null otherwise.
> > +   */
> > +  async _getAssetInfo(path) {
> > +    return OS.File.stat(path).catch(err => {
> > +      Cu.reportError(`Failed to get info of ${path}`, err);
> 
> Cu.reportError doesn't take 2 args (but the logging module's functions
> typically do)
> 
> @@ +273,5 @@
> > +      return false;
> > +    }
> > +
> > +    let keys = this.providersInUse.split(",");
> > +    let configHandleTime = Math.floor(Date.now() / 1000);
> 
> It appears you are storing the local time here, but later you compare it
> with the lastModifiedTime as reported by the provider. This is going to be
> an issue with clock skew. Is it possible for you to store the lastModified
> time of each file (ie, avoid the use of Date.now completely)?
> 

Handled by updating lastConfigModifiedTime pref with the most recent config handled timestamp
> @@ +287,5 @@
> > +      let publicList = await Downloads.getList(Downloads.ALL);
> > +      for (let key of keys) {
> > +        // Add downloads history item of a provider to history panel.
> > +        for (let historyItem of this.downloadsHistoryMetaData[key].history) {
> > +          await this.addDownloadHistoryItem(historyItem, this.downloadsHistoryMetaData[key].downloadPath, publicList);
> 
> It might be wise to catch and log errors here to ensure that one strange
> item doesn't kill the entire feature.
> 
Done
> @@ +604,5 @@
> > +
> > +    try {
> > +      await OS.File.remove(path);
> > +    } catch (ex) {
> > +      // failure to delete it isn't an error.
> 
> Is it possible that failure to delete it due to a transient error might bite
> you later if the user reconfigures the feature and this code finds an old,
> stale file left behind?
+1, handled to reset lastConfigModifiedTime pref only when all provider config are successfully deleted.

Comment 9

[Punam Dahiya [:pdahiya]]

Thanks Gijs, Mark for your feedback. I have updated patch with feedback provided.

Attached patch is using a hybrid of weave:engine:sync notifications and nsINavHistoryObserver to know that the history engine is currently syncing history download and triggers syncDownloadHistory method during this time.

Pieces remaining are 
- improve logging using Log.jsm 
- delete old download history entries from config JSON after a certain time interval (configurable via pref)
- some more test coverage

Comment 10

[Punam Dahiya [:pdahiya]]

Attachment: Bug 1432349 - Cloud Storage Download Sync API

Review commit: https://reviewboard.mozilla.org/r/221746/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221746/

Comment 11

[Punam Dahiya [:pdahiya]]

Hi Mark, Gijs

Attaching download sync API patch for your review. Patch is updated to sync cloud storage downloads to user other devices if provider app and sync exists.

Link to try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=161a0569cc6702d18b45ed9e4f7036694bcb3d43

For now to easily test download sync API, one can install legacy shield add-on (xpi attached to bug) that invoke writeConfigDownload when user selects 'Save to <provider app>' in the door-hangar prompt.

https://github.com/mozilla/shield-cloudstorage/commit/3cdb5345687d2b4eaec4ee1c6b04f0f2d68f5e36

Thanks!

Comment 12

[Punam Dahiya [:pdahiya]]

Attachment: Legacy add-on to test download sync api

Comment 13

[Punam Dahiya [:pdahiya]]

Comment on attachment 8952528 [details]
Bug 1432349 - Cloud Storage Download Sync API

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221746/diff/1-2/

Comment 14

[Punam Dahiya [:pdahiya]]

Pushed for review with tests fixed

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf5b8b61cd3f37d61ee686c684d95469ae3a1b9e&selectedJob=163366023

Comment 15

[:Gijs (he/him)]

Mark's a Firefox peer, you won't need 2 people to review this.

Comment 16

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

Comment on attachment 8952528 [details]
Bug 1432349 - Cloud Storage Download Sync API

https://reviewboard.mozilla.org/r/221746/#review230476

I'm very sorry for the slow review. This is looking in good shape, but I think there's still a little stuff to be done.

::: browser/components/nsBrowserGlue.js:1232
(Diff revision 2)
> +    // Load CloudStorage module to observe cloud.services.api.enabled
> +    // synced pref. Only when this synced pref is true
> +    // it initiates loading provider metadata and cloud downloads sync
> +    // See Bug 1432349
> +
> +    Services.tm.idleDispatchToMainThread(() => {

I wonder if there is anything we can do to avoid this module being loaded for all users quickly after startup. There was alot of work recently to improve startup performance which we want to be careful to not undo - and talos might even complain.

Eg, one option might be to import the module on the "weave:service:ready" notification? That should mean non-sync users don't pay any penalty and sync users pay the penalty when Sync itself finally gets around to initializing)

(I guess if we can't come up with other ideas and talos does *not* complain we can probably live with it though)

::: toolkit/components/cloudstorage/DownloadSync.jsm:118
(Diff revision 2)
> +    * Init method to initiate download sync. This method is invoked from CloudStorage.jsm
> +    * init method if cloud.services.api.enabled pref is true and provider app exists on
> +    * user device
> +    */
> +  async init(metadata) {
> +    let isInitialized = null;

should we initialize this to false, so the function always returns a book?

::: toolkit/components/cloudstorage/DownloadSync.jsm:137
(Diff revision 2)
> +    }
> +    return isInitialized;
> +  },
> +
> +  async uninit(metadata) {
> +    DownloadSyncInternal.removeObservers();

It seems possible that this might be called even if we haven't setup the observers (eg, if the catch above is entered). I'd expect removeObservers to throw in that case, meaning the following uninit will not get called.

Further, I'm guessing that if .init() returns false, then uninit isn't going to be called - so if DownloadSyncInternal.syncDownloadHistory() returns false, we'll never remove the observers.

So ISTM that:
* init() probably shouldn't setupOBservers is !isInitialized.
* a try/catch around removeObservers sounds prudent.

::: toolkit/components/cloudstorage/DownloadSync.jsm:203
(Diff revision 2)
> +   * @resolves
> +   * boolean value of file existence check
> +   */
> +  _checkIfAssetExists(path) {
> +    return OS.File.exists(path).catch(err => {
> +      logger.error(`Couldn't check existance of ${path}: ${err}`);

here and elsewhere in this file - you will almost certainly get better logging if you pass the error object as the 2nd param to the logging functions - the logging module knows how to convert them to a string better than will happen here.

ie:
logger.error(`Couldn't check existance of ${path}`, err);

(it will also put a ": " between them)

::: toolkit/components/cloudstorage/DownloadSync.jsm:217
(Diff revision 2)
> +   *
> +   * @return {Promise} which resolves to File.info of asset, null otherwise.
> +   */
> +  async _getAssetInfo(path) {
> +    return OS.File.stat(path).catch(err => {
> +      logger.error(`Failed to get info of ${path}: ${err}`);

ditto here.

::: toolkit/components/cloudstorage/DownloadSync.jsm:240
(Diff revision 2)
> +    let json = null;
> +    try {
> +      let response = await fetch(uri);
> +      if (response.ok) {
> +        json = await response.json();
> +      }

It probably makes sense to log stuff from the response if !ok (eg, the http status code or something) - otherwise this will return null with no log output to indicate what went wrong.

::: toolkit/components/cloudstorage/DownloadSync.jsm:242
(Diff revision 2)
> +      let response = await fetch(uri);
> +      if (response.ok) {
> +        json = await response.json();
> +      }
> +    } catch (e) {
> +      logger.error("Fetching " + uri + " results in error: ", e);

As above, you can take the trailing ": " from the first string - the logging module will add it when there's a second param.

::: toolkit/components/cloudstorage/DownloadSync.jsm:259
(Diff revision 2)
> +   */
> +  async syncDownloadHistory() {
> +    // Check for pref 'providersInUse' that will return keys of providers for which
> +    // config files should be read. Don't continue if this pref is not set.
> +    if (!this.providersInUse) {
> +      return true;

This function appears to always return true - it should probably just return nothing?

::: toolkit/components/cloudstorage/DownloadSync.jsm:343
(Diff revision 2)
> +
> +    if (!data) {
> +      return false;
> +    }
> +
> +    if (!Object.keys(this.downloadsHistoryMetaData).includes(key)) {

this seems a bot convoluted - can downloadsHistoryMetaData be made a map?

::: toolkit/components/cloudstorage/DownloadSync.jsm:386
(Diff revision 2)
> +    if (!this._shouldSyncDownload(history.lastModifiedTime)) {
> +      logger.debug("Previously added download history item: " + history.title);
> +      return false;
> +    }
> +
> +    // Check if destPath exists, only than add download history

s/than/then/?

::: toolkit/components/cloudstorage/DownloadSync.jsm:395
(Diff revision 2)
> +
> +    let file = null;
> +    try {
> +      file = new FileUtils.File(destPath);
> +    } catch (ex) {
> +      return false;

you probably want to log something here given you've already checked the file exists?

::: toolkit/components/cloudstorage/DownloadSync.jsm:467
(Diff revision 2)
> +  /**
> +   * Waits for the download annotations to be set for the given page, required
> +   * because the addDownload method will add these to the database asynchronously.
> +   */
> +  async _waitForAnnotations(sourceUriSpec) {
> +    let sourceUri = Services.io.newURI(sourceUriSpec);

I don't understand how this actually waits - there are no `await` statements nor promises returned.

::: toolkit/components/cloudstorage/DownloadSync.jsm:534
(Diff revision 2)
> +      // Read JSON file at path into json object
> +      json = await this._downloadJSON(Services.io.newFileURI(new FileUtils.File(path)).spec);
> +    }
> +
> +    let historyObj = {};
> +    Object.defineProperty(historyObj, "title", {

can't these just be regular properties? ie, `let historyObj = {title: ...};` etc?

::: toolkit/components/cloudstorage/tests/unit/test_cloudstoragedwnldsync.js:279
(Diff revision 2)
> +    await configReadAndWrite(GDRIVE_KEY, result);
> +  }
> +
> +  // Uninstall GDrive by removing discovery folder
> +  nsIGDriveFile.remove(false);
> +});

I think the tests should cover the history observers too - eg, make sure that you are interacting with places correctly