Closed Bug 1336233 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed

People

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

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

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)
Note: I also included an xpcshell test for DecayFrecency, as I couldn't find an existing one.
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+
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)
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)
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
https://hg.mozilla.org/mozilla-central/rev/6c6a814dd5af
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.