Bug #17142

New Unsafe Browser tabs have the "Private Browsing" name and the Tor Browser icon

Added by intrigeri 2019-10-09 14:40:58 . Updated 2019-10-19 11:51:02 .

Status:
Resolved
Priority:
Elevated
Assignee:
Category:
Target version:
Tails_4.0
Start date:
Due date:
% Done:

0%

Feature Branch:
bugfix/17142-unsafe-browser-tabs
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Unsafe Browser
Deliverable for:

Description

Regression with Tor Browser 9.0a7.

Might be because Torbutton, which is now integrated, in Tor Browser, forces private browsing mode even for the Unsafe Browser.

Subtasks

Related issues

Related to Tails - Feature #17055: Update Unsafe Browser tweaks for Tor Browser 9 Resolved 2019-01-14
Blocks Tails - Feature #16209: Core work: Foundations Team Confirmed

History

#1 Updated by intrigeri 2019-10-09 14:41:21

#2 Updated by intrigeri 2019-10-09 14:41:51

  • related to Feature #17055: Update Unsafe Browser tweaks for Tor Browser 9 added

#3 Updated by intrigeri 2019-10-09 14:42:37

#4 Updated by segfault 2019-10-11 21:25:15

Maybe setting browser.privatebrowsing.autostart to false fixes the private browsing part.

#5 Updated by segfault 2019-10-11 22:40:10

  • Assignee set to segfault

segfault wrote:
> Maybe setting browser.privatebrowsing.autostart to false fixes the private browsing part.

That seems to work.

Regarding the icon: I tried to fix this by removing the icons in /var/lib/unsafe-browser/chroot//usr/local/lib/tor-browser/browser/chrome/icons/default in the configure_chroot_browser() function, but that didn’t work. I have no idea where else Tor Browser gets this icon from.

#6 Updated by intrigeri 2019-10-12 04:22:09

> I have no idea where else Tor Browser gets this icon from.

It could be bundled in one of the omni.ja (be it in a separate file or as data in some XML or whatever file).

#7 Updated by segfault 2019-10-13 21:29:49

  • Status changed from Confirmed to In Progress

Applied in changeset commit:tails|d1c0dec4e5ac9ea6bd747961cdc94e493e672484.

#8 Updated by segfault 2019-10-13 21:31:03

  • Status changed from In Progress to Confirmed
  • Feature Branch set to bugfix/17142-unsafe-browser-tabs

intrigeri wrote:
> > I have no idea where else Tor Browser gets this icon from.
>
> It could be bundled in one of the omni.ja (be it in a separate file or as data in some XML or whatever file).

Indeed, there are also Tor Browser icons in chrome/browser/content/branding in /usr/local/lib/tor-browser/browser/omni.ja, and when those are removed, the tabs don’t have the icon anymore.

#9 Updated by segfault 2019-10-16 14:38:50

  • Status changed from Confirmed to In Progress

Applied in changeset commit:tails|5552dffb8bb03d78bb1462ce3005e54064adc4ed.

#10 Updated by segfault 2019-10-16 14:39:21

  • Status changed from In Progress to Needs Validation
  • Assignee deleted (segfault)

#11 Updated by intrigeri 2019-10-17 06:55:05

  • Assignee set to intrigeri

#12 Updated by segfault 2019-10-17 07:03:47

  • Status changed from Needs Validation to In Progress

Applied in changeset commit:tails|1186b5848b578e1eeb2167dca22879913888829c.

#13 Updated by intrigeri 2019-10-17 07:06:07

  • Status changed from In Progress to Needs Validation

Here as well, I’ve transplanted the branch onto testing (git rebase --onto testing devel bugfix/17142-unsafe-browser-tabs) so we can have fix in 4.0.

#14 Updated by intrigeri 2019-10-17 13:54:44

  • Status changed from Needs Validation to In Progress
  • Assignee changed from intrigeri to segfault

Hi segfault,

unfortunately, this breaks the Unsafe Browser scenarios on Jenkins. I did not investigate why. Do you want to do so?

One thing I’ve noticed while reviewing the branch is that this introduces a 2nd case when we’re de+recompressing "${chroot}/${TBB_INSTALL}/browser/omni.ja" when starting the Unsafe Browser. I’m a bit concerned that it could make it even slower to start than it already is. But I’m not sure how we could fix that, short of updating this archive when we build images. Perhaps combining the newly introduced code with delete_chroot_browser_searchplugins() would help. Anyway, I did not measure the actual impact of this branch so for now: whatever.

#15 Updated by segfault 2019-10-17 21:47:17

intrigeri wrote:
> Hi segfault,
>
> unfortunately, this breaks the Unsafe Browser scenarios on Jenkins. I did not investigate why. Do you want to do so?

I’ll take a look now. If I can’t fix this today, I won’t have time before Sunday evening, so in that case you might want to take over.

> One thing I’ve noticed while reviewing the branch is that this introduces a 2nd case when we’re de+recompressing "${chroot}/${TBB_INSTALL}/browser/omni.ja" when starting the Unsafe Browser.

No, it does not. It only uses 7z d to delete files from an archive without de+recompressing the whole thing. That’s done in a split second.

> I’m a bit concerned that it could make it even slower to start than it already is. But I’m not sure how we could fix that, short of updating this archive when we build images.

I also thought about that, and actually even implemented something in the 11-unsafe-browser hook. But that adds another 17M to the image, and I’m not sure if that’s worth it. Also, while debugging the Unsafe Browser setup script, I noticed that compared to the start time of the Browser itself, the de+recompressing doesn’t take that long.

> Perhaps combining the newly introduced code with delete_chroot_browser_searchplugins() would help.

We could combine set_chroot_browser_name() and delete_chroot_browser_searchplugins() to only decompress once.

#16 Updated by segfault 2019-10-17 22:55:48

  • Assignee changed from segfault to intrigeri

segfault wrote:
> intrigeri wrote:
> > Hi segfault,
> >
> > unfortunately, this breaks the Unsafe Browser scenarios on Jenkins. I did not investigate why. Do you want to do so?
>
> I’ll take a look now. If I can’t fix this today, I won’t have time before Sunday evening, so in that case you might want to take over.

In the videos of the failed test scenarios the Unsafe Browser just doesn’t start. But when I start the Unsafe Browser in a VM with an image built from the same commit, it does start. So I don’t know how to debug this further. Maybe you could ssh into a Jenkins VM and investigate there.

> We could combine set_chroot_browser_name() and delete_chroot_browser_searchplugins() to only decompress once.

Regarding this, I noticed that the decompression in delete_chroot_browser_searchplugins() is actually unnecessary. I pushed a commit which removes that.

#17 Updated by intrigeri 2019-10-18 07:49:20

Hi,

first, I’ve read your comments wrt. omni.ja-related stuff and I fully agree with your conclusions.

Regarding the test suite regression:

segfault wrote:
> In the videos of the failed test scenarios the Unsafe Browser just doesn’t start. But when I start the Unsafe Browser in a VM with an image built from the same commit, it does start. So I don’t know how to debug this further. Maybe you could ssh into a Jenkins VM and investigate there.

I took a look at the Journal for the first failing scenario and I see this:

Oct 18 04:10:06 amnesia unsafe-browser.desktop[6228]: * Starting Unsafe Browser
Oct 18 04:10:06 amnesia sudo[8031]:     root : TTY=tty2 ; PWD=/home/amnesia ; USER=amnesia ; COMMAND=/usr/bin/xhost +SI:localuser:clearnet
Oct 18 04:10:06 amnesia sudo[8031]: pam_unix(sudo:session): session opened for user amnesia by (uid=0)
Oct 18 04:10:06 amnesia unsafe-browser.desktop[6228]: localuser:clearnet being added to access control list
Oct 18 04:10:06 amnesia sudo[8031]: pam_unix(sudo:session): session closed for user amnesia
Oct 18 04:10:07 amnesia systemd[4872]: gnome-terminal-server.service: Succeeded.
Oct 18 04:10:12 amnesia unsafe-browser.desktop[6228]: Fontconfig warning: "/usr/local/lib/tor-browser/TorBrowser/Data/fontconfig/fonts.conf", line 85: unknown element "blank"
Oct 18 04:10:22 amnesia unsafe-browser.desktop[6228]: Segmentation fault

Interesting. I’ll try to reproduce locally.

Note: we had a similar issue a few weeks ago in the very same area, with the very same result, that I fixed with commit:6a14bcfac80a1d1f9a197a78355999a861045150 back then.

#18 Updated by intrigeri 2019-10-18 08:00:22

Interestingly, the “Unsafe Browser can be used in all languages supported in Tails” scenario passes. The main difference is that in that scenario, we start it from a terminal instead of the Activities Overview. I have a hunch that it’s related to umask and files permissions (elsewhere we explicitly do chmod a+r "${pack}" but delete_chroot_browser_icons() does not); I will play a bit with this.

> Regarding this, I noticed that the decompression in delete_chroot_browser_searchplugins() is actually unnecessary. I pushed a commit which removes that.

Sweet! The updated code implicitly relies on the fact that there’s no shell expansion for "${searchplugins_dir}"/*/manifest.json (and thus the glob is passed to 7z as-is). I’ll push a follow-up commit to fix that. Not that I think this glob will ever expand in practice, but developers would have to wonder about this every time they work on this piece of code, which is a tiny bit distracting.

#19 Updated by intrigeri 2019-10-18 08:41:51

> But when I start the Unsafe Browser in a VM with an image built from the same commit, it does start.

I can reproduce this but only if I run sudo unsafe-browser in a Terminal (non-login shell, umask = 022). But if I start the Unsafe Browser from the Activities Overview (where our custom umask = 077 is honored), then I reproduce the problem that Jenkins pointed us to. The umask divergence could be seen as a bug, feel free to file a ticket about it if you’d like. Anyhow, this shows us that it’s important, when developing, to test stuff in a way that’s as close as possible to what actual users would do :)

I’ve pushed a commit that fixes this in my manual tests. Let’s see if this fixes the test suite!

#20 Updated by intrigeri 2019-10-18 08:44:53

Forgot to say, since I was distracted by the regression: I confirm that this branch fixes the bug this ticket is about :)

#21 Updated by intrigeri 2019-10-18 12:34:31

  • Status changed from In Progress to Needs Validation
  • Assignee deleted (intrigeri)

I’ve seen the full non-fragile test suite pass locally.

#22 Updated by anonym 2019-10-18 12:54:28

  • Assignee set to anonym

#23 Updated by anonym 2019-10-18 14:00:15

  • Status changed from Needs Validation to Fix committed
  • % Done changed from 0 to 100

Applied in changeset commit:tails|cf81509465aa91d8aa9cdbc6cf8d1a348e6138bb.

#24 Updated by anonym 2019-10-18 14:00:23

  • Assignee deleted (anonym)
  • % Done changed from 100 to 0

Note that the Unsafe Browser in Tails 3.16 also uses Tor Browser’s icon, so this was not a regression, but it’s still nice to have it fixed! :)

Code looks excellent, nothing to remark except that @segfault’s commit:09fcbc152a021485773c3310180c4b6fedef6813 really tickled my gray matter! Thanks! :)

#25 Updated by intrigeri 2019-10-19 11:51:02

  • Status changed from Fix committed to Resolved