Closed Bug 1291822 Opened 8 years ago Closed 7 years ago

Bookmarks buffer validation

Categories

(Firefox for Android Graveyard :: Android Sync, defect, P1)

Product:
Firefox for Android Graveyard
Component:
Android Sync
Platform:
All
Android
Type:
defect

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: dlim, Assigned: tcsc)

References

Details

(Whiteboard: [Sync Q3 OKR])

Attachments

(2 files)

Bug 1291822 - Part 1. Implement bookmark client server validator
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1291822 - Part 2. Integrate bookmark validator into android sync flow
59 bytes, text/x-review-board-request
Grisha
: review+
Details 
      No description provided.
Blocks: 814801
Now that Bug 1291821 landed and we have in-memory buffers for all collection except for history, we could actually perform some of the validation of our buffers. It's probably fine to focus on just bookmarks validation for now, as that is where our focus is across platforms.

Desktop bookmark validator: https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/bookmark_validator.js
Relevant iOS work: https://github.com/mozilla-mobile/firefox-ios/blob/master/Storage/SQL/SQLiteBookmarksSyncing.swift#L773 and Bug 1324558.
Depends on: 1291821
Summary: Consistency checking to downloaded buffer on Android → Bookmarks buffer validation
This is a KR for this quarter.
Priority: -- → P2
Assigning to Thom, and NI myself to write up an implementation plan to make Thom's life easier.
Assignee: nobody → tchiovoloni
Flags: needinfo?(gkruglov)
The "plan" is that it seems like the simplest first stab at this is a separate sync stage. A requirement for a validation is that we need a full fetch of current server data. Generally, we don't do that in a regular sync, and so that largely rules out a regular bookmark stage - unless we want to add one-off validation code paths into it, which I'd rather we didn't.

This is intended to be a non-frequent event on a small subset of our population, and as such we can probably afford to simply re-download a full data set whenever we want to run validation. A stage which always fetches from 0, where local repository is a BufferingMiddleware wrapping a "validating repository session" seems like an Android Sync™ way to do this.

Implementation should also keep the "sync deadline" in mind, and take a conservative approach there.

Whenever we switch our BufferingStorage for the bookmarks stage to be persistent (a mirroring database table), we can re-assess this approach.
Flags: needinfo?(gkruglov)
Comment on attachment 8875471 [details]
Bug 1291822 - Part 1. Implement bookmark client server validator

https://reviewboard.mozilla.org/r/146908/#review150988

::: mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/sync/TestBookmarkValidator.java:16
(Diff revision 1)
> +
> +import java.util.ArrayList;
> +import java.util.Arrays;
> +import java.util.List;
> +
> +public class TestBookmarkValidator extends AndroidSyncTestCase {

Everybody (especially you!) will be much happier if these are JUnit 4 tests, sibling to https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/telemetry/TelemetryCollectorTest.java say.

JUnit 3 tests require a device and are generally obnoxious.  Since you don't actually need an Android device for your tests (you're fabricating your data anyway) you should be able to use JUnit 4 and run on your machine.

Bonus: JUnit 4 tests run in automation (the `android-test` job).
(In reply to :Grisha Kruglov from comment #5)
> A requirement for a validation is that we need a full
> fetch of current server data.

I'm not sure I follow.

There are two different kinds of validation:

1. To compute statistics so Mozilla can determine how well the system is working.
2. To determine if a merge of incrementally downloaded data is a good idea.

The latter, by definition, works with only changes since last time. This bug is to implement the latter -- it blocks Bug 814801.

You seem to be talking about the former: download a bunch of server records and find out if they're consistent.

If you're implementing an Android version of a server validator, perhaps it should be in a different bug? Even so, are you sure it's worth doing? It would seem to only be worthwhile if there's a non-trivial number of users who have Android devices but no desktops, no?
Flags: needinfo?(gkruglov)
(In reply to Richard Newman [:rnewman] from comment #8)
> (In reply to :Grisha Kruglov from comment #5)
> > A requirement for a validation is that we need a full
> > fetch of current server data.
> 
> I'm not sure I follow.
> 
> There are two different kinds of validation:
> 
> 1. To compute statistics so Mozilla can determine how well the system is
> working.
> 2. To determine if a merge of incrementally downloaded data is a good idea.
> 
> The latter, by definition, works with only changes since last time. This bug
> is to implement the latter -- it blocks Bug 814801.
> 
> You seem to be talking about the former: download a bunch of server records
> and find out if they're consistent.
> 
> If you're implementing an Android version of a server validator, perhaps it
> should be in a different bug? Even so, are you sure it's worth doing? It
> would seem to only be worthwhile if there's a non-trivial number of users
> who have Android devices but no desktops, no?

The two things did get morphed into one bug somehow. At this point with WIP patches in place, it's probably better to split (2) into a separate bug, since this is now actually tracking the client/server validator work.

I think there's a lot of value in being able to validate the state of android bookmarks as they are present on the clients. It's almost certain that the clients have diverged in their representation of the data from what's on the server, and being able to track this over time should help a lot in any work that aims to improve reconciliation, etc. Do you agree?

I agree that desktop is certainly a better vehicle for running pure server validation, but this does go beyond that.
Flags: needinfo?(gkruglov)
Worth noting that the code can be changed (without much effort at all) in order to do things other than client/server.

In fact, if you ignore the names, any (complete) collection of BookmarkRecords can be compared to another collection, or consistency checked against itself. Although it's possible that there are different validations we'd want to run for the buffer case, and it would require coming up with a complete "view" of the buffer, which... seems at least like it *should* be possible to me, or at least, necessary (I don't see how we could reasonably implement validation on incremental changes without having the rest available).
Comment on attachment 8887218 [details]
Bug 1291822 - Part 2. Integrate bookmark validator into android sync flow

https://reviewboard.mozilla.org/r/157992/#review163104

This isn't complete (although it both validates, and reports validation results via telemetry), but I think I'll need help for the last couple parts, and I'm getting it up in the mean time.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetrySyncPingBuilder.java:31
(Diff revision 1)
>   * Whenever hashing of data is involved, we expect it to be performed at the time of collection,
>   * somewhere in {@link org.mozilla.gecko.sync.telemetry.TelemetryCollector} and friends.
>   */
>  public class TelemetrySyncPingBuilder extends TelemetryLocalPingBuilder {
>      public TelemetrySyncPingBuilder setStages(@NonNull final Serializable data) {
>          HashMap<String, TelemetryStageCollector> stages = castSyncData(data);

It seems pointless to include a telemetry stage for bookmarkValidation in the results when we are including the `validation` property of bookmarks... I'd like to address this as well, but it doesn't seem to be as important as the other issue.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:103
(Diff revision 1)
> +    protected boolean isEnabled() throws MetaGlobalException {
> +        if (session == null || session.getContext() == null) {
> +            return false;
> +        }
> +
> +        // TODO: Need to check that

This will need to be addressed before this can land.
Comment on attachment 8887218 [details]
Bug 1291822 - Part 2. Integrate bookmark validator into android sync flow

https://reviewboard.mozilla.org/r/157992/#review166350

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:85
(Diff revision 1)
> +
> +    @Override
> +    protected Repository getLocalRepository() {
> +        TelemetryStageCollector bookmarkCollector =
> +                this.telemetryStageCollector.getSyncCollector().collectorFor("bookmarks");
> +        return new BufferingMiddlewareRepository(

You don't need a buffer here. Your validator session already acts as a memory buffer, and your already run your validation only once everything has been fetched.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:104
(Diff revision 1)
> +        if (session == null || session.getContext() == null) {
> +            return false;
> +        }
> +
> +        // TODO: Need to check that
> +        // - Bookmark sync is on (and happened?)

Very similar to AndroidBrowserRecentHistoryServerSyncStage's isEnabled

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:105
(Diff revision 1)
> +            return false;
> +        }
> +
> +        // TODO: Need to check that
> +        // - Bookmark sync is on (and happened?)
> +        // - We have enough time left this sync to do a validation

`session.getSyncDeadline()` will give you a deadline, but you'd need to figure out a good estimate for how long you guesstimate this may take (talk to network, process data, run validation...).

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:106
(Diff revision 1)
> +        }
> +
> +        // TODO: Need to check that
> +        // - Bookmark sync is on (and happened?)
> +        // - We have enough time left this sync to do a validation
> +        // - We are running nightly.

AppConstants.NIGHTLY_BUILD

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:107
(Diff revision 1)
> +
> +        // TODO: Need to check that
> +        // - Bookmark sync is on (and happened?)
> +        // - We have enough time left this sync to do a validation
> +        // - We are running nightly.
> +        // - It's been over a day since the last validation.

Similarly to how you're checking if bookmark stage completed, you can check if when _this_ stage completed (via SyncConfiguration.localBundle...)

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:108
(Diff revision 1)
> +        // TODO: Need to check that
> +        // - Bookmark sync is on (and happened?)
> +        // - We have enough time left this sync to do a validation
> +        // - We are running nightly.
> +        // - It's been over a day since the last validation.
> +        // - We have fewer than 1000 records.

You can't know for sure how many records you're going to see unless you either fetch and count them, or interrogate `info/collection_counts` endpoint. Back in the day we used to have a SafeConstrainedServerRepositorySession, which would do exactly that on first sync, and refuse to sync bookmarks if there were more than... 3000, IIRC. See https://hg.mozilla.org/mozilla-central/rev/386f67f3f987#l12.40

And so you might choose to follow that route, but that seems unnecessary.

It's probably safe to assume that if you synced bookmarks recently, how many you have locally will be representative of how many you have on the server. It's possible that the two will vary greatly if another device uploaded a bunch of bookmarks since current device synced last time... You'd have to judge if the risk of running validation on a potentially much larger amount of bookmarks is worth the complexity of adding another network round-trip. I don't think it is.

Looking at a local count should be easy enough. Something like this should work:

```
final Cursor cursor = context.getContentResolver().query(
  BrowserContractHelpers.BOOKMARKS_CONTENT_URI,
  new String[] {Bookmarks._ID},
  null, null, null
);

try {
  count = cursor.getCount();
} finally {
  cursor.close();
}
```

... and you have access to `context` in the GlobalSession.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:109
(Diff revision 1)
> +        // - Bookmark sync is on (and happened?)
> +        // - We have enough time left this sync to do a validation
> +        // - We are running nightly.
> +        // - It's been over a day since the last validation.
> +        // - We have fewer than 1000 records.
> +        // - We're definitely going to be sending the sync ping.

Check out https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java#185

But I fear that due to how we strucutured packages, you won't actually be able to access what you'd need from here.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:110
(Diff revision 1)
> +        // - We have enough time left this sync to do a validation
> +        // - We are running nightly.
> +        // - It's been over a day since the last validation.
> +        // - We have fewer than 1000 records.
> +        // - We're definitely going to be sending the sync ping.
> +        // - this isn't one of the 9/10 times we randomly are going to skip validation

Why?
Comment on attachment 8887218 [details]
Bug 1291822 - Part 2. Integrate bookmark validator into android sync flow

https://reviewboard.mozilla.org/r/157992/#review168478

::: mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetrySyncPingBuilder.java:88
(Diff revision 1)
>              if (stage.error != null) {
>                  stageJSON.put("failureReason", stage.error);
>              }
> +            // As above for validation too.
> +            if (stage.validation != null) {
> +                stageJSON.put("validation", stage.validation);

I think this is covered by tests; you'll need to augment them to see that the validation object ends up making into a final payload.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/ConfigurableServer15Repository.java:30
(Diff revision 1)
>  public class ConfigurableServer15Repository extends Server15Repository {
>    private final String sortOrder;
>    private final long batchLimit;
>    private final ServerSyncStage.MultipleBatches multipleBatches;
>    private final ServerSyncStage.HighWaterMark highWaterMark;
> -
> +  final boolean forceFullFetch;

private?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/Server15RepositorySession.java:123
(Diff revision 1)
>     * repository is set to fetch oldest-first, and it's greater than collection's last-synced timestamp.
>     * Otherwise, returns repository's last-synced timestamp.
>     */
>    @Override
>    public long getLastSyncTimestamp() {
> +    if (serverRepository.getFullFetchForced()) {

After 1364644 this should be moved to `fetchModified`, and either call into `fetchAll` if the flag is set.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java:331
(Diff revision 1)
>     * @return
>     *        True if the resulting array is "clean" (i.e., reflects the content of the database).
>     * @throws NullCursorException
>     */
>    @SuppressWarnings("unchecked")
> -  private boolean getChildrenArray(long folderID, boolean persist, JSONArray childArray) throws NullCursorException {
> +  static boolean getChildrenArray(AndroidBrowserBookmarksDataAccessor dataAccessor,

I don't think this change is necessary anymore?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksValidationRepository.java:37
(Diff revision 1)
> +
> +import java.util.ArrayList;
> +import java.util.concurrent.ConcurrentLinkedQueue;
> +import java.util.concurrent.ExecutorService;
> +
> +public class BookmarksValidationRepository extends Repository {

It's worth describing in a class comment here what we're aiming to cover with validation, and what we're explicitly not covering.

We're concerned with the client's view of the world, and how client propagates that view outwards. That is why we're wrapping a regular bookmarks session here, and capturing any side-effects that its internal 'fetch' methods will have.

We're not concerned (yet!) with how client's view of the world is shaped - that is, we're not capturing record reconciliation here directly. It is, in a way, captured in a series of validations running on the client, but that might be too opaque to be useful.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksValidationRepository.java:55
(Diff revision 1)
> +        delegate.onSessionCreated(new BookmarksValidationRepositorySession(this, context));
> +    }
> +
> +    public class BookmarksValidationRepositorySession extends RepositorySession {
> +
> +        ConcurrentLinkedQueue<BookmarkRecord> local = new ConcurrentLinkedQueue<>();

final all that you can.
minimal access possible (can you private these?)

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksValidationRepository.java:58
(Diff revision 1)
> +    public class BookmarksValidationRepositorySession extends RepositorySession {
> +
> +        ConcurrentLinkedQueue<BookmarkRecord> local = new ConcurrentLinkedQueue<>();
> +        ConcurrentLinkedQueue<BookmarkRecord> remote = new ConcurrentLinkedQueue<>();
> +        long startTime;
> +        ExtendedJSONObject validationResults = null;

Unused, right?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksValidationRepository.java:63
(Diff revision 1)
> +        ExtendedJSONObject validationResults = null;
> +        AndroidBrowserBookmarksRepositorySession wrappedSession;
> +
> +        public BookmarksValidationRepositorySession(Repository r, Context context) {
> +            super(r);
> +            startTime = SystemClock.elapsedRealtime();

I guess it's intentional that validation timing includes the fetching of bookmarks as well?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksValidationRepository.java:114
(Diff revision 1)
> +            Logger.info(LOG_TAG, "Completed validation in " + (SystemClock.elapsedRealtime() - startTime) + " ms");
> +            parentCollector.validation = o;
> +        }
> +
> +        @Override
> +        public void fetchSince(long timestamp, final RepositorySessionFetchRecordsDelegate delegate) {

After 1364644 this becomes fetchModified, and it should call into 'wrappedSession.fetchAll' instead.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksValidationRepository.java:117
(Diff revision 1)
> +
> +        @Override
> +        public void fetchSince(long timestamp, final RepositorySessionFetchRecordsDelegate delegate) {
> +            wrappedSession.fetchSince(0, new RepositorySessionFetchRecordsDelegate() {
> +                @Override
> +                public void onFetchFailed(Exception ex) {}

You probably want to clean up your local state here.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:45
(Diff revision 1)
> +    public Integer getStorageVersion() {
> +        return VersionConstants.BOOKMARKS_ENGINE_VERSION;
> +    }
> +
> +    /**
> +     * We can't validate without all of the records, so a HWM doesn't amke sense.

"make"
Comment on attachment 8887218 [details]
Bug 1291822 - Part 2. Integrate bookmark validator into android sync flow

https://reviewboard.mozilla.org/r/157992/#review168498
Attachment #8887218 - Flags: review?(gkruglov)
Priority: P2 → P1
Comment on attachment 8875471 [details]
Bug 1291822 - Part 1. Implement bookmark client server validator

https://reviewboard.mozilla.org/r/146908/#review168520

Validator looks good to me!

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/validation/BookmarkValidator.java:43
(Diff revision 2)
> +        }
> +        HashSet<String> seenChildGUIDs = new HashSet<>();
> +
> +        for (int i = 0; i < r.children.size(); ++i) {
> +            // Not clear if we should catch the possible ClassCastException, desktop doesn't bother
> +            // checking that these are strings, so we don't either (but maybe we should?).

If they're not strings, this will just blow up. We would have seen crashes around this if that was the case from other sync code. I wouldn't bother.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/validation/BookmarkValidator.java:115
(Diff revision 2)
> +            results.parentNotFolder.add(
> +                    new BookmarkValidationResults.ParentChildPair(parentID, record.guid)
> +            );
> +        }
> +
> +        if (!listedParent.guid.equals(parentID)) {

As written, it doesn't seem like this will ever actually happen though? If anything, this will indicate that your guid>record map got corrupted somehow, but not an actual data inconsistency.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/validation/BookmarkValidator.java:147
(Diff revision 2)
> +            this.client = client;
> +            this.server = server;
> +        }
> +    }
> +
> +    // Should return false is one or both are missing (and so further comparisons are meaningless.

"...if one..."

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/validation/BookmarkValidator.java:154
(Diff revision 2)
> +    private boolean checkMissing(ClientServerPair p, BookmarkValidationResults results) {
> +
> +        boolean clientExists = p.client != null && !p.client.deleted;
> +        boolean serverExists = p.server != null && !p.server.deleted;
> +
> +        boolean serverTombstone = p.server != null && p.server.deleted;

Do we ever validate that tombstones don't contain any other record info? e.g. only guid&isDeleted are present?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/validation/BookmarkValidator.java:160
(Diff revision 2)
> +
> +        if (clientExists && !serverExists) {
> +            if (serverTombstone) {
> +                results.serverDeleted.add(p.client.guid);
> +            } else {
> +                results.serverMissing.add(p.client.guid);

It's possible that a local record was added during a sync, after/during a bookmark sync and before the validation runs, and wasn't uploaded to the server. Hopefully we'll upload it during next sync.

Post 1364644 this should probably only complain if localVersion=syncVersion for the record, indicating that client won't actually upload it on next sync.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/validation/BookmarkValidator.java:185
(Diff revision 2)
> +        if (p.client.children != null && p.server.children != null) {
> +            if (p.client.children.size() == p.server.children.size()) {
> +                for (int i = 0; i < p.client.children.size(); ++i) {
> +                    String clientChildGUID = (String) p.client.children.get(i);
> +                    String serverChildGUID = (String) p.server.children.get(i);
> +                    if (!clientChildGUID.equals(serverChildGUID)) {

So I bet this fails quite a bit due to re-ordering issues?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/validation/BookmarkValidator.java:231
(Diff revision 2)
> +                pairsById.put(r.guid, new ClientServerPair(normalizeRecord(r), null));
> +            }
> +        }
> +        for (Entry<String, ClientServerPair> e : pairsById.entrySet()) {
> +            ClientServerPair p = e.getValue();
> +            if (!checkMissing(p, results)) {

Android intentionally doesn't store anything that's neither a folder nor a bookmark. In the other patch though, I do think we'll add those types of records to the list of remotes, and will mark them as missing here.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/validation/BookmarkValidator.java:239
(Diff revision 2)
> +
> +            // Skip checking for differences if we see a structural difference, since congruentWith
> +            // checks structural differences as well.
> +            boolean sawStructuralDifference = checkStructuralDifferences(p, results);
> +
> +            if (!sawStructuralDifference && !p.client.congruentWith(p.server)) {

I hope our implementation of congruentWith is commutative! I suppose you could check for that explicitly with `p.server.congruentWith(p.client)`.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/validation/BookmarkValidator.java:257
(Diff revision 2)
> +            }
> +        }
> +        results.multipleParents = filteredMultipleParents;
> +    }
> +
> +    public BookmarkValidationResults validate() {

private, right?
Attachment #8875471 - Flags: review?(gkruglov) → review+
Whiteboard: [Sync Q3 OKR]
Comment on attachment 8875471 [details]
Bug 1291822 - Part 1. Implement bookmark client server validator

https://reviewboard.mozilla.org/r/146908/#review168520

> As written, it doesn't seem like this will ever actually happen though? If anything, this will indicate that your guid>record map got corrupted somehow, but not an actual data inconsistency.

Yes, this should be a check that the child's guid exists in the parent.

> Do we ever validate that tombstones don't contain any other record info? e.g. only guid&isDeleted are present?

No, although we could (Not sure how much value it would provide though -- iOS doesn't care, and I think all clients would ignore the extra data)

> It's possible that a local record was added during a sync, after/during a bookmark sync and before the validation runs, and wasn't uploaded to the server. Hopefully we'll upload it during next sync.
> 
> Post 1364644 this should probably only complain if localVersion=syncVersion for the record, indicating that client won't actually upload it on next sync.

I think we should just add a case when deciding whether or not to run the validator for this, like we have for desktop.

It's also a very narrow time window, and so is probably statistically negligable.

> So I bet this fails quite a bit due to re-ordering issues?

It's fairly common, yeah.

> Android intentionally doesn't store anything that's neither a folder nor a bookmark. In the other patch though, I do think we'll add those types of records to the list of remotes, and will mark them as missing here.

So you think we should only report the error if the server record is a bookmark or folder?

> I hope our implementation of congruentWith is commutative! I suppose you could check for that explicitly with `p.server.congruentWith(p.client)`.

I've read through it and it seems to be more or less what we want. Do you have a reason to think it wouldn't be eventually? I've added the check for `server.congruentWith(client)` regardless.
Comment on attachment 8887218 [details]
Bug 1291822 - Part 2. Integrate bookmark validator into android sync flow

https://reviewboard.mozilla.org/r/157992/#review172518

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksValidationRepository.java:64
(Diff revision 1)
> +        AndroidBrowserBookmarksRepositorySession wrappedSession;
> +
> +        public BookmarksValidationRepositorySession(Repository r, Context context) {
> +            super(r);
> +            startTime = SystemClock.elapsedRealtime();
> +            wrappedSession = new AndroidBrowserBookmarksRepositorySession(r, context);

You'll need to override this session's begin method, and make sure wrappedSession.begin runs - it needs to do a bunch of "setup" work. Specifically, it'll initialize `parentIDToGuidMap` and `parentGuidToIDMap`. The former is specifically used while fetching records.
Comment on attachment 8887218 [details]
Bug 1291822 - Part 2. Integrate bookmark validator into android sync flow

https://reviewboard.mozilla.org/r/157992/#review166350

> Very similar to AndroidBrowserRecentHistoryServerSyncStage's isEnabled

The reason is partly because we need somewhere to include the validation results in telemetry, and partly because we don't want to run validation if the user doesn't even sync bookmarks.

So I think maybe a decent solution will be to look and see if there's a TelemetryStageCollector for the bookmarks collection (will require a new method like hasCollectorFor on TelemetryCollector)? This also allows us to check if there was an error.

So this is what I've done.

> `session.getSyncDeadline()` will give you a deadline, but you'd need to figure out a good estimate for how long you guesstimate this may take (talk to network, process data, run validation...).

Using 2 minutes since that should be far more than enough time.

> Similarly to how you're checking if bookmark stage completed, you can check if when _this_ stage completed (via SyncConfiguration.localBundle...)

Since this isn't updated if we return false, and we return false conditionally based on a random variable, it seems better to mimic what desktop does and use a separater preference for storing this timestamp. Let me know if this is a problem.

> Check out https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java#185
> 
> But I fear that due to how we strucutured packages, you won't actually be able to access what you'd need from here.

I think it's probably better to address this in a follow up then. Leaving this issue open until I get an updated patch up, and open a followup for it.

> Why?

Because validation is expensive and of dubious value, so we don't want to run it that frequently.
Comment on attachment 8887218 [details]
Bug 1291822 - Part 2. Integrate bookmark validator into android sync flow

https://reviewboard.mozilla.org/r/157992/#review168478

> I think this is covered by tests; you'll need to augment them to see that the validation object ends up making into a final payload.

It doesn't seem to be covered by tests, but I've added them to cover building "engines" from "stages". I opened bug 1389233 for an issue i found with `outgoing`, which I haven't bothered covering in the test.

> I guess it's intentional that validation timing includes the fetching of bookmarks as well?

Yes.

> After 1364644 this becomes fetchModified, and it should call into 'wrappedSession.fetchAll' instead.

If you want me to wait until that lands, let me know, and I can try rebasing.

> You probably want to clean up your local state here.

I added a call to the delegates version, and cleared the arrays. It's unclear if this is what you meant, so let me know if you meant something else.
Comment on attachment 8875471 [details]
Bug 1291822 - Part 1. Implement bookmark client server validator

https://reviewboard.mozilla.org/r/146908/#review168520

> I think we should just add a case when deciding whether or not to run the validator for this, like we have for desktop.
> 
> It's also a very narrow time window, and so is probably statistically negligable.

Full history sync stage runs between bookmarks and bookmark validation stages, and so on the off chance and with the right star alignment, the time window might be pretty big actually. You're likely correct that all things considered, it's pretty negligable.

> So you think we should only report the error if the server record is a bookmark or folder?

At the cost of somewhat diluting our errors, we should probably treat this as a legitimate client validation error, actually fix android's behaviour here which seems rather incorrect/incomplete, and observe this metric drop.
Comment on attachment 8887218 [details]
Bug 1291822 - Part 2. Integrate bookmark validator into android sync flow

https://reviewboard.mozilla.org/r/157992/#review173668

LGTM, but I don't think you're checking if bookmark sync is enabled correctly/ran, which will need to be addressed before this lands.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksValidationRepository.java:53
(Diff revisions 1 - 2)
> + *
> + * We're not concerned with how client's view of the world is shaped - that is, we're
> + * not capturing record reconciliation here directly. These sorts of effects will
> + * (hopefully) be determined from validation results in aggregate.
> + *
> + * @see BookmarkValidationResults for the concrete set of problams it checks for.

problams

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:137
(Diff revisions 1 - 2)
>          return new BookmarkRecordFactory();
>      }
>  
> +    private final long getLocalBookmarkRecordCount() {
> +        final Context context = session.getContext();
> +        final Cursor cursor = context.getContentResolver().query(

iirc this may return a null cursor :(

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:161
(Diff revisions 1 - 2)
> +            return false;
> +        }
> +
> +        session.config.persistLastValidationCheckTimestamp(now);
> +
> +        if (!AppConstants.NIGHTLY_BUILD) {

This should probably be your first check.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ValidateBookmarksSyncStage.java:177
(Diff revisions 1 - 2)
> +            return false;
> +        }
> +
> +        // See if we'll have somewhere to store the validation results
> +        TelemetryCollector syncCollector = telemetryStageCollector.getSyncCollector();
> +        if (!syncCollector.hasCollectorFor("bookmarks")) {

I think we'll always have a bookmark collector, even if bookmark sync is disabled remotely, locally, or for this sync. We check if stage is enabled after creating the collector...

Perhaps mimicking ServerSyncStage's isEnabled behaviour might be a better approach. See https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ServerSyncStage.java#86

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/SyncConfiguration.java:99
(Diff revision 2)
>    public static final long CURRENT_PREFS_VERSION = 1;
>  
>    public static final String CLIENTS_COLLECTION_TIMESTAMP = "serverClientsTimestamp";  // When the collection was touched.
>    public static final String CLIENT_RECORD_TIMESTAMP = "serverClientRecordTimestamp";  // When our record was touched.
>    public static final String MIGRATION_SENTINEL_CHECK_TIMESTAMP = "migrationSentinelCheckTimestamp";  // When we last looked in meta/fxa_credentials.
> +  public static final String VALIDATION_CHECK_TIMESTAMP = "validationCheckTimestamp"; // Last time we considered performing validation

BOOKMARK_VALIDATION_CHECK_TIMESTAMP
Attachment #8887218 - Flags: review?(gkruglov) → review+
Comment on attachment 8887218 [details]
Bug 1291822 - Part 2. Integrate bookmark validator into android sync flow

https://reviewboard.mozilla.org/r/157992/#review173668

> I think we'll always have a bookmark collector, even if bookmark sync is disabled remotely, locally, or for this sync. We check if stage is enabled after creating the collector...
> 
> Perhaps mimicking ServerSyncStage's isEnabled behaviour might be a better approach. See https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/ServerSyncStage.java#86

I think we do this, since we call that function as super.isEnabled(), which uses getEngineName(), which for us returns "bookmarks".
Comment on attachment 8875471 [details]
Bug 1291822 - Part 1. Implement bookmark client server validator

https://reviewboard.mozilla.org/r/146908/#review168520

> Full history sync stage runs between bookmarks and bookmark validation stages, and so on the off chance and with the right star alignment, the time window might be pretty big actually. You're likely correct that all things considered, it's pretty negligable.

I've made it so validation runs immediately after bookmarks.
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f992db1f216
Part 1. Implement bookmark client server validator r=Grisha
https://hg.mozilla.org/integration/autoland/rev/20572708f3d6
Part 2. Integrate bookmark validator into android sync flow r=Grisha
Product: Android Background Services → Firefox for Android
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: