887868 - remove search.sqlite migration code

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Marco Bonardo [:mak]]

_asyncMigrateOldDB is async executing a statement created through createStatement, it should use createAsyncStatement instead, to avoid creating jank.

Comment 1

[Steven MacLeod [:smacleod]]

Gavin and I discussed this migration in IRC, and decided we might be able to just remove it all together. I'm going to look into whether this would be suitable.

Comment 2

[Steven MacLeod [:smacleod]]

Attachment: Patch - Remove Search Service DB Migration

This removes the migration from the old Search Service DB (search.sqlite) to the new JSON file. Now if the JSON file does not exist, the metadata will be initialized to an empty store.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Hmm, can we simplify further and remove JSON_LOADING_ATTEMPTED?

Comment 4

[Steven MacLeod [:smacleod]]

Attachment: Patch - Remove Search Service DB Migration

Updated patch which also removes the JSON_LOADING_ATTEMPTED state.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 776429 [details] [diff] [review]
Patch - Remove Search Service DB Migration

Hmm, now I'm wondering whether we can get rid of _InitStates entirely in favor of a single "done loading" flag. Maybe that's too invasive. What do you think?

Comment 6

[Marco Bonardo [:mak]]

may we also remove search.sqlite in a migrateUI step?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

We have bug 769557 on file for that, but I don't see that as a high priority (this database doesn't really contain any sensitive data).

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> Hmm, now I'm wondering whether we can get rid of _InitStates entirely in
> favor of a single "done loading" flag. Maybe that's too invasive. What do
> you think?

I spoke to Steven about this in person, he filed bug 896688 to cover this.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 776429 [details] [diff] [review]
Patch - Remove Search Service DB Migration

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>             } catch (ex) {
>               if (this._initState == engineMetadataService._InitStates.FINISHED_SUCCESS) {
>                 // No need to pursue asynchronous initialization,
>                 // synchronous fallback was called and has finished.
>                 return;
>               }
>+              // Coudln't load json, use an empty store

"coudln't" typo

>+              LOG("metadata init: could not load JSON file " + ex);
>               this._store = {};
>+              this._initState = this._InitStates.FINISHED_SUCCESS;
>+              return;
>             }

Hmm, I guess this bug existed before, but the "metadata init: complete" LOG call below won't ever be hit now because both branches return early. I think you can get rid of the returns here now, since there's no other code in the Task function. You can also put the setting of this_initState outside of the try/catch, since both paths through that set it to FINISHED_SUCCESS.

>-   _syncMigrateOldDB: function SRCH_SVC_EMS_migrate() {

Similarly it seems like this could simplified, but we can investigate that in the followup bug.

Comment 10

[Steven MacLeod [:smacleod]]

Attachment: Patch - Remove Search Service DB Migration

Updated patch with small fixes from Gavin's review.

Comment 11

[Steven MacLeod [:smacleod]]

Attachment: Patch - Remove Search Service DB Migration

Fixed a bug in the previous patch (case statement falling through). Tests are passing now.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 779786 [details] [diff] [review]
Patch - Remove Search Service DB Migration

r=me, but I'll attach a supplemental patch on top of this that address a minor nit (seemed easier than explaining it)

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: nit fix

(applies on top of attachment 779786 [details] [diff] [review])

Comment 14

[Steven MacLeod [:smacleod]]

Comment on attachment 780013 [details] [diff] [review]
nit fix

Review of attachment 780013 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, more clear.

Comment 15

[Steven MacLeod [:smacleod]]

Attachment: Patch - Remove Search Service DB Migration

Rolled Gavin's nit fix into the patch

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1213d6035d

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fe1213d6035d

Comment 18

[Dão Gottwald [:dao]]

This is being backed out in bug 898713:

https://hg.mozilla.org/integration/mozilla-inbound/rev/36326783bd82

Comment 19

[Ryan VanderMeulen [:RyanVM]]

FWIW, this still does reproduce easily on Try with bug 873378 re-landed.
https://tbpl.mozilla.org/?tree=Try&rev=1aaf8295268a

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Now that bug 914437 (and thus bug 896054) is fixed, we should be clear to land this again!

Comment 21

[Steven MacLeod [:smacleod]]

Attachment: Patch - Remove Search Service DB Migration

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/749897db57ac

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/749897db57ac