New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 876308 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 876301



Sign in to add a comment

Write integration test to verify that Wallet USS implementation handles gc directives correctly

Project Member Reported by feuunk@google.com, Aug 21

Issue description

The server currently sends version info based GC directives in order to
clear the clients entire sync storage for that datatype.

This doesn't work in USS, because the processor only clears the metadata
when receiving a GC directive.

We want to change the clients behavior for datatypes on USS to always
clear the entire client storage when receiving a GC directive that has a
version info marker set.

This is implemented by making the client tag based processor call 
MergeSyncData instead of ApplySyncChanges on the bridge when an update
comes in that includes the version info directive.

This bug is for tracking that we have integration tests that verify this behavior for Wallet
 
Blocking: 876301
Owner: tschumann@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79

commit cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79
Author: Tim Schumann <tschumann@chromium.org>
Date: Thu Aug 23 15:30:16 2018

Production-like server behavior for Wallet data in sync integration tests.

Change the sync Wallet integration tests to simulate a more realistic server
behavior.

In particular, provide a realistic version number, GC directives, no client defined
unique tags, and non-incremental updates.

Wallet data cannot be served by the loopback server (which only supports incremental
updates) and shouldn't as that server is used for local sync. Instead, this
functionality is added to the FakeServer.

Worth noting: Adding time-based values to the progress marker (gc directive's
version_watermark) required changes in the way how integration tests verify two
clients are in an equal sync-state. Before this was done by comparing the complete
progress markers.

Verified that when enabling USS for autofill wallet data, the incorrect behavior of
dropping cards without a client tag is exercised.

Bug:  876308 
Change-Id: I9fc8929a508ce5fb62df79a1acf62c65ff98d0fe
Reviewed-on: https://chromium-review.googlesource.com/1184821
Commit-Queue: Tim Schumann <tschumann@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585492}
[modify] https://crrev.com/cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79/chrome/browser/sync/test/integration/quiesce_status_change_checker.cc
[modify] https://crrev.com/cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc
[modify] https://crrev.com/cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79/components/sync/test/fake_server/fake_server.cc
[modify] https://crrev.com/cd4f8bde2ec7c33075f1d9c46f0617feb8b47c79/components/sync/test/fake_server/fake_server.h

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0a1056527ca6c83155192d4e51c03a9671d17d49

commit 0a1056527ca6c83155192d4e51c03a9671d17d49
Author: Tim Schumann <tschumann@chromium.org>
Date: Mon Aug 27 16:23:35 2018

Add a Wallet integration test covering non-incremental wallet updates.

This verified GC directives are properly implemented.

In a subsequent CL, we should add a test that verifies empty updates are interpreted
correctly. In particular in cases where the client state should not change, e.g.
because the server decided to not fetch updates from the backend and signal a "no change"
instead.

Bug:  876308 
Change-Id: I94420353b8aa63f6ec3799d4a44a9c8c89835a97
Reviewed-on: https://chromium-review.googlesource.com/1190202
Commit-Queue: Tim Schumann <tschumann@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586278}
[modify] https://crrev.com/0a1056527ca6c83155192d4e51c03a9671d17d49/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc

Cc: tschumann@chromium.org
Owner: feuunk@chromium.org
The following test is still missing and needs to be added to fix this bug:
Unassigning myself as I'm OOO for a week.

A test that verifies empty updates are interpreted
correctly. In particular in cases where the client state should not change, e.g.
because the server decided to not fetch updates from the backend and signal a "no change"
instead.
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a

commit f21eb69ffa8b8b5909f5245c4ea49a67c3da673a
Author: Florian Uunk <feuunk@chromium.org>
Date: Tue Aug 28 15:07:26 2018

Handle version watermark gc directives as full clears.

The server currently sends version_watermark based GC directives in order to
clear the clients entire sync storage for that datatype.

This doesn't work in USS, because the processor only clears the metadata
when receiving a GC directive. This means that the data itself is left
as-is.

This CL changes the expectations on the client when receiving a
version_watermark GC directive to always clear the entire client storage.
This is implemented in USS by handling an update that includes that GC
directive as a full update that replaces any existing client data.

We do this by allowing bridges to specify whether they support incremental
updates or if they require full data updates. The processor then reports a
ModelError when receiving an incremental update for a bridge that doesn't
support incremental updates, or when receiving a full update for a bridge
that supports incremental updates.

When receiving a full update, the processor calls MergeSyncData instead of
ApplySyncChanges on the bridge.

Bug:  876308 
Change-Id: I7fc83e75eb039346866cc6ebd7cf5aa4d9546fdb
Reviewed-on: https://chromium-review.googlesource.com/1183902
Commit-Queue: Florian Uunk <feuunk@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586703}
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.cc
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.h
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge_unittest.cc
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model/fake_model_type_sync_bridge.cc
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model/fake_model_type_sync_bridge.h
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model/model_type_sync_bridge.cc
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/protocol/sync.proto
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/test/engine/mock_model_type_worker.cc
[modify] https://crrev.com/f21eb69ffa8b8b5909f5245c4ea49a67c3da673a/components/sync/test/engine/mock_model_type_worker.h

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/539d51ca61501491c455b2847890b0832fe97bd7

commit 539d51ca61501491c455b2847890b0832fe97bd7
Author: Jan Krcal <jkrcal@chromium.org>
Date: Wed Aug 29 18:28:16 2018

[USS] Do not expect GC clear all directive for initial sync

Before this CL, migration to USS for wallet data USS bridge fails
because
 - this bridge does not support incremental updates,
 - a clear-all gc directive is expected for every (non-empty) update
   for such bridges, and
 - the uss migrator does not provide the directive (as it has no clue
   about which data types expect which directives).

This CL exempts initial updates from this rule and allows them to
arrive without any gc directive.

Bug:  876308 
Change-Id: I2dd91f077ee3602d88e96c5532c3bb910b48c224
Reviewed-on: https://chromium-review.googlesource.com/1194366
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587206}
[modify] https://crrev.com/539d51ca61501491c455b2847890b0832fe97bd7/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/539d51ca61501491c455b2847890b0832fe97bd7/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/539d51ca61501491c455b2847890b0832fe97bd7/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

Status: Fixed (was: Assigned)
Owner: jkrcal@chromium.org
Status: Started (was: Fixed)
hmm... I'm not sure we have complete test coverage for empty wallet updates in the integration test (left a comment in https://chromium-review.googlesource.com/c/chromium/src/+/1193885).

Re-opening this bug and assigning to jkrcal@ to double-check.
Cc: feuunk@google.com
Thanks for raising it Tim, I have a CL to review (https://chromium-review.googlesource.com/c/chromium/src/+/1221311) so the bug is in progress :)
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/95077d65ebd9deaf8698285db56cebf3dd611633

commit 95077d65ebd9deaf8698285db56cebf3dd611633
Author: Jan Krcal <jkrcal@chromium.org>
Date: Thu Sep 20 06:44:33 2018

[AF USS] Fix the integration test for empty updates

This CL refines a previous test and the FakeServer so that it actually
tests what it is supposed to test.

The CL also does minor polish in the FakeServer code (so that I can
understand what is going on).

Double-checked by breaking the code.

Bug:  876308 
Change-Id: I0885647c42ec029db0b76f998840c7378d3496c1
Reviewed-on: https://chromium-review.googlesource.com/1221311
Reviewed-by: Tim Schumann <tschumann@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592702}
[modify] https://crrev.com/95077d65ebd9deaf8698285db56cebf3dd611633/chrome/browser/sync/test/integration/quiesce_status_change_checker.cc
[modify] https://crrev.com/95077d65ebd9deaf8698285db56cebf3dd611633/components/sync/test/fake_server/fake_server.cc
[modify] https://crrev.com/95077d65ebd9deaf8698285db56cebf3dd611633/components/sync/test/fake_server/fake_server.h

Status: Fixed (was: Started)
Hopefully fixed, finally :)

Sign in to add a comment