search

freedomofpress / dangerzone

Take potentially dangerous PDFs, office documents, or images and convert them to safe PDFs
https://dangerzone.rocks/
GNU Affero General Public License v3.0
3.35k stars 152 forks source link

Order list of PDF viewers and return default application first (Linux) #832

Closed rocodes closed 2 weeks ago

rocodes commented 3 weeks ago

Closes #814

@apyrgio : let me know if this is what you had in mind :)

rocodes commented 3 weeks ago

The segfault in CI looks like https://github.com/freedomofpress/dangerzone/issues/493

apyrgio commented 3 weeks ago

Yeap, that's pretty much it! You've also added GUI tests, which is great. I'll take a look at the segfaults, and then we can merge it. My guess would be that the fixtures are instantiated in a way that doesn't clean them up properly, but we'll see.

almet commented 3 weeks ago

Running the tests locally in an ubuntu jammy container fails to reproduce the errors with the following command (all tests run fine):

./dev_scripts/env.py --distro ubuntu --version jammy run --dev bash -c "cd dangerzone && poetry run make test"
rocodes commented 3 weeks ago

Running the tests locally in an ubuntu jammy container fails to reproduce the errors with the following command (all tests run fine)

Yeah, they pass locally for me too. I think alex is right that there's something funky with the cleanup somewhere, and/or that something unthreadsafe is happening.

The closest thing to a root cause type explanation seems to be this discussion, but I am grasping a bit - I haven't looked into where or how we could be triggering this kind of issue, and it's an issue we've intermittently encountered and tried to diagnose on securedrop client too. (In that case when there's more than one instance of QApplication, which is not supported and can happen during rapid randomized test teardown/setup).

anyways, I don't have any great ideas I'm afraid, but fresh eyes are welcome and/or I can revisit this at the end of my workday and see if I can spot the problem

rocodes commented 3 weeks ago

Re: orthognal - it is possible I reintroduced a testing issue for you, so I'd say if main is passing, let's try to get to the bottom of this before merge so that I don't break main on my first dz pr :)

apyrgio commented 3 weeks ago

This seems related to https://github.com/freedomofpress/dangerzone/issues/493, that we thought we fixed in https://github.com/freedomofpress/dangerzone/pull/813, but maybe we didn't 🙄

Most likely that's what it is. I recall that this issue was a huge rabbit hole, so maybe it makes sense to reinstate our workaround, if the tests pass for this PR.

almet commented 3 weeks ago

I've just pushed a commit which reverts 3ba9181 (#813).

@rocodes Feel free (really) to just overwrite this with anything else. It's mainly to see if it solves our current issues with the CI.

apyrgio commented 2 weeks ago

The tests now pass! Alexis, feel free to merge this PR, I think we're good. Thanks again Ro!