Opened 4 years ago

Closed 4 years ago

#18915 closed defect (fixed)

Omnibox in a non-english Tor Browser has no Disconnect.me as search engine in 6.0a5

Reported by: gk Owned by: gk
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Critical Keywords: ff45-esr, TorBrowserTeam201605R, GeorgKoppen201605
Cc: mcs, brade, arthuredelstein, anonym Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We lost Disconnect.me as search engine somehow in our non-en-US bundles. Seems #11236 is showing its ugly head again (see comment 9 and https://bugzilla.mozilla.org/show_bug.cgi?id=1126722). Sad that our test suite is broken by the transition to ESR45 as well otherwise we would have caught this one earlier (too) :/

Child Tickets

Change History (11)

comment:1 Changed 4 years ago by gk

Keywords: GeorgKoppen201604 added
Owner: changed from tbb-team to gk
Status: newassigned

comment:2 Changed 4 years ago by arthuredelstein

I noticed that in our Omnibox patch we add various *.xml files (search engine definitions) to the directory browser/locales/en-US/searchplugins/. So it would seem that this patch is specifically adding search engines for the US English locale only. I'm not sure why this approach would have provided the correct search engines in non-English in TBB/ESR38, but it at least seems like a plausible explanation for the problem we see in TBB/ESR45.

There's another directory, browser/locales/generic/, that at least superficially looks like it may provide its contents to every locale. So I added a matching searchplugins directory and added disconnect.xml and youtube.xml therein. I'm currently building the full tor-browser-bundle.git with this change, and I'll report the results soon.

comment:3 Changed 4 years ago by gk

Thanks to fqueze for debugging help! It seems that https://bugzilla.mozilla.org/show_bug.cgi?id=1162569 is the likely culprit.

We have a bunch of options here

1) We override resource://search-plugins/ to point to a custom location
2) We ship our search plugins in a distribution folder
3) We replace the lang pack search engines and list.txt with the ones we want to get used during the bundling step.

I think I prefer 3) for at least two reasons: a) We can be sure that only our plugins get shipped. No other code path can use search plugins we don't want to get used. b) we avoid another possible Tor Browser patch.

comment:4 Changed 4 years ago by gk

I forgot to mention that fqueze thinks all three approaches have some kind of fragility involved, thus that argument does not count here. :)

comment:5 Changed 4 years ago by gk

Keywords: TorBrowserTeam201605 added; TorBrowserTeam201604 removed

Moving tickets

comment:6 Changed 4 years ago by gk

Keywords: GeorgKoppen201605 added; GeorgKoppen201604 removed

Moving things for me to May.

comment:7 Changed 4 years ago by gk

Cc: anonym added
Keywords: TorBrowserTeam201605R added; TorBrowserTeam201605 removed
Status: assignedneeds_review

bug_18915_v2 (https://gitweb.torproject.org/user/gk/tor-browser-bundle.git/commit/?h=bug_18915_v2&id=9ec08e87c4ce181fa79f8d5355a471ba8939788e) is up for review. I tested this with a hardened-nightly and on all platforms we support by rebundling an alpha.

Adding anonym, as I don't know how Tails is doing the localization and whether they are using the lang packs we ship or not.

comment:8 in reply to:  7 ; Changed 4 years ago by anonym

Replying to gk:

Adding anonym, as I don't know how Tails is doing the localization and whether they are using the lang packs we ship or not.

Thanks for thinking about us! :)

In Tails we do localization essentially like this:

  • We install all your langpacks in the extensions directory, then Tor Browser picks the right one according to the LANG env var. Furthermore, it will pick localized search plugins from distribution/searchplugins/locale/$LANGCODE/ (used below).
  • We dynamically generate localized search plugins for Disconnect, Startpage and Wikipedia, replacing your variants of these. Since we provide the English Wikipedia plugin for all locales, we generate localized icons (using imagemagick) for Wikipedia, by putting the lang code in the bottom-right corner.
  • We also install all search plugins from Debian's ìceweasel-l10n-* for which we have langpacks. (Personally I'd happily drop these since they mainly seem to add commercial crap.)

So, we are not affected by this bug since we generate our own thing. However, I worked on integrating 6.0a5 into Tails yesterday and noticed a related issue (for us), namely that you now put your search engine plugins inside distribution/omni.ja, which makes it harder for us to *replace* them. I guess the best, touch, would be if we settled on the same search plugins, and we did nothing regarding this on Tails' side. I also realize that this is not the right ticket for this -- if you think there's an interest in harmonizing this I can open a new ticket.

comment:9 in reply to:  8 Changed 4 years ago by gk

Replying to anonym:

Replying to gk:

Adding anonym, as I don't know how Tails is doing the localization and whether they are using the lang packs we ship or not.

Thanks for thinking about us! :)

In Tails we do localization essentially like this:

  • We install all your langpacks in the extensions directory, then Tor Browser picks the right one according to the LANG env var. Furthermore, it will pick localized search plugins from distribution/searchplugins/locale/$LANGCODE/ (used below).

Okay, you should be fine then I think.

  • We dynamically generate localized search plugins for Disconnect, Startpage and Wikipedia, replacing your variants of these. Since we provide the English Wikipedia plugin for all locales, we generate localized icons (using imagemagick) for Wikipedia, by putting the lang code in the bottom-right corner.
  • We also install all search plugins from Debian's ìceweasel-l10n-* for which we have langpacks. (Personally I'd happily drop these since they mainly seem to add commercial crap.)

So, we are not affected by this bug since we generate our own thing. However, I worked on integrating 6.0a5 into Tails yesterday and noticed a related issue (for us), namely that you now put your search engine plugins inside distribution/omni.ja, which makes it harder for us to *replace* them. I guess the best, touch, would be if we settled on the same search plugins, and we did nothing regarding this on Tails' side. I also realize that this is not the right ticket for this -- if you think there's an interest in harmonizing this I can open a new ticket.

If that helps you then let's look at it. However, I am not sure I understand you. We actually did not change anything with our switch from ESR38 to ESR45 which is why we have this bug :). Specifically, we don't put our search engines into distribution/omni.ja. If I extract e.g. the Linux 32bit en-US bundle, the only omni.ja files I get are Browser/omni.ja, Browser/browser/omni.ja and Browser/webapprt/omni.ja. The search engines are still in the second one.

comment:10 Changed 4 years ago by mcs

Status: needs_reviewneeds_revision

Kathy and I reviewed these changes (although we did not run a bundling step with them).
The changes look fine except we think that the chrome directory should be cleaned up after extracting the search plugin files from omni.ja (as is done for defaults), e.g., after:
mv chrome/en-US/locale/browser/searchplugins ~/build
add:
rm -rf chrome

comment:11 Changed 4 years ago by gk

Resolution: fixed
Status: needs_revisionclosed

Good catch. I changed the patched and merged the result to master (commit 94c6a1c73291f8f456249d9719de57ff002fe6fe) and hardened-builds (commit 467796a7b7901fc22f26716860ed6ed8d4102416).

Note: See TracTickets for help on using tickets.