944242 - [OTA][Data Migration][Do Not Track] "Do Not Track" radio button options is not selected after update from v1.1 to v1.2

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Hubert Lu[:hlu] <[e-mail]>]

Attachment: Do Not Track (New).png

* Build Information
Gaia:     92cd11ea023dd6598d82d859ae3c945ff6589ce6
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/14e91ab12441
BuildID   20131127004001
Version   26.0
ro.build.version.incremental=eng.cltbld.20131127.073824


* Reproduce Steps
1. Flash device to v1.1 build from PVT
2. Change update URL to http://update.boot2gecko.org/unagi/1.2.0/nightly/update_20131127004001.xml
3. Open Settings -> Do Not Track
4. Enable "Do Not Track" radio butoon.
5. Download the update package and install the update.

* Actual results:
1. After update, none of "Do Not Track" radio button is selected.

* Expected result:
1. The option that Do Not track should be selected.

Comment 1

[Hubert Lu[:hlu] <[e-mail]>]

Attachment: Do Not Track (Original v1.1).png

Comment 2

[Hubert Lu[:hlu] <[e-mail]>]

Attachment: logcat.txt

Comment 3

[Jason Smith [:jsmith]]

Besides the fact that this is data loss, this has bad privacy impact, as a user would expect do not track to be enabled post the update until they happen to stumble upon the fact that the setting was flipped.

Comment 4

[Askeing Yen[:askeing][:fyen]]

Arthur, could you help to take a look on this issue?
Thank you :)

Comment 5

[Preeti Raghunath(:Preeti)]

Gabriele,

Can you please take a look at this bug?

Comment 6

[Gabriele Svelto [:gsvelto]]

Sure, I'll take it.

Comment 7

[Gabriele Svelto [:gsvelto]]

I need to know which v1.1 PVT build was used because I'm not having much luck finding one that works on my Unagi :-|

Comment 8

[Hubert Lu[:hlu] <[e-mail]>]

The build file link path is in below.

https://pvtbuilds.mozilla.org/pub/mozilla.org/b2g/nightly/mozilla-b2g18-unagi-eng/2013/11/2013-11-27-04-12-03/

Comment 9

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #5)
> Gabriele,
> 
> Can you please take a look at this bug?

Switching to koi+ per triage

Comment 10

[Gabriele Svelto [:gsvelto]]

I've successfully reproduced this going from v1.1 to v1.2, I believe this also applies to master.

Comment 11

[Gabriele Svelto [:gsvelto]]

Verified on master too. The problem is fairly trivial and I've got a patch almost ready: on v1.1 we only had the 'privacy.donottrackheader.enabled' setting, while v1.2 introduced 'privacy.donottrackheader.value'. The latter is set by default to -1 which means "No preference". The relevant code does not further check the value of 'privacy.donottrackheader.enabled' so even if it is set to 'true' it will still appear in the panel as if DNT was not enabled.

Comment 12

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Properly handle do-not-track options when upgrading from 1.1

This patch detects the condition where 'privacy.donottrackheader.value' do not exists and reacts according to the presence/value of 'privacy.donottrackheader.enabled':

- if 'privacy.donottrackheader.enabled' is true, then 'privacy.donottrackheader.value' is sets to 1 (do not track enabled)

- if 'privacy.donottrackheader.value' is false or undefined then 'privacy.donottrackheader.value' is set to -1 (no preference)

I've tested the code by trying the three different upgrade paths on my Unagi.

Comment 13

[Kai-Chih Hu [:khu]]

Because 1.2 is going to be finished, should we mark this bug from koi+ to 1.3+? Thanks.

Comment 14

[Gabriele Svelto [:gsvelto]]

Review ping.

Comment 15

[Fabien Cazenave [:kaze]]

Comment on attachment 8342535 [details] [diff] [review]
[PATCH] Properly handle do-not-track options when upgrading from 1.1

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

LGTM except for the `if (enabled === null)` detail.

Sorry for my super-late review, this bug went off my radar. I promise I won’t miss your patch update.

::: apps/settings/js/do_not_track.js
@@ +1,5 @@
>  /* -*- Mode: js; js-indent-level: 2; indent-tabs-mode: nil -*- */
>  /* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
>  
> +/* globals Settings */
> +

nit: what does this coment mean?

@@ +23,2 @@
>      return;
> +  }

<3

@@ +64,2 @@
>        return;
> +    }

Warning, this has a different result than the original `enable == null` test, and it will never occur. I guess the original author meant:

  if (!(result.settingValue in enabledMap)) {
    return;
  }
  var enabled = enabledMap[result.settingValue];

or:

  var enabled = enabledMap[result.settingValue];
  if (enabled === undefined) {
    return;
  }

We probably should log a warning or generate an exception in such a case, BTW.

Comment 16

[Fabien Cazenave [:kaze]]

Ugh, there’s not enough context for my comment. This is the part of your patch I’m talking about:

     var enabled = enabledMap[result.settingValue];
-    if (enabled == null)
+    if (enabled === null) {
       return;
+    }

Comment 17

[Gabriele Svelto [:gsvelto]]

(In reply to Fabien Cazenave [:kaze] from comment #15)
> Sorry for my super-late review, this bug went off my radar. I promise I
> won’t miss your patch update.

No problem and thanks for the review. I'll post the updated patch ASAP.

> ::: apps/settings/js/do_not_track.js
> @@ +1,5 @@
> >  /* -*- Mode: js; js-indent-level: 2; indent-tabs-mode: nil -*- */
> >  /* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> >  
> > +/* globals Settings */
> > +
> 
> nit: what does this comment mean?

It is used to inform jshint that the 'Settings' symbol is available globally and quiets all jshint warnings related to it. I've been consistently running 'make hint' on the files I modify and progressively addressing all the warnings; the final objective is to be able to enable jshint by default on Travis which can help us catch all sorts of issues before we land.

> Warning, this has a different result than the original `enable == null`
> test, and it will never occur.

Ah, right, because `undefined == null` is true but `undefined === null` is not. I'm still a newbie when dealing with these JS subtleties :-) I'll check for undefined instead and log a warning as you suggested.

Comment 18

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Properly handle do-not-track options when upgrading from 1.1

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Properly handle do-not-track options when upgrading from 1.1

Comment 20

[Fabien Cazenave [:kaze]]

Comment on attachment 8348692 [details] [diff] [review]
[PATCH v2] Properly handle do-not-track options when upgrading from 1.1

Thanks for the quick update. LGTM but Travis is red:

> test_enable_do_not_track_via_settings_app (test_settings_do_not_track.TestSettingsDoNotTrack)
> Enable do not track via the Settings app ... FAIL
> ======================================================================
> FAIL: None
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.7.1-py2.7.egg/marionette/marionette_test.py", line 143, in run
> testMethod()
> File "/home/travis/build/mozilla-b2g/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/settings/test_settings_do_not_track.py", line 29, in test_enable_do_not_track_via_settings_app
> self.assertEqual(self.data_layer.get_bool_pref('privacy.donottrackheader.enabled'), True)
> TEST-UNEXPECTED-FAIL | test_settings_do_not_track.py test_settings_do_not_track.TestSettingsDoNotTrack.test_enable_do_not_track_via_settings_app | AssertionError: False != True
> ----------------------------------------------------------------------
> Ran 1 test in 10.288s
> FAILED (failures=1)

Naive and unrelated question: why do you submit a text patch *and* a github PR? Wouldn’t the latter be enough?

(In reply to Gabriele Svelto [:gsvelto] from comment #17)
> because `undefined == null` is true but `undefined === null` is not

Aren’t we living in a wonderful world? :-)

Comment 21

[Gabriele Svelto [:gsvelto]]

(In reply to Fabien Cazenave [:kaze] from comment #20)
> Thanks for the quick update. LGTM but Travis is red:
> 
> > test_enable_do_not_track_via_settings_app (test_settings_do_not_track.TestSettingsDoNotTrack)
> > Enable do not track via the Settings app ... FAIL
> > ======================================================================
> > FAIL: None
> > ----------------------------------------------------------------------
> > Traceback (most recent call last):
> > File "/usr/local/lib/python2.7/dist-packages/marionette_client-0.7.1-py2.7.egg/marionette/marionette_test.py", line 143, in run
> > testMethod()
> > File "/home/travis/build/mozilla-b2g/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/settings/test_settings_do_not_track.py", line 29, in test_enable_do_not_track_via_settings_app
> > self.assertEqual(self.data_layer.get_bool_pref('privacy.donottrackheader.enabled'), True)
> > TEST-UNEXPECTED-FAIL | test_settings_do_not_track.py test_settings_do_not_track.TestSettingsDoNotTrack.test_enable_do_not_track_via_settings_app | AssertionError: False != True
> > ----------------------------------------------------------------------
> > Ran 1 test in 10.288s
> > FAILED (failures=1)

I'll see if I can reproduce this locally.

> Naive and unrelated question: why do you submit a text patch *and* a github
> PR? Wouldn’t the latter be enough?

Yeah but I prefer to have the patch for review so all the review comments are on Bugzilla instead of Github (as well as the final patch) and then use the PR for Travis.

Comment 22

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] Properly handle do-not-track options when upgrading from 1.1

Here's the updated patch with the test breakage fixed, I had made a completely silly mistake in the settings cset constructor I assumed that this:

var kEnabledKey = "privacy.donottrackheader.enabled"

{ kEnabledKey: enabled }

Would yield

{ "privacy.donottrackheader.enabled": enabled }

Which it quite obviously doesn't :-)

Comment 23

[Fabien Cazenave [:kaze]]

I confess I fell into this one once, too. :-)

Comment 24

[Fabien Cazenave [:kaze]]

Comment on attachment 8348838 [details] [diff] [review]
[PATCH v3] Properly handle do-not-track options when upgrading from 1.1

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

LGTM

::: apps/settings/js/do_not_track.js
@@ +67,3 @@
>  
>      var lock = settings.createLock();
> +    var cset = { 'privacy.donottrackheader.enabled': enabled };

Nitpick, in such cases we often use that:

  var cset = {};
  cset[kEnabledKey] = enabled;

Comment 25

[Fabien Cazenave [:kaze]]

(In reply to Gabriele Svelto [:gsvelto] from comment #21)
> (In reply to Fabien Cazenave [:kaze] from comment #20)
> > Naive and unrelated question: why do you submit a text patch *and* a github
> > PR? Wouldn’t the latter be enough?
> 
> Yeah but I prefer to have the patch for review so all the review comments
> are on Bugzilla instead of Github (as well as the final patch) and then use
> the PR for Travis.

I see. On the other hand, addressing comments with successive commits on a single PR makes it much easier to review, especially for big patches (assuming that all commits are properly squashed before being merged).

I’ll start a discussion about that on m.d.gaia.

Comment 26

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v4] Properly handle do-not-track options when upgrading from 1.1 r=kaze

(In reply to Fabien Cazenave [:kaze] from comment #24)
> ::: apps/settings/js/do_not_track.js
> @@ +67,3 @@
> >  
> >      var lock = settings.createLock();
> > +    var cset = { 'privacy.donottrackheader.enabled': enabled };
> 
> Nitpick, in such cases we often use that:
> 
>   var cset = {};
>   cset[kEnabledKey] = enabled;

Thanks! Here's a final patch with this bit addressed, I'm carrying over the review flag. I'll wait for Travis to turn green and then land on master / v1.2 and v1.3. It applies cleanly on all branches fortunately.

Comment 27

[Gabriele Svelto [:gsvelto]]

Pushed to:

master  6df05c061f9815384ae9b1c8465e1e570c08c7af
v1.2    c82c957e9d4078cd4043de6b7d21a2667a73adf7
v1.3    50d3c9e24e4b7f218ea5d1aa93f7005240acac2c