1460675 - Clear old social prefs from profiles

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, enhancement)

Description

[:Felipe Gomes (needinfo for replies!)]

My years-old prefs.js file has ~61kB, and around 9.5kB (15%) of it is taken by just two prefs (which includes a data: url for an image) related to the old social providers.

social.manifest.www-facebook.com
social.manifest.www-goal.com

and there's also social.activeProviders, which is shorter, but if we're clearing, we might as well clear them too.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

I'm happy to mentor someone on this bug! To implement it, the Services.prefs.getBranch() to get all "social." prefs, and then use .getChildList to iterate through all of them, calling clearUserPref on them.

This should probably be done as a one-time action that can be added in the _migrateUI function in the file nsBrowserGlue.js

Comment 2

[Himanshuteli]

I want to contribute and solve this issue. Can you please guide me how to do that as i am new to contributing to open source.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Hi there! Which programming languages do you have experience with? And have you used Mercurial or Git before?

To get started you'll first need to download Firefox's source code and compile it. There's a helpful guide here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Once you get that working, I can guide you with the details of our code base and how to write and test your changes.

Comment 4

[Himanshuteli]

I have experience with python and some javascript and have used git.

Comment 5

[Himanshuteli]

As instructed I downloaded Firefox's source code and compiled it. What i have to do now?

Comment 6

[bobslept]

I would love to follow how this is done.

If I'm correct, we need to add the functionality in the _migrateUI function.
Located in mozilla-central/browser/components/nsBrowserGlue.js.

Totally new to this, my first shot would be to add something like this (not tested, based on looking at the code):

let socialPrefs = Services.prefs.getBranch("social.");
if (socialPrefs) {
    let socialPrefsArray = socialPrefs.getChildList("");
    for (let item of socialPrefsArray) {
        Services.prefs.clearUserPref(item);
    }
}

Let's wait for Felipe for more info.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

(In reply to bobslept from comment #6)
> I would love to follow how this is done.
> 
> If I'm correct, we need to add the functionality in the _migrateUI function.
> Located in mozilla-central/browser/components/nsBrowserGlue.js.
> 
> Totally new to this, my first shot would be to add something like this (not
> tested, based on looking at the code):
> 
> let socialPrefs = Services.prefs.getBranch("social.");
> if (socialPrefs) {
>     let socialPrefsArray = socialPrefs.getChildList("");
>     for (let item of socialPrefsArray) {
>         Services.prefs.clearUserPref(item);
>     }
> }
> 
> Let's wait for Felipe for more info.

Yeah, this is exactly it.

This code should be added at the end of the _migrateUI function in nsBrowserGlue.js. You'll notice that this function has a counter at the beginning (UI_VERSION) which is used to make sure each piece only ever runs once for a user profile.

This counter should be incremented and this code should be added in a new `if` block based on the new value of UI_VERSION

Comment 8

[bobslept]

Attachment: 1460675-clear-old-social-prefs.patch

Alright, I hope this patch is correct. I have bumped UI_VERSION and added the new check to clear the old prefs. It builds and runs alright. It passes |./mach mochitest -f browser browser/components/tests/|. Do we need to create a new testcase for this? If so, I need some help on that.

Comment 9

[Himanshuteli]

Can you please guide me with the details of the code base and how to write and test changes.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

(In reply to bobslept from comment #8)
> Created attachment 8975457 [details] [diff] [review]
> 1460675-clear-old-social-prefs.patch
> 
> Alright, I hope this patch is correct. I have bumped UI_VERSION and added
> the new check to clear the old prefs. It builds and runs alright. It passes
> |./mach mochitest -f browser browser/components/tests/|. Do we need to
> create a new testcase for this? If so, I need some help on that.

Yeah. Unfortunately _migrateUI is not well covered by tests, but there is one testcase testing the specific migration step 41 (to clear some old loop prefs).

You can take a look at it here: https://searchfox.org/mozilla-central/source/browser/components/tests/unit/test_browserGlue_migration_loop_cleanup.js

You can just create a new test file in that same folder, copy this one and make it work similarly: first artificially create some of these prefs to be cleared, then send the notification that will force _migrateUI to run, and then check that the prefs were in fact cleared.

Note that you'll need to add your new file to this list: https://searchfox.org/mozilla-central/source/browser/components/tests/unit/xpcshell.ini

Comment 11

[bobslept]

Attachment: 1460675-clear-old-social-prefs.patch

Well after writing the test, it didn't get passed the test. I have now reset the branch like case 41. Now it is passing the test, but is it correct this way? Tested with |./mach xpcshell-test browser/components/tests/|.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Sorry, I should have provided some more hints.. What happened to the test that it didn't pass?

I imagine it was because the call to getBoolPref was throwing an error. It happened because when your code calls .clearUserPref, that pref becomes non-existent, and then get*Pref throws for non-existing prefs.

Note: you should use setStringPref in your test instead of setBoolPref to make it closer to reality.

I prefer that you go back to the original implementation, and fix the test. There are two ways to solve this:

1 - you can provide a second argument to getStringPref that specifies a default value to be used when the pref doesn't exist (instead of throwing an error).

2 - Instead of calling getStringPref, you can call Services.prefs.prefHasUserValue to verify if a pref exists.


I suggest using 2).  In summary: set the prefs, call _migrateUI, check that the prefs don't exist anymore.

Also, a minor nit: do not use facebook.com in the test. Let's replace it with example.com

Comment 13

[:Felipe Gomes (needinfo for replies!)]

(In reply to Himanshuteli from comment #9)
> Can you please guide me with the details of the code base and how to write
> and test changes.

Hello! Here's a page that should help in your next steps: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

Specially the section "Step 3, Fix the bug". Since Bob is almost finished in this bug, feel free to choose some other good first bug, and I can help you with it if there are no other mentors assigned!

Another good approach to learn where to start and how things work is to take a look at the patches posted here (and in other bugs marked as good first bugs that have already been resolved) to understand what their solutions look like.

And one extra tip: if you go to about:config and set the pref devtools.chrome.enabled to true, You can go to Developer Tools -> Browser Console and have an interactive JS command line which will allow you to call run code directly in the Firefox UI.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Felipe Gomes (needinfo me!) from comment #12)
> I prefer that you go back to the original implementation, and fix the test.
> There are two ways to solve this:

Actually, you could keep using resetBranch, but use it directly from Services.prefs instead of using Preferences.jsm (we want to deprecate Preferences.jsm in the future)

Comment 15

[bobslept]

Attachment: 1460675-clear-old-social-prefs.patch

Back to the old situation, but I can't get the test passed. It keeps saying that somehow the pref is still there. I just don't get it.

Comment 16

[bobslept]

Using Services.prefs.resetBranch results in Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIPrefBranch.resetBranch]. Using the 41 case will pass the test. Strange.

Comment 17

[bobslept]

If I use Services.prefs.deleteBranch("social."); it will pass the test. What do you think?

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Ah, I just figured out what was the problem with the original patch. The problem is that .getChildList returns an array that only contains the suffixes. You need to manually include "social." in the beginning of each pref name to correctly clear it.

Comment 19

[bobslept]

Aah that makes sense. Do you prefer the patch that way or should I use that one liner?

Comment 20

[:Felipe Gomes (needinfo for replies!)]

deleteBranch is not widely used in the codebase outside of tests, and the original approach, even though a bit more verbose, is more battle tested.. I'm just being extra cautious, but I prefer the original version.

When you post the final patch, please request review from Shane (:mixedpuppy) as he should be the final reviewer for this instead of me

Comment 21

[bobslept]

Attachment: 1460675-clear-old-social-prefs.patch

This should be the final one. I have incremented UI_VERSION and added a check to clear the old social prefs. I've also added a corresponding test file. Builds and runs alright. Also passes |./mach xpcshell-test browser/components/tests/|.

Comment 22

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00750569d17e9519e29cc728accfd5960ff65b43

Comment 23

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8975751 [details] [diff] [review]
1460675-clear-old-social-prefs.patch

I don't think the tests for socia. and sociall. are necessary.  Otherwise LGTM.  I kicked off a small try to validate before landing.  Thanks!

Comment 24

[bobslept]

Sorry for that. Do you want me to remove those lines?

Comment 25

[Shane Caraveo (:mixedpuppy)]

(In reply to bobslept from comment #24)
> Sorry for that. Do you want me to remove those lines?

Sure, lets do that.  Once it's up I'll mark it for checkin.  Thanks!

Comment 26

[Shane Caraveo (:mixedpuppy)]

oh, or you can just mark it for checkin

Comment 27

[bobslept]

Attachment: 1460675-clear-old-social-prefs.patch

Well I now removed those lines.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84ec88aaa796
Clear old social prefs from profiles

Comment 29

[:Felipe Gomes (needinfo for replies!)]

I checked this in! I forgot to advise you to add r=mixedpuppy to the commit message, and to check for this myself before pushing, but that's ok.

Thanks Bob for your first patch! And also to Himanshuteli for volunteering on this bug too.

Comment 30

[bobslept]

(In reply to :Felipe Gomes (needinfo me!) from comment #29)
> I checked this in! I forgot to advise you to add r=mixedpuppy to the commit
> message, and to check for this myself before pushing, but that's ok.
> 
> Thanks Bob for your first patch! And also to Himanshuteli for volunteering
> on this bug too.

Is it alright to edit the patch as comment to add before checkin? Because before commit i don't know who will review?

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Usually at the moment that you're attaching the final patch you also choose the reviewer, so we just include that in the patch before attaching.

Or by using MozReview that is done automatically

Comment 32

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/84ec88aaa796