v0.10.0 / Firefox 77.0.1 release

[englehardt]

I followed the steps in the new release checklist for this.

[englehardt]

The missing packages discussed in comment #682 (comment) are missing from conda env create --force -q -f environment-unpinned.yaml. Unfortunately it looks like conda doesn't disable ~/.local when generating the list of packages to install via pip, see conda/conda#7173 and conda/conda#448. This means that some requirements may be "already satisfied" when creating the environment if the user already has them installed in ~/.local:

steven@apollo:~/research/OpenWPM/scripts$ conda env create --force -f environment-unpinned.yaml
Collecting package metadata (repodata.json): done
Solving environment: done
Preparing transaction: done
Verifying transaction: done
Executing transaction: done
Ran pip subprocess with arguments:
['/home/steven/miniconda3/envs/openwpm/bin/python', '-m', 'pip', 'install', '-U', '-r', '/home/steven/research/OpenWPM/scripts/condaenv.o4ueqpf1.requirements.txt']
Pip subprocess output:
Requirement already up-to-date: plyvel in /home/steven/.local/lib/python3.8/site-packages (from -r /home/steven/research/OpenWPM/scripts/condaenv.o4ueqpf1.requirements.txt (line 1)) (1.2.0)
Requirement already up-to-date: domain-utils in /home/steven/.local/lib/python3.8/site-packages (from -r /home/steven/research/OpenWPM/scripts/condaenv.o4ueqpf1.requirements.txt (line 2)) (0.7.1)

Note that we shouldn't assume this is unusual. Pip has long been recommended users install things in ~/.local (via the --user flag) when not using a virtualenv, and I believe they've recently made that the default (pypa/pip#1668). Thankfully there's a simple workaround for the repin script (see conda/conda#448 (comment)).

I suspect users who have site packages installed that are incompatible with the conda environment's packages may still see issues when trying to run OpenWPM (I believe @vringar experienced this before?). I wonder if we should write a simple conda-activate.sh that sets PYTHONNOUSERSITE=True and runs conda activate openwpm? My understanding is that virtualenv will do something like this for proper isolation (pypa/virtualenv#24). However this comment (conda/conda#9788 (comment)) suggests this is a misuse of conda, and that instead a virtualenv should be used in conjunction with conda to achieve this type of isolation. @birdsarah wdyt?

[birdsarah]

Note that we shouldn't assume this is unusual. Pip has long been recommended users install things in ~/.local (via the --user flag) when not using a virtualenv,

........I do agree that openwpm's user base are not software engineers typically so we should be mindful to make things robust.

I suspect users who have site packages installed that are incompatible with the conda environment's packages may still see issues when trying to run OpenWPM (I believe @vringar experienced this before?)

hmmmm....

I wonder if we should write a simple conda-activate.sh that sets PYTHONNOUSERSITE=True and runs conda activate openwpm?

I really like the PYTHONNOUSERSITE fix. I do not believe it will be possible to make a conda-activate script, otherwise I would have already as the wierd line that's at the top of all the sh scripts is activating the conda environment for that script. But I may be wrong.

[birdsarah]

i don't believe it's necessary to pin all of these dependencies. but this is a pure gut instinct reaction.

[birdsarah]

My concern is that overpinning creates future tech debt when packages should be updated to more secure / uptodate versions but we don't know / remember why packages were pinned.

[birdsarah]

But your comment is clear, so I'm "meh" about it in this case.

[birdsarah]

I take above comments back. I no longer think its necessary to pin localstack at all.

[englehardt]

I narrowed it down to moto-ext (at least when I run the tests locally)

I was also wondering whether to pin all of them or just that one. I chose to pin all of them because it took me a while for me to recognize that this was the issue. localstack didn't give a helpful error, and I didn't initially realize that we added some of localstack's dependencies separate from localstack itself. Let's say all of these packages were updated in the future, the user would have to test them 1 by 1 to find the responsible package (or set of packages).

The risk I see if the pinned versions are incompatible with other (non-pinned) python packages. And of course security vulnerabilities, but hopefully GitHub will warn us there and we can update as needed.

I'm secretly hoping we don't have to keep this around too long and can just get rid of localstack (#652).

wdyt?

[birdsarah]

Hmmmmm....They're timing out here too: https://travis-ci.org/github/mozilla/OpenWPM/jobs/699484251

[birdsarah]

There's two runs associated with each commit - I only successfully re-ran the tests on one. I don't know whether there's a meaningful difference between the two runs.

[birdsarah]

OK. My apologies. I now see the difference between the two runs. Please ignore all my comments regarding pinning. Glad you've pinned it down to moto-ext. Does this mean we can advance localstack and just pin moto-ext?

[englehardt]

Glad you've pinned it down to moto-ext. Does this mean we can advance localstack and just pin moto-ext?

Yes looks like it. I just tried it locally and that worked. I'll push a new commit.

[englehardt]

okay I spoke too soon. I was bitten by my local packages. There are still failures on travis https://travis-ci.org/github/mozilla/OpenWPM/jobs/701020698. I've now re-pinned localstack.

[birdsarah]

@englehardt, I restarted the tests from just before you pinned localstack.

The a_e that were previously failing, now passed. The _s tests that are failing appear to be failing due to xvfb which I doubt has anything to do with localstack.

[birdsarah]

I just noticed that pyvirtualdisplay is showing a big version bump in this PR " pyvirtualdisplay=0.2.5" -> "pyvirtualdisplay=1.3.2" which I would at least want to investigate as the source of the xvfb errors.

[birdsarah]

The problem is accessing display_port. Which we probably can still do we just need to figure out the new API which is probably something like display.display.??. But do we need display_port?
As best as I can tell it's only used to remove a /tmp file. Why do we need this cleanup?

        if self.display_port is not None:  # xvfb diplay lock
            lockfile = "/tmp/.X%s-lock" % self.display_port
            try:
                os.remove(lockfile)
            except OSError:
                self.logger.debug("BROWSER %i: Screen lockfile (%s) already "
                                  "removed" % (self.crawl_id, lockfile))
                pass

[englehardt]

Note that we shouldn't assume this is unusual. Pip has long been recommended users install things in ~/.local (via the --user flag) when not using a virtualenv,

........I do agree that openwpm's user base are not software engineers typically so we should be mindful to make things robust.

I don't see how this distinction is important. We shouldn't make assumptions about the user's system setup.

I suspect users who have site packages installed that are incompatible with the conda environment's packages may still see issues when trying to run OpenWPM (I believe @vringar experienced this before?)

hmmmm....

Unfortunately this seems to be the case :-/. I filed #689 to track that.

I wonder if we should write a simple conda-activate.sh that sets PYTHONNOUSERSITE=True and runs conda activate openwpm?

I really like the PYTHONNOUSERSITE fix. I do not believe it will be possible to make a conda-activate script, otherwise I would have already as the wierd line that's at the top of all the sh scripts is activating the conda environment for that script. But I may be wrong.

Yeah I see what you mean. I didn't play around with it, but included this as an option in #689.

[birdsarah]

I don't see how this distinction is important. We shouldn't make assumptions about the user's system setup.

I deleted an unhelpful set of comments from myself. I've never run into this issue before and I attribute that fact, perhaps incorrectly, to the fact that I learned from day 1 as a software engineer that everything should be done in a virtualenv (or equivalent) otherwise you can run into all kind of weirdness.

But without the comments I deleted the second thought about being robust lost it's meaning.

[englehardt]

The problem is accessing display_port. Which we probably can still do we just need to figure out the new API which is probably something like display.display.??. But do we need display_port?
As best as I can tell it's only used to remove a /tmp file. Why do we need this cleanup?

        if self.display_port is not None:  # xvfb diplay lock
            lockfile = "/tmp/.X%s-lock" % self.display_port
            try:
                os.remove(lockfile)
            except OSError:
                self.logger.debug("BROWSER %i: Screen lockfile (%s) already "
                                  "removed" % (self.crawl_id, lockfile))
                pass

Yes, at least for the time being. This was initially added because long-running crawls would fill the tmp directory with too many lock files and would eventually lead XVFB to fail because it would be assigned a port that already had a lock file.

This can probably be fixed by actually shutting down xvfb properly in https://github.com/mozilla/OpenWPM/blob/43922bc18cc458eab3aaea618c1f4bb4b14ccf8f/automation/BrowserManager.py#L503-L512 and not just killing the process like we do right now.

[birdsarah]

The problem is accessing display_port. Which we probably can still do we just need to figure out the new API which is probably something like display.display.??. But do we need display_port?
As best as I can tell it's only used to remove a /tmp file. Why do we need this cleanup?

        if self.display_port is not None:  # xvfb diplay lock
            lockfile = "/tmp/.X%s-lock" % self.display_port
            try:
                os.remove(lockfile)
            except OSError:
                self.logger.debug("BROWSER %i: Screen lockfile (%s) already "
                                  "removed" % (self.crawl_id, lockfile))
                pass

Yes, at least for the time being. This was initially added because long-running crawls would fill the tmp directory with too many lock files and would eventually lead XVFB to fail because it would be assigned a pid that already had a lock file.

This can probably be fixed by actually shutting down xvfb properly in

https://github.com/mozilla/OpenWPM/blob/43922bc18cc458eab3aaea618c1f4bb4b14ccf8f/automation/BrowserManager.py#L503-L512
and not just killing the process like we do right now.

This sounds good. If we properly shutdown xvfb then even if we have some rogue lock files due to a bad shutdown we certainly wouldn't expect the tmp directory to fill up because if every shutdown isn't shutting down properly then you have other problems. So this seems like a very satisfactory solution.

I'm also fine with pinning pyvirtualdisplay to <1 for this PR and then having a seperate issue to tackle this.l

[birdsarah]

sorry pressed the wrong button

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@abd613d). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #682   +/-   ##
=========================================
  Coverage          ?   58.80%           
=========================================
  Files             ?       44           
  Lines             ?     4071           
  Branches          ?        0           
=========================================
  Hits              ?     2394           
  Misses            ?     1677           
  Partials          ?        0           

Impacted Files	Coverage Δ
automation/Commands/browser_commands.py	16.66% <0.00%> (ø)
automation/utilities/multiprocess_utils.py	34.48% <0.00%> (ø)
test/test_env.py	100.00% <0.00%> (ø)
test/test_profile.py	26.98% <0.00%> (ø)
test/test_simple_commands.py	99.52% <0.00%> (ø)
automation/DeployBrowsers/deploy_browser.py	42.85% <0.00%> (ø)
test/conftest.py	94.73% <0.00%> (ø)
test/test_js_instrument_mock_window_property.py	96.87% <0.00%> (ø)
automation/DeployBrowsers/deploy_firefox.py	15.45% <0.00%> (ø)
automation/Commands/Types.py	79.36% <0.00%> (ø)
... and 34 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 abd613d...cab0f52. Read the comment docs.