Opened 14 months ago

Last modified 2 months ago

#30832 needs_review defect

Fix tor-browser tbb-tests

Reported by: acat Owned by: acat
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ReleaseTrainMigration, TorBrowserTeam202006R
Cc: tbb-team Actual Points: 3
Parent ID: #33654 Points:
Reviewer: gk,sysrqb Sponsor: Sponsor58-must

Description

With current rebased tor-browser ESR68 branch I can only run tbb-tests (with run-tbb-tests script) when pref("network.file.path_blacklist", "/net") is removed and pref("extensions.torbutton.use_nontor_proxy", true); is set, apart from disabling tor-launcher. The second pref disables the domain isolator, which makes sense since it expects SOCKS5 proxies, but mochitests override that. For the other pref, not sure why network.file.path_blacklist needs to be unset (at least for Linux).

We could put these prefs in testing/marionette/prefs/marionette.js so that tests can be run (unless there is a simpler way to get the tests tor run that I'm missing).

Child Tickets

TicketStatusOwnerSummaryComponent
#17809newtbb-teamtbb-tests/browser_tor_TB4.js is out-of-dateApplications/Tor Browser
#19670newtbb-teamRegression test for blocking of Components.interfaces failsApplications/Tor Browser

Change History (15)

comment:1 Changed 12 months ago by pili

Sponsor: Sponsor44-can

Adding Sponsor 44 to ESR68 tickets

comment:2 Changed 4 months ago by acat

Sponsor: Sponsor44-canSponsor58

comment:3 Changed 4 months ago by acat

Summary: Fix tor-browser tbb-tests for ESR68Fix tor-browser tbb-tests

comment:4 Changed 4 months ago by acat

Keywords: ReleaseTrainMigration added; ff68-esr removed

Removing ff68-esr keyword, I think it's more realistic to aim for ReleaseTrainMigration.

comment:5 Changed 4 months ago by acat

Actual Points: 2

This took longer than expected.

Here is a branch with 6 fixups for tests: https://github.com/acatarineu/tor-browser/commits/30832.

  • f5202200d1ecdc56f0437f83210cb292ae40559e: the old <script> tags were not working, and I had to set a targetOrigin of one postMessage to *, otherwise it would complain about target being a different origin.
  • c817518a544e1fada4a55f9ad3a064ed433ba90c: these are already tested in tbb-testsuite, so leaving just a few just to check that our prefs file is applied. Also removed a couple of files in tbb-tests/browser.ini which still do not exist at this point.
  • 581241ecfd605045561a73b648d1f3e017008c01: update to new async API and add test to browser.ini.
  • f452d6bbc04fc891b05a99173edaf408674b0012: update APIs and add test to browser.ini.
  • c73bfe7f3b21421c7349267f2abac9f90f503b73: we need layout.css.line-height.normal-as-resolved-value.enabled = false, otherwise line-height property is read as normal and we cannot get the number in pixels. Also found that I needed to apply a Math.round to the read fontSize, otherwise the calculated values would not match.
  • 2e845b4c67ab78ca5096fd67fb9c031af9a34aac: ignoring onboarding tests, changed logic for getting the starting commit in run-tbb-tests and added a few necessary prefs.

comment:6 in reply to:  5 ; Changed 4 months ago by Thorin

Replying to acat:

  • c73bfe7f3b21421c7349267f2abac9f90f503b73: we need layout.css.line-height.normal-as-resolved-value.enabled = false, otherwise line-height property is read as normal and we cannot get the number in pixels

You should update that to fall back to clientRect (which I do on TZP): you'll need to anyway once you are based on ESR78 (or rapid release)

comment:7 in reply to:  6 ; Changed 4 months ago by acat

Actual Points: 23

Replying to Thorin:

Replying to acat:

  • c73bfe7f3b21421c7349267f2abac9f90f503b73: we need layout.css.line-height.normal-as-resolved-value.enabled = false, otherwise line-height property is read as normal and we cannot get the number in pixels

You should update that to fall back to clientRect (which I do on TZP): you'll need to anyway once you are based on ESR78 (or rapid release)

Thanks for the suggestion. I spent some time playing with getBoundingClientRect() to find out the line-height, but I saw some inconsistent behaviour. One of the testscases in test_tor_bug23104.html has arabic characters, which sets its font to Noto Naskh Arabic. That one, despite reporting a line-height of 20.4px, when measured via getBoundingClientRect() it turns out to be 29px (using a div to measure, instead of a span, the height of which is not affected by line-height). I assume that there must be some metric of the arabic font which is affecting the actual height of the element, which is different than the reported in line-height.

I'm not sure if the issue that #23104 tried to fix is still there, and can be measured via getBoundingClientRect or similar. In any case, the fact that the actual height does not match the line-height with some fonts (e.g. Noto Naskh Arabic) seems to be a different issue. So perhaps we can just leave the test as it is for now (using layout.css.line-height.normal-as-resolved-value.enabled = false) and investigate it later.

Still, I was thinking that the issues in #23104 and #29563 only allow distinguishing platform, but not subplatform, right? If that's the case, perhaps we could consider just dropping the patch for #23104, as leaking the os (via fonts?) is not something that we can really avoid.

Last edited 4 months ago by acat (previous) (diff)

comment:8 in reply to:  7 Changed 4 months ago by Thorin

Replying to acat:

using a div to measure, instead of a span

Yeah. ... dcf's unicode glyphs test (which is also used as a base on TZP) - https://www.bamsoftware.com/talks/fc15-fontfp/fontfp.html#demo

From my notes
Read the span width, but the div height. Firefox always reports the same value for the span's offsetHeight, even if the div around it is changing size

---

I don't see a problem with "leaking" the major OS (Win, Mac, Android, Linux: we'll never stop that), its the entropy across Linux (and Android for now, see below) that worries me. And we should always make it as hard as possible for the bastards. The patch is still protecting most/all users from alternative measuring - e.g. domrect.

FWIW: I also see occasional weird measurements, e.g. with textMetrics, with various codepoints and/or fonts: still trying to pinpoint exactly what it is: but I think it might mainly be to do with right-to-left.

I'm not sure I see the issue with Noto Naskh Arabic: if all users are the same, then it's OK. Sure, it's not 19.2, but it might just be an odd outlier (for everyone?). If someone is spoofing as English - their default font is Times New Roman. And if they're not, then they are already different: navigator languages etc - e.g. TB-fa. The question is, is everyone the same?

#23104 also lacks parity effectiveness in Android (and least on my phone): it's doing something (toggle RFP) but it sure isn't 19.2 either. I sent sysrqb an email about it months ago (last December) - it's something to with the 19.2 is only returned at certain zoom levels: and on my Android phone, the devicePixelRatio is 2.609... and it screws up - so it's really a case of this being a symptom of #29563

---

At the end of the day, even once ESR78 is used (or you flip the pref early: there's nothing to say you can't do that now), I would still keep the patch: because domrect can still measure it

Keep the test, close this, and re-purpose #29563 to deal with anomalies (getting it always 19.2 across the board would neutralize domrect in lineheight)?

comment:9 Changed 4 months ago by pili

Parent ID: #33654

comment:10 Changed 3 months ago by pili

Sponsor: Sponsor58Sponsor58-must

comment:11 Changed 3 months ago by gaba

Owner: changed from tbb-team to acat
Status: newassigned

comment:12 Changed 3 months ago by acat

Cc: tbb-team added
Keywords: TorBrowserTeam202005R added
Status: assignedneeds_review

comment:13 Changed 3 months ago by acat

Reviewer: gk

comment:14 Changed 2 months ago by gk

Keywords: TorBrowserTeam202006R added; TorBrowserTeam202005R removed

Moving review tickets.

comment:15 Changed 2 months ago by acat

Reviewer: gkgk,sysrqb
Note: See TracTickets for help on using tickets.