802808 - Tabs engine should explicitly skip its own record

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

12:24:51 <@rnewman> ally: tabs engine doesn't provide its own implementation of _reconcile
12:25:29 <@rnewman> or _processIncoming, for that matter
12:26:28 <@rnewman> so I'm guessing this happens:
12:26:35 <@rnewman> * We reset the tabs client on every sync
12:26:48 <@rnewman> * It pulls down its own record from last time, which has a non-zero timestamp
12:26:59 <@rnewman> * The usual processIncoming/reconcile runs, reporting that both have changed
12:27:22 <@rnewman> * It calls tabstore.update, which prints "Ignoring tab updates as local ones win".
12:27:45 <@rnewman> really, _processIncoming for tabs should skip its own record entirely.

Comment 1

[kunal bansal]

i want to work for bugzilla but i am new to development so can i start my work with this one?

Comment 2

[Stephen Gowan]

Attachment: tabs-skip-own-record.patch

The TabEngine will now skip its own record when it reconciles incoming records.

All services/ xpcshell-tests are passing on my machine, and syncs are working with my build.  I see in the output that the incoming record is skipped and the local one is still uploaded.

1416080759586	Sync.Engine.Tabs	TRACE	Event: weave:engine:sync:start
1416080759589	Sync.Engine.Tabs	DEBUG	First sync, uploading all items
1416080759590	Sync.Engine.Tabs	INFO	1 outgoing items pre-reconciliation
1416080759591	Sync.Engine.Tabs	TRACE	Downloading & applying server changes
1416080759602	Sync.BrowserIDManager	DEBUG	_ensureValidToken already has one
1416080759846	Sync.Engine.Tabs	TRACE	Incoming: { id: OokdTriLUDDw  index: 0  modified: 1416032990.13  ttl: 604800  payload: {"id":"OokdTriLUDDw","clientName":"gowansg's Firefox Developer Edition on Stephens-MacBook-Pro", ...*removed for brevity*} collection: tabs }
1416080759847	Sync.Engine.Tabs	TRACE	Reconciling OokdTriLUDDw. exists=false; modified=false; local age=null; incoming age=47769.299999952316
1416080759847	Sync.Engine.Tabs	TRACE	No duplicate found for incoming item: OokdTriLUDDw
1416080759847	Sync.Engine.Tabs	TRACE	Applying incoming because local item does not exist and was not deleted.
1416080759849	Sync.Store.Tabs	DEBUG	Adding remote tabs from gowansg's Firefox Developer Edition on Stephens-MacBook-Pro
> 1416080759873	Sync.Engine.Tabs	TRACE	Ignoring incoming tab item because its id: wM-KkFKPeQNv is the same as the local tab item.
1416080759874	Sync.Engine.Tabs	TRACE	Skipping reconciled incoming item wM-KkFKPeQNv

...

1416080760228	Sync.Engine.Tabs	TRACE	Uploading local changes to server.
1416080760228	Sync.Engine.Tabs	TRACE	Preparing 1 outgoing records
1416080760272	Sync.Store.Tabs	TRACE	Created tabs 2 of 2
1416080760273	Sync.Store.Tabs	TRACE	Wrapping tab: {"title":"Google","urlHistory":["https://www.google.com/?gws_rd=ssl"],"icon":"","lastUsed":1416080715}
1416080760273	Sync.Store.Tabs	TRACE	Wrapping tab: {"title":"Product reviews and prices, software downloads, and tech news - CNET","urlHistory":["http://www.cnet.com/"],"icon":"","lastUsed":1416080699}
1416080760274	Sync.Engine.Tabs	TRACE	Outgoing: { id: wM-KkFKPeQNv  index: 0  modified: undefined  ttl: 604800  payload: {"id":"wM-KkFKPeQNv","clientName":"gowansg's Nightly on Stephens-MacBook-Pro","tabs":[{"title":"Google","urlHistory":["https://www.google.com/?gws_rd=ssl"],"icon":"","lastUsed":1416080715},{"title":"Product reviews and prices, software downloads, and tech news - CNET","urlHistory":["http://www.cnet.com/"],"icon":"","lastUsed":1416080699}]}  collection: tabs }
> 1416080760305	Sync.Engine.Tabs	INFO	Uploading all of 1 records
1416080760309	Sync.BrowserIDManager	DEBUG	_ensureValidToken already has one
1416080760328	Sync.Collection	DEBUG	POST Length: 656
1416080760535	Sync.Collection	DEBUG	mesg: POST success 200 https://sync-121-us-west-2.sync.services.mozilla.com/1.5/8501801/storage/tabs
1416080760535	Sync.Collection	DEBUG	POST success 200 https://sync-121-us-west-2.sync.services.mozilla.com/1.5/8501801/storage/tabs
1416080760540	Sync.Engine.Tabs	TRACE	Finishing up sync
1416080760541	Sync.Engine.Tabs	TRACE	Event: weave:engine:sync:finish

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8523452 [details] [diff] [review]
tabs-skip-own-record.patch

Review of attachment 8523452 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for tackling this!

::: services/sync/modules/engines/tabs.js
@@ +86,5 @@
>      return urls;
> +  },
> +
> +  _reconcile: function (item) {
> +    // Tab engine should explicitly skip its own record

It's not obvious to the reader that TabStore.itemExists returns true only for a record with our client ID, so:

  // Skip our own record.
  // TabStore.itemExists tests only against our local client ID.

@@ +89,5 @@
> +  _reconcile: function (item) {
> +    // Tab engine should explicitly skip its own record
> +    if (this._store.itemExists(item.id)) {
> +      this._log.trace("Ignoring incoming tab item because its id: " + item.id +
> +                      " is the same as the local tab item.");

No need for this line.

::: services/sync/tests/unit/test_tab_engine.js
@@ +47,5 @@
> +  let messages = [];
> +  engine._log.trace = function (message) {
> +    let regEx = "Ignoring incoming tab item because its id: .* is the same as the local tab item.";
> +    if (message.match(regEx)) {
> +      messages.push(message);

Rather than this logging-based approach, I suggest:

1. A unit test: get a TabStore and see what _reconcile returns for various records.

2. An integration test: mock applyIncoming for TabStore, and ensure that it's never called with the record that we're trying to filter out.

Comment 4

[Stephen Gowan]

Attachment: tabs-skip-own-record.patch

Thanks for the feedback Richard.  This patch includes a much better testing effort.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8525512 [details] [diff] [review]
tabs-skip-own-record.patch

Review of attachment 8525512 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not crazy keen on mixing testing approaches in the same file (do_check_eq, notEqual), but hey, if it passes, great.

::: services/sync/tests/unit/test_tab_engine.js
@@ +47,5 @@
> +  _("Ensure incoming records that match local client ID are never applied.");
> +  let [engine, store] = getMocks(),
> +      localID = engine.service.clientsEngine.localID,
> +      apply = store.applyIncoming,
> +      applied = [];

Prefer separate `let` statements, one per line.

Comment 6

[Stephen Gowan]

Attachment: tabs-skip-own-record.patch

Sorry for taking so long to get this done.  

I went ahead and replaced the deprecated assertion methods in my patch with supported ones.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8547933 [details] [diff] [review]
tabs-skip-own-record.patch

Review of attachment 8547933 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Comment 8

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/05f68fbdbcd9

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05f68fbdbcd9