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

RESOLVED FIXED in Firefox 51

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: markh, Assigned: lina)

Tracking

unspecified
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox50 unaffected, firefox51 fixed, firefox52 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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 hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
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.
Attachment #8802170 - Flags: review?(tchiovoloni) → review+

Comment 3

2 years ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ae77edf8ed2
Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes. r=tcsc

Comment 4

2 years ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/ef7b401149f2
Backed out changeset 3ae77edf8ed2 for frequent intermittent xpcshell failure in test_bookmark_engine.js. r=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
Flags: needinfo?(kcambridge)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
Forgot a `.sort()` in the new test.
(Assignee)

Updated

2 years ago
Flags: needinfo?(kcambridge)

Comment 9

2 years ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5cd0ba0180c3
Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes. r=tcsc

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5cd0ba0180c3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
(Assignee)

Updated

2 years ago
Assignee: nobody → kcambridge
Priority: -- → P1
(Assignee)

Comment 11

2 years ago
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.
Attachment #8802170 - Flags: approval-mozilla-aurora?
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.
Attachment #8802170 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/66d78c1bcadc
status-firefox51: affected → fixed
You need to log in before you can comment on or make changes to this bug.