Project

General

Profile

Bug #16475

Feature #14568: Additional Software Packages

Opening docs via asp notification result in a browser lacking AT-SPI access

Added by anonym 10 months ago. Updated 9 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
02/20/2019
Due date:
% Done:

100%

Feature Branch:
tails: bugfix/16475-asp-notifications-gnome-env submodules/pythonlib: bugfix/16475-asp-notifications-gnome-env
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Additional Software Packages

Description

I.e. the browser opening the docs doesn't support the screen reader, and cannot be seen by Dogtail so automated testing is requires workarounds.

It can easily be reproduced with:

python3 -c "import importlib; importlib.machinery.SourceFileLoader('m', '/usr/local/sbin/tails-additional-software').load_module()._notify('', documentation_target='about')" 

(We just import /usr/local/sbin/tails-additional-software as a package, and run its _notify() method.)

Clicking the "Documentation" action button results in this exact call:

sudo -u amnesia /usr/local/lib/tails-additional-software-notify '' '' '' '' 'about' ''

Indeed, if I run that command myself, the resulting browser lacks access to AT-SPI. But if I drop the sudo part and run it directly as amnesia it works fine. Comparing to how we solved this problem in /usr/local/sbin/tails-notify-user, I bet what is missing is the Python equivalent of:

source /usr/local/lib/tails-shell-library/gnome.sh && export_gnome_env

Associated revisions

Revision 36612449 (diff)
Added by anonym 10 months ago

ASP: fix AT-SPI for browser started via notification (Will-fix: #16475).

We lacked sufficient environment for AT-SPI to work for the browser
child processes started for browsing the docs.

Revision b66cb28f
Added by intrigeri 10 months ago

Merge remote-tracking branch 'origin/bugfix/16475-asp-notifications-gnome-env' into stable (Fix-committed: #16475)

History

#1 Updated by intrigeri 10 months ago

  • Assignee set to alant

@alant, can you please take a look?

BTW, I suspect IBus will be broken there as well.

#2 Updated by anonym 10 months ago

  • Assignee changed from alant to anonym
  • % Done changed from 0 to 50

intrigeri wrote:

@alant, can you please take a look?

I actually already started looking into this yesterday (I wrote a comment abou it but only pressed "Preview", not "Submit"...) so I hope it is ok that I take it. I want to know the fundamentals around AT-SPI a bit better for future Dogtail work.

BTW, I suspect IBus will be broken there as well.

Good call!

#3 Updated by anonym 10 months ago

  • Status changed from Confirmed to In Progress

#4 Updated by anonym 10 months ago

  • Assignee deleted (anonym)
  • QA Check set to Ready for QA
  • Feature Branch set to tails: bugfix/16475-asp-notifications-gnome-env submodules/pythonlib: bugfix/16475-asp-notifications-gnome-env

Fixed! Turns out IBus still worked, and only XDG_RUNTIME_DIR was missing in the environment for AT-SPI.

To verify the fix:

  1. Boot an image builtfFrom this branch
  2. Set an admin password (for apt)
  3. sudo apt update
  4. sudo apt install cowsay (or whatever)
  5. Wait for the notification to appear; click the "Documentation" button
  6. Wait for Tor Browser to start
  7. Verify that Tor Browser's AT-SPI works with:
    1. Make sure this doesn't print an exception: python2 -c "from dogtail import tree; tree.root.application('Firefox', retry=False)"
    2. Enable the screen reader and made sure it works in Tor Browser

#5 Updated by intrigeri 10 months ago

  • Assignee set to intrigeri

#6 Updated by intrigeri 10 months ago

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Info Needed

@anonym, here's a code review:

  • Why is most of the code from gnome_env_dict duplicated at the top of gnome.py?
  • What is the export_gnome_env function for?
  • I wonder if we really need to export these environment variables here (it seems we don't); if we don't, I'm slightly worried about side effects.
  • I doubt we really need to use the low-level subprocess.Popen here: "The recommended approach to invoking subprocesses is to use the run() function for all use cases it can handle" (https://docs.python.org/3/library/subprocess.html).

#7 Updated by anonym 10 months ago

  • Assignee changed from anonym to intrigeri
  • % Done changed from 50 to 60
  • QA Check changed from Info Needed to Ready for QA

intrigeri wrote:

@anonym, here's a code review:

  • Why is most of the code from gnome_env_dict duplicated at the top of gnome.py?

I was too lazy to refactor (actually I'm surprised!). Fixed!

  • What is the export_gnome_env function for?

It is the exact analog of the shell wrapper's export_gnome_env. It was my first attempt, but after having similar concerns as you in your next point I chose the ... 'env', *gnome_env_vars ...() approach instead. I chose to leave export_gnome_env around since I thought it could help in our shell → python porting effort nt I now realize that the other approach I came up with probably always will be superior, so let's drop everything else.

  • I wonder if we really need to export these environment variables here (it seems we don't); if we don't, I'm slightly worried about side effects.

We don't; we just set the relevant variables (via gnome_env_vars()) in the subprocess' env.

Thanks for the tip!

I think that fixes all your concenrs so far. I'm much happier with the state of gnome.sh now so thanks again!

#8 Updated by intrigeri 10 months ago

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Dev Needed

It seems that you did not push your submodules changes. I guess that's one drawback of allowing usage of local-only submodule refs: nothing forces anyone to push their commits until the reviewer notices the problem. This would not be a problem if we followed https://tails.boum.org/contribute/merge_policy/#submit more strictly: you would have noticed that this makes the branch FTBFS on Jenkins.

#9 Updated by anonym 10 months ago

  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

It seems that you did not push your submodules changes.

Sorry, pushed!

I guess that's one drawback of allowing usage of local-only submodule refs: nothing forces anyone to push their commits until the reviewer notices the problem.

True.

This would not be a problem if we followed https://tails.boum.org/contribute/merge_policy/#submit more strictly: you would have noticed that this makes the branch FTBFS on Jenkins.

Lately I've definitely been too eager about throwing my tickets on other people for QA. Sorry about this! Like I used to do in the past, I'll start to assign tickets to myself as RFQA with a note to "Let's see what jenkins thinks" and then look back the next day.

So, let's see what jenkins thinks! :)

#10 Updated by intrigeri 10 months ago

So, let's see what jenkins thinks! :)

:)

#11 Updated by anonym 10 months ago

  • Assignee changed from anonym to intrigeri

#12 Updated by intrigeri 10 months ago

  • Assignee changed from intrigeri to anonym

Hi @anonym,

code review now passes! A couple comments below though.

  • I wonder if we really need to export these environment variables here (it seems we don't); if we don't, I'm slightly worried about side effects.

We don't; we just set the relevant variables (via gnome_env_vars()) in the subprocess' env.

Right, sorry I was confused: we're calling export_gnome_env(), which does export the variables in a way that's visible from a shell script that uses it and its future child processes; but in the case at hand, since we're running this shell code in a child process, export has no side effect on the parent Python program.

Thanks for the tip!

Great.

In passing, I think a more idiomatic way to use this would be to:

  • use subprocess.run's built-in check=True functionality and drop the ad-hoc assert proc.returncode == 0
  • use capture_output=True instead of the lower level stdout = subprocess.PIPE (by the way, I think one does not insert spaces around the equal sign in Python there)

But I'm not going to block on this.

The fix on this branch is targeted and minimal enough for me to feel comfortable merging into stable once I'll have built and successfully tested.

#13 Updated by intrigeri 10 months ago

  • Assignee changed from anonym to intrigeri

anonym wrote:

To verify the fix:

  1. Boot an image builtfFrom this branch

FTR this must be an ISO image, otherwise there's no "Documentation" link in the notification. (These days, when I read "image", I assume "USB image".)

#14 Updated by intrigeri 10 months ago

  • Assignee changed from intrigeri to anonym
  • % Done changed from 60 to 70
  • QA Check changed from Ready for QA to Info Needed

It works!

anonym, I took a step back and spotted a potential design issue: here we're patching @_notify, which is merely a wrapper around /usr/local/lib/tails-additional-software-notify. So tails-additional-software-notify will still be broken if it's ever used from elsewhere, without going through the code you're adding to _notify. It would feel better design to me if we fixed tails-additional-software-notify itself. What do you think?

#15 Updated by anonym 10 months ago

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Info Needed to Ready for QA

intrigeri wrote:

  • I wonder if we really need to export these environment variables here (it seems we don't); if we don't, I'm slightly worried about side effects.

We don't; we just set the relevant variables (via gnome_env_vars()) in the subprocess' env.

Right, sorry I was confused: we're calling export_gnome_env(), which does export the variables in a way that's visible from a shell script that uses it and its future child processes; but in the case at hand, since we're running this shell code in a child process, export has no side effect on the parent Python program.

Yes, that was how it worked when export_gnome_env() existed but now there is only gnome_env_vars() which doesn't touch the environment, but just return a list of VAR=VALUE:s for the four "GNOME" variables, that we pass to env that is wrapping the child process.

Thanks for the tip!

Great.

In passing, I think a more idiomatic way to use this would be to:

  • use subprocess.run's built-in check=True functionality and drop the ad-hoc assert proc.returncode == 0
  • use capture_output=True instead of the lower level stdout = subprocess.PIPE (by the way, I think one does not insert spaces around the equal sign in Python there)

Nice! capture_output is too new for Tails' python3 (3.5.3, we need 3.7), but I found subprocess.check_output() which does exactly what we want.

FTR this must be an ISO image, otherwise there's no "Documentation" link in the notification. (These days, when I read "image", I assume "USB image".)

Aha! I will update my terminology! :)

anonym, I took a step back and spotted a potential design issue: here we're patching _notify, which is merely a wrapper around /usr/local/lib/tails-additional-software-notify. So tails-additional-software-notify will still be broken if it's ever used from elsewhere, without going through the code you're adding to _notify. It would feel better design to me if we fixed tails-additional-software-notify itself. What do you think?

Sure! I was definitely mirroring the design of tails-notify-user too much! :P

#16 Updated by intrigeri 10 months ago

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Info Needed

Nice! capture_output is too new for Tails' python3 (3.5.3, we need 3.7), but I found subprocess.check_output() which does exactly what we want.
[...]

anonym, I took a step back and spotted a potential design issue: here we're patching _notify, which is merely a wrapper around /usr/local/lib/tails-additional-software-notify. So tails-additional-software-notify will still be broken if it's ever used from elsewhere, without going through the code you're adding to _notify. It would feel better design to me if we fixed tails-additional-software-notify itself. What do you think?

Sure! I was definitely mirroring the design of tails-notify-user too much! :P

Did you forget to push, or?

#17 Updated by anonym 10 months ago

  • Assignee changed from anonym to intrigeri
  • % Done changed from 70 to 60
  • QA Check changed from Info Needed to Ready for QA

Yeah, this time I remembered to push the submodule but not the main repo.

#18 Updated by intrigeri 10 months ago

  • Status changed from In Progress to 11
  • % Done changed from 60 to 100

#19 Updated by intrigeri 10 months ago

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

#20 Updated by CyrilBrulebois 9 months ago

  • Status changed from 11 to Resolved

Also available in: Atom PDF