1112092 - Implement a migrator for mozSettings values in B2G

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Runtime, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

With bug 1109451, and bug 1042135 and other cases, it is clear we need to migrate mozSettings database in Gecko, since DB_VERSION is kept in Gecko.

The alternative, which is what we currently do, is to have the code run it's migration test *every time* it runs (at least *once* when the phone boots up), and check every legacy value we have ever used. There exists other smarter approach which simply stores the Gaia version as a key, but I honestly thinks we are not solving the problem in a systematic way, re-inventing the wheel all over, resulting waste of engineering effort and performance issues.

So my proposal, is, just like Firefox UI do in [1] for Gecko pref, we should insert a B2G settings migrator in [2] (maybe by trying to lazily load a migrator module in b2g/), and only run it when DB_VERSION changes.

([2] currently "migrates" the settings by simply going through settings.json and get the new defaults -- which is not enough for the mentioned use cases.)

With the migrator ready, we can go ahead clean up many of the overloading stuff in gaia/apps/system/ and gaia/shared/.

Gregor, as the original author of the Settings API, I would like to know if this approach sounds good to you or not. I would also like ask for your help from the systemfe team on this.

We similarly would need to figure out migration for Gecko pref in B2G as well, since it's so tightly coupled like [3].

[1] https://hg.mozilla.org/mozilla-central/rev/9a16137bc7b4#l2.22
[2] http://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsDB.jsm#56
[3] http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#74

Arthur, this is essentially what we have discussed offline. As owner of Settings app, we need to gather up our migration data as well. We can figured out who should do this later (thankfully this part of Gecko is in JavaScript).

Comment 1

[Arthur Chen [:arthurcc][inactive after 6/30]]

The following are the settings that need migration. All of them are resulted from the we added the support for dual sim.

- ril.iccInfo.mbdn
- ril.cellbroadcast.disabled
- ril.cellbroadcast.searchlist
- ril.data.apnSettings

Details on how the logic is implemented in gecko still need investigation. Especially on the dependent modules that required in the migration process.

Comment 2

[:gerard-majax]

And I think solving this would definitively help with bug 1095034

Comment 3

[Gregor Wagner [:gwagner]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #0)
> With bug 1109451, and bug 1042135 and other cases, it is clear we need to
> migrate mozSettings database in Gecko, since DB_VERSION is kept in Gecko.
> 
> The alternative, which is what we currently do, is to have the code run it's
> migration test *every time* it runs (at least *once* when the phone boots
> up), and check every legacy value we have ever used. There exists other
> smarter approach which simply stores the Gaia version as a key, but I
> honestly thinks we are not solving the problem in a systematic way,
> re-inventing the wheel all over, resulting waste of engineering effort and
> performance issues.
> 
> So my proposal, is, just like Firefox UI do in [1] for Gecko pref, we should
> insert a B2G settings migrator in [2] (maybe by trying to lazily load a
> migrator module in b2g/), and only run it when DB_VERSION changes.
> 
> ([2] currently "migrates" the settings by simply going through settings.json
> and get the new defaults -- which is not enough for the mentioned use cases.)
> 
> With the migrator ready, we can go ahead clean up many of the overloading
> stuff in gaia/apps/system/ and gaia/shared/.
> 
> Gregor, as the original author of the Settings API, I would like to know if
> this approach sounds good to you or not. I would also like ask for your help
> from the systemfe team on this.
> 
> We similarly would need to figure out migration for Gecko pref in B2G as
> well, since it's so tightly coupled like [3].
> 
> [1] https://hg.mozilla.org/mozilla-central/rev/9a16137bc7b4#l2.22
> [2]
> http://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsDB.jsm#56
> [3]
> http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.
> js#74
> 
> Arthur, this is essentially what we have discussed offline. As owner of
> Settings app, we need to gather up our migration data as well. We can
> figured out who should do this later (thankfully this part of Gecko is in
> JavaScript).

Let me see if I understand the problem correctly.
So I guess we need to solve 2 issues here. 1) being able to force setting changes and 2) have a way to trigger those setting changes in a performance optimized way (without having to increment the DB version and/or read the file every single time).

I agree with you on 1) We can add this functionality to [2] and even extend the setting.json format to indicate an overwrite.
We talked about it a couple times but we said we want to wait until we actually need it.
Lets do it.

For 2) Thats more tricky since we have to perform this migration during startup and we don't want to make the startup slower by migrating it every single time. We need a mechanism that only triggers the migration if something actually changes.
I guess prefs are the right way to go here.

Comment 4

[Arthur Chen [:arthurcc][inactive after 6/30]]

For 1), we need to derive the new setting value based on the original one instead of just overwriting it. There should be a place executing such code for migration.
As For 2), the current mechanism when bumping DB_VERSION seems perfect for the scenario, is it required to add a new pref?

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: WIP Part 1

Greger, please tell me if this is an approach you would agree with. I intentionally left some comment explaining why the patch is written this way.

If you agree I will introduce part 2 of the patch that would at least migrate something (maybe v1.0 keyboard settings) first, as a demonstration.

This part of Gecko is unfortunately not covered in any of the tests. I will follow-up and create tests too if you agree with my approach.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: WIP Part 2

Both patches shouldn't land unless they are verified by tests, so it's fine if you would like to wait for the tests before giving reviews.

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8598516 [details] [diff] [review]
WIP Part 2

Looks good to me.
We probably also need to set English and number layout as the default layouts with the key, 'keyboard.default-layouts'.

Thanks.

Comment 8

[Arthur Chen [:arthurcc][inactive after 6/30]]

I'll take over the bug and continue to move things forward.

Comment 9

[Gregor Wagner [:gwagner]]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #5)
> Created attachment 8598488 [details] [diff] [review]
> WIP Part 1
> 
> Greger, please tell me if this is an approach you would agree with. I
> intentionally left some comment explaining why the patch is written this way.
> 
> If you agree I will introduce part 2 of the patch that would at least
> migrate something (maybe v1.0 keyboard settings) first, as a demonstration.
> 
> This part of Gecko is unfortunately not covered in any of the tests. I will
> follow-up and create tests too if you agree with my approach.

I applied your patch and tried to play around with it but I get syntax errors. Are you only looking for high-level feedback?

Comment 10

[Arthur Chen [:arthurcc][inactive after 6/30]]

Gregor, can you provide high-level feedback first if it does not run smoothly yet? Thanks.

Comment 11

[Gregor Wagner [:gwagner]]

Has been a long time since I played around with the settingsDB :/

I am trying to figure out what new features this patch creates.
Currently we only migrate settings values when we bump the db version number. This will remain the same right?

We already can delete settings entries by just removing them from the settins.json file.
Line 129 will take care of it: http://hg.mozilla.org/mozilla-central/file/0ca37b3cb73d/dom/settings/SettingsDB.jsm#l129

Comment 12

[Arthur Chen [:arthurcc][inactive after 6/30]]

(In reply to Gregor Wagner [:gwagner] from comment #11)
> Has been a long time since I played around with the settingsDB :/
> 
> I am trying to figure out what new features this patch creates.
> Currently we only migrate settings values when we bump the db version
> number. This will remain the same right?
Yes. However, the patch is trying to address another use case that in some cases we are not replacing the value with a new one specified in settings.json but migrating the value from an older format to a new one. For example, recently we added the feature of encrypting the lockscreen passcode which is stored plain in the settings db. To make this change transparent to users we need to encrypt the original passcode and save it back to the same field.
Currently the migration is performed upon the starting up of the device and some of them are in the first access to the fields, which leads to extra db access and slows down the start-up performance. That is the reason why we are moving the migration to gecko.
> 
> We already can delete settings entries by just removing them from the
> settins.json file.
> Line 129 will take care of it:
> http://hg.mozilla.org/mozilla-central/file/0ca37b3cb73d/dom/settings/
> SettingsDB.jsm#l129

Comment 13

[:gerard-majax]

Arthur, I'm not sure I do get your usecase clearly. Would it be like bug 1022654 and specifically what I documented in bug 1022654 comment 21 ?

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

No, they are not the same thing.

The migration we are talking about is to derive the new setting value based on the old one in runtime. Here is another example, the field 'ril.data.apnSettings'. Before supporting dual sim the value stored in this field is a plain object. After we support dual sim the code expects the field to be an array because we could have separate apn setting for each sim card. In order to prevent users from losing their apn settings after applying the system update, we create a new array, put the original object stored under the field to it, then save the array back to the field. Currently this is being implemented here[1].

The patch is trying to define a way of doing this kind of migration in gecko, then we are able to move the related code from gaia to here with follow-up bugs afterward.

[1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/operator_variant_handler.js#L572

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8598488 [details] [diff] [review]
WIP Part 1

Yes, I was asking for high level feedback. So review is probably not the right flag to set....

Comment 16

[:gerard-majax]

Thanks Arthur, I do understand much better now. I'll re-look at the patch now.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Alexandre LISSY :gerard-majax from comment #16)
> Thanks Arthur, I do understand much better now. I'll re-look at the patch
> now.

You could also look at the bugs blocked by this bug to understand what I was trying to centralize here.

Comment 18

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8598488 [details] [diff] [review]
WIP Part 1

Setting the flag for tracking.

Comment 19

[:gerard-majax]

Sorry to be picky, but it also means we will move code that handles settings values into the settings database upgrade flow. So indeed, let us consider the example you referred to in comment 14:
 - https://github.com/mozilla-b2g/gaia/blob/6fbb088da67e3efce11ab4d362e9bc972babe97e/apps/system/js/operator_variant_handler.js#L572
 - this logic would get killed from gaia and moved into gecko's SettingsDBMigrator, within the migrateValue() method.

I'm not sure that it's a good thing we do this kind of move, since it will make it harder from the app point of view to track values evolutions.

That being said, I don't have any good idea of how we should properly handle this for now.

You stated in comment 12 that this was crippling performances during the startup. I fear that moving the changes into Gecko will not help a lot, since this will delay the migration earlier, while opening the database.

I also remember several discussion with kyle and gregor where we were not very happy about the dependency against the database version bump to trigger migration. The current proposed approach would move forward into more tight links between Gecko and Gaia on those two topics, while I think we should instead loosen those links.

What's your point of view regarding this?

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Alexandre LISSY :gerard-majax from comment #19)
> Sorry to be picky, but it also means we will move code that handles settings
> values into the settings database upgrade flow. So indeed, let us consider
> the example you referred to in comment 14:
>  -
> https://github.com/mozilla-b2g/gaia/blob/
> 6fbb088da67e3efce11ab4d362e9bc972babe97e/apps/system/js/
> operator_variant_handler.js#L572
>  - this logic would get killed from gaia and moved into gecko's
> SettingsDBMigrator, within the migrateValue() method.
> 
> I'm not sure that it's a good thing we do this kind of move, since it will
> make it harder from the app point of view to track values evolutions.
> 
> That being said, I don't have any good idea of how we should properly handle
> this for now.
> 

I don't understand what the above code do so I can say for sure we could move this into Gecko or not, either.

> You stated in comment 12 that this was crippling performances during the
> startup. I fear that moving the changes into Gecko will not help a lot,
> since this will delay the migration earlier, while opening the database.
> 

The migration here only happens when there is a version bump, and everywhere else will cost us time on every reboot.

> I also remember several discussion with kyle and gregor where we were not
> very happy about the dependency against the database version bump to trigger
> migration. The current proposed approach would move forward into more tight
> links between Gecko and Gaia on those two topics, while I think we should
> instead loosen those links.

I would love that as well but unless we expose the versionchange transaction to System app (and System app will have to grap the transaction in time) there is no way to put migration logic except here. 

> What's your point of view regarding this?

I am open to better ideas if there is one...

Comment 21

[[:fabrice] Fabrice Desré]

We have other products using the settings api (like browser.html) so we can't hardcode the migration algorithm in gecko. I don't see what's wrong with:
1) Adding the db version as a setting.
2) Doing the migration in the system app, which is really the platform side of gaia.

Comment 22

[Arthur Chen [:arthurcc][inactive after 6/30]]

The goals were to:
- make sure that all settings fields are in correct format before they are being used.
- avoid redundant db access during startup.

Implementing the migration in gecko achieves the goals and it uses the existing indexed db upgrade mechanism as a leverage. The same thing can be done in gaia as long as we can ensure the migration is performed before executing other modules and only performed when required. This means we have to implement versioning and upgrading mechanism which already exist in an indexed db in gaia, but the benefit would be that if a feature introduces a format change to a field the migration logic can be in the same commit.

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

Per the above discussions, there seems no strong reason of moving the migrator from gaia to gecko. Resolve the bug as WONTFIX.