1206741 - [OTA update] Unable to effectively change the update URL

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jan Keromnes [:janx]]

(See bug 1201556 comment 36.)

Comment 1

[Jan Keromnes [:janx]]

Attachment: [OTA] Make URL pref changes overwrite B2G's setting.

Untested.

Comment 2

[Jan Keromnes [:janx]]

That's solution 1 from the other bug, to err on the side of caution.

Comment 3

[Jan Keromnes [:janx]]

Attachment: [OTA] Make URL pref changes overwrite B2G's setting.

Tested and fixed.

Comment 4

[Jan Keromnes [:janx]]

Comment on attachment 8665437 [details] [diff] [review]
[OTA] Make URL pref changes overwrite B2G's setting.

Alex, I chose to implement option 1 anyway, because it seems safer to me.

For example, imagine that someone changes the update URL in order to switch to a community-managed OTA server, but for some reason the phone needs to be rebooted before an update can be received (bad network reception, battery is empty... anything really). It's just more practical to keep the setting unchanged, until the underlying pref changes, instead of resetting it on every reboot.

Comment 5

[:gerard-majax]

How do I reset if I want to go back to default values ?

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8665437 [details] [diff] [review]
[OTA] Make URL pref changes overwrite B2G's setting.

Fabrice, I've had to modify the behavior of my update URL setting somewhat in order to support FOTA updates for foxfood devices.

In short, the setting will now watch for changes to the underlying pref, and reset itself to the new value if there is a change (e.g. when an OTA update modifies the update URL format).

More details about the change can be found in bug 1201556 comment 36 (I chose to go with solution 1).

Comment 7

[:gerard-majax]

Comment on attachment 8665437 [details] [diff] [review]
[OTA] Make URL pref changes overwrite B2G's setting.

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

That would look good, but I would feel better if we could find a way to cover this by tests: it is the kind of stuff we really don't want to break.

Breakage here might equal no more updates.

::: b2g/chrome/content/settings.js
@@ +317,5 @@
> +    // changes the update URL format.
> +    let backupName = prefName + '.old';
> +    try {
> +      let backupValue = Services.prefs.getCharPref(backupName);
> +      if (defaultValue !== backupValue) {

And everything relies on this comparison: when pushing a new Gecko that changes app.update.url or app.update.channel, that will get triggered.

@@ +319,5 @@
> +    try {
> +      let backupValue = Services.prefs.getCharPref(backupName);
> +      if (defaultValue !== backupValue) {
> +        // If the pref has changed since our last backup, overwrite the setting.
> +        navigator.mozSettings.createLock().set(defaultSetting);

So we initialize the setting here.

@@ +320,5 @@
> +      let backupValue = Services.prefs.getCharPref(backupName);
> +      if (defaultValue !== backupValue) {
> +        // If the pref has changed since our last backup, overwrite the setting.
> +        navigator.mozSettings.createLock().set(defaultSetting);
> +        Services.prefs.setCharPref(backupName, defaultValue);

And we initialize backup.

@@ +336,3 @@
>          return;
>        }
> +      defaultBranch.setCharPref(prefName, value);

And here we will overwrite the pref defined by the setting value.

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 8665437 [details] [diff] [review]
[OTA] Make URL pref changes overwrite B2G's setting.

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

::: b2g/chrome/content/settings.js
@@ +324,5 @@
> +        Services.prefs.setCharPref(backupName, defaultValue);
> +      }
> +    } catch(e) {
> +      // If there was no backup, create one.
> +      Services.prefs.setCharPref(backupName, defaultValue);

You do the same thing here and in the try {} block. You can just move that after the catch {}

Comment 9

[Jan Keromnes [:janx]]

Attachment: [OTA] Make URL pref changes overwrite B2G's setting. r=fabrice f=gerard-majax

Rebased and included comments from comment 7, keeping Fabrice's r+.

Comment 10

[Jan Keromnes [:janx]]

Attachment: [OTA] Make URL pref changes overwrite B2G's setting.

Last-minute tweak on Alex's request: If there is no backup value, overwrite the setting (so that the backup-creating patch doesn't have to be applied *before* the pref changes).

Comment 11

[Jan Keromnes [:janx]]

Attachment: [UPDATE] Add logging.

Quick-and-dirty patch for Alex to add debugging logs to the code.

Comment 12

[:gerard-majax]

Comment on attachment 8667255 [details] [diff] [review]
[OTA] Make URL pref changes overwrite B2G's setting.

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

I checked several workflows:
 (1) landing app.update.url AND this patch at the same time
 (2) landing app.update.url before this patch
 (3) landing app.update.url after this patch

Results are:
 (1) works well, the new pref value is properly used to perform update checking
 (2) first boot with only app.update.url change, the pref is not considered and we still use the old value from setting. second boot after adding this patch, we do the update query against the new update url
 (3) first boot with only this patch, update checking is performed against the old url as expected. second boot after changing the update url pref, is uses the new update url value

So it would look to be working as expected and in all cases it should be good to land this with bug 1201556 attachment 8662338 [details] [diff] [review]

Comment 13

[Jan Keromnes [:janx]]

I didn't find an easy way to test this update-pref/setting sync glue, so let's use bug 1209503 to investigate further, and land this FOTA-unblocking fix in the meantime.

Comment 14

[Jan Keromnes [:janx]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bb573cd0440

Comment 15

[:gerard-majax]

Is that good? Can you land?

Comment 16

[Gregor Wagner [:gwagner]]

blocking a blocker

Comment 17

[Jan Keromnes [:janx]]

Thanks for setting "checkin-needed", I meant to do it right after the try results were green. OK to land.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/9be12cdd5daf

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/9be12cdd5daf