Comment from bugzilla on this ugly hack:
While processing bookmarks, we sometimes need to mark them for re-upload as we're inserting new ones or updating existing ones. For example, we might set or update a dateAdded field.
We perform insertions "in-bulk", and so we might be inserting some bookmarks which need to be re-uploaded, and some bookmarks which don't. We compile an array of ContentValue objects, and make a single call to our ContentProvider. This means we can't use a URI param to indicate our intent, and so a non-column field in ContentValues objects - modifiedFromSync - is set for those bookmarks which need special treatment during insertion.
Bug 1392802 added a similar mechanism for updating bookmarks. However, updates are done differently - currently, we perform a single call to our ContentProvider for each bookmark. Which means we _can_ use a URI field as a signaling mechanism, which is what that patch did. However, in haste I forgot to take into consideration existing signaling mechanism, which lead to update failures.
And so we're left with an even clumsier interface to our data store, with two ways to signal the same thing in different circumstances... A quick solution is to just make sure 'modifiedBySync' field never makes its way to contentprovider on updates; a more refined fix would probably modify update logic to use a ContentValues field for consistency... Either way, there's going to be something ugly, somewhere in the code.
I anticipate a lot of this code changing sometime soon in order to support better transactionality of bookmark syncing, and smarter merging, and so I'm inclined to just to the simple thing for now.
MozReview-Commit-ID: H10LFsqjbFY
--HG--
extra : rebase_source : f7f311d266d75c505bb8871a567ac96d39f1b1cb
This patch moves some of the state tracking (fetchEnd/storeEnd timestamps) away from RecordsChannel
and into individual RepositorySessions. The core assumption behind this move is that
sessions are better suited to know when they were fetched from during this sync, and when they
were stored to.
Sessions are growing in complexity - local ones are wrapped in a buffer, remote
now support batching downloads and uploads. In order to hide these details, it's easier to let
sessions keep track of the fetch/store timestamps in the way that fits their implementations.
Instead of flowing these timestamps upwards from sessions and into the SynchronizerSession,
the latter now simply queries sessions at the end of their flows.
The default behavior if a certain operation wasn't performed - that is, if fetchEnd or storeEnd
aren't set during sync for a session - is to return timestamp persisted during the previous sync.
This allows us to skip certain flows (no remote data available), and ensure that we're always
using correct timestamps of the same origin for any given session.
Prior behaviour was to "make up" a timestamp at the RecordsChannel level in cases of certain
errors or skipped flows, which resulted in comparing timestamps of different origins on the consequent sync.
MozReview-Commit-ID: 2wqeTo7mhz3
--HG--
extra : rebase_source : 21b02d4164abf75422920225749ffcfd3fc71e91
We might decide that there's an older dateAdded timestamp present for an incoming bookmark while processing it,
in which case we need to ensure that our changes will be uploaded.
MozReview-Commit-ID: BKLh4rYBiRu
--HG--
extra : rebase_source : 3f8ac41de99d7082cd9d7fc7254386d99d5431bd
This is meant as a stop-gap measure to stop the obviously bad thing from happening.
MozReview-Commit-ID: Gqvc32K04xD
--HG--
extra : rebase_source : 04f1b5cb7ead6b7949b8433b3fc75c0d67283315
Versioned syncing work later in the patch series introduces functionality that is best suited to live
within the RepositorySession inheritance chain.
We'd like to introduce a new RepositorySession subclass which individual RepositorySessions are able to
inherit. And that's when the current inheritance structure gets in the way: history and bookmarks both share
a superclass, and we'd like to only introduce this new functionality for bookmarks.
This makes our task, as stated, impossible without breaking apart the current inheritance structure.
This patch introduces a few "delegate" objects:
- SessionHelper
--> HistorySessionHelper
--> BookmarksSessionHelper
... which absorb most of the functionality from AndroidBrowserRepositorySession (removed) and bookmark and history
repository sessions.
This change is not functional - everything remains as before otherwise.
MozReview-Commit-ID: 7WwUmY3Wql7
--HG--
extra : rebase_source : a8cd49fd14cdc76b9e2906d4ee8c2052b9152413
As part of moving toward versioned syncing, we need to start decoupling change tracking
concepts from parts of the system that facilitate flow of records. This allows us to track
what changed differently for different data types, while maintaining a consistent and predictable API.
A move toward that is to let repositories own determinining that a record has been modified.
Repositories are now asked to provide modified records, instead of a very specific "records modified since".
This patch does not change behaviour of the system: every repository still uses timestamp-based
change tracking to actually provide modified records to the caller. A changeover to version
tracking will come later in this series for bookmarks, and as part of Bug 1383894 for other repositories.
MozReview-Commit-ID: LQuWYdlNHpt
--HG--
extra : rebase_source : 5552d74d4a967ce85af09aaa57ca438fe5b949f3
We're moving toward version-based syncing. This is one of the bricks in that road,
removing an unused timestamp-based interface for accessing changed records.
MozReview-Commit-ID: CYUyASWXrMW
--HG--
extra : rebase_source : b95687409bc5f3e8e21fb9b084efdcd14a975d01
Error reporting, and especially the split between per-stage and global session errors,
are a bit of a mess; at the very least, this patch unifies things a little bit, and
ensures we're not passing around nulls unexpectedly.
MozReview-Commit-ID: JTSp7GuOji0
--HG--
extra : rebase_source : 19bbd2c98776b32b803d7febb55549bc430cbc3e
TLS_DHE_RSA_WITH_AES_128_CBC_SHA is no longer supported in API26+.
MozReview-Commit-ID: AtNf2xZh2Bz
--HG--
extra : rebase_source : fef7d2018e77a4a4a7594bf32de750c8fa39e2ea
Remove all references to Build.SDK_INT comparing 14 and lower
MozReview-Commit-ID: JdAjYvQ6mfX
--HG--
extra : rebase_source : f6cae8af84c26f42dcc02c133e7bc702f1af61e6
Parent class (FxAccountSyncAdapter) is essentially a singleton, and so we'd end up re-using class
fields between syncs, among them the collected telemetry data. It's cleaner and safer to move
ownership of TelemetryCollector into IntrumentedSessionCallback. With this change, telemetry
data is contained within and eventually emitted from a single owner object.
MozReview-Commit-ID: Abx13VmILcE
--HG--
extra : rebase_source : b68b44951361727015c2a10895e42f6a34806b27
While this patch does make it clearer that telemetry error handling could be factored better,
at least it gets us to a consistent usage pattern.
MozReview-Commit-ID: 4Oamt9D03Ue
--HG--
extra : rebase_source : da73247ae0a27ba6ae3d6ad0d8814c1e2249e722
The approach here is to simply mark current TelemetryCollector as having restarted.
The downside of this approach is that two technically separate syncs are combined into one
telemetry ping. However, the two syncs are logically connected to each other, and combining
their telemetry will make it easier to figure out why a restart occurred, as well as what
happened after the restart.
MozReview-Commit-ID: AtJbge2ulMz
--HG--
extra : rebase_source : 4f9efb83da8f31b2e0470df6538c67533872f23a
While this is a "named" stage, it doesn't follow the Repository<->Repository
semantics of other named stages, and so it needs to be instrumented separately.
MozReview-Commit-ID: IKrc5Fb1bYm
--HG--
extra : rebase_source : 59c83e44235101f76b42f0eced867ce7b9d5a464
SyncAdapter owns a TelemetryCollector, which is passed into GlobalSession to be "filled up"
with telemtry data.
GlobalSession obtains instances of TelemetryStageCollector from the TelemetryCollector, and
passes them into individual stages. They are filled up with telemetry as stages are executed.
Stage errors are recorded in TelemetryStageCollector.
Various global errors are recorded in TelemetryCollector itself.
On completion (success, failure, abort), telemetry is "built" and broadcasted via LocalBroadcastManager.
TelemetryContract is used to establish a key convention between the "broadcaster" and whoever is
on the receiving end of this telemetry.
This patch instruments stages which follow the Repository<->Repository flow semantics. Other stages,
such as the clients stage, meta/globa, info/* and crypto/keys are instrumented separately in follow-up
patches.
MozReview-Commit-ID: 5VLRc96GLdV
--HG--
extra : rebase_source : 4c7a7e1fde2e32d401eb28c70b9f04fdbd148ffd
This is what we (and other platforms) use as part of telemetry payloads in place of either
our local FxA Device ID or the sync client ID.
Note that this server API is currently undocumented.
Parameter introduced in 2021994ca4
MozReview-Commit-ID: 64sY5RZ2ZxK
--HG--
extra : rebase_source : d1790feae1c0f46dc5f420aeed347da12a6ac85c
We will need them later for telemetry reporting. For now we're just keeping the last exception which
we encountered (which agrees with desktop's behaviour), and Bug 1362208 explores follow-up work to
aggregate and count the exceptions as we see them.
MozReview-Commit-ID: 8yKkZVGJZ9e
--HG--
extra : rebase_source : 501ff746ecfb3022a0fe89844e307153bfdb5164
This patch:
- introduces a way to signal that a record has been reconciled; this is not a "flow control"
event type, and must be used in addition to regular "recordStored" delegate call
- draws a clearer distinction between "attempted to store" and "stored, as reported by session's storage layer"
MozReview-Commit-ID: 99UbUJzu57w
--HG--
extra : rebase_source : d7424fec748b9a2d07d1c98b78ce89fd418750e4
It's not just used for testing, and annotation is causing IDE to highlight its uses in code as invalid.
MozReview-Commit-ID: JvzX2VgNKom
--HG--
extra : rebase_source : a16933121371818307329523916d35e82b2446c9
The primary issue is that we use a throwing InputStreamReader
constructor. If it throws, then any nested streams will be lost.
We can fix that by using the non-throwing InputStreamReader
constructor (which uses a Charset as the second parameter,
instead of a String which causes an Exception to be thrown
if it can't be parsed)
We also simplify some nested Stream's a little: most of the
Stream constructors don't throw, so there's no harm in not keeping
individual references to those that don't throw - and that
results in less Stream references for us to handle.
MozReview-Commit-ID: 2hyRFGVmGnU
--HG--
extra : rebase_source : 9d2b25997e0f71089c0ef56c0069cafe068f821e
