1310941 - Bookmark tracking using new "source" observer param doesn't work

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

I noticed log entries like:

> 1476777777032	Sync.Store.Bookmarks	DEBUG	Applying record z3iZEO9wDFAE
> 1476777777032	Sync.Store.Bookmarks	DEBUG	Remote parent is 1sRNFdejqkRa
> 1476777777039	Sync.Store.Bookmarks	DEBUG	Updated bookmark z3iZEO9wDFAE under 1sRNFdejqkRa: {"syncId":"z3iZEO9wDFAE","kind":"bookmark","title":"About Us","parentSyncId":"1sRNFdejqkRa","url":{},"tags":[],"keyword":null,"description":null,"loadInSidebar":false}
1476777777141	Sync.SyncScheduler	DEBUG	Global Score threshold hit, triggering sync.
...

And noticed 2 things:

* The "Global Score threshold hit, triggering sync." as we are applying records happens every time, so obviously is suspicious ;) Instrumenting the observers shows the source param is an empty string. IOW, our tracker isn't ignoring our own changes.

* A minor point, but the "Updated bookmark" log doesn't show the URL.

Comment 1

[Lina Butler [:lina]]

Attachment: Bug 1310941 - Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes.

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

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8802170 [details]
Bug 1310941 - Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes.

https://reviewboard.mozilla.org/r/86668/#review85610

Looks good to me.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ae77edf8ed2
Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes. r=tcsc

Comment 4

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef7b401149f2
Backed out changeset 3ae77edf8ed2 for frequent intermittent xpcshell failure in test_bookmark_engine.js. r=backout

Comment 5

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

Backed out for frequent xpcshell failures in test_bookmark_engine.js:

https://hg.mozilla.org/integration/autoland/rev/ef7b401149f259afe1f5eaa562e233a377463cea

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3ae77edf8ed2c28d08ca951ca491327935da5d48
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=5255841&repo=autoland

[task 2016-10-18T19:50:07.016557Z] 19:50:07     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property this.onSavedChangedIDs" {file: "resource://services-sync/engines.js" line: 138}]"
[task 2016-10-18T19:50:07.018165Z] 19:50:07     INFO -  PROCESS | 9169 | 1476820205402	Sqlite.Connection	DEBUG	places.sqlite#1: Stmt #0 finished.
[task 2016-10-18T19:50:07.019898Z] 19:50:07     INFO -  PROCESS | 9169 | 1476820205405	Sqlite.Connection	TRACE	places.sqlite#1: Stmt #2 PRAGMA page_count
[task 2016-10-18T19:50:07.022402Z] 19:50:07     INFO -  PROCESS | 9169 | 1476820205407	Sqlite.Connection	DEBUG	places.sqlite#1: Stmt #1 finished.
[task 2016-10-18T19:50:07.024336Z] 19:50:07     INFO -  TEST-PASS | services/sync/tests/unit/test_bookmark_engine.js | test_change_during_sync - [test_change_during_sync : 91] Folder should have 1 child before first sync - ["iZq6AdDHBTlf"] deepEqual ["iZq6AdDHBTlf"]
[task 2016-10-18T19:50:07.025794Z] 19:50:07     INFO -  PROCESS | 9169 | Perform first sync
[task 2016-10-18T19:50:07.027410Z] 19:50:07     INFO -  PROCESS | 9169 | 1476820205432	Sqlite.Connection	DEBUG	places.sqlite#1: Stmt #2 finished.
[task 2016-10-18T19:50:07.029096Z] 19:50:07     INFO -  PROCESS | 9169 | 1476820205435	Sqlite.Connection	TRACE	places.sqlite#1: Stmt #3 PRAGMA freelist_count
[task 2016-10-18T19:50:07.031193Z] 19:50:07  WARNING -  TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_bookmark_engine.js | test_change_during_sync - [test_change_during_sync : 97] Should track bookmark and folder created before first sync - ["iZq6AdDHBTlf","toolbar","yexV7fuYc8OG"] deepEqual ["yexV7fuYc8OG","iZq6AdDHBTlf","toolbar"]
[task 2016-10-18T19:50:07.033026Z] 19:50:07     INFO -  /home/worker/workspace/build/tests/xpcshell/tests/services/sync/tests/unit/test_bookmark_engine.js:test_change_during_sync:97
[task 2016-10-18T19:50:07.034532Z] 19:50:07     INFO -  resource://gre/modules/Task.jsm:TaskImpl_run:322
[task 2016-10-18T19:50:07.036123Z] 19:50:07     INFO -  resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:Handler.prototype.process:937
[task 2016-10-18T19:50:07.037713Z] 19:50:07     INFO -  resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:this.PromiseWalker.walkerLoop:816
[task 2016-10-18T19:50:07.039126Z] 19:50:07     INFO -  exiting test

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8802170 [details]
Bug 1310941 - Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes.

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

Comment 7

[Lina Butler [:lina]]

Forgot a `.sort()` in the new test.

Comment 8

[Lina Butler [:lina]]

Green-looking push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a92fcbf1bf4.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cd0ba0180c3
Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes. r=tcsc

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5cd0ba0180c3

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8802170 [details]
Bug 1310941 - Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes.

Approval Request Comment
[Feature/regressing bug #]: Bug 1299338.
[User impact if declined]: Unnecessary syncs for bookmarks changed on another device, increasing Sync server load and potential for conflicts.
[Describe test coverage new/current, TreeHerder]: Modified test in https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a92fcbf1bf4
[Risks and why]: Low risk. One-line fix with test.
[String/UUID change made/needed]: None.

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8802170 [details]
Bug 1310941 - Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes.

Fix an issue related to bookmark syncing. Take it in 51 aurora.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/66d78c1bcadc