Clear old social prefs from profiles

RESOLVED FIXED in Firefox 62

Status

enhancement
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: Felipe, Assigned: bobslept, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 62

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [fxperf:p3][lang=js])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Comment 1

a year ago
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

a year ago
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.
(Reporter)

Comment 3

a year ago
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

a year ago
I have experience with python and some javascript and have used git.

Comment 5

a year ago
As instructed I downloaded Firefox's source code and compiled it. What i have to do now?
(Assignee)

Comment 6

a year ago
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.
(Assignee)

Updated

a year ago
Flags: needinfo?(felipc)
(Reporter)

Comment 7

a year ago
(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
Flags: needinfo?(felipc)
(Assignee)

Comment 8

a year ago
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

a year ago
Can you please guide me with the details of the code base and how to write and test changes.
Flags: needinfo?(felipc)
(Reporter)

Comment 10

a year ago
(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
Assignee: nobody → bobslept
Status: NEW → ASSIGNED
(Assignee)

Comment 11

a year ago
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/|.
Attachment #8975457 - Attachment is obsolete: true
(Reporter)

Comment 12

a year ago
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
(Reporter)

Comment 13

a year ago
(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.
Flags: needinfo?(felipc)
(Reporter)

Comment 14

a year ago
(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)
(Assignee)

Comment 15

a year ago
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.
Attachment #8975555 - Attachment is obsolete: true
(Assignee)

Comment 16

a year ago
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.
(Assignee)

Comment 17

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

Comment 18

11 months ago
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.
(Assignee)

Comment 19

11 months ago
Aah that makes sense. Do you prefer the patch that way or should I use that one liner?
(Reporter)

Comment 20

11 months ago
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
(Assignee)

Comment 21

11 months ago
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/|.
Attachment #8975582 - Attachment is obsolete: true
Attachment #8975751 - Flags: review?(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!
Attachment #8975751 - Flags: review?(mixedpuppy) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
(Assignee)

Comment 24

11 months ago
Sorry for that. Do you want me to remove those lines?
(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!
Flags: needinfo?(bobslept)
oh, or you can just mark it for checkin
(Assignee)

Comment 27

11 months ago
Well I now removed those lines.
Attachment #8975751 - Attachment is obsolete: true
(Assignee)

Updated

11 months ago
Flags: needinfo?(bobslept)
Attachment #8975840 - Flags: review?(mixedpuppy)
Attachment #8975840 - Flags: review?(mixedpuppy) → review+

Comment 28

11 months ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84ec88aaa796
Clear old social prefs from profiles
Keywords: checkin-needed
(Reporter)

Comment 29

11 months ago
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.
(Assignee)

Comment 30

11 months ago
(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?
(Reporter)

Comment 31

11 months ago
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/84ec88aaa796
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62

Updated

2 months ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.