Update pyvirtualdisplay to >1.0.0 version

[englehardt]

In #682 we pinned pyvirtualdisplay to 0.2.5 due to an incompatible interface change. See the excerpt from this test run:

Traceback (most recent call last):

  File "/home/travis/build/mozilla/OpenWPM/automation/BrowserManager.py", line 450, in BrowserManager

    driver, prof_folder, browser_settings = deploy_browser.deploy_browser(

  File "/home/travis/build/mozilla/OpenWPM/automation/DeployBrowsers/deploy_browser.py", line 13, in deploy_browser

    return deploy_firefox.deploy_firefox(status_queue, browser_params,

  File "/home/travis/build/mozilla/OpenWPM/automation/DeployBrowsers/deploy_firefox.py", line 126, in deploy_firefox

    display_pid, display_port = display.pid, display.cmd_param[-1][1:]

AttributeError: 'Display' object has no attribute 'cmd_param'

We use cmd_param to retrieve the "display port", which is then used to identify a temporary lockfile placed in the users tmp folder. See:

        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

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.

I think the new >1.0.0 interface still exposes the same info via the display property. See the lockfile creation code. But as Sarah points out in #682 (comment) this might not even be necessary, since a proper shutdown should clear the lockfile and loads of improper shutdowns point to another issue. We should verify that's the case (i.e., a proper shutdown removes the lockfile).

[MollieBakal]

First time contributor--could I look into this?

[vringar]

Go right ahead :) Feel free to ask any questions you have here or in our Matrix channel

[MollieBakal]

I've been looking into this, and I think the interface change was in moving away from inheriting a class with cmd_params. I think we could get the display's port from display.new_display_var for an easy patch.
However, I've been trying to figure out whether a proper shutdown would delete the temporary files (and I think so?). I think a good way to do that might be to have the deploy_firefox method return the Display object, as XVFB is directly started by pyvirtualdisplay--calling display.stop() would cause pyvirtualdisplay to kill it itself, without dealing with regexing the status queue to get the port number. Is there a good reason not to do this?

[vringar]

Yes, by default we should just shut down properly and don't go around killing processes and deleting files manually.
However the way things are currently implemented the BrowserManager process/function may just exit without shutting down the display. And when that happens we still need to able to kill the xvfb process as there are only a finite number of xvfb processes that can run in parallel.

[vringar]

Closed by #831