Restore stateful crawling support #864

boolean5 · 2021-03-03T13:02:39Z

This PR restores support for stateful crawling by addressing the core issue that broke it, i.e., the fact that geckodriver would delete the browser profile when closing or crashing before we had a chance to archive it. It does so by using a custom profile instead of creating the profile via the FirefoxProfile Selenium class. The custom profile is set via the arguments of the Options class.

Note:
Geckodriver has a bug that makes it write the browser preferences we set, as well as its own default browser preferences, to a user.js file in the wrong profile directory when using a custom profile: mozilla/geckodriver#1844. As a temporary workaround until this issue gets fixed, we create the user.js file ourselves. In order to do this, we keep a copy of geckodriver's default preferences in configure_firefox.py.

This PR also:

Fixes a bug in PatchedGeckoDriverService class that caused geckodriver not to receive the service_args passed when starting the browser.
Removes the code that intercepts the location of the browser profile from the geckodriver logs, as we no longer need it.
Updates manual_test.py

Closes #423, Closes #383, Closes #161, Closes #253, Closes #384, Closes #680

Reenable stateful crawling and profile tests. Also, update the docs now that stateful crawling is supported. Currently, stateful crawling is broken, as geckodriver deletes the browser profile when closing or crashing before we can archive it.

Use a custom profile by setting it as an argument via the Options class, instead of using the FirefoxProfile class. This way geckodriver does not delete it when crashing or closing. Also, remove some unused arguments from the function that configures privacy settings in Firefox. Finally, remove the code that clears driver.profile before calling driver.quit(), as driver.profile is always None when using a custom profile.

Fix a bug in PatchedGeckoDriverService that caused geckodriver not to receive the service_args passed when starting the browser. PatchedGeckoDriverService is a modified version of Selenium's Service class and this bug has been fixed in the original version.

Geckodriver has a bug that makes it write the browser preferences we set, as well as its own default browser preferences, to a user.js file in the wrong profile directory when using a custom profile: mozilla/geckodriver#1844. As a temporary workaround until this issue gets fixed, we create the user.js file ourselves. In order to do this, we keep a copy of geckodriver's default preferences in our code. Closes openwpm#423

codecov · 2021-03-03T13:07:15Z

Codecov Report

Merging #864 (37271ba) into master (cb95ecc) will increase coverage by 0.52%.
The diff coverage is 32.27%.

@@            Coverage Diff             @@
##           master     #864      +/-   ##
==========================================
+ Coverage   49.86%   50.38%   +0.52%     
==========================================
  Files          35       35              
  Lines        3309     3384      +75     
==========================================
+ Hits         1650     1705      +55     
- Misses       1659     1679      +20

Impacted Files	Coverage Δ
openwpm/task_manager.py	`80.06% <ø> (+2.10%)`	⬆️
openwpm/deploy_browsers/configure_firefox.py	`8.77% <9.17%> (+6.57%)`	⬆️
openwpm/deploy_browsers/deploy_firefox.py	`21.97% <11.11%> (-0.25%)`	⬇️
openwpm/command_sequence.py	`89.23% <66.66%> (+1.73%)`	⬆️
openwpm/commands/profile_commands.py	`64.51% <75.00%> (+39.90%)`	⬆️
openwpm/browser_manager.py	`63.05% <83.33%> (+4.05%)`	⬆️
openwpm/config.py	`96.63% <100.00%> (ø)`
openwpm/deploy_browsers/selenium_firefox.py	`28.81% <100.00%> (+0.02%)`	⬆️
openwpm/commands/browser_commands.py	`26.87% <0.00%> (-0.80%)`	⬇️
openwpm/commands/utils/webdriver_utils.py	`21.59% <0.00%> (-0.57%)`	⬇️
... and 7 more

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 cb95ecc...37271ba. Read the comment docs.

openwpm/deploy_browsers/deploy_firefox.py

test/test_profile.py

openwpm/browser_manager.py

boolean5 · 2021-03-09T15:27:13Z

manual_test.py still sets the browser profile via the FirefoxProfile class. Should we change it to use a custom profile instead?
My impression is that since we're not interested in being able to archive the profile in the context of manual_test.py, there's no need to change it, but I'd like a confirmation on this.

vringar · 2021-03-09T18:38:15Z

Yes, we should use a custom profile Here.
Good job for remembering manual_test.py!

openwpm/browser_manager.py

openwpm/deploy_browsers/configure_firefox.py

openwpm/deploy_browsers/deploy_firefox.py

test/test_profile.py

openwpm/browser_manager.py

englehardt

This is great! Thank you Georgia! I left a bunch of comments, but nothing major.

openwpm/browser_manager.py

openwpm/deploy_browsers/configure_firefox.py

openwpm/deploy_browsers/deploy_firefox.py

test/test_crawl.py

test/test_profile.py

1. In `deploy_firefox` do not use `driver.capabilities["moz:profile"]` to get the profile location. Custom profiles, unlike profiles created via `FirefoxProfile`, are used in-place, so we already know the location. 2. In `launch_browser_manager`, `spawned_profile_path` and `driver_profile_path` point to the same location now that we are using a custom profile. Replace them with a single `browser_profile_path` variable. 3. Rename `prof_folder` and `browser_profile_folder` to `browser_profile_path` for consistency. 4. Improve naming of the temporary Firefox profile.

Running manual_test.py resulted in an error because the `xpi()` fixture was called directly. Apply the fix suggested in https://docs.pytest.org/en/stable/deprecations.html#calling-fixtures-directly Also, use a custom profile instead of `FirefoxProfile` and update some docstrings.

boolean5 · 2021-03-15T12:06:01Z

Yes, we should use a custom profile Here.
Good job for remembering manual_test.py!

manual_test.py happened to be broken for a different reason and also needed another update (check f5bacae)

Move the core implementation of profile dumping into a `dump_profile` function, which can be used both internally when closing or restarting a crashed browser and from the `execute()` method of `DumpProfileCommand`. Also, make compression the default in `DumpProfileCommand`. Finally, do not compress the tar archive of the crashed browser's profile when restarting from a crash. We should avoid the extra compression/ decompression step as this is a short-lived tar file.

boolean5 · 2021-03-16T16:41:19Z

I assume that in these three cases [1] [2] [3] reset=True was used because stateful crawling was disabled and we can now let it take the default False value. Is there a reason that I'm missing to keep reset=True for any of them?

englehardt · 2021-03-16T17:46:03Z

I assume that in these three cases [1] [2] [3] reset=True was used because stateful crawling was disabled and we can now let it take the default False value. Is there a reason that I'm missing to keep reset=True for any of them?

For [1] we should keep reset=True. We want those crawls to be stateless (of course other users can change as they see fit). For [2] and [3] I'm not sure it matters, but I'll defer to @vringar just in case.

vringar · 2021-03-16T17:50:26Z

[2] and [3] can be removed as well.

Use the public suffix + 1 instead of the public suffix when comparing the domains in the crawl database with those in the profile history. Also, update an incorrectly formed query to the crawl database.

boolean5 · 2021-03-22T09:42:27Z

As we discussed, I checked and the original tar file is kept intact after extracting, so I removed the intermediate step of copying it from load_profile().

boolean5 · 2021-03-22T09:59:52Z

@vringar, @englehardt, thanks for reviewing! This is ready, I've covered all the requested changes. You can take another look if you'd like.

test/test_profile.py

openwpm/deploy_browsers/selenium_firefox.py

vringar

Do we delete the profile folders on a normal shutdown? I don't think we need to but maybe we should still do it.

boolean5 · 2021-03-22T11:34:05Z

Do we delete the profile folders on a normal shutdown? I don't think we need to but maybe we should still do it.

I think we do that here, in shutdown_browser(). Does that cover what you had in mind?

vringar · 2021-03-22T11:58:31Z

Do we delete the profile folders on a normal shutdown? I don't think we need to but maybe we should still do it.

I think we do that here, in shutdown_browser(). Does that cover what you had in mind?

Yeah. Sorry for missing this and thanks for pointing it out.

Make `PatchedGeckoDriverService` class subclass selenium.webdriver.firefox.service.Service instead of selenium.webdriver.common.service.Service, so that we only have to keep track of the changes in the `__init__()` method of the former class.

birdsarah · 2021-04-12T17:27:55Z

WOOOOTTTT!!!!

vringar · 2021-04-12T18:02:31Z

Not officially released as of yet though. Gonna publish it with the next FF release.
Although it seems that FF 89 is in the process of being released see the tags
So I might just do a release tomorrow.

Restore stateful crawling support

boolean5 added 4 commits March 1, 2021 17:18

Enable stateful crawling and tests

daa6dba

Reenable stateful crawling and profile tests. Also, update the docs now that stateful crawling is supported. Currently, stateful crawling is broken, as geckodriver deletes the browser profile when closing or crashing before we can archive it.

Do not intercept profile location from logs

2237822

englehardt requested review from englehardt and vringar March 8, 2021 16:51

boolean5 commented Mar 9, 2021

View reviewed changes

openwpm/deploy_browsers/deploy_firefox.py Outdated Show resolved Hide resolved

test/test_profile.py Show resolved Hide resolved

openwpm/browser_manager.py Outdated Show resolved Hide resolved

openwpm/browser_manager.py Outdated Show resolved Hide resolved

vringar reviewed Mar 11, 2021

View reviewed changes

vringar self-assigned this Mar 11, 2021

englehardt requested changes Mar 12, 2021

View reviewed changes

boolean5 added 2 commits March 15, 2021 11:03

Remove unused status string "Proxy Ready"

d2aff83

boolean5 mentioned this pull request Mar 15, 2021

Revert workaround of manually creating user.js #867

Closed

boolean5 added 2 commits March 15, 2021 12:23

Reference our own issue instead of geckodriver's

1d3de72

boolean5 added 4 commits March 15, 2021 15:15

Add reminder to update geckodriver prefs

aa1de92

Rename temp dir of crashed browser's profile tar

9ea8e8a

Skip test_browser_profile_coverage locally

3f7efc2

boolean5 added 3 commits March 19, 2021 21:09

Add some type annotations

3b4219d

Remove reset=True from tests

a19b124

Fix minor typos

c51f9e5

boolean5 added 3 commits March 21, 2021 23:59

Fix test_browser_profile_coverage

9e8298c

Use the public suffix + 1 instead of the public suffix when comparing the domains in the crawl database with those in the profile history. Also, update an incorrectly formed query to the crawl database.

Add comment for Marionette port race condition

e536c63

Do not copy tar before extracting in load_profile

06b8359

boolean5 requested review from vringar and englehardt March 22, 2021 10:00

vringar reviewed Mar 22, 2021

View reviewed changes

test/test_profile.py Outdated Show resolved Hide resolved

Use local test server in profile tests

49965db

vringar reviewed Mar 22, 2021

View reviewed changes

openwpm/deploy_browsers/selenium_firefox.py Outdated Show resolved Hide resolved

vringar approved these changes Mar 22, 2021

View reviewed changes

Simplify PatchedGeckoDriverService class

9a21d86

Make `PatchedGeckoDriverService` class subclass selenium.webdriver.firefox.service.Service instead of selenium.webdriver.common.service.Service, so that we only have to keep track of the changes in the `__init__()` method of the former class.

vringar approved these changes Mar 22, 2021

View reviewed changes

Remove unnecessary import

37271ba

englehardt approved these changes Mar 29, 2021

View reviewed changes

englehardt merged commit bfc4644 into openwpm:master Mar 29, 2021

Zaxeli pushed a commit to Zaxeli/OpenWPM that referenced this pull request Aug 10, 2021

Merge pull request openwpm#864 from boolean5/restore-stateful-crawls

fac50ab

Restore stateful crawling support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore stateful crawling support #864

Restore stateful crawling support #864

boolean5 commented Mar 3, 2021 •

edited

Loading

codecov bot commented Mar 3, 2021 •

edited

Loading

boolean5 commented Mar 9, 2021

vringar commented Mar 9, 2021

englehardt left a comment

boolean5 commented Mar 15, 2021

boolean5 commented Mar 16, 2021

englehardt commented Mar 16, 2021

vringar commented Mar 16, 2021

boolean5 commented Mar 22, 2021

boolean5 commented Mar 22, 2021

vringar left a comment

boolean5 commented Mar 22, 2021

vringar commented Mar 22, 2021

birdsarah commented Apr 12, 2021

vringar commented Apr 12, 2021 •

edited

Loading

Restore stateful crawling support #864

Restore stateful crawling support #864

Conversation

boolean5 commented Mar 3, 2021 • edited Loading

codecov bot commented Mar 3, 2021 • edited Loading

Codecov Report

boolean5 commented Mar 9, 2021

vringar commented Mar 9, 2021

englehardt left a comment

Choose a reason for hiding this comment

boolean5 commented Mar 15, 2021

boolean5 commented Mar 16, 2021

englehardt commented Mar 16, 2021

vringar commented Mar 16, 2021

boolean5 commented Mar 22, 2021

boolean5 commented Mar 22, 2021

vringar left a comment

Choose a reason for hiding this comment

boolean5 commented Mar 22, 2021

vringar commented Mar 22, 2021

birdsarah commented Apr 12, 2021

vringar commented Apr 12, 2021 • edited Loading

boolean5 commented Mar 3, 2021 •

edited

Loading

codecov bot commented Mar 3, 2021 •

edited

Loading

vringar commented Apr 12, 2021 •

edited

Loading