Intermittent toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js | Frecency of the page is the expected one (http://mochi.test:8888/tests/toolkit/components/places/tests/browser/redirect_thrice.sjs) - Got 73, expected 75

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: intermittent-bug-filer, Assigned: standard8)

Tracking

({intermittent-failure})

unspecified
mozilla54
intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox52 unaffected, firefox53 unaffected, firefox54 fixed)

Details

Attachments

(1 attachment)

Mark, any idea?
Flags: needinfo?(standard8)
Marco and I chatted on irc and came to the conclusion that this is a result of the idle-daily timer kicking in.

The idle-daily timer causes the nsNavHistory::DecayFrecency function to be called, which reduced the frecencies by 0.975, and 75 * 0.975 = 73.

I'll add a hidden pref that the tests can use to avoid a frecency decay.
Assignee: nobody → standard8
Flags: needinfo?(standard8)
Comment hidden (mozreview-request)
Note: I also included an xpcshell test for DecayFrecency, as I couldn't find an existing one.

Comment 5

2 years ago
mozreview-review
Comment on attachment 8833987 [details]
Bug 1336233 - Fix intermittent failures in browser_multi_redirect_frecency.js by ensuring the idle service updates for frecency are ignored.

https://reviewboard.mozilla.org/r/110094/#review111482

::: toolkit/components/places/nsNavHistory.cpp:3117
(Diff revision 1)
>    );
>    NS_ENSURE_STATE(decayFrecency);
>  
> +  nsCOMPtr<mozIStorageBindingParamsArray> paramsArray;
> +  rv = decayFrecency->NewBindingParamsArray(getter_AddRefs(paramsArray));
> +  NS_ENSURE_SUCCESS(rv, rv);

You don't need all of this complication, bindingParamsArray is an helper needed only when you want to run the same query with multiple values, for example if you'd want to re-run this query with 3 different decays values.

all you need here is just:
rv = decayFrecency->BindDoubleByName(...

::: toolkit/components/places/tests/PlacesTestUtils.jsm:173
(Diff revision 1)
> +   * @return {Promise}
> +   * @resolves Returns the frecency.
> +   * @rejects JavaScript exception.
> +   */
> +  frecencyInDB: Task.async(function* (aURI) {
> +    let url = aURI instanceof Ci.nsIURI ? aURI.spec : aURI;

May also be an URL. A more complete path would be:
let url = aURI instanceof Ci.nsIURI ? new URL(aURI.spec) : new URL(aURI);
(note: new URL of a URL is still a URL)

And then just use url.href.

::: toolkit/components/places/tests/unit/test_frecency_decay.js:2
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

nit: PD headers are not required in tests, tests code defaults to PD.

::: toolkit/components/places/tests/unit/test_frecency_decay.js:5
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// See nsNavHistory.cpp.
> +const PREF_FREC_DECAY_RATE_DEF = 0.975;

Let's just set the pref to this value, rather than trying to keep these in sync. The actual value doesn't matter, what this is testing is the functionality.

::: toolkit/components/places/tests/unit/test_frecency_decay.js:15
(Diff revision 1)
> + * @param {nsIURI} expectedURI The URI to check frecency for.
> + * @param {Number} expectedFrecency The expected frecency for the URI.
> + * @returns {Promise} A promise which is resolved when the URI is seen.
> + */
> +function promiseFrecencyChanged(expectedURI, expectedFrecency) {
> +  let deferred = Promise.defer();

Promise.defer() is deprecated, either return a new Promise or use PromiseUtils.defer()

::: toolkit/components/places/tests/unit/test_frecency_decay.js:20
(Diff revision 1)
> +  let deferred = Promise.defer();
> +  let obs = new NavHistoryObserver();
> +  obs.onFrecencyChanged =
> +    (uri, newFrecency, guid, hidden, visitDate) => {
> +      PlacesUtils.history.removeObserver(obs);
> +      do_check_true(!!uri);

Please use Assert.jsm in new xpcshell code (Assert.ok and Assert.equal), the do_check methods are (theorically) deprecated.

::: toolkit/components/places/tests/unit/test_frecency_decay.js:35
(Diff revision 1)
> + * Promises that the many frecencies changed notification has been seen.
> + *
> + * @returns {Promise} A promise which is resolved when the notification is seen.
> + */
> +function promiseManyFrecenciesChanged() {
> +  let deferred = Promise.defer();

ditto

::: toolkit/components/places/tests/unit/test_frecency_decay.js:39
(Diff revision 1)
> +function promiseManyFrecenciesChanged() {
> +  let deferred = Promise.defer();
> +  let obs = new NavHistoryObserver();
> +  obs.onManyFrecenciesChanged = () => {
> +    PlacesUtils.history.removeObserver(obs);
> +    do_check_true(true);

ditto

::: toolkit/components/places/tests/unit/test_frecency_decay.js:47
(Diff revision 1)
> +  PlacesUtils.history.addObserver(obs, false);
> +  return deferred.promise;
> +}
> +
> +add_task(function* test_frecency_decay() {
> +  let unvistedBookmarkFrecency = Services.prefs.getIntPref("places.frecency.unvisitedBookmarkBonus");

typo: unvisted

::: toolkit/components/places/tests/unit/test_frecency_decay.js:52
(Diff revision 1)
> +  let unvistedBookmarkFrecency = Services.prefs.getIntPref("places.frecency.unvisitedBookmarkBonus");
> +
> +  // Add a bookmark and check its frecency.
> +  let bm = PlacesUtils.bookmarks;
> +  let uri = NetUtil.newURI("http://example.com/b");
> +  bm.insertBookmark(bm.unfiledBookmarksFolder, uri,

Please use the new async bookmarking API:
let url = "http://example.com/b";
yield PlacesUtils.bookmarks.insert({
  url,
  parentGuid: PlacesUtils.bookmarks.unfiledGuid
});

See Bookmarks.jsm for documentation.
Moreover with the change to frecencyInDB you won't need an nsIURI nor a URL, it will be converted internally.

::: toolkit/components/places/tests/unit/test_frecency_decay.js:54
(Diff revision 1)
> +  // Add a bookmark and check its frecency.
> +  let bm = PlacesUtils.bookmarks;
> +  let uri = NetUtil.newURI("http://example.com/b");
> +  bm.insertBookmark(bm.unfiledBookmarksFolder, uri,
> +                    Ci.nsINavBookmarksService.DEFAULT_INDEX, "test");
> +  yield promiseFrecencyChanged(uri, unvistedBookmarkFrecency);

ideally you should store the promise before the insert, and check it after it. We can't guarantee a specific order of those events.

::: toolkit/components/places/tests/unit/test_frecency_decay.js:60
(Diff revision 1)
> +
> +  // Trigger DecayFrecency.
> +  PlacesUtils.history.QueryInterface(Ci.nsIObserver)
> +             .observe(null, "idle-daily", "");
> +
> +  yield promiseManyFrecenciesChanged();

just for coherence, store the promise before the notification, and check it after.
Attachment #8833987 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 7

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7fbc8eb28f78
Fix intermittent failures in browser_multi_redirect_frecency.js by ensuring the idle service updates for frecency are ignored. r=mak
Backed out for Windows build bustage at nsNavHistory.cpp(3100):

https://hg.mozilla.org/integration/autoland/rev/5dfdb03c618d3fd9d222e0b3701f98d0756adf4f

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7fbc8eb28f78d9a878f294d170157a71fec95a11
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=75193960&repo=autoland

08:34:12     INFO -  c:/builds/moz2_slave/autoland-w64-d-000000000000000/build/src/toolkit/components/places/nsNavHistory.cpp(3100): error C2220: warning treated as error - no 'object' file generated
08:34:12     INFO -  Warning: C4305 in c:\builds\moz2_slave\autoland-w64-d-000000000000000\build\src\toolkit\components\places\nsNavHistory.cpp: 'argument': truncation from 'double' to 'float'
08:34:12     INFO -  c:/builds/moz2_slave/autoland-w64-d-000000000000000/build/src/toolkit/components/places/nsNavHistory.cpp(3100): warning C4305: 'argument': truncation from 'double' to 'float'
08:34:12     INFO -  c:/builds/moz2_slave/autoland-w64-d-000000000000000/build/src/config/rules.mk:1015: recipe for target 'Unified_cpp_components_places0.obj' failed
08:34:12     INFO -  mozmake.EXE[5]: *** [Unified_cpp_components_places0.obj] Error 2
Flags: needinfo?(standard8)
Comment hidden (mozreview-request)
I've just updated the patch with a static_cast. Knew I should have added that earlier...

I'll put it through try server and then reland.
Flags: needinfo?(standard8)
Comment hidden (mozreview-request)

Comment 12

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c6a814dd5af
Fix intermittent failures in browser_multi_redirect_frecency.js by ensuring the idle service updates for frecency are ignored. r=mak

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6c6a814dd5af
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
You need to log in before you can comment on or make changes to this bug.