993901 - Implement logic to decide when to fetch remotely hosted links

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

Could probably build this into DirectoryLinksProvider that gets triggered on init then checks various preconditions to fetching, e.g., recently fetched, no need to fetch, etc.

Marking depends on bug 986521 where that bug would implement the actual fetch with maybe no callers of fetch() until this bug.

Comment 1

[Olivier Yiptong [:oyiptong]]

The events that trigger the fetch() could be:

1. on browser start
2. if the last update is >= 24h, using the `idle-daily` notification, described on the [Observer Notification](https://developer.mozilla.org/en-US/docs/Observer_Notifications) documentation page

Comment 2

[Olivier Yiptong [:oyiptong]]

The events should furthermore only trigger when there are < N links with frecency < 1000.

Where N is the number of links shown in the newtab page.

Comment 3

[Olivier Yiptong [:oyiptong]]

The events should furthermore only trigger when there are < N links with frecency < 1000. Where N is the number of links shown in the newtab page.

Possible implementation: 
* Determine whether to fetch by finding N and checking a last update timestamp every time the newtab page is shown.
* Need to store the timestamp of the last time the JSON payload was successfully fetched.
* Need to run decision to download upon browser startup

This more or less takes care of the requirement to:
* Determine what N is
* Determine what the best way to trigger fetch once every 24 hours

The cost is the new tab page perhaps showing old links before triggering a download, even if last update time is > 24h.

The upside is that there is no `idle-daily` dependency, which introduces another set of complexities.

Comment 4

[Ed Lee :Mardak]

Fetching also on idle-daily would probably make counting of requests more consistent instead of only checking on startup. We could implement the idle-daily additional check in a separate bug with the initial landing here just on startup.

Comment 5

[u279076]

Please needinfo me if you'd like QA feedback on this bug.

Comment 6

[Ed Lee :Mardak]

We'll also want to fetch on pref change even if it was fetched within 24 hours. Perhaps a separate bug?

Comment 7

[maxim zhilyaev]

Attachment: v1: logic to fetch external directory links

- Download happens if it's forced or 24 hours passed since the last download.
- We check if download needed on newtab load and startup
- if (locale or endpoint) preference is changed, download is forced

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8414534 [details] [diff] [review]
v1: logic to fetch external directory links

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

::: browser/app/profile/firefox.js
@@ +1324,5 @@
>  pref("browser.newtabpage.columns", 3);
>  
> +pref("browser.newtabpage.directory.source", "data:application/json,{}");
> +
> +// last directory download time in millisceonds

It's actually seconds since you divide Date.now() by 1000.

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +197,5 @@
>                this._callObservers("onManyLinksChanged");
>              },
>              () => {
>                deferred.reject("Error writing uri data in profD.");
> +              this._callObservers("onDownloadFail");

Instead of calling onDownloadFail in every possible failure path, this should work (at the end of the method):

return deferred.promise.then(
  () => this._callObservers("onManyLinksChanged"),
  err => {
    this._callObservers("onDownloadFail"));
    throw err;
  }
);

@@ +219,5 @@
>    /**
> +   * Downloads directory links if needed
> +   * @return promise resolved immediately if no download needed, or upon completion
> +   */
> +  _fetchDirectoryContent: function(forceDoanload=false) {

Typo: forceDoanload -> forceDownload

Nit: This function should named DirectoryLinksProvider_fetchDirectoryContent to match the style here.

I like that _fetchDirectoryContent and _fetchAndCacheLinks are separate methods, but their names are confusing.  How about one of these:

* rename _fetchDirectoryContent to _maybeFetchAndCacheLinks or _fetchAndCacheLinksIfNecessary

* rename _fetchDirectoryContent to _fetchAndCacheLinks, and rename _fetchAndCacheLinks to _fetchAndCacheLinksHelper or _reallyFetchAndCacheLinks

I think it would probably be clearest to rename _fetchDirectoryContent to _fetchAndCacheLinksIfNecessary.

@@ +220,5 @@
> +   * Downloads directory links if needed
> +   * @return promise resolved immediately if no download needed, or upon completion
> +   */
> +  _fetchDirectoryContent: function(forceDoanload=false) {
> +    if( this._downloadPromise != null) {

if (this._downloadPromise) {

@@ +222,5 @@
> +   */
> +  _fetchDirectoryContent: function(forceDoanload=false) {
> +    if( this._downloadPromise != null) {
> +      // fetching links already - just return the promise
> +      return this._downloadPromise;

_downloadPromise is actually a "deferred" value (not sure what to call that), so this should return _downloadPromise.promise.  But that points to _downloadPromise being a bad name.  Either it should actually be a promise, or it should be called _downloadDeferred.  IMO it should be a promise, and then below you'd keep the deferred value in a different variable and set _downloadPromise to deferred.promise.

@@ +233,5 @@
> +        Services.prefs.setIntPref(PREF_DIRECTORY_LASTDOWNLOAD, Date.now() / 1000);
> +        this._downloadPromise.resolve();
> +        this._downloadPromise = null;
> +      },
> +      (error) => {

Nit: error => {

@@ +234,5 @@
> +        this._downloadPromise.resolve();
> +        this._downloadPromise = null;
> +      },
> +      (error) => {
> +        this._downloadPromise.resolve();

Shouldn't the promise be rejected in this case since the fetch failed?

@@ +247,5 @@
> +
> +  /**
> +   * @return true if download is needed, false otherwise
> +   */
> +  _needsDownload: function() {

Please make this

get _needsDownload() {

@@ +249,5 @@
> +   * @return true if download is needed, false otherwise
> +   */
> +  _needsDownload: function() {
> +    // fail if last download occured less then 24 hours ago
> +    let lastDownloaded = Services.prefs.getIntPref(PREF_DIRECTORY_LASTDOWNLOAD) * 1000;

What do you think about using OS.File.stat() instead of a pref?  It would be more complicated (this would need to be async for one thing, plus test changes), but I think it'd also be more correct since you'd be basing the check on the actual file.

Actually, if you go with my suggestion in bug 986521 to use localProfileDir (I think you should!), stat is even more attractive because user prefs are kept in profileDir.  So the possibility that the cached file is gone but the LAST_DOWNLOAD pref is present is more likely to occur in that setup.  Of course it's not a huge deal since it would only mean that the links would maybe be out of date for 24 hours at most.  But stat'ing still seems like the right thing to do.

If you disagree, I have some comments about the pref, but I'll keep them to myself until I hear back.

@@ +287,5 @@
>    },
>  
>    init: function DirectoryLinksProvider_init() {
>      this._addPrefsObserver();
> +    // fecth directory on startup without force

fecth -> fetch

::: toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js
@@ +34,5 @@
> +
> +// app/profile/firefox.js are not avaialble in xpcshell: hence, preset them
> +Services.prefs.setCharPref(kLocalePref, "en-US");
> +Services.prefs.setCharPref(kSourceUrlPref, kTestSource);
> +Services.prefs.setIntPref(kLastDownloadPref, 0);

kLastDownloadPref should no longer be necessary (re: my comment above).  Is setting kSourceUrlPref here really necessary?  Don't the test tasks generally set this pref themselves?  Finally, it probably is a good idea to set kLocalePref (even though tests run under en-US I'm pretty sure), but then please add a do_register_cleanup here that clears it.

@@ +112,5 @@
>  }
>  
> +function LinksChangeObserver() {
> +  this.deferred = Promise.defer();
> +  this.onManyLinksChanged = function() {this.deferred.resolve();}.bind(this);

this.onManyLinksChanged = () => this.deferred.resolve();

@@ +119,5 @@
> +
> +function promiseDirectoryDownloadOnPrefChange(pref, newValue) {
> +  let oldValue = Services.prefs.getCharPref(pref);
> +  if (oldValue != newValue) {
> +    // only if old and new value differ, setup observer

Is that because the preference service doesn't notify observers in that case?  (I don't actually know.)  If that's true, please explain that in the comment.

@@ +282,5 @@
> +  Services.prefs.setIntPref(kLastDownloadPref, Date.now()/1000);
> +  do_check_false(DirectoryLinksProvider._needsDownload());
> +  Services.prefs.setIntPref(kLastDownloadPref, (Date.now()/1000 - 60*60*24 + 1));
> +  do_check_true(DirectoryLinksProvider._needsDownload());
> +  Services.prefs.setIntPref(kLastDownloadPref, 0);

clearUserPref(kLastDownloadPref)

@@ +297,5 @@
> +  isIdentical(data, kSourceData);
> +
> +  // inspect lastDownload timestamp which should be few seconds less then now()
> +  let lastDownload = Services.prefs.getIntPref(kLastDownloadPref);
> +  do_check_true((Date.now()/1000 - lastDownload) < 5);

I/O performance on the test machines varies widely (it's crazy sometimes), so please get Date.now() right after the _fetchDirectoryContent() (i.e., before the readJsonFile()) so it's less likely to be outside the 5 second range.

@@ +329,5 @@
> +  let testObserver = new LinksChangeObserver();
> +  DirectoryLinksProvider.addObserver(testObserver);
> +
> +  yield cleanJsonFile();
> +  // insure that provider does not think it needs to download

insure -> ensure

@@ +364,5 @@
> +  cleanDirectoryLinksProvider();
> +});
> +
> +add_task(function test_DirectoryLinksProvider_fetchDirectoryOnInit() {
> +  // insure preferences are set to defaults

insure -> ensure

Comment 9

[maxim zhilyaev]

Attachment: v2: logic to fetch external directory links

Comment 10

[maxim zhilyaev]

Attachment: v2.1: logic to fetch external directory links

Comment 11

[maxim zhilyaev]

Attachment: v2.2: logic to fetch external directory links

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8419774 [details] [diff] [review]
v2.2: logic to fetch external directory links

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

::: browser/app/profile/firefox.js
@@ +1325,5 @@
>  
> +pref("browser.newtabpage.directory.source", "data:application/json,{}");
> +
> +// last directory download time in seconds
> +pref("browser.newtabpage.directory.lastDownload", 0);

This should be removed.

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +55,5 @@
>  
>    _observers: [],
>  
> +  // links download promise, resolved upon download completion
> +  _downloadDeferred: null,

The comment calls this a promise, but it's actually a deferred value, so either the comment needs to be changed or the property should really store a promise and its name should be changed.

@@ +207,5 @@
> +   */
> +  _fetchAndCacheLinksIfNecessary: function DirectoryLinksProvider_fetchAndCacheLinksIfNecessary(forceDownload=false) {
> +    if (this._downloadDeferred) {
> +      // fetching links already - just return the promise
> +      return this._downloadDeferred;

return this._downloadDeferred.promise;

There should be a test that covers this case.

@@ +217,5 @@
> +        // the new file was successfully downloaded and cached, so update a timestamp
> +        this._lastDownload = Date.now() / 1000;
> +        this._downloadDeferred.resolve();
> +        this._downloadDeferred = null;
> +        this._callObservers("onManyLinksChanged")

_fetchAndCacheLinks already broadcasted onManyLinksChanged at this point.  But actually I think it's better to do it here so that it and onDownloadFail are sent from the same place, and this seems like the right place since _fetchAndCacheLinks is lower-level.  So the broadcast in _fetchAndCacheLinks needs to be removed.

@@ +236,5 @@
> +   * @return true if download is needed, false otherwise
> +   */
> +  get _needsDownload () {
> +    // fail if last download occured less then 24 hours ago
> +    if (((Date.now() / 1000) - this._lastDownload) > this._downloadInterval) {

Nit:

return (Date.now() / 1000) - this._lastDownload > this._downloadInterval;

But if you don't agree that's clearer, no problem.

@@ +275,5 @@
>    },
>  
>    init: function DirectoryLinksProvider_init() {
>      this._addPrefsObserver();
> +    // setup directory file path and last download timestamp

This method is called on app startup by BrowserGlue._finalUIStartup, so this adds some I/O to startup, even if it's a very small amount.  I also don't like how this makes init's completion time even more unbounded.  I know init already calls _fetchAndCacheLinksIfNecessary, which is fine, but that seems less like initialization to me since it's potentially called many times.

What I was suggesting was having _fetchAndCacheLinksIfNecessary (or a helper it calls) do the stat.  So in summary: _lastDownload is 0 on init.  _fetchAndCacheLinksIfNecessary checks if _lastDownload is 0.  If no, no problem, compare it to Date.now().  If yes, then stat the file and set _lastDownload to the modification time, and then do the compare.  When the fetch successfully completes, set _lastDownload to Date.now().

BTW, I suggest calling it _lastDownloadSecs, though there doesn't seem to be any reason not to use milliseconds which would mean you can skip the division by 1000, and then I'd suggest _lastDownloadMS or _lastDownloadTime.

Does that make sense?  Maybe I'm overlooking something.

::: toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js
@@ +32,5 @@
>  const kSourceUrlPref = DirectoryLinksProvider._observedPrefs.linksURL;
>  
> +// app/profile/firefox.js are not avaialble in xpcshell: hence, preset them
> +Services.prefs.setCharPref(kLocalePref, "en-US");
> +Services.prefs.setCharPref(kSourceUrlPref, kTestURL);

Please add a do_register_cleanup here that clears these.

Comment 13

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #12)
> _fetchAndCacheLinks already broadcasted onManyLinksChanged at this point. 
> But actually I think it's better to do it here so that it and onDownloadFail
> are sent from the same place, and this seems like the right place since
> _fetchAndCacheLinks is lower-level.  So the broadcast in _fetchAndCacheLinks
> needs to be removed.
Should we have emtwo remove it from the patch in bug 986521?

Comment 14

[maxim zhilyaev]

Actually the patch already removes onManyLinksChanged call from _fetchAndCacheLinks()

> -  self._callObservers("onManyLinksChanged"); 

I suggest to keep 986521 as is, for removing onManyLinksChanged will cause test failures.

Comment 15

[maxim zhilyaev]

(In reply to Drew Willcoxon :adw from comment #12)

> >  
> >    init: function DirectoryLinksProvider_init() {
> >      this._addPrefsObserver();
> > +    // setup directory file path and last download timestamp
> 
> This method is called on app startup by BrowserGlue._finalUIStartup, so this
> adds some I/O to startup, even if it's a very small amount.  I also don't
> like how this makes init's completion time even more unbounded.  I know init
> already calls _fetchAndCacheLinksIfNecessary, which is fine, but that seems
> less like initialization to me since it's potentially called many times.
> 
> What I was suggesting was having _fetchAndCacheLinksIfNecessary (or a helper
> it calls) do the stat.  So in summary: _lastDownload is 0 on init. 
> _fetchAndCacheLinksIfNecessary checks if _lastDownload is 0.  If no, no
> problem, compare it to Date.now().  If yes, then stat the file and set
> _lastDownload to the modification time, and then do the compare.  When the
> fetch successfully completes, set _lastDownload to Date.now().
> 

I am not clear how moving "file stat" logic into _fetchAndCacheLinksIfNecessary helps initialization
1. init sets lastDownload to 0
2. init calls _fetchAndCacheLinksIfNecessary
3. _fetchAndCacheLinksIfNecessary discoveres that lastDownload is 0 (because it was set in init)
4. _fetchAndCacheLinksIfNecessary runs the "stat the file" anyway

If we want to avoid init() slowdown, then perhaps we just remove _fetchAndCacheLinksIfNecessary from init() altogether.  Then the directory links will be triggered when the next newtab is loaded.

Please advice.

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 8419774 [details] [diff] [review]
v2.2: logic to fetch external directory links

(In reply to maxim zhilyaev from comment #14)
> Actually the patch already removes onManyLinksChanged call from
> _fetchAndCacheLinks()
> 
> > -  self._callObservers("onManyLinksChanged"); 

Sorry!  I missed that somehow.

(In reply to maxim zhilyaev from comment #15)
> I am not clear how moving "file stat" logic into
> _fetchAndCacheLinksIfNecessary helps initialization

D'oh, you're right!  I have done a terrible job on this review.

> If we want to avoid init() slowdown, then perhaps we just remove
> _fetchAndCacheLinksIfNecessary from init() altogether.  Then the directory
> links will be triggered when the next newtab is loaded.

As we talked about over IRC, that wouldn't work when preloading is disabled, even if preloading is enabled for the vast majority of users.

Let's just keep init as it is then.  We could call init or just the fetch on the browser's delayedStartup instead of in nsBrowserGlue, but both the stat and the network request are off the main thread, and a stat on delayedStartup is still I/O on startup, so I don't think there would be much benefit.  (delayedStartup is where we stuff a bunch of initializers that we want to run after the browser window has been drawn: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=8a7537ba6f18#937)

r+ with my other comments addressed.

Comment 17

[maxim zhilyaev]

Attachment: v3: final patch with reviewer concerns addressed

Comment 18

[maxim zhilyaev]

Attachment: v4. fixed failing tests

Comment 19

[maxim zhilyaev]

Comment on attachment 8425701 [details] [diff] [review]
v4. fixed failing tests

Please review the changes to browser/base/content/test/newtab/head.js and browser/base/content/test/general/browser_tabopen_reflows.js

These tests were failing due to async nature of directory source preference change.  The tests now wait until the source url is downloaded and json content is written to a file.

Comment 20

[Ed Lee :Mardak]

Comment on attachment 8425701 [details] [diff] [review]
v4. fixed failing tests

>+++ b/browser/base/content/test/newtab/head.js
>@@ -83,22 +82,52 @@ function test() {
>  */
> let TestRunner = {
>   /**
>    * Starts the test runner.
>    */
>   run: function () {
>     waitForExplicitFinish();
> 
>-    this._iter = runTests();
>+    this._iter = this.prefAndTestGenerator();
>     this.next();
>   },
> 
>   /**
>-   * Runs the next available test or finishes if there's no test left.
>+   * Runs directory source download, then through available tests, and then pref cleanup
>+   *
>+   * setting directory source causes download and async file write, hence we have to wait
>+   * until write completes, then run tests, then call clearUserPref and wait again until
>+   * file write completes for the default directory source.
>+   */
>+  prefAndTestGenerator: function () {
If you need to do some pre-test work, why not just do it as plain async callbacks instead of generators? Having a generator trigger the test generator seems unnecessarily complex. And could the post-test cleanup be handled with a registerCleanupFunction although if it does end up sending events, that might impact the next test.

Although a step back, the reason we're running into this issue is because? Setting the pref to {} triggers what unwanted behavior?

Comment 21

[maxim zhilyaev]

Attachment: v4.1 simplified test fixes

Comment 22

[maxim zhilyaev]

Attachment: v4.2 - typo fix

please review a removeObserver method addition to DirectoryLinksProvider and changes to browser/base/content/test/general/browser_tabopen_reflows.js and browser/base/content/test/newtab/head.js.
The test changes are needed to accommodate to new directory.source behavior, where changing the preference causes async directory download and disk write

Comment 23

[Marco Mucci [:MarcoM]]

Removed from iteration.  Will be tracked the same way we track "non-regular" contributions from the community.

Comment 24

[Drew Willcoxon :adw]

Comment on attachment 8426511 [details] [diff] [review]
v4.2 - typo fix

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

r+ with comments addressed.

::: browser/base/content/test/general/browser_tabopen_reflows.js
@@ +63,5 @@
>   * uninterruptible reflows when opening new tabs.
>   */
>  function test() {
>    waitForExplicitFinish();
> +  let DirectoryLinksProvider = (Components.utils.import("resource://gre/modules/DirectoryLinksProvider.jsm", {})).DirectoryLinksProvider;

Nit: Cu should work here, no?  And the parentheses around (Ci.import()) aren't necessary.

::: browser/base/content/test/newtab/head.js
@@ +86,5 @@
>   * Provide the default test function to start our test runner.
>   */
>  function test() {
> +  waitForExplicitFinish();
> +  // start TestRunner.run() after directory source is downloaded and written to disk

Nit: directory source is download -> directory links are downloaded

just to make it a little clearer.

@@ +125,5 @@
>        clearHistory(function () {
> +        whenPagesUpdated(() => {
> +          // call finish when directory source is cleared
> +          watchLinksChangeOnce(finish);
> +          Services.prefs.clearUserPref(PREF_NEWTAB_DIRECTORYSOURCE);

Although this will be fine 99% of the time, I think we should still clear this pref in the cleanup function because there can be failures that prevent TestRunner.finish from being called.  It shouldn't be too hard; the cleanup function just needs to return a promise like the patch in bug 981251 does (by returning Task.spawn()).

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +307,5 @@
>    },
>  
> +  removeObserver: function DirectoryLinksProvider_removeObserver(aObserver) {
> +    let index = 0;
> +    while (index < this._observers.length) {

let index = this._observers.indexOf(aObserver);

Better yet, this._observers should be a Set instead of an array.

Comment 25

[maxim zhilyaev]

Attachment: v5. refactored observers with Set, refactored tests with a promise

Comment 26

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/94eef72cb9b7

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/94eef72cb9b7

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/mozilla-central/rev/9506880e4879 for making OSX browser-chrome-1 frequently fail like: https://tbpl.mozilla.org/php/getParsedLog.php?id=40534679&tree=Mozilla-Central

Comment 29

[Ed Lee :Mardak]

maksik, the test failure is "unexpected uninterruptible reflow '_calcMouseTargetRect" and seems to be similar to the test failures in bug 975228. The fix there was to make sure empty tiles were shown. I'm guessing the test isn't waiting for the pref to be set to {} to result in empty tiles.

Comment 30

[maxim zhilyaev]

Failures of browser_tabopen_reflows.js are caused by bug 993904 changes.
The new patch with a fix is attached to bug 993904.

Comment 31

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/9f810f13f251

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9f810f13f251