Remove support for random_attributes, ua_strings, screen_res #745

[ankushduacodes]

Running tests to fix #745

[codecov]

Codecov Report

Merging #775 into master will decrease coverage by 0.58%.
The diff coverage is 12.50%.

@@            Coverage Diff             @@
##           master     #775      +/-   ##
==========================================
- Coverage   40.06%   39.48%   -0.59%     
==========================================
  Files          29       29              
  Lines        3157     3118      -39     
==========================================
- Hits         1265     1231      -34     
+ Misses       1892     1887       -5     

Impacted Files	Coverage Δ
automation/Commands/command_executor.py	19.23% <ø> (ø)
automation/Commands/profile_commands.py	15.62% <ø> (+0.81%)	⬆️
automation/DeployBrowsers/deploy_firefox.py	19.10% <0.00%> (+3.64%)	⬆️
automation/BrowserManager.py	48.82% <50.00%> (-0.20%)	⬇️
automation/CommandSequence.py	56.71% <0.00%> (-29.86%)	⬇️
automation/Commands/Types.py	68.25% <0.00%> (-11.12%)	⬇️
automation/TaskManager.py	76.34% <0.00%> (-1.27%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e989ce5...0414ade. Read the comment docs.

[vringar]

This PR is looking very good. But there are some little changes to be made.

[vringar]

You should also remove
https://github.com/mozilla/OpenWPM/blob/e989ce5f74c1737185c89a33bde4015b55df066a/automation/Commands/profile_commands.py#L13-L35
And the places where this code is called further down in the file.

[ankushduacodes]

You should also remove
https://github.com/mozilla/OpenWPM/blob/e989ce5f74c1737185c89a33bde4015b55df066a/automation/Commands/profile_commands.py#L13-L35

And the places where this code is called further down in the file.

Do you mean remove the comments? or the whole code snippet and its calls down the line?
if I need to remove the whole code snippet then, do I need to remove only these two functions and its calls? or is there other functions that I need to remove?

[vringar]

I mean the whole snippet and it's calls down the line as well as any references to browser_settings. browser_settings were only used to store ua_strings and screen_res and since we want to remove both, we don't need them anymore.
I don't think you need to remove any other functions but I might have missed something.

[ankushduacodes]

I mean the whole snippet and it's calls down the line as well as any references to browser_settings. browser_settings were only used to store ua_strings and screen_res and since we want to remove both, we don't need them anymore.
I don't think you need to remove any other functions but I might have missed something.

What do you suggest I do to remove browser_settings as in this case https://github.com/mozilla/OpenWPM/blob/e989ce5f74c1737185c89a33bde4015b55df066a/automation/Commands/profile_commands.py#L143-L190
browser_settings are returned here https://github.com/mozilla/OpenWPM/blob/e989ce5f74c1737185c89a33bde4015b55df066a/automation/DeployBrowsers/deploy_firefox.py#L66-L71
And here
https://github.com/mozilla/OpenWPM/blob/e989ce5f74c1737185c89a33bde4015b55df066a/automation/DeployBrowsers/deploy_firefox.py#L77-L82

[vringar]

What do you suggest I do to remove browser_settings as in this case
https://github.com/mozilla/OpenWPM/blob/e989ce5f74c1737185c89a33bde4015b55df066a/automation/Commands/profile_commands.py#L143-L190

Here I'd suggest deleting Lines 182 and 190

https://github.com/mozilla/OpenWPM/blob/e989ce5f74c1737185c89a33bde4015b55df066a/automation/DeployBrowsers/deploy_firefox.py#L66-L71

And here

https://github.com/mozilla/OpenWPM/blob/e989ce5f74c1737185c89a33bde4015b55df066a/automation/DeployBrowsers/deploy_firefox.py#L77-L82

Delete the assignment and keep the call.
So change it to

load_profile( 
     browser_profile_path, 
     manager_params, 
     browser_params, 
     browser_params["recovery_tar"], 
 ) 

And then go down in those functions to remove everything that mentions profile_settings

[ankushduacodes]

https://github.com/mozilla/OpenWPM/blob/e989ce5f74c1737185c89a33bde4015b55df066a/automation/Commands/profile_commands.py#L59-L140
this code snippet is unreachable due to the return statement above.
Do you want to also remove this code snippet or do you want to keep it?

[ankushduacodes]

@vringar Can you please briefly explain what exactly load_profile function is doing?

To my understanding we are using load_profile function to load browser_settings and return the browser_settings and assigning it to profile_settings which we want to get rid of. and we are also extracting the tar.gz file into browser_profile_folder directory.

what exactly does full_profile.tar.gz or profile.tar.gz includes, I mean what kind of information?

[vringar]

So this is off the top of my head and might not be completely accurate, but as far as I remember:

what exactly does full_profile.tar.gz or profile.tar.gz includes, I mean what kind of information?

These files contain a tarred Firefox Profile. This is where your cookies, history and settings are stored in one place.

We use Selenium to control Firefox. Selenium creates an empty profile folder in a temporary directory when we tell it to start a new Firefox instance.
But we want Firefox to use the profile we have had created somewhere else, so we ask it where that empty profile is and put the contents of the tar in there. This is what load_profile does.

Somehow we decided that we also needed to save some extra information for each browser which wasn't in the browser_params and not in the profile so we created browser_settings which contained things like screen size and just saved that right next to the tar.
But now we have changed our minds on this and have decided that the benefits of this aren't worth all of the complexity. So now we try to purge any usage of browser_settings to allow us to simplify load_profile even further in the future.

If any of this makes sense to you and you feel like you can shorten this a bit, please write this as a docstring on load_profile

[vringar]

Two small nitpicks but otherwise this looks good to me!

[vringar]

https://github.com/mozilla/OpenWPM/blob/e989ce5f74c1737185c89a33bde4015b55df066a/automation/Commands/profile_commands.py#L59-L140

this code snippet is unreachable due to the return statement above.
Do you want to also remove this code snippet or do you want to keep it?

Yes please keep this code around. We eventually want to re-enable profile dumping but didn't have the time yet.

[ankushduacodes]

So this is off the top of my head and might not be completely accurate, but as far as I remember:

what exactly does full_profile.tar.gz or profile.tar.gz includes, I mean what kind of information?

These files contain a tarred Firefox Profile. This is where your cookies, history and settings are stored in one place.

We use Selenium to control Firefox. Selenium creates an empty profile folder in a temporary directory when we tell it to start a new Firefox instance.
But we want Firefox to use the profile we have had created somewhere else, so we ask it where that empty profile is and put the contents of the tar in there. This is what load_profile does.

Somehow we decided that we also needed to save some extra information for each browser which wasn't in the browser_params and not in the profile so we created browser_settings which contained things like screen size and just saved that right next to the tar.
But now we have changed our minds on this and have decided that the benefits of this aren't worth all of the complexity. So now we try to purge any usage of browser_settings to allow us to simplify load_profile even further in the future.

If any of this makes sense to you and you feel like you can shorten this a bit, please write this as a docstring on load_profile

Thank you for explaining, I think now have a big picture of what goes on when we save cookies in the browser.
Does this apply when a real user is using firefox, Is that how the cookies are stored in the system?

[vringar]

Thank you for explaining, I think now have a big picture of what goes on when we save cookies in the browser.
Does this apply when a real user is using firefox, Is that how the cookies are stored in the system?

Cookies are always stored in your profile. However usually your Profile just stays in one place on the system and doesn't get tarred or copied somewhere else.

[vringar]

Do you feel like there is anything else you'd want to do as part of this change?
Otherwise I'd be willing to merge it rn.

[ankushduacodes]

Do you feel like there is anything else you'd want to do as part of this change?
Otherwise I'd be willing to merge it rn.

I think I have made all the necessary changes. I think its good to go!...

[vringar]

Then thank you very much for following through on this!