1075160 - Support action: reset a pref to the default setting

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Benjamin Smedberg]

Implement a support action which takes an internal pref name and resets that pref to the factory default.

Note that this intentionally will not support setting prefs to an arbitrary value.

Comment 1

[André Reinald]

I'm not sure what a "support action" means, but isn't this what we want?
https://mxr.mozilla.org/mozilla-central/source/modules/libpref/prefapi.h#140

This API is exposed in JS, and even used in some places throughout our code.

Comment 2

[Benjamin Smedberg]

Have you read the context in bug 1031506 and the google doc specification? This bug is specifically about exposing this functionality through the support API, MozSelfSupport.webidl

Comment 3

[André Reinald]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Have you read the context in bug 1031506 and the google doc specification?
> This bug is specifically about exposing this functionality through the
> support API, MozSelfSupport.webidl

Yes, I read both. Though I more or less understand the purpose of webidl, this is something I still have to learn about. For instance I am wondering if it's possible/enough to create an identifier in MozSelfSupport.webidl and connect it with the existing C PREF_ClearUserPref() function?

Comment 4

[Benjamin Smedberg]

You'd be connecting it with Preferences::ClearUser (or the JS equivalent of that, since this is implemented in JS). But yes.

Comment 5

[André Reinald]

Attachment: bugzilla-1075160-1.patch

WIP

Comment 6

[André Reinald]

Attachment: bugzilla-1075160-2.patch

WIP

Comment 7

[André Reinald]

Attachment: bugzilla-1075160-3.patch

WIP

Comment 8

[André Reinald]

Attachment: bugzilla-1075160-4.patch

Test OK, but considering still WIP.

Comment 9

[André Reinald]

Attachment: bugzilla-1075160-5.patch

Comment 10

[André Reinald]

Launched try build/test :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b457e3ddb98

Comment 11

[André Reinald]

Try closed during above build because of bug 1147853.
Launching again :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f32aa9d15d7

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8582379 [details] [diff] [review]
bugzilla-1075160-5.patch

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

::: browser/components/selfsupport/test/browser_1075160.js
@@ +1,1 @@
> +const PREF_TO_TEST = "security.sandbox.macos.content";

Lets give this test a more descriptive name, maybe browser_selfsupportAPI.js?

@@ +1,3 @@
> +const PREF_TO_TEST = "security.sandbox.macos.content";
> +const PREF_NEW_VALUE = 3;
> +const PREF_UNDEF_VALUE = -1;

You don't use the constants outside of the test() function, can we just move them into it?

@@ +2,5 @@
> +const PREF_NEW_VALUE = 3;
> +const PREF_UNDEF_VALUE = -1;
> +
> +function test() {
> +  let level_original = PREF_UNDEF_VALUE;

We use camelCase.
If you use Preferences.jsm you avoid the try-catch juggle:
let originalLevel = Preferences.get(<prefName>, <fallbackValue>);

We should make sure to always reset the pref using: registerCleanupFunction(() => ...

I also think we need to cover more cases here, see the list in the webidl comment.

::: browser/experiments/Experiments.jsm
@@ +320,5 @@
>        let payload = yield reporter.collectAndObtainJSONPayload();
>        return payload;
>      });
>    },
> +  

Unneeded whitespace change.

::: dom/webidl/MozSelfSupport.webidl
@@ +38,5 @@
>     * @return Promise<Object>
>     *         Resolved when the FHR payload data has been collected.
>     */
>    Promise<object> getHealthReportPayload();
> +  

Nit: Trailing whitespace.

@@ +39,5 @@
>     *         Resolved when the FHR payload data has been collected.
>     */
>    Promise<object> getHealthReportPayload();
> +  
> +  void clearUserPref(DOMString name);

Add documentation like the above example.
What is the expected behavior, can this throw?

I assume that we need to handle this being called for:
* prefs that we ship a default for, which may be user-set or not
* prefs that were added as a user pref
* prefs that don't exist

Comment 13

[André Reinald]

Attachment: bugzilla-1075160-6.patch

Georg, I think I replied to all your comments on my previous patch.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8590745 [details] [diff] [review]
bugzilla-1075160-6.patch

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

::: browser/components/selfsupport/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  EXTRA_COMPONENTS += [
>      'SelfSupportService.js',
> +    'SelfSupportService.manifest'

This change is unneeded?
It seems actually nice to use trailing commas for less diff noise when adding entries, any issue with that?

::: browser/components/selfsupport/test/browser_selfsupportAPI.js
@@ +5,5 @@
> +  const PREF_NEW_VALUE = 3;
> +  const PREF_UNDEF_VALUE = -1;
> +
> +  registerCleanupFunction(function() {
> +    Services.prefs.clearUserPref(PREF_TO_TEST);

In general we can't use clearUserPref() here.
The test harness may have set the pref to a specific value, which will be a user-set value.
We have to reset it to the value it had at the test-start (or reset if it was not present).

@@ +7,5 @@
> +
> +  registerCleanupFunction(function() {
> +    Services.prefs.clearUserPref(PREF_TO_TEST);
> +  });
> +  let level_original = Preferences.get(PREF_TO_TEST, PREF_UNDEF_VALUE);

Nit: camelCase here and below please.

For proper test-coverage we should specifically test the cases mentioned before:
* prefs that we ship a default for (which are not user-test, we can just assert that)
* prefs that were added as a user pref (we can use a test-specific pref here)
* prefs that don't exist (some test-specific pref-name too)

Comments on what we're testing for in which section would help with test readability and maintenance.

::: dom/webidl/MozSelfSupport.webidl
@@ +43,5 @@
> +  /**
> +   * Resets a named pref to its original value.
> +   *
> +   * Refer to nsPrefBranch::ClearUserPref in
> +   * source/modules/libpref/nsPrefBranch.cpp for more information.

We shouldn't point to the implementation for the doc comment (the implementation may change, but the interface behavior should stay consistent).
Can you specify this methods behavior in the documentation here?

I'd also add a @param and documentation.

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #14)
> For proper test-coverage we should specifically test the cases mentioned
> before:
> * prefs that we ship a default for (which are not user-test, we can just
> assert that)

... "not user-set" is what i mean't. I think "extensions.hotfix.id" looks like a good candidate.
We can just Assert for prefHasUserValue() being false to be sure.

Comment 16

[André Reinald]

Attachment: bugzilla-1075160-7.patch

The pref I test has a default value of 1 in a real browser, as per bug 1083344. But in the test environment it is undefined. I'm fine adding another pref to test (for the case it IS set in the test environment).

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8591663 [details] [diff] [review]
bugzilla-1075160-7.patch

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

::: browser/components/selfsupport/test/browser_selfsupportAPI.js
@@ +6,5 @@
> +  const prefValueUndefined = -1;
> +
> +  let prefValueOriginal = Preferences.get(prefToTest, prefValueUndefined);
> +  registerCleanupFunction(function() {
> +    if (prefValueOriginal == prefValueUndefined)

We should find a pref that is expected to have a default value.
Then we can just do something like (adding some comments so the test gets easier to read):
Assert.ok(Preferences.has(DEFAULT_TEST_PREF));
registerCleanupFunction(() => /* reset pref */);

// Test that resetting default pref doesn't change its value
...

// Test that resetting that pref after it was user-set works.
...

// Test resetting with a user-set pref that has no default value.
...

// Test resetting a non-existant pref.

Comment 18

[André Reinald]

Attachment: bugzilla-1075160-8.patch

Factored the test function and call it:
1. for a pref which is set (found "browser.search.log" to be set in mochitest environment)
2. for a pref which is unset

Added comments to ease maintenance.

Comment 19

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8592182 [details] [diff] [review]
bugzilla-1075160-8.patch

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

In general this looks much clearer now thanks to the comments and structuring.
I think we still need to work on covering the different test cases though.

::: browser/components/selfsupport/test/browser_selfsupportAPI.js
@@ +9,5 @@
> +  // Record the original value
> +  let prefValueOriginal = Preferences.get(prefToTest, prefValueUndefined);
> +
> +  // Register cleanup to restore orignal value
> +  registerCleanupFunction(function() {

Let's move this and the preceding lines out of this helper function.
If we keep it in the top-level function we can actually assert that the pref is set as expected and avoid the conditional juggles.
I believe that is much clearer to read than factoring it out (it's just a test, so clarity should trump).

@@ +31,5 @@
> +  let prefValueCleared = Preferences.get(prefToTest, prefValueUndefined);
> +  Assert.equal(prefValueCleared, prefValueOriginal, "failed to reset pref value: " + prefToTest);
> +}
> +
> +function test() {

So far this test only covers two of the four cases mentioned in comment 17.
I think we can't factor out all the four cases in one common helper function, so it's probably easier to just write things out in this test function.

@@ +33,5 @@
> +}
> +
> +function test() {
> +  // Test changing a pref which is set in the mochitest context
> +  testOnePref(true, "browser.search.log", true);

Is this a pref that has a default value and no user-set value?
If yes let's assert that as suggested in comment 17 (in case someone changes that in the future we want clear failures).

@@ +36,5 @@
> +  // Test changing a pref which is set in the mochitest context
> +  testOnePref(true, "browser.search.log", true);
> +
> +  // Test setting a pref which is unset in the mochitest context
> +  testOnePref(false, "thisPrefDoesntExist", 50);

I don't think that we want to test setting a non-existing pref to a value.
We want to test that calling MozSelfSupport.clearUserPref() makes no changes at all for a pref that doesn't exist, i.e.:
* check pref doesnt exist
* clearUserPref
* check pref still doesnt exist

Comment 20

[André Reinald]

Ok, I see your point.

I still think I can and should factor to some degree in order to ease reading.
Basically I can cover the 4 cases you talk about in comment 17 by
1. check pref exists (or doesn't exist)
2. call clearUserPref
3. check we're consistent with 1
4. change (or set) the pref to a new value
5. call clearUserPref
6. check we're consistent with 1

And to the above with:
1. a pref which has a default value
2. a pref which has no default value

Is my understanding right?

Comment 21

[Georg Fritzsche [:gfritzsche]]

(In reply to André Reinald from comment #20)
> I still think I can and should factor to some degree in order to ease
> reading.
> Basically I can cover the 4 cases you talk about in comment 17 by
> 1. check pref exists (or doesn't exist)
> 2. call clearUserPref
> 3. check we're consistent with 1
> 4. change (or set) the pref to a new value
> 5. call clearUserPref
> 6. check we're consistent with 1
> 
> And to the above with:
> 1. a pref which has a default value
> 2. a pref which has no default value

You don't need 1-3 for the pref with no default value and 6 will differ.
I'd personally rather see less conditionals for this (pretty short) test so it's easier to understand from logs what happened.
If we can do that and also factor it out, nice, otherwise it seems simpler to just write it out here.

Comment 22

[André Reinald]

Attachment: bugzilla-1075160-9.patch

WIP

In browser_selfsupportAPI.js, assertions on lines 57 and 59 fail.

My understanding is that a deleted pref is not restored by calling Services.prefs.clearUserPref()

Is there something we can do about this?

Comment 23

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8592354 [details] [diff] [review]
bugzilla-1075160-9.patch

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

::: browser/components/selfsupport/test/browser_selfsupportAPI.js
@@ +2,5 @@
> +
> +function test() {
> +  const prefValueUndefined = -1;
> +
> +  let prefNewName = "browser.newpref.fake";

Let's make that `const prefNewName`.

@@ +3,5 @@
> +function test() {
> +  const prefValueUndefined = -1;
> +
> +  let prefNewName = "browser.newpref.fake";
> +  Assert.ok(!Preferences.has(prefNewName));

A general point: Lets also add the additional parameter with a description here and below (e.g. "Should not have that pref yet")

@@ +4,5 @@
> +  const prefValueUndefined = -1;
> +
> +  let prefNewName = "browser.newpref.fake";
> +  Assert.ok(!Preferences.has(prefNewName));
> +  

Nit: Trailing whitespace.

@@ +5,5 @@
> +
> +  let prefNewName = "browser.newpref.fake";
> +  Assert.ok(!Preferences.has(prefNewName));
> +  
> +  let prefExistingName = "extensions.hotfix.id";

const

@@ +6,5 @@
> +  let prefNewName = "browser.newpref.fake";
> +  Assert.ok(!Preferences.has(prefNewName));
> +  
> +  let prefExistingName = "extensions.hotfix.id";
> +  Assert.ok(Preferences.has(prefExistingName));

Let's also assert that it is not user-set, i.e. `!Preference.isSet()`

@@ +7,5 @@
> +  Assert.ok(!Preferences.has(prefNewName));
> +  
> +  let prefExistingName = "extensions.hotfix.id";
> +  Assert.ok(Preferences.has(prefExistingName));
> +  let prefExistingOriginal = Preferences.get(prefExistingName, prefValueUndefined);

We made sure that the pref exists, so we can get rid of prefValueUndefined.
prefExistingOriginalValue might be more clear?

@@ +11,5 @@
> +  let prefExistingOriginal = Preferences.get(prefExistingName, prefValueUndefined);
> +
> +  registerCleanupFunction(function() {
> +    Preferences.set(prefExistingName, prefExistingOriginal);
> +    Services.prefs.deleteBranch(prefNewName);

clearUserPref should be enough?

@@ +18,5 @@
> +  // 1. do nothing on an inexistent pref
> +  MozSelfSupport.clearUserPref(prefNewName);
> +  Assert.ok(!Preferences.has(prefNewName));
> +
> +

Nit: Only one newline. Dito the ones below.

@@ +23,5 @@
> +  // 2. creation of a new pref
> +  Preferences.set(prefNewName, 10);
> +  Assert.ok(Preferences.has(prefNewName));
> +  let prefValueModified = Preferences.get(prefNewName, prefValueUndefined);
> +  Assert.equal(prefValueModified, 10, "failed to set new pref value");

We just set it, so i don't think we need to test the existence and the value here.
(The pref system itself should have test coverage elsewhere)

@@ +33,5 @@
> +  // 3. do nothing on an unchanged existing pref
> +  MozSelfSupport.clearUserPref(prefExistingName);
> +  Assert.ok(Preferences.has(prefExistingName));
> +  let prefValueCleared = Preferences.get(prefExistingName, prefValueUndefined);
> +  Assert.equal(prefValueCleared, prefExistingOriginal, "failed to reset to existing value");

We already assert that it exists, so we could just shorten to:
Assert.equal(Preferences.get(prefExistingName), prefExistingOriginal, ...

@@ +40,5 @@
> +  // 4. change the value of an existing pref
> +  Preferences.set(prefExistingName, "[e-mail]");
> +  Assert.ok(Preferences.has(prefExistingName));
> +  prefValueModified = Preferences.get(prefExistingName, prefValueUndefined);
> +  Assert.equal(prefValueModified, "[e-mail]", "failed to set existing pref value");

Same here, i think we don't need the checks after setting the pref.

@@ +45,5 @@
> +
> +  MozSelfSupport.clearUserPref(prefExistingName);
> +  Assert.ok(Preferences.has(prefExistingName));
> +  prefValueCleared = Preferences.get(prefExistingName, prefValueUndefined);
> +  Assert.equal(prefValueCleared, prefExistingOriginal, "failed to reset to existing value");

`Assert.equal(Preferences.get(prefExistingName), ...` ?

@@ +48,5 @@
> +  prefValueCleared = Preferences.get(prefExistingName, prefValueUndefined);
> +  Assert.equal(prefValueCleared, prefExistingOriginal, "failed to reset to existing value");
> +
> +
> +  // 5. delete an existing pref

Per IRC, lets drop this part.

Comment 24

[Georg Fritzsche [:gfritzsche]]

One last point:
I think MozSelfSupport.resetPref() would be better than MozSelfSupport.clearUserPref().

Comment 25

[André Reinald]

Attachment: bugzilla-1075160-10.patch

I commented out the "case 5".
I still think it would make sense to have it covered by clearUserPref().
But maybe not worth opening a bug?

Comment 26

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8592761 [details] [diff] [review]
bugzilla-1075160-10.patch

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

This looks close now. We only have minor points now and need to clear up the question on test-case #5.

::: browser/components/selfsupport/test/browser_selfsupportAPI.js
@@ +3,5 @@
> +function test() {
> +  const prefValueUndefined = -1;
> +
> +  const prefNewName = "browser.newpref.fake";
> +  Assert.ok(!Preferences.has(prefNewName), "pref should not be defined");

"pref should not exist"

@@ +6,5 @@
> +  const prefNewName = "browser.newpref.fake";
> +  Assert.ok(!Preferences.has(prefNewName), "pref should not be defined");
> +
> +  const prefExistingName = "extensions.hotfix.id";
> +  Assert.ok(Preferences.has(prefExistingName), "pref should be defined");

"pref should not exist"

@@ +7,5 @@
> +  Assert.ok(!Preferences.has(prefNewName), "pref should not be defined");
> +
> +  const prefExistingName = "extensions.hotfix.id";
> +  Assert.ok(Preferences.has(prefExistingName), "pref should be defined");
> +  Assert.ok(!Preferences.isSet(prefExistingName), "pref should not be set");

Nit: "pref should not be user-set"

@@ +17,5 @@
> +  });
> +
> +  // 1. do nothing on an inexistent pref
> +  MozSelfSupport.clearUserPref(prefNewName);
> +  Assert.ok(!Preferences.has(prefNewName), "pref should not be defined");

"pref should still not exist"?
And so on with s/defined/exist/ for the assert messages below?

@@ +19,5 @@
> +  // 1. do nothing on an inexistent pref
> +  MozSelfSupport.clearUserPref(prefNewName);
> +  Assert.ok(!Preferences.has(prefNewName), "pref should not be defined");
> +
> +

You still have 2 empty lines here and below; one is enough.

@@ +23,5 @@
> +
> +  // 2. creation of a new pref
> +  Preferences.set(prefNewName, 10);
> +  Assert.ok(Preferences.has(prefNewName), "pref should be defined");
> +  Assert.equal(Preferences.get(prefNewName), 10, "failed to set new pref value");

We have the assert message describe the expected state, not failure messages.
Same for the "failed" messages below.

@@ +32,5 @@
> +
> +  // 3. do nothing on an unchanged existing pref
> +  MozSelfSupport.clearUserPref(prefExistingName);
> +  Assert.ok(Preferences.has(prefExistingName), "pref should be defined");
> +  Assert.equal(Preferences.get(prefExistingName), prefExistingOriginalValue, "failed to reset to existing value");

"should still have the same value"?

::: dom/webidl/MozSelfSupport.webidl
@@ +42,5 @@
> +
> +  /**
> +   * Resets a named pref:
> +   * - to its default value if it has one,
> +   * - delete the named pref it it has no default value.

This should still have @param documentation.
Also, it should document whether it can throw.

@@ +44,5 @@
> +   * Resets a named pref:
> +   * - to its default value if it has one,
> +   * - delete the named pref it it has no default value.
> +   */
> +  void clearUserPref(DOMString name);

It is not a very big deal, but i now think that resetPref() is a better name.

Comment 27

[Georg Fritzsche [:gfritzsche]]

(In reply to André Reinald from comment #25)
> I still think it would make sense to have it covered by clearUserPref().
> But maybe not worth opening a bug?

The open question here is on test-case #5 in the browser test.
clearUserPref() doesn't reset a Services.prefs.deleteBranch(defaultPref) - do we care for the purposes of this API?

Comment 28

[André Reinald]

Attachment: bugzilla-1075160-11.patch

Comment 29

[Benjamin Smedberg]

I can't see why NEEDINFO was set for me. Is there a question I need to answer here?

Comment 30

[André Reinald]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #29)
> I can't see why NEEDINFO was set for me. Is there a question I need to
> answer here?

Yes, question is stated in comment 27.

Comment 31

[Benjamin Smedberg]

I still don't understand what you're asking me to decide.

Comment 32

[Georg Fritzsche [:gfritzsche]]

If you delete a default pref via say Services.prefs.deleteBranch(), the implementation here (using clearUserPref()), doesn't restore it until browser restart.
Is that ok for the API behavior?
If not, what do you know how to properly restore those prefs?

Comment 33

[Benjamin Smedberg]

That sounds like a footgun API. I think we should probably remove that API (or make it reset a branch instead of removing it entirely), but I don't think it needs to be important for this API.

Comment 34

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8592777 [details] [diff] [review]
bugzilla-1075160-11.patch

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

This looks pretty good now, we should do a try-run on this.

::: browser/components/selfsupport/test/browser_selfsupportAPI.js
@@ +41,5 @@
> +  MozSelfSupport.resetPref(prefExistingName);
> +  Assert.ok(Preferences.has(prefExistingName), "pref should still exist");
> +  Assert.equal(Preferences.get(prefExistingName), prefExistingOriginalValue, "pref value should be the same as original");
> +
> +  // 5. delete an existing pref

Let's remove this per the preceding discussion.

::: dom/webidl/MozSelfSupport.webidl
@@ +42,5 @@
> +
> +  /**
> +   * Resets a named pref:
> +   * - to its default value if it has one,
> +   * - delete the named pref it it has no default value.

Does this throw? Please document this.

Comment 35

[André Reinald]

Attachment: bugzilla-1075160-12.patch

Comment 36

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8593363 [details] [diff] [review]
bugzilla-1075160-12.patch

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

This looks good to me now.
One minor fix needed below and we should check that try results come back green.
After that this should be good to go.

::: browser/components/selfsupport/test/browser_selfsupportAPI.js
@@ +1,4 @@
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +
> +function test() {
> +  const prefValueUndefined = -1;

This is unused, let's remove that line.

Comment 37

[André Reinald]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ec347bd9a22

Comment 38

[André Reinald]

Attachment: bugzilla-1075160-13.patch

Removed prefValueUndefined. Not relaunching another try after that minor change.

Comment 39

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

This needs review from a DOM peer for the webidl change.

Comment 40

[André Reinald]

Georg, anyone to suggest for the webidl change?

Comment 41

[André Reinald]

Comment on attachment 8593390 [details] [diff] [review]
bugzilla-1075160-13.patch

Asking for review for the webidl change. Thanks.

Comment 42

[Bobby Holley (:bholley)]

Comment on attachment 8593390 [details] [diff] [review]
bugzilla-1075160-13.patch

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

Seems fine given that this is [ChromeOnly], but we'll need a proper security review if we ever expose this to other things, as bsmedberg tells me we plan to do.

Comment 43

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

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

Comment 44

[Carsten Book [:Tomcat]]

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