Closed Bug 1102924 Opened 10 years ago Closed 3 years ago

Sync search history into search history provider

Categories

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

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

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: rnewman, Unassigned)

References

Details

Attachments

(4 files)

MozReview Request: Bug 1102924 - Sync search history into search history provider
40 bytes, text/x-review-board-request
sergej
: review?
rnewman
Details
MozReview Request: Bug 1102924 - Sync search history into search history provider
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1102924 - Follow-up: fix tests.
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1102924 - Follow-up: review comments.
40 bytes, text/x-review-board-request
Details 
We already sync form history. We have a search-history content provider.

Downstream changes should be applied to one of the two destinations according to whether the incoming record is search-history.

The same filter should be applied to records retrieved from the form-history database for upload: we shouldn't be storing searches into form-history in Fennec, because we want to use them in the search activity, and we don't want collisions.

Then we'll also upload from search-history into server-side form-history.

(If we are storing searches into form-history from various places inside Fennec itself (long-tap search?), then we should fix that.)

This will require a version bump/migration pref somewhere such that we re-sync, or we need to migrate searches out of form-history and into the search-history provider.

We'll need to ensure that search-history entries have GUIDs et al.

This probably isn't straightforward for anyone but me and Nick, alas -- lots of moving parts that need to be just right -- but I'm happy to mentor anyone who wants to take on the challenge.
See Also: → 1095138
Blocks: 1189719
Blocks: 1190967
No longer blocks: 1189719
No longer blocks: 1190967
Depends on: 1190967
Blocks: 1189719
Blocks: 1190967
No longer depends on: 1190967
No longer blocks: 1189719
Ally, is this something we need to do for our v1?
Flags: needinfo?(ally)
I don't think this needs to block shipping search history terms in the UI, but it would be great to ship this at the same time (or follow up with this in the next release).
Barbara, no I don't think so, given that on phones we get at _most_ two saved search suggestions. I think the value is proportional with the number of saved searches shown. Tablets get a max of 4, so its more valuable there.

I think this is squarely in a nice-to-have category at best. I think the ROI on the engineering effort from this is highly questionable. 

Keep in mind rnewman's remarks in comment 0. This sounds like a complex project for anyone who isn't rnewman or nalexander.
Flags: needinfo?(ally)
+1 what Ally and Margaret, said, let's mute this for now and remove the block for v1.
No longer blocks: awesomescreen-v1
Nick or Mike, do either of you know of a contributor who's looking for a good challenge? It would be great to ship this in 44 with search history suggestions, but I don't know that we'll be able to get to it without extra help.
Flags: needinfo?(nalexander)
Flags: needinfo?(michael.l.comella)
(In reply to :Margaret Leibovic from comment #5)
> Nick or Mike, do either of you know of a contributor who's looking for a
> good challenge? It would be great to ship this in 44 with search history
> suggestions, but I don't know that we'll be able to get to it without extra
> help.

I don't.  This is a good project to offer; I can help break down the ticket.  Leaving the NI for me to do so.
Flags: needinfo?(nalexander)
One implementation strategy for this, just to note, is for both the search provider and forms provider to know about the other. They can read and write across the boundary so that Sync queries for forms produce synthetic search form records, etc
Sergej, are you up for a challenge? We're looking to land this for Firefox 44 so we'd hope for it to be done by the merge on 11/2 (a month).

(Adding Nick to the NI because I don't think he meant to cancel)
Flags: needinfo?(sergej)
Flags: needinfo?(nalexander)
Flags: needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #8)
> Sergej, are you up for a challenge? We're looking to land this for Firefox
> 44 so we'd hope for it to be done by the merge on 11/2 (a month).
> 
> (Adding Nick to the NI because I don't think he meant to cancel)

I can try. But I will need help, because I'm familiar only with several modules. Description is not very informative for me.
Flags: needinfo?(sergej) → needinfo?(michael.l.comella)
Sergej, Richard has the most information here so I'd recommend talking to him.

Richard, can you simplify comment 0?
Flags: needinfo?(michael.l.comella) → needinfo?(sergej)
(In reply to Richard Newman [:rnewman] from comment #0)
> We already sync form history. We have a search-history content provider.

Form history is synced in https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java.

Search history is stored in https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/db/SearchHistoryProvider.java

> Downstream changes should be applied to one of the two destinations
> according to whether the incoming record is search-history.

Form history fields are just (name, value) -- see http://docs.services.mozilla.com/sync/objectformats.html#forms.  Search history fields are saved as ("searchbar-history", search) -- see https://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/SearchSuggestionController.jsm#17.

> The same filter should be applied to records retrieved from the form-history
> database for upload: we shouldn't be storing searches into form-history in
> Fennec, because we want to use them in the search activity, and we don't
> want collisions.

Inbound records are processed at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java#543.  That logic will need to be made conditional -- if we have a "searchbar-history" name, then we route it into the SearchHistoryProvider.

> Then we'll also upload from search-history into server-side form-history.

Outbound records (both regular and deleted) are fetched at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java#289.  That logic will need to be doubled, to also fetch records from SearchHistoryProvider.

> (If we are storing searches into form-history from various places inside
> Fennec itself (long-tap search?), then we should fix that.)
> 
> This will require a version bump/migration pref somewhere such that we
> re-sync, or we need to migrate searches out of form-history and into the
> search-history provider.
> 
> We'll need to ensure that search-history entries have GUIDs et al.

GUIDs and lastModified timestamps.  The Search History table is created at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserDatabaseHelper.java#395; it doesn't included GUIDs or timestamps, so that will need to be added first.

> This probably isn't straightforward for anyone but me and Nick, alas -- lots
> of moving parts that need to be just right -- but I'm happy to mentor anyone
> who wants to take on the challenge.

Richard also said:

(In reply to Richard Newman [:rnewman] from comment #7)
> One implementation strategy for this, just to note, is for both the search
> provider and forms provider to know about the other. They can read and write
> across the boundary so that Sync queries for forms produce synthetic search
> form records, etc

This is a possible approach.  This would have the Search History write into it's own database and also into the Form History provider, so that every search record had a corresponding form record.  Then outbound Sync would "just work", since Form History has GUIDs and timestamps, etc.  We'd also have Form History write the "searchbar-history" entries into Search History, so that inbound Sync would "just work".

I would explore this approach first, since it's easier to test with existing Robocop tests.  We can be certain reads and writes pass through correctly; and then we can be pretty confident that the Sync part would work.
Flags: needinfo?(nalexander)
I'll start working on it
Flags: needinfo?(sergej)
Can we simply remove SearchHistoryProvider and pull/store all search suggestions from/in form-history db? Then we can filter "searchbar-history", if request is not search suggestion. All will work automatically.
Flags: needinfo?(nalexander)
(In reply to Sergej Kravcenko from comment #14)
> Can we simply remove SearchHistoryProvider and pull/store all search
> suggestions from/in form-history db? Then we can filter "searchbar-history",
> if request is not search suggestion. All will work automatically.

Perhaps.  Comparing the search history table definition in

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserDatabaseHelper.java#395

to the form history definition in

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormHistory.js#40

it does look like this is sensible.  I'd want to check the original SearchHistoryProvider commits to understand why we didn't do this at first!
Flags: needinfo?(nalexander)
I think I'd phrase it as "implement the search history provider on top of the form history database".

The search history provider is used from the search activity and the Fennec frontend, and the form history provider uses SQLiteBridge, which is a can of worms. Tread carefully; concurrency dangers await.
> it does look like this is sensible.  I'd want to check the original
> SearchHistoryProvider commits to understand why we didn't do this at first!

Probably because it was designed as local thing? Now it's used only in 2 places.

The only thing that is missing, visit count. But as I see, it's not used. The question is, can we drop it? I don't know why it was added (probably for sorting, but not used) and can't make a decision to remove it. If we need this count, then it should be somehow added to form-history, via table/sync change or store it in value field like <count>,<value> or multiple rows.

Can I remove SearchHistoryProvider and go this way? 
Can I remove visit count?
Flags: needinfo?(nalexander)
(In reply to Sergej Kravcenko from comment #17)
> > it does look like this is sensible.  I'd want to check the original
> > SearchHistoryProvider commits to understand why we didn't do this at first!
> 
> Probably because it was designed as local thing? Now it's used only in 2
> places.

You may be correct.

> The only thing that is missing, visit count. But as I see, it's not used.

After skimming the code, I agree.

> The question is, can we drop it? I don't know why it was added (probably for
> sorting, but not used) and can't make a decision to remove it.

I think we will want this, since it provides the "frequency" in "frecency" searching.  At some point, we will want to do better than just "recency".

 If we need
> this count, then it should be somehow added to form-history, via table/sync
> change or store it in value field like <count>,<value> or multiple rows.

We don't have much scope to do this, since Sync clients don't include usage counts and we can't change all Sync clients.  (Wish we could!)

> Can I remove SearchHistoryProvider and go this way? 
> Can I remove visit count?

I'd like to get feedback from margaret.  Personally, if we want this feature soon, we should remove the special SearchHistoryProvider and use FormHistoryProvider throughout.  (Possibly with massaging to make the intent clear; or possibly by routing the SearchHistoryProvider to return data from the FormHistoryProvider.)  It's worth noting that the SHP is used from the Search Activity, which doesn't assume Gecko is running; and that the FHP uses a Gecko-provided database; but I don't think there's a conflict here.  rnewman could confirm but I think I understand this interaction well enough to just say it is so.

So, margaret: how much do we care to maintain frequency for future improvements?
Flags: needinfo?(margaret.leibovic)
(In reply to Richard Newman [:rnewman] from comment #16)
> The search history provider is used from the search activity and the Fennec
> frontend, and the form history provider uses SQLiteBridge, which is a can of
> worms. Tread carefully; concurrency dangers await.

As I understand gecko and android sync are using different db connections to the same db, but sync doesn't handle SQLITE_BUSY errors. I can't find any concurrency code at all. So when gecko updates form history sync can fail? It's hard for me to find how gecko handles this from his side. Maybe you can explain briefly.
Flags: needinfo?(rnewman)
(In reply to Sergej Kravcenko from comment #19)

> As I understand gecko and android sync are using different db connections to
> the same db, but sync doesn't handle SQLITE_BUSY errors. I can't find any
> concurrency code at all. So when gecko updates form history sync can fail?
> It's hard for me to find how gecko handles this from his side. Maybe you can
> explain briefly.

We open both connections with a WAL, so concurrent use is safe if only one connection is writing.

If both connections both issue writes on their connection at the same time, then one of them will fail with a SQLITE_BUSY. 

Ordinarily this would look like a multi-thread sqlite system with one connection per thread, which is totally safe. But we have a little bit of an unusual situation: see Bug 1006947 Comment 19.

We essentially count on luck and retries to make this work.

The chance of a form history entry being written at the same time as a form history item being written by Sync is very low -- Sync doesn't take long-running transactions, syncs don't happen often, and browser form writes are rare and short. If Sync fails to write because of a SQLITE_BUSY it'll eventually try again and succeed.

The reason why I advise caution in selecting an approach: writing every search the browser performs into the form history database will dramatically increase the number of form history writes, and we'll be doing those writes from Java and at a different point in the app lifecycle.

Having Sync talk to both locations is safer in a few respects: it means that ordinary browser operation doesn't involve writing to the form history database from Java; it limits the scope of bugs to users who use Sync; and it increases the likelihood that Sync-related writes will happen when the app is not in the foreground, which eliminates the chance of write contention altogether.

A truly safe configuration here essentially means aping Bug 946857 and not having Gecko manage this database at all.
Flags: needinfo?(rnewman)
Bug 1102924 - Sync search history into search history provider
Attachment #8673417 - Flags: review?(rnewman)
Ok, this is not final path, just to share some preliminary thoughts. I've decided keep SearchHistoryProvider with separate table for now and add some routing in FormHistoryRepositorySession.

Still some moments:

1. Visit count is still in the table and counting local searches but will reset to 1 on sync. If we need this, then we must use TIMES_USED, but is not used/synced now. Is that incomplete android sync implementation or sync protocol doesn't support TIMES_USED field for form history?

2. Is "searchbar-history" field already used in live systems as search history? If so, we need to add form history table migration and move all "searchbar-history" rows into searchhistory table.

Anyway, waiting for feedback. I still think that SearchHistoryProvider removal is the right way, but gecko db access must be somehow synchronized with android to eliminate SQLITE_BUSY errors.
Flags: needinfo?(rnewman)
(In reply to Sergej Kravcenko from comment #22)
> Ok, this is not final path, just to share some preliminary thoughts. I've
> decided keep SearchHistoryProvider with separate table for now and add some
> routing in FormHistoryRepositorySession.

Wow, fast progress!  Thanks for digging into this.

> Still some moments:
> 
> 1. Visit count is still in the table and counting local searches but will
> reset to 1 on sync. If we need this, then we must use TIMES_USED, but is not
> used/synced now. Is that incomplete android sync implementation or sync
> protocol doesn't support TIMES_USED field for form history?

Correct.  Sync uses a trivial format right now: just (name, value).
 
> 2. Is "searchbar-history" field already used in live systems as search
> history? If so, we need to add form history table migration and move all
> "searchbar-history" rows into searchhistory table.

Yes, Desktop devices Sync searchbar-history items now.  It would be good to migrate, but that can be a pre-patch.

> Anyway, waiting for feedback. I still think that SearchHistoryProvider
> removal is the right way, but gecko db access must be somehow synchronized
> with android to eliminate SQLITE_BUSY errors.

I'm not sold either way, but we can discuss further.  It's a long process to improve this area.

Thanks for digging in!  I'll try to look at the actual patch shortly.
Flags: needinfo?(nalexander)
Comment on attachment 8673417 [details]
MozReview Request: Bug 1102924 - Sync search history into search history provider

We use feedback? for early-stage review, and there's no need to set needinfo in addition to the review/feedback flag.

I likely won't get to this before next week, so flagging Nick, too.
Flags: needinfo?(rnewman)
Attachment #8673417 - Flags: review?(rnewman)
Attachment #8673417 - Flags: feedback?(rnewman)
Attachment #8673417 - Flags: feedback?(nalexander)
https://reviewboard.mozilla.org/r/21919/#review19761

::: mobile/android/base/db/BrowserContract.java:473
(Diff revision 1)
>      public static final class SearchHistory implements CommonColumns, HistoryColumns {

This no longer implements HistoryColumns, right?

::: mobile/android/base/db/BrowserContract.java:478
(Diff revision 1)
> -        public static final String DATE = "date";
> +        public static final String TIMES_USED = "timesUsed";

Ah, I see, you're aligning with `FormHistory`.  I don't think it's worth extracting `FormHistory` columns.

I don't think I understand how deleted records will work, since we're not storing deleted records in the SearchHistory table.  I may need to think with rnewman about this.

::: mobile/android/base/db/BrowserDatabaseHelper.java:677
(Diff revision 1)
> +        // Generate guids for every row in new table to support sync

nits: full sentences throughout; capitalize GUID; capitalize Sync.

Not major, just "house style".

::: mobile/android/base/db/BrowserDatabaseHelper.java:698
(Diff revision 1)
> +            if (c != null)

nit: house style is always to use curlies, even if the clause is one line.  So:
```
if (c != null) {
    ...
}
```

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:70
(Diff revision 1)
> +  protected final ContentProviderClient serachHistoryProvider;

nit: s/serach/search/.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:91
(Diff revision 1)
> +

Is this a spurious white space change?

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:100
(Diff revision 1)
> +

These need to be done in separate try catch blocks, since both can fail independently.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:145
(Diff revision 1)
> -  @Override
> +  private void guidSinceFromHelper(RepoUtils.QueryHelper helper, ContentProviderClient provider, String[] columns,

nit: since there's no restriction on selection, how about calling this `guidsFromHelper`?

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:172
(Diff revision 1)
> +        ArrayList<String> guids = new ArrayList<String>();

nit: final.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:180
(Diff revision 1)
>            delegate.onGuidsSinceFailed(e);

This seems wrong: you shouldn't fall through.  Did you want that return?

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:192
(Diff revision 1)
>      if (cursor.getColumnCount() == BrowserContractHelpers.FormHistoryColumns.length) {

Hrm.  Risky business, this.  I think we should throw an `IllegalStateException` if this won't work.  Things will fail quickly; we should make sure tests see these failures.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:231
(Diff revision 1)
> +    String guid = RepoUtils.getStringFromCursor(cursor, SearchHistory.GUID);

nit: `final` as much as possible.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:249
(Diff revision 1)
> +

nit: no whitespace changes, unless it's to remove trailing whitespace, and then prefer a Pre: commit.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:467
(Diff revision 1)
> +            cursor.close();

Since you're re-using `cursor`, set it to null after?

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:530
(Diff revision 1)
> +      throw new IllegalArgumentException("Deleted record passed to insertNewRegularRecord.");

This comment looks wrong.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:563
(Diff revision 1)
> +      }

nit: cuddle else, like `} else {`

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:565
(Diff revision 1)
> -      // Store the ContentValues, rather than the record.
> +        // Store the ContentValues, rather than the record.

nit: lift this comment up before the if; or remove it entirely.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:580
(Diff revision 1)
>            return;

This isn't right -- you could have records in the other query.  Consider extracting a helper that takes the buffer and the URI.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:662
(Diff revision 1)
> +    FormHistoryRecord record = (FormHistoryRecord) existingRecord;

nit: final.  This is dangerous, but I need to think more.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:672
(Diff revision 1)
> -    int updated = formsProvider.update(FORM_HISTORY_CONTENT_URI, cv, GUID_IS, new String[] { existingRecord.guid });
> +      int updated = formsProvider.update(FORM_HISTORY_CONTENT_URI, cv, GUID_IS, new String[]{existingRecord.guid});

nit: leave the whitespace alone.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:848
(Diff revision 1)
>            Logger.debug(LOG_TAG, "Wiping form history and deleted form history...");

nit: update comments before and after.

::: mobile/android/base/sync/repositories/domain/FormHistoryRecord.java:38
(Diff revision 1)
> +   * Indicates row is search history

nit: full sentence, so "search history."

Sergej: great patch!  I have a bunch of nits, but nothing really serious.  I would like you to split this patch in two: first patch, the DB changes and the migration; second patch, the Sync changes.

We have some tests in https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit3/src/db/TestFormHistoryRepositorySession.java that you should be able to run from within IntelliJ.  (They aren't well supported and we don't run them in automation.)  Can you try running them, and potentially adding some search history tests?

Great work!
> I don't think I understand how deleted records will work, since we're not
> storing deleted records in the SearchHistory table.  I may need to think
> with rnewman about this.

As I understand, from the android side, deleted table is not not used, at least no inserts. I'm not clear about gecko. From the server side it should work, because items will be deleted from SearchHistory table using GUIDs. Is that all TTL stuff is server side?

There are some "TODO" comments in FormHistoryRepositorySession. I don't know why it's not finished.
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java#567
(In reply to Nick Alexander :nalexander from comment #18)
> (In reply to Sergej Kravcenko from comment #17)
> > > it does look like this is sensible.  I'd want to check the original
> > > SearchHistoryProvider commits to understand why we didn't do this at first!
> > 
> > Probably because it was designed as local thing? Now it's used only in 2
> > places.
> 
> You may be correct.
> 
> > The only thing that is missing, visit count. But as I see, it's not used.
> 
> After skimming the code, I agree.
> 
> > The question is, can we drop it? I don't know why it was added (probably for
> > sorting, but not used) and can't make a decision to remove it.
> 
> I think we will want this, since it provides the "frequency" in "frecency"
> searching.  At some point, we will want to do better than just "recency".
> 
>  If we need
> > this count, then it should be somehow added to form-history, via table/sync
> > change or store it in value field like <count>,<value> or multiple rows.
> 
> We don't have much scope to do this, since Sync clients don't include usage
> counts and we can't change all Sync clients.  (Wish we could!)
> 
> > Can I remove SearchHistoryProvider and go this way? 
> > Can I remove visit count?
> 
> I'd like to get feedback from margaret.  Personally, if we want this feature
> soon, we should remove the special SearchHistoryProvider and use
> FormHistoryProvider throughout.  (Possibly with massaging to make the intent
> clear; or possibly by routing the SearchHistoryProvider to return data from
> the FormHistoryProvider.)  It's worth noting that the SHP is used from the
> Search Activity, which doesn't assume Gecko is running; and that the FHP
> uses a Gecko-provided database; but I don't think there's a conflict here. 
> rnewman could confirm but I think I understand this interaction well enough
> to just say it is so.

It looks like rnewman addressed this, but I worry about trying to use the gecko DBs from the search activity. IIRC, the SQLiteBridge code is pretty fragile and error-prone, so it would be good to reduce (rather than increase) our dependencies on that (bug 946857 would be another step in this direction).

> So, margaret: how much do we care to maintain frequency for future
> improvements?

I don't care about maintaining frequency if we don't use it right now. I'm not sure why we added it, but I don't know that "frecency" is as useful for search terms as it is for bookmarks/history, since it's much easier to recover search terms that aren't remembered (worst case, you just type the term, as opposed to never finding the correct website again).
Flags: needinfo?(margaret.leibovic)
> As I understand, from the android side, deleted table is not not used, at
> least no inserts. I'm not clear about gecko. From the server side it should
> work, because items will be deleted from SearchHistory table using GUIDs. Is
> that all TTL stuff is server side?

Desktop Sync tracks changes through observer notifications. When a record is deleted, Sync is watching, and records the GUID of the deleted item in an object called a Tracker.

Sync on Android doesn't do this. It uses the data source itself, in combination with timestamps (which is terribly flawed, but let's ignore that for now), to figure out what's changed and what's been deleted.

But a deleted item is no longer present in the database!

To track deletions, then, we could either add a flag column to the existing data and mark items as deleted, or we could move deleted items to a separate table. Each approach has its advantages.

Satchel (the form history layer) does the latter:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormHistory.js#49

and we refer to it here:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/db/FormHistoryProvider.java#26


The Java frontend only deletes form history items as a result of syncing, so it never inserts a deleted item. "Deletions" come from JS and C++ code via nsFormHistory, and are then synced up and wiped by Sync in Java-land. Think of the deleted table as a one-way messaging channel between Gecko-side deletions and Android Sync.


TTLs are an interesting topic. They're a server-side concept: "throw away this record this long after upload". The idea is that a new client won't get really old data -- only stuff that's in regular use.

They do play into client code a little, but not in this context, so let's ignore them for now.
Comment on attachment 8673417 [details]
MozReview Request: Bug 1102924 - Sync search history into search history provider

Nick's doing a great job on this.
Attachment #8673417 - Flags: feedback?(rnewman)
> To track deletions, then, we could either add a flag column to the existing
> data and mark items as deleted, or we could move deleted items to a separate
> table. Each approach has its advantages.

I don't see why it's needed. There is no UI in android to delete the search history items. Sync also doesn't add items to the "deleted" table, it simply delete a row (there is no difference which table, normal or deleted). Maybe I'm missing something, but if the search history items cannot be deleted from the UI, then we don't need the deleted table.
(In reply to Sergej Kravcenko from comment #30)

> I don't see why it's needed. There is no UI in android to delete the search
> history item.

1. Swipe-to-delete a form suggestion in a drop-down.

2. Clear Private Data.

3. It can be done through the form history API, so we need to support it even if it weren't currently exposed in the UI.


> Sync also doesn't add items to the "deleted" table, it simply
> delete a row (there is no difference which table, normal or deleted).

Correct -- that's because Sync is the thing that _reads from_ the deleted table. It never needs to write to it, because nothing else cares about seeing which items were deleted.
Comment on attachment 8673417 [details]
MozReview Request: Bug 1102924 - Sync search history into search history provider

Initial feedback provided a while back, and discussion ongoing.  Exciting work!
Attachment #8673417 - Flags: feedback?(nalexander)
Attachment #8673417 - Flags: review?(rnewman)
Comment on attachment 8673417 [details]
MozReview Request: Bug 1102924 - Sync search history into search history provider

Bug 1102924 - Sync search history into search history provider
Assignee: nobody → sergej
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/21917/#review20393

Something missing from this patch:

There's nothing to migrate existing search entries from the form history database into the search provider database. That means that existing Sync users won't see all of their desktop searches -- only the new ones.

Neither do we remove those from the form history database, so they'll just stick around.

::: mobile/android/base/db/BrowserDatabaseHelper.java:400
(Diff revision 2)
> -                    SearchHistory.DATE_LAST_VISITED + " INTEGER, " +
> +                    SearchHistory.DELETED + " INTEGER NOT NULL, " +

You can use TINYINT as a hint that this is a boolean-ish.

::: mobile/android/base/db/BrowserDatabaseHelper.java:674
(Diff revision 2)
> +        Cursor c = null;

Don't use this pattern.

Instead:

```
final Cursor c = db.query(…);
if (c == null) {
  // This should never happen: usually implies a SQL parse error.
  return;
}

try {
  …
} finally {
  c.close();
}

::: mobile/android/base/db/BrowserDatabaseHelper.java:687
(Diff revision 2)
> +                values.put(SearchHistory.LAST_USED, c.getLong(c.getColumnIndex(BrowserContract.HistoryColumns.DATE_LAST_VISITED)));

DATE_LAST_VISITED (and VISITS) can both -- in theory, at least -- be NULL when retrieved from the old table.

You should consider checking that here, or add "DATE_LAST_VISITED IS NOT NULL" and similar constraints to your query.

::: mobile/android/base/db/BrowserDatabaseHelper.java:693
(Diff revision 2)
> +                db.insert(SearchHistory.TABLE_NAME, null, values);

Don't do this one at a time. Build up an array of `ContentValues` by looping over this cursor. Then close the cursor. Then use `db.bulkInsert` to add all of the `ContentValues` at once.

You can actually do this whole migration in a single raw SQL query, though, if you're motivated…

::: mobile/android/base/db/SearchHistoryProvider.java:103
(Diff revision 2)
> +        else {

Nit: cuddle else.

::: mobile/android/base/db/SearchHistoryProvider.java:108
(Diff revision 2)
> +            values.put(SearchHistory.LAST_USED, time);

This isn't a modified time, right? It's LAST_USED. If this is just to support uploading modified items, consider setting a flag for NEEDS_UPLOAD, or always upload-then-flush deleted items…

::: mobile/android/base/db/SearchHistoryProvider.java:131
(Diff revision 2)
> +                selection = selection + " AND " + SearchHistory.DELETED + " = 0";

For safety and clarity:

```
"(" + selection + ") AND (" + SearchHistory.DELETED + " = 0)";
```

::: mobile/android/base/db/SearchHistoryProvider.java:133
(Diff revision 2)
> +            else {

Nit: cuddle else.

::: mobile/android/base/home/BrowserSearch.java:881
(Diff revision 2)
> -            final String sortOrderAndLimit = BrowserContract.SearchHistory.DATE +" DESC LIMIT " + maxSavedSuggestions;
> +            final String sortOrderAndLimit = BrowserContract.SearchHistory.LAST_USED +" DESC LIMIT " + maxSavedSuggestions;

You didn't introduce it, but add the missing space between + and ".

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:126
(Diff revision 2)
> +      throw new NoContentProviderException(uri);

In this case -- the constructor is throwing -- you should also attempt to release `formsProvider` that you acquired on line 118.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:196
(Diff revision 2)
> +    } else if (cursor.getColumnCount() == BrowserContractHelpers.SearchHistoryColumns.length) {

Pretty sure there has to be a better way to do this.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:422
(Diff revision 2)
> +        cursor = null;

This kind of thing is why I favor `final` and not reusing cursors… it's very easy to get wrong.

Use two separate `final` local variables, and rely on the compiler and the VM to be efficient.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:458
(Diff revision 2)
>        Cursor cursor = null;

Same pattern here.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:505
(Diff revision 2)
> +    if (record.searchHistory) {

This seems fragile: you're relying on this function being called with a record that we already populated with .searchHistory = true.

That seems like something that might no longer be true in the future.

You're also only ever running one block or the other: that also seems like it might be the wrong thing to do in the face of some kinds of bug.

Is there any harm in unconditionally taking both paths, deleting from both providers by GUID?
Comment on attachment 8673417 [details]
MozReview Request: Bug 1102924 - Sync search history into search history provider

Great progress!
Attachment #8673417 - Flags: review?(rnewman) → feedback+
> ::: mobile/android/base/db/SearchHistoryProvider.java:108
> (Diff revision 2)
> > +            values.put(SearchHistory.LAST_USED, time);
> 
> This isn't a modified time, right? It's LAST_USED. If this is just to
> support uploading modified items, consider setting a flag for NEEDS_UPLOAD,
> or always upload-then-flush deleted items…

It's not for uploading, it's more for preventing delete. I still can't find a correct solution to support query text unique key. Maybe it's better to use double table like form history does.


> mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.
> java:196
> (Diff revision 2)
> > +    } else if (cursor.getColumnCount() == BrowserContractHelpers.SearchHistoryColumns.length) {
> 
> Pretty sure there has to be a better way to do this.

Yep, it's a hack, but it was not introduced by me. I can rewrite whole fetch part of the FormHistoryRepositorySession. Otherwise we still need some sort of a strange check, like column names.

> 
> :::
> mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.
> java:505
> (Diff revision 2)
> > +    if (record.searchHistory) {
> 
> This seems fragile: you're relying on this function being called with a
> record that we already populated with .searchHistory = true.
> 
> That seems like something that might no longer be true in the future.
> 
> You're also only ever running one block or the other: that also seems like
> it might be the wrong thing to do in the face of some kinds of bug.
> 
> Is there any harm in unconditionally taking both paths, deleting from both
> providers by GUID?

This check will still be left in replace function. This is because the changes are too close to data provider, maybe it's better to separate form history from search history at a higher level, but then I need to rewrite half of the file. I don't like how code looks now, too many repeating blocks.

In delete case we can take a little overhead.
Sergej, how is your work here going? Let us know if you need any help. It would be great to finish this :)
Flags: needinfo?(sergej)
Comment on attachment 8673417 [details]
MozReview Request: Bug 1102924 - Sync search history into search history provider

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21919/diff/2-3/
Attachment #8673417 - Flags: feedback+ → review?(rnewman)
Sorry, I was sick last week. Uploaded fixed patch, added migration from form history to search history.
Flags: needinfo?(sergej)
Attachment #8673417 - Flags: review?(rnewman)
Comment on attachment 8673417 [details]
MozReview Request: Bug 1102924 - Sync search history into search history provider

https://reviewboard.mozilla.org/r/21919/#review22893

::: mobile/android/base/db/BrowserDatabaseHelper.java:411
(Diff revision 3)
> -                    SearchHistory.QUERY + " TEXT UNIQUE NOT NULL, " +
> +                SearchHistory.QUERY + " TEXT UNIQUE NOT NULL, " +

Gentle preference for keeping the block indenting.

::: mobile/android/base/db/BrowserDatabaseHelper.java:685
(Diff revision 3)
> +            while (c.moveToNext()) {

This is a reason not to use this pattern: if `db.query` returns null, then this will NPE.

Do the right thing instead:

```
final Cursor c = db.query(...);
if (c == null) {
    Log.w(LOGTAG, "Couldn't migrate search history.");
    return;
}
try {
    while (c.moveToNext()) {
        ...
    }
} finally {
    c.close();
}
```

::: mobile/android/base/db/BrowserDatabaseHelper.java:690
(Diff revision 3)
> +                values.put(SearchHistory.QUERY, c.getString(c.getColumnIndex(SearchHistory.QUERY)));

`getColumnIndexOrThrow`

::: mobile/android/base/db/BrowserDatabaseHelper.java:697
(Diff revision 3)
> +                debug("Migrating search history: " + values.getAsString(SearchHistory.QUERY));

Kill this; it logs users' search activity to the system log.

::: mobile/android/base/db/BrowserDatabaseHelper.java:713
(Diff revision 3)
> +        Cursor cursor = null;

Same comment about the cursor style.

::: mobile/android/base/db/BrowserDatabaseHelper.java:726
(Diff revision 3)
> +                values.put(SearchHistory.LAST_USED, cursor.getLong(cursor.getColumnIndex(BrowserContract.FormHistory.FIRST_USED)));

Why are you using `FIRST_USED` instead of `LAST_USED`?

::: mobile/android/base/db/BrowserDatabaseHelper.java:730
(Diff revision 3)
> +                    debug("Migrating search history: " + values.getAsString(SearchHistory.QUERY));

Again, don't log users' searches.

::: mobile/android/base/db/BrowserDatabaseHelper.java:737
(Diff revision 3)
> +                client.delete(BrowserContract.FormHistory.CONTENT_URI, BrowserContract.FormHistory.GUID + " = ?", new String[] { guid });

Generally one doesn't issue N deletions while iterating over a cursor. And it's not good to issue N insertions, either!

Instead, iterate over the cursor once, collecting an array of `ContentValues`.

Then -- ideally in a single transaction -- do a `bulkInsert` of the `ContentValues`, and issue a single delete with the same `WHERE` clause as your original query.

That is -- find what to process it, process it, and delete it all in one go.

::: mobile/android/base/db/BrowserDatabaseHelper.java:739
(Diff revision 3)
> +                cursor.moveToNext();

You're advancing the cursor twice for each row you process.

::: mobile/android/base/db/BrowserDatabaseHelper.java:748
(Diff revision 3)
> +                client.release();

These should be nested `try..finally`s; if `cursor.close()` throws, we still want to release the client. 

Fortunately, if you switch to the preferred `Cursor` creation style, you'll get the right structure as a consequence.

::: mobile/android/base/db/BrowserDatabaseHelper.java:1116
(Diff revision 3)
> +        createSearchHistoryTable(db);

It's more efficient to do the insertions from the migration calls before creating the index.

Consider splitting the index creation out of `createSearchHistoryTable`.
(In reply to Richard Newman [:rnewman] from comment #40)

> ::: mobile/android/base/db/BrowserDatabaseHelper.java:685
> (Diff revision 3)
> > +            while (c.moveToNext()) {
> 
> This is a reason not to use this pattern: if `db.query` returns null, then
> this will NPE.
> 
> Do the right thing instead:
> 
> ```
> final Cursor c = db.query(...);
> if (c == null) {
>     Log.w(LOGTAG, "Couldn't migrate search history.");
>     return;
> }
> try {
>     while (c.moveToNext()) {
>         ...
>     }
> } finally {
>     c.close();
> }
> ```
> 

SQLiteDatabase.query cannot return null cursor.



> ::: mobile/android/base/db/BrowserDatabaseHelper.java:690
> (Diff revision 3)
> > +                values.put(SearchHistory.QUERY, c.getString(c.getColumnIndex(SearchHistory.QUERY)));
> 
> `getColumnIndexOrThrow`
> 
> ::: mobile/android/base/db/BrowserDatabaseHelper.java:697
> (Diff revision 3)
> > +                debug("Migrating search history: " + values.getAsString(SearchHistory.QUERY));
> 
> Kill this; it logs users' search activity to the system log.
>

Isn't this development debug? It is used for bookmark migration for example.
 
 
> ::: mobile/android/base/db/BrowserDatabaseHelper.java:726
> (Diff revision 3)
> > +                values.put(SearchHistory.LAST_USED, cursor.getLong(cursor.getColumnIndex(BrowserContract.FormHistory.FIRST_USED)));
> 
> Why are you using `FIRST_USED` instead of `LAST_USED`?
 
Form history uses only FIRST_USED date, don't ask me why, I have no idea.


> 
> ::: mobile/android/base/db/BrowserDatabaseHelper.java:737
> (Diff revision 3)
> > +                client.delete(BrowserContract.FormHistory.CONTENT_URI, BrowserContract.FormHistory.GUID + " = ?", new String[] { guid });
> 
> Generally one doesn't issue N deletions while iterating over a cursor. And
> it's not good to issue N insertions, either!
> 
> Instead, iterate over the cursor once, collecting an array of
> `ContentValues`.
> 
> Then -- ideally in a single transaction -- do a `bulkInsert` of the
> `ContentValues`, and issue a single delete with the same `WHERE` clause as
> your original query.
> 
> That is -- find what to process it, process it, and delete it all in one go.
> 

There is more severe problem, I can't use transactions here, because we copying items from SQLBridge to Android SQL using different connections. And I'm have no idea when gecko is initialising and access form history. There is possibility that we will lost all history because migration will fail. One by one copy is looks safer for me.
(In reply to Sergej Kravcenko from comment #41)

> SQLiteDatabase.query cannot return null cursor.

Myself and others have seen it do so, particularly for a SQL query that didn't parse into a statement.

But that doesn't matter; if you're happy throwing a NPE in that case (your existing version would do so!), just omit the null check in my example, and keep the unambiguous `final` assignment and `finally` clause.


> Isn't this development debug? It is used for bookmark migration for example.

No:

    private static final boolean logDebug   = Log.isLoggable(LOGTAG, Log.DEBUG);

    protected static void debug(String message) {
        if (logDebug) {
            Log.d(LOGTAG, message);
        }
    }

That'll print to the system log. In some of our code we have a much better logging mechanism for this kind of thing (grep for LOG_PERSONAL_INFORMATION), but we don't use it here.

Just kill the log line; if a random user's log would help debug something then it's privacy-impacting, so we can't do that in the wild. If someone ever needs to figure out a bug here, they can add lots more logging or use the debugger.

The line that logs bookmark titles should go, too.


> > Why are you using `FIRST_USED` instead of `LAST_USED`?
>  
> Form history uses only FIRST_USED date, don't ask me why, I have no idea.

I think Satchel writes lastUsed:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormHistory.js#119

(Remember, this DB is written from two separate clients.)

Sync might do this wrong, but there's no harm in migrating the right (or later) value. You can use MAX() to pick the largest if you want to be thorough.


> > That is -- find what to process it, process it, and delete it all in one go.
> > 
> 
> There is more severe problem, I can't use transactions here, because we
> copying items from SQLBridge to Android SQL using different connections.

The important part is the bulkInsert and the single deletion, not the use of transactions.

You know after the bulkInsert if it succeeded, so you do something like this pseudocode:

  let select = "SELECT * FROM form_history WHERE field_name = 'searchbar-history'";
  let cleanup = "DELETE FROM form_history WHERE field_name = 'searchbar-history'";

  let cursor = formHistoryClient.query(select);
  let contentValues = cursor.map { ContentValues(...) };
  
  if (!searchHistoryClient.bulkInsert(contentValues)) {
    Log.w(LOGTAG, "Oh no! Couldn't migrate!");
    return;
  }

  formHistoryClient.delete(cleanup);
    

This does 3 queries rather than 2N+1.

The value in the use of transactions is that by wrapping the two form history operations -- the query and the delete -- in a transaction you can avoid deleting rows that were added in the meantime. But that's so vanishingly unlikely that I wouldn't worry about it.
Comment on attachment 8673417 [details]
MozReview Request: Bug 1102924 - Sync search history into search history provider

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21919/diff/3-4/
Attachment #8673417 - Flags: review?(rnewman)
(In reply to Richard Newman [:rnewman] from comment #42)
> (In reply to Sergej Kravcenko from comment #41)
> 
> > SQLiteDatabase.query cannot return null cursor.
> 
> Myself and others have seen it do so, particularly for a SQL query that
> didn't parse into a statement.
> 

Hmm, example please? I've never seen null here.

> > > Why are you using `FIRST_USED` instead of `LAST_USED`?
> >  
> > Form history uses only FIRST_USED date, don't ask me why, I have no idea.
> 
> I think Satchel writes lastUsed:
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/
> nsFormHistory.js#119
> 
> (Remember, this DB is written from two separate clients.)
> 
> Sync might do this wrong, but there's no harm in migrating the right (or
> later) value. You can use MAX() to pick the largest if you want to be
> thorough.
>

Sync items will have LAST_USED set to null. I've added check for null.


> 
> > > That is -- find what to process it, process it, and delete it all in one go.
> > > 
> > 
> > There is more severe problem, I can't use transactions here, because we
> > copying items from SQLBridge to Android SQL using different connections.
> 
> The important part is the bulkInsert and the single deletion, not the use of
> transactions.
> 
> You know after the bulkInsert if it succeeded, so you do something like this
> pseudocode:
> 
>   let select = "SELECT * FROM form_history WHERE field_name =
> 'searchbar-history'";
>   let cleanup = "DELETE FROM form_history WHERE field_name =
> 'searchbar-history'";
> 
>   let cursor = formHistoryClient.query(select);
>   let contentValues = cursor.map { ContentValues(...) };
>   
>   if (!searchHistoryClient.bulkInsert(contentValues)) {
>     Log.w(LOGTAG, "Oh no! Couldn't migrate!");
>     return;
>   }
> 
>   formHistoryClient.delete(cleanup);
>     
> 
> This does 3 queries rather than 2N+1.
> 
> The value in the use of transactions is that by wrapping the two form
> history operations -- the query and the delete -- in a transaction you can
> avoid deleting rows that were added in the meantime. But that's so
> vanishingly unlikely that I wouldn't worry about it.

SQLiteDatabase doesn't have bulkInsert... Keep in mind we are working with SQLiteDatabase, not ContentProvider. Even
ContentProvider uses simple "for" cycle to do bulkInsert.


> Do the right thing instead:
> 
> ```
> final Cursor c = db.query(...);
> if (c == null) {
>     Log.w(LOGTAG, "Couldn't migrate search history.");
>     return;
> }
> try {
>     while (c.moveToNext()) {
>         ...
>     }
> } finally {
>     c.close();
> }
> ```


You can't do this. Because .query can throw. Even IF .query can return null, you are creating another problem. On query throw you are 
aborting DB upgrade, on null you are ignoring data migration.

Currently if anything is wrong in DB upgrade, all DB access functions will throw an exception, because you cannot access DB. I've
wrapped migration in try/catch to ignore all errors. In case of error we will loose all search history. But if somehow search table get corrupted,
we will leave whole app without DB access until user reinstall the app.
(In reply to Sergej Kravcenko from comment #44)

> SQLiteDatabase doesn't have bulkInsert... Keep in mind we are working with
> SQLiteDatabase, not ContentProvider.

Oh, you're right.

You can still do the deletion in a single query, though.

And you can (and should) still do all of the insertions in a single transaction. See how bulkInsert is implemented for our own CPs:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/AbstractTransactionalProvider.java#262

((Sidenote: it's also possible to implement bulkInsert more efficiently: construct a single query like

  INSERT INTO foo (a, b) VALUES (?, ?) (?, ?) (?, ?)…;

But it probably doesn't matter too much with a sqlite connection in the same process.))


> You can't do this. Because .query can throw.

The version I was commenting on didn't have a catch there, so it's equivalent; I see you just added one in revision 4. If you want to catch, then you can do that outside the finally, and return early, or you can catch when calling the migration method.


> In case of error we
> will loose all search history. But if somehow search table get corrupted,
> we will leave whole app without DB access until user reinstall the app.

There's no good approach here. On iOS we move the whole DB out of the way and reset Sync. On Android we just end up in a busted state. Throwing on non-migration failures (structural) seems sensible. Throwing on migration failures is perhaps also a good idea, at least in pre-release channels: if the migration fails, it's almost certainly because we screwed up somehow and didn't account for a real-world situation.
Attachment #8673417 - Flags: review?(rnewman)
Comment on attachment 8673417 [details]
MozReview Request: Bug 1102924 - Sync search history into search history provider

https://reviewboard.mozilla.org/r/21919/#review24209

::: mobile/android/base/db/BrowserDatabaseHelper.java:418
(Diff revision 4)
> +                SearchHistory.GUID + " TEXT NOT NULL) ");

Is there any reason this shouldn't be UNIQUE?

::: mobile/android/base/db/BrowserDatabaseHelper.java:711
(Diff revision 4)
> +                db.insert(SearchHistory.TABLE_NAME, null, values);

No, really. Do all of these in a single transaction.

::: mobile/android/base/db/BrowserDatabaseHelper.java:716
(Diff revision 4)
> +            if (c != null) {

No, really. Don't do this.

::: mobile/android/base/db/BrowserDatabaseHelper.java:734
(Diff revision 4)
> +            while (cursor != null && cursor.moveToNext()) {

No need to check for != null on every loop. Just do the right thing and early return on failure -- exception or null cursor, or continue into the main body with a live cursor.

::: mobile/android/base/db/BrowserDatabaseHelper.java:738
(Diff revision 4)
> +                String guid = cursor.getString(cursor.getColumnIndexOrThrow(BrowserContract.FormHistory.GUID));

Compute the indices outside the loop.

::: mobile/android/base/db/BrowserDatabaseHelper.java:750
(Diff revision 4)
> +                    db.insert(SearchHistory.TABLE_NAME, null, values);

Again: collect values, establish a transaction, insert all of the values, commit the transaction. You are going to thrash the database to death with one commit per row.

::: mobile/android/base/db/BrowserDatabaseHelper.java:752
(Diff revision 4)
> +                    // Ignore duplicates

No. Use `insertWithOnConflict` and specify `CONFLICT_IGNORE`.

::: mobile/android/base/db/BrowserDatabaseHelper.java:756
(Diff revision 4)
> +                client.delete(BrowserContract.FormHistory.CONTENT_URI, BrowserContract.FormHistory.GUID + " = ?", new String[]{guid});

Nope. Do a single deletion with the same query as on line 731, when you're done inserting, and do so within the same transaction.

::: mobile/android/base/db/BrowserDatabaseHelper.java:768
(Diff revision 4)
> +            client.release();

I would rather see this in a separate enclosing `finally` than smushed into this one.

::: mobile/android/base/db/SearchHistoryProvider.java:102
(Diff revision 4)
> +        } else {

No need for else after return.

::: mobile/android/base/db/SearchHistoryProvider.java:107
(Diff revision 4)
> +            values.put(SearchHistory.LAST_USED, time);

You should also clear the value itself; the only thing left in the row should be metadata.

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:464
(Diff revision 4)
> +          cursor = searchHistoryHelper.safeQuery(searchHistoryProvider, ".findExistingRecordByPayload",

To answer your earlier question: safeQuery was implemented about four years ago because we were seeing null cursors in exactly this situation. It turns them from nulls into NullCursorExceptions.

You should move this outside the try and avoid the null check. (And the code below is wrong in the same way.)

::: mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java:470
(Diff revision 4)
> +          if (cursor != null) {

Same comment as always.
> ::: mobile/android/base/db/BrowserDatabaseHelper.java:756
> (Diff revision 4)
> > +                client.delete(BrowserContract.FormHistory.CONTENT_URI, BrowserContract.FormHistory.GUID + " = ?", new String[]{guid});
> 
> Nope. Do a single deletion with the same query as on line 731, when you're
> done inserting, and do so within the same transaction.
> 

Same transaction? We are working on different DB connections.
Comment on attachment 8673417 [details]
MozReview Request: Bug 1102924 - Sync search history into search history provider

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21919/diff/4-5/
Attachment #8673417 - Flags: review?(rnewman)
I've reverted checks for Cursor nulls in SQLiteDatabase because it can't happen... You can create separate bug and refactor this. Including bookmarks and other BrowserDatabaseHelper queries.

> ::: mobile/android/base/db/SearchHistoryProvider.java:107
> (Diff revision 4)
> +            values.put(SearchHistory.LAST_USED, time);
>
>
> You should also clear the value itself; the only thing left in the row should be metadata.

UNIQUE NOT NULL
(In reply to Sergej Kravcenko from comment #49)

> > You should also clear the value itself; the only thing left in the row should be metadata.
> 
> UNIQUE NOT NULL

Easy enough to fix in the table definition; make the column UNIQUE, and add:

    CONSTRAINT queryOrDeleted CHECK (query IS NOT NULL OR deleted = 1)

In short: if you don't have a query, it had better be because the row is deleted.

This is important for two reasons:

* When a user deletes something, they expect it to no longer be on disk.
* If we delete a search history item, then record another, we ought to create a new record with a different GUID. The behavior of a constellation of synced devices if a record flip-flops isn't obvious.


The current review request causes a bunch of tests to fail to build; I've fixed them locally, rebased on top of current fx-team, and I'll also address my own comments if I have time.
Bug 1102924 - Sync search history into search history provider
Remaining:

* Re-add 'count', probably via multiple rows, with one GUID per visit and thus an immutable timestamp.
* Make sure that searches sync up; I already verified migration from form history manually.
> * Re-add 'count', probably via multiple rows, with one GUID per visit and
> thus an immutable timestamp.

I should elaborate on this.

We have three choices:

1. Store each search term only once, and store the last time it was used.

This is what the current patch does. But uploading a new record with the same GUID when the timestamp changes doesn't map perfectly to Sync: desktop ignores changed form records, so it's impossible to tell desktop that a search was re-run at a later date.

But this is what desktop does, so perhaps it's OK.


2. Store each search term once per use, with an immutable timestamp.

We can wrap this in an interface to expose the same last visited/visit count logic that we previously exposed, which will give us the ability to offer frecency for searches. It also gives us simpler insertions -- just insert rather than handling failure via UPDATE.

It requires a little work to synthesize the right thing for Sync. It's possible that we can have multiple records, each with a different GUID but the same search term, and have desktop not fall over. That's my preferred choice, because it allows Android (and eventually iOS) clients to preserve data.


3. Store local counts, as we did before.

This doesn't allow us to sync anything more sophisticated, but it does give us richer data on the same device.


I don't know for sure which one we'll settle on. Regardless, we need to update testSearchHistoryProvider.

needinfo on me to follow up.
Flags: needinfo?(rnewman)
https://reviewboard.mozilla.org/r/27329/#review24689

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:804
(Diff revision 1)
>          } finally {

You need to add catch here or in line 740, otherwise we can abort db migration because exception from endTransaction will propagate to helper
(In reply to Richard Newman [:rnewman] from comment #50)

>     CONSTRAINT queryOrDeleted CHECK (query IS NOT NULL OR deleted = 1)

N.B., this requires sqlite 3.8.0 or higher. We can either check for that directly (which is what we do on iOS) or conditionalize on Android version.
> N.B., this requires sqlite 3.8.0 or higher. We can either check for that
> directly (which is what we do on iOS) or conditionalize on Android version.

Wait, no, I think I'm going crazy. We're not actually creating a partial index, so we don't need to conditionalize. But now I have well-tested sqlite version checking code, so hey, we can add partial indices whenever it makes sense :)
I think UNIQUE constraint creates index by default, and it will be partial if you want to have several NULL's in the column, no?
(In reply to Richard Newman [:rnewman] from comment #55)

> It requires a little work to synthesize the right thing for Sync. It's
> possible that we can have multiple records, each with a different GUID but
> the same search term, and have desktop not fall over.

This is not the case. nsIFormHistory doesn't prevent you from inserting the same key/value pair more than once, and the UI doesn't combine them, so you get the same search appearing twice in your search bar.

So we can either:

* Synthesize single records for Sync.
* Track just timestamps locally.
* Track timestamps and counts locally.

Our form history format is just terrible. On a new device, all searches will look like they happened at the moment of upload, and reuploads will only change that time for new devices. Ugh.
(In reply to Sergej Kravcenko from comment #59)
> I think UNIQUE constraint creates index by default, and it will be partial
> if you want to have several NULL's in the column, no?

The partial index support I'm referring to is this:

  CREATE INDEX foo_idx ON bar(foo) WHERE deleted = 0;

Otherwise, sqlite indices are always full, even for columns containing nulls.

https://www.sqlite.org/partialindex.html
Flags: needinfo?(rnewman)
Are there plans to use this count, because I've asked this question before. Now even delete is not used.

Main question, do we need sync that count, if we do, then we should use multiple same value rows. It'll simplify inserts, but enlarge table and we will need bulk delete.

if we don't need to sync count, we can use simple count column.
Flags: needinfo?(rnewman)
(In reply to Sergej Kravcenko from comment #62)
> Are there plans to use this count, because I've asked this question before.
> Now even delete is not used.

We'll need to implement handling of deletion before this lands, if it doesn't already work.

(The deletion column is there to allow Sync to upload deleted records to the server, then clean up locally. See the use of SyncColumns.IS_DELETED elsewhere in the mobile/android codebase.)


> Main question, do we need sync that count, if we do, then we should use
> multiple same value rows. It'll simplify inserts, but enlarge table and we
> will need bulk delete.

My point in Comment 60 is that not only do we not currently sync that count, but we cannot do so with the current form history object format. I had hoped that the desktop client would do the right thing with repeated records, but it doesn't.

Given that we also don't use the count locally, we can ignore it for the purposes of this bug.


I've fixed up remaining bugs that caused test failures locally (in particular a syntax error in the UPDATE for re-insertion), and it's at <https://reviewboard.mozilla.org/r/27337>. Please take a look.

I'll push again to Try shortly.

Then we need to look at deletion (with and without Sync) before we land.

Do you want to take over the reins again, Sergej?
Flags: needinfo?(rnewman)
(In reply to Richard Newman [:rnewman] from comment #64)
> (In reply to Sergej Kravcenko from comment #62)
> > Are there plans to use this count, because I've asked this question before.
> > Now even delete is not used.
> 
> We'll need to implement handling of deletion before this lands, if it
> doesn't already work.
> 
> (The deletion column is there to allow Sync to upload deleted records to the
> server, then clean up locally. See the use of SyncColumns.IS_DELETED
> elsewhere in the mobile/android codebase.)
> 

I was talking about delete from UI side, not Sync. As I understand, now you cannot delete search suggestions.
 
> I've fixed up remaining bugs that caused test failures locally (in
> particular a syntax error in the UPDATE for re-insertion), and it's at
> <https://reviewboard.mozilla.org/r/27337>. Please take a look.
> 
> I'll push again to Try shortly.
> 
> Then we need to look at deletion (with and without Sync) before we land.
> 
> Do you want to take over the reins again, Sergej?

What parts are missing now? Or we simply need to test whole thing? I'm confused, do we need to implement counting now?
Flags: needinfo?(rnewman)
(In reply to Sergej Kravcenko from comment #67)

> I was talking about delete from UI side, not Sync. As I understand, now you
> cannot delete search suggestions.

You can, in two ways:

1. Via Clear Private Data. See LocalSearches.clearSearchHistory, called from LocalBrowserDB.clearHistory.
2. Via the Search Activity. Swipe a suggestion row to the right. I haven't looked at how this is implemented.


> What parts are missing now? Or we simply need to test whole thing? I'm
> confused, do we need to implement counting now?

We do not need to implement counting.

We do need to:

* Make sure that deletions via Clear Private Data and the Search Activity both result in rows flagged with deleted=1.
* Make sure that Sync syncs deleted rows up to the server and that desktop handles them correctly.
* Make sure that there's a hook somewhere to expire old rows that are flagged for deletion -- either after a successful sync or on a timer. If there isn't, we should file a follow-up bug.
* Test the whole thing again:
  * Migrations from the previous database schema, with and without synced data.
  * Disconnecting and reconnecting Sync.
  * Adding and removing search history rows on each platform and syncing.
Flags: needinfo?(rnewman)
Sergej, any news on this?
Flags: needinfo?(sergej)
Sorry, I was little busy with my job projects and holidays. Will try to find some time this week to finish this.
Flags: needinfo?(sergej)
QA Contact: flaviu.cos
> * Make sure that deletions via Clear Private Data and the Search Activity
> both result in rows flagged with deleted=1.

Search Activity is ok. It seems Clear Private Data is working via gecko engine, so this is not ok. The question is where it must be implemented. Seems in PrivateDataPreference.

> * Make sure that Sync syncs deleted rows up to the server and that desktop
> handles them correctly.

I did simple tests, but without sync server access it's hard. Do you have some tool to browse sync server?

> * Make sure that there's a hook somewhere to expire old rows that are
> flagged for deletion -- either after a successful sync or on a timer. If
> there isn't, we should file a follow-up bug.

How it's done with form history right now? Gecko do this or deleted form history rows stay forever? If it is not implemented for form history, then separate bug will be preferred way to do it.   

> * Test the whole thing again:
>   * Migrations from the previous database schema, with and without synced
> data.
>   * Disconnecting and reconnecting Sync.
>   * Adding and removing search history rows on each platform and syncing.

Did some tests, but as I said, without sync server access it's hard to do. 


Seems current patch is ok, the only thing missing is Clear Private Data handling. Waiting for your suggestion where it can be implemented or I can try do it in PrivateDataPreference
Flags: needinfo?(rnewman)
Grisha, do you have enough context to help pick up this review? Sergej, are you still interested in working on this?

I'm sorry it's been so long since you've received any feedback. Richard has been stretched too thin, and now he's officially working on another project, so unfortunately he's not available to help us here.
Mentor: rnewman
Flags: needinfo?(sergej)
Flags: needinfo?(rnewman)
Flags: needinfo?(gkruglov)
Whiteboard: [lang=java][diamond bug]
Margaret, I'll get caught up on this stuff over the week.
Flags: needinfo?(gkruglov)
Sure, I'll check the state of the patch and will rebase it. If everything is ok, the only thing left here is history clear handling.
Flags: needinfo?(sergej)
Comment on attachment 8673417 [details]
MozReview Request: Bug 1102924 - Sync search history into search history provider

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21919/diff/5-6/
Flags: needinfo?(gkruglov)
Sergej, apologies for the delay - will update you soon!
Hi Sergej,

My apologies, but unfortunately at this time we won't be able to follow through with your patches. The team is switching gears to work on projects outside of the browser, and currently we don't have the bandwidth to go through the more involved/risky changes such as these. Hopefully in the coming months situation will change, and we can pick it up there if you're still interested in contributing this work.

Really sorry about the run-around :-(
Flags: needinfo?(gkruglov)
Priority: -- → P3
Product: Android Background Services → Firefox for Android
Assignee: sergej → nobody
Status: ASSIGNED → NEW
QA Contact: cos_flaviu
QA Contact: cos_flaviu
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
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: