1336233 - 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

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Treeherder Bug Filer]

Filed by: wkocher [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=73903669&repo=mozilla-inbound

https://queue.taskcluster.net/v1/task/HH03ZNsWQ4yhEVRRyifsMg/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[Marco Bonardo [:mak]]

Mark, any idea?

Comment 2

[Mark Banner (: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.

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1336233 - Fix intermittent failures in browser_multi_redirect_frecency.js by ensuring the idle service updates for frecency are ignored.

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

Comment 4

[Mark Banner (:standard8)]

Note: I also included an xpcshell test for DecayFrecency, as I couldn't find an existing one.

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Mark Banner (:standard8)]

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.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 9

[Mark Banner (:standard8)]

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.

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

Comment 10

[Mark Banner (:standard8)]

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.

Comment 11

[Mark Banner (:standard8)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110094/diff/3-4/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

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

https://hg.mozilla.org/mozilla-central/rev/6c6a814dd5af