Closed Bug 1428165 Opened 2 years ago Closed 2 years ago

History expiration's broken: records are bulk-inserted from sync with a null 'modified' value

Categories

(Firefox for Android :: Android Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox57 --- wontfix
firefox58 + fixed
firefox59 --- fixed

People

(Reporter: Grisha, Assigned: Grisha)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Sync bulk-inserts history & visits, but mistakenly omits the 'modified' column, which breaks our history expiration query that makes an assumption about presence of 'modified' values.
The expiration patch (part 2) doesn't touch expiration thresholds. Currently they're set at:
- normal (kicked off when fennec is backgrounded): don't touch records older than a month, leave no more than 2000 otherwise
- aggressive (memory pressure from OS): leave no more than 500 records

From a heavy sync user's point of view, these numbers are quite low. Expiration will delete a big chunk of your non-recent browser history, and you'll eventually have a bad time if you're relying on Sync as a backup service. From an average user's POV these numbers are probably just fine - they keep things speedy, and (hopefully!) most people do not rely on Sync as a back-up service anyway.

Another point I'd like to highlight: when looking at the system end-to-end, the situation is quite peculiar. Sync will meticulously download all of your history on the server, only for Fennec to expire (delete locally) a bunch of them in a month or so.

A user who's just setting up their Sync account, connecting to a profile with lots of history, will experience degraded performance until the history is expired in a month. We have only one easy lever here - modify expiration query further to not use the 'modified' timestamp at all, allowing history records older than a month to be deleted. In other words, delete enough of history records we just downloaded to make things fast. Using this lever implies that we will address underlying perf problems soon.

Essentially, we did half of the work: in Bug 1291821 sync was made more robust and resilient without fully considering impact on the browser as a whole. We could backtrack on the decision made in that bug and download less, negating benefits a full data set on the mobile clients provides. Alternatively, we can try to ensure browser's sufficiently fast in presence of large amounts of data - the "eat your cake" approach.
Some reviewer notes: I choose to generate timestamps on the ContentProvider side, instead of passing them along with other history data. This is consistent with how we normally do this sort of thing for timestamps - going via the regular CP interfaces will take care of the timestamps for you. From Sync's point of view, there is some subtlety here: the 'modified' timestamp in particular is used for change tracking, and so we must be careful not to kick-off any sync loops. A record is downloaded, inserted, read as modified, uploaded, etc.

This shouldn't happen in practice, with one caveat: records are inserted during the first flow, and sync bumps sessions' 'lastSynced' timestamp in compatible way - that time will be strictly after the time we inserted records. The caveat here is that we're using wall time, which of course might change under us - at that point, all bets are off. But that's what we get for using timestamps to track changes! (see bookmarks, e.g. Bug 1364644 which don't suffer from this anymore).
Comment on attachment 8940064 [details]
Bug 1428165 - Part 1: ensure that 'modified' and 'created' timestamps are set when inserting history from sync

https://reviewboard.mozilla.org/r/210352/#review216380

Two notes:

- why are we not saying that CREATED and MODIFIED are `NOT NULL` in this patch sequence

- as discussed in person, having the tests insert data that is valid at the data layer but does not reflect the domain model is not ideal.  (In this case, we insert multiple history GUIDs, all with the same URL.  Sync would normally dedupe these history items into a single entry).  I'm OK with this for this sequence, since it makes some of the subsequent testing smoother.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2830
(Diff revision 1)
>                      false
>              );
>          }
>      }
>  
>      private int bulkInsertHistory(final SQLiteDatabase db, ContentValues[] values) {

These are guaranteed to be new visits, correct?  So there's _definitely_ no existing `DATE_CREATED` that would be blown away here?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2836
(Diff revision 1)
>          int inserted = 0;
> +        // Set 'modified' and 'created' timestamps to current wall time.
> +        // 'modified' specifically is used by Sync for change tracking, and so we must ensure it's
> +        // set to our own clock (as opposed to record's modified timestamp as record by the server).
> +        final long now = System.currentTimeMillis();
>          final String fullInsertSqlStatement = "INSERT INTO " + History.TABLE_NAME + " (" +

Consider using `StringBuffer` and append to avoid what looks like might be quadratic allocations.

And isn't it perhaps better just to include `now` as a parameter?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/HistoryDataAccessor.java:36
(Diff revision 1)
>      return BrowserContractHelpers.HISTORY_CONTENT_URI;
>    }
>  
>    @Override
>    protected ContentValues getContentValues(Record record) {
> +    // NB: these values sets must agree with the BrowserProvider#bulkInsertHistory implementation.

nit: *value* sets.  Or sets of values?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/HistoryDataAccessor.java:64
(Diff revision 1)
>  
>    @Override
>    public Uri insert(Record record) {
>      HistoryRecord rec = (HistoryRecord) record;
>  
> +    // TODO this _could_ use BrowserContract.METHOD_INSERT_HISTORY_WITH_VISITS_FROM_SYNC.

Comment on why, or why not, you might want to do this: reduce duplication?  Faster?  ...

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/HistoryDataAccessor.java:107
(Diff revision 1)
>  
>    /**
>     * Insert records.
>     * <p>
> -   * This inserts all the records (using <code>ContentProvider.bulkInsert</code>),
> -   * then inserts all the visit information (also using <code>ContentProvider.bulkInsert</code>).
> +   * This inserts all the records and their visit information using a custom ContentProvider interface.
> +   * ContentProvider must handle call method 'BrowserContract.METHOD_INSERT_HISTORY_WITH_VISITS_FROM_SYNC'.

"Must *handle calling* ...", or "Must call ...".

::: mobile/android/services/src/test/java/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:42
(Diff revision 1)
>      @Before
>      @Override
>      public void setUp() throws Exception {
>          super.setUp();
>          final ShadowContentResolver cr = new ShadowContentResolver();
> +

nit: Is this extraneous?  Or trimming space?

::: mobile/android/services/src/test/java/org/mozilla/gecko/db/BrowserProviderHistoryTest.java:308
(Diff revision 1)
> +                    assertEquals(visits, (Integer) c.getInt(remoteVisitsCol));
> +                    assertEquals(visits, (Integer) c.getInt(visitsCol));
> +                }
> +
> +                // Make sure 'modified' and 'created' timestamps are present.
> +                assertFalse(c.isNull(dateCreatedCol));

nit: and reasonable... fix a convenient timestamp to compare > than.
Attachment #8940064 - Flags: review?(nalexander) → review+
Comment on attachment 8940065 [details]
Bug 1428165 - Part 2: fix history expiration query

https://reviewboard.mozilla.org/r/210354/#review216392

This avoids an explicit migration, right?  That is, the next expiration will immediately cull a lot (all?) of the history bulk inserted by Sync and not subsequently visited on the local device.  That's expensive, and maybe we want to do that at upgrade time rather than first expiration time.

Second, this pays the cost of `COALESCE` every expiration -- even after we set MODIFIED to be `NOT NULL`.  Can we track follow-up to make those fields `NOT NULL` (and perhaps to assert the URL uniqueness)?  That'll obviously need a migration for safety.
Attachment #8940065 - Flags: review?(nalexander) → review+
Blocks: 1428510
(In reply to Nick Alexander :nalexander (less responsive until Jan 3, 2018) from comment #6)
> Comment on attachment 8940065 [details]
> Bug 1428165 - Part 2: fix history expiration query
> 
> https://reviewboard.mozilla.org/r/210354/#review216392
> 
> This avoids an explicit migration, right?  That is, the next expiration will
> immediately cull a lot (all?) of the history bulk inserted by Sync and not
> subsequently visited on the local device.  That's expensive, and maybe we
> want to do that at upgrade time rather than first expiration time.

Right; we're depending on the normal expiration mechanism actually start working. Explicit migration wouldn't win us much here, unless we want to special-case what exactly is expired. The expiration itself is kicked off when Fennec is backgrounded (or, much rarer, if Fennec is under 'memory pressure'), which should be a nicer experience than doing this work on upgrade.

> Second, this pays the cost of `COALESCE` every expiration -- even after we
> set MODIFIED to be `NOT NULL`.  Can we track follow-up to make those fields
> `NOT NULL` (and perhaps to assert the URL uniqueness)?  That'll obviously
> need a migration for safety.

Sounds like a useful thing to straighten out. I've filed Bug 1428496, Bug 1428510 and Bug 1428513, all of which are possible cans of worms and/or potential foot guns if not approached carefully.
Comment on attachment 8940064 [details]
Bug 1428165 - Part 1: ensure that 'modified' and 'created' timestamps are set when inserting history from sync

https://reviewboard.mozilla.org/r/210352/#review216380

> These are guaranteed to be new visits, correct?  So there's _definitely_ no existing `DATE_CREATED` that would be blown away here?

They're thought to be new history records, yes (ambiguity in that sentence is not accidental). We're inserting here, so won't affect existing data. We also do have _some_ constraints in the DB which will trigger here, e.g. GUIDs must be unique.

> Consider using `StringBuffer` and append to avoid what looks like might be quadratic allocations.
> 
> And isn't it perhaps better just to include `now` as a parameter?

I imagine this will be optimized at the compile time due to its trivial nature (https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.18.1) - and since this isn't a hot path, I'm going to leave it as-is for clarity.
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9e432fa5a8c
Part 1: ensure that 'modified' and 'created' timestamps are set when inserting history from sync r=nalexander
https://hg.mozilla.org/integration/autoland/rev/ef89f9d60f81
Part 2: fix history expiration query r=nalexander
https://hg.mozilla.org/mozilla-central/rev/d9e432fa5a8c
https://hg.mozilla.org/mozilla-central/rev/ef89f9d60f81
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Does this fix bug 1411226 completely? Should we uplift it to Beta?
This doesn't "fix" Bug 1411226 - the underlying issues of slow queries are still there - but it buys us time, and should regain performance. This does address a regression introduced way back in 54.

We should uplift this. Very early telemetry numbers (see link in Comment 12) are looking quite promising, better than they've been in a long time.
Please request uplift ASAP then, we're building the last 58 beta (before release candidates) tomorrow.
Flags: needinfo?(gkruglov)
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1291821

[User impact if declined]: A growing user profile due to broken history expiration in presence of Sync.

[Is this code covered by automated tests?]: Yes.

[Has the fix been verified in Nightly?]: Yes.

[Needs manual test from QE? If yes, steps to reproduce]: Not necessary; if keen, inspect profile database manually after syncing to verify that all of the inserted history records have 'modified' and 'created' timestamps. Additionally, could verify that history expiration is running correctly - whenever application is background, number of history records older than a month is trimmed down to 2000.

[List of other uplifts needed for the feature/fix]: N/A.

[Is the change risky?]: No.

[Why is the change risky/not risky?]: The change does two things, both are simple from the point of view of this codebase: 1) it fixes a bug in how history records were inserted into a database during a Sync, which is a trivial change; 2) changes an expiration query to be more robust in presence of null values. The latter change will have an effect of trimming user profiles after an update, deleting a bunch (usage-dependent) of history records.
Deleting portions of user's history is non-ideal from the point of view of "Sync-as-my-browser's-memory", but it is inline with how we normally approach "expiration" of user data locally.

[String changes made/needed]: N/A.
Flags: needinfo?(gkruglov)
Attachment #8941555 - Flags: approval-mozilla-beta?
Comment on attachment 8941555 [details] [diff] [review]
history-expiration-beta.patch

Fix a broken history expiration issue related to sync. Beta58+.
Attachment #8941555 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.