Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#10095 closed defect (fixed)

Setting screen resolution to a multiple of 200 x 100 is not working reliably with overlaid toolbar buttons

Reported by: gk Owned by: mikeperry
Priority: Very High Milestone:
Component: TorBrowserButton Version:
Severity: Keywords: tbb-fingerprinting, tbb-testcase, MikePerry201401R
Cc: intrigeri, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There are cropping more and more issues up with the patch developed in #8478. We should investigate either a better approach or tune the existing one to fix the problem reliably.

Child Tickets

Attachments (1)

panopticlick-screen-sizes.txt (14.1 KB) - added by cypherpunks 5 years ago.
(Updated with absolute values) All 1001 Panopticlick screen size results for easy grepping

Download all attachments as: .zip

Change History (42)

comment:1 Changed 6 years ago by gk

Some random notes:

The currently known issues are:
1) https://trac.torproject.org/projects/tor/ticket/8478#comment:25 ff.
2) #9738
3) beginning with TBB 3.0b1 there are regressions visible on my Linux box and on my Windows test computer as well.

At least 1) and 3) could be avoided if we explicitly chose the small icons for the toolbar.
It might be interesting to explore what caused the regression from TBB 3.0a4 to TBB 3.0b1, maybe that helps developing a better fix.
Don't forget to think about #9268 when writing a next generation fix.

comment:2 in reply to:  1 Changed 6 years ago by gk

Replying to gk:

3) beginning with TBB 3.0b1 there are regressions visible on my Linux box and on my Windows test computer as well.

That's due to the fancy new HTTPS Everywhere toolbar button, *sigh*.

comment:3 Changed 6 years ago by gk

I have been looking into this for the last days and have not found a way to solve this issue properly with extension means. The problem we face is described pretty well by https://bugzilla.mozilla.org/show_bug.cgi?id=870346#c1: The outerWindow dimensions are set, then the window is made visible and then depending on other extensions overlaying stuff the size of the innerWindow is adapted. Thus, having a fixed innerWindow size regardless the overlayed content of other extensions AND having the resizing we do not visible to the user is currently not possible.
If we want to keep our current model (resizing the inner dimensions to a multiple 200 x 100 and faking the outer dimensions accordingly) we have a bunch of ad-hoc solutions for this ticket. We could:

1) Use only small icons
2) Craft a specific HTTPS Everyhwere icon that does not enlarge the chrome area anymore
3) We could resize the window additionally with setTimeout(0)/or after xul-overlay-merged got fired. Thus, if the innerwindow has no multiple of 200 x 100 anymore after other overlays got merged we would enlarge the window a tiny bit again which would minimize the usability impact of it (compared to the scenario where we would enlarge the window from its original size to, say, 1000 x 900 having the window already visible).

I am not happy with either of these stopgaps, though, for various reasons (they don't solve the underlying issue; they won't be acceptable by Mozilla; the user might shoot herself in the foot with installing just one further legitimate extension...).

Last edited 5 years ago by gk (previous) (diff)

comment:4 Changed 6 years ago by gk

I think I go with 3) to provide a short term fix and try to make it as smooth as I can. Then we should follow up with Mozilla to get that feature upstream/implement a solution they can live with.

comment:5 Changed 6 years ago by gk

Interesting things to note: If a modal dialog is shown on start-up the dimensions are correct. The width and height are correct as well if one presses the New Identity button (without "needing" a modal dialog, i.e. that works in the default bundles, too). And: I am probably confronted with #9268 on my machine again: I only get 899 as max height although the algorithm says "900" while setting the value manually to "800" results in 800px as innerHeight.

comment:6 Changed 6 years ago by gk

Inspired by my previous comment I tried to load a modal dialog during startup but without it being visible before it gtes closed again. This works pretty well on Linux (surprisingly) and solves this issue there (tested on different computers with different distros) but alas on Windows the modal dialog is shortly visible. But even if I could fix this we would still have the Mac where all the crazy stuff is happening. It seems the content of about:tor gets loaded into that modal dialog instead of the browser window. So, this road seems blocked. But maybe the New Identity hint helps, like "clicking" the New Identity button before the browser window gets visible on start-up? If not, then I fall back to 3).

comment:7 Changed 5 years ago by cypherpunks

I've scraped Panopticlick results for Tor Browser 3.5 requests with screens that are 1000 px wide and between 400 and 1400 px high. The 9 px bug seems to affect many people!

screen = value of the video variable in the POST request to Panopticlick
requests = absolute number (calculated, Panopticlick itself shows relative numbers)

screenrequests
1000x600x2468
1000x591x2424
1000x891x2422
1000x700x2417
1000x900x2416
1000x691x2411
1000x590x249
1000x800x248
1000x500x246
1000x789x245
1000x790x245
1000x791x245
1000x883x245
1000x1000x245
1000x991x244
1000x787x243
1000x400x242
1000x572x242
1000x598x242
1000x599x242
1000x673x242
1000x798x242
1000x888x242
1000x892x242
1000x898x242
1000x1300x242
1000x508x241
1000x557x241
1000x561x241
1000x563x241
1000x565x241
1000x567x241
1000x574x241
1000x585x241
1000x587x241
1000x588x241
1000x592x241
1000x594x241
1000x595x241
1000x608x241
1000x612x241
1000x627x241
1000x639x241
1000x651x241
1000x687x241
1000x690x241
1000x699x241
1000x714x241
1000x721x241
1000x724x241
1000x728x241
1000x731x241
1000x752x241
1000x763x241
1000x769x241
1000x831x241
1000x850x241
1000x887x241
1000x890x241
1000x908x241
1000x964x241
1000x968x241
1000x999x241
1000x1054x241
1000x1211x241
1000x1216x241
1000x1240x241

(ommitting screens with zero requests)

Last edited 5 years ago by cypherpunks (previous) (diff)

Changed 5 years ago by cypherpunks

(Updated with absolute values) All 1001 Panopticlick screen size results for easy grepping

comment:8 Changed 5 years ago by cypherpunks

I've converted the bits/denominators into absolute numbers and updated the table and attachment above. It's pretty bad:

Only 45% of requests were correctly rounded to 100 px. This of course hurts even the people for which rounding does work by shrinking the anonymity set.

Looks like at the time the first window is resized, not all extensions' toolbar buttons have necessarily been drawn yet, varying from system to system. For example, 9 px missing height (24% of requests) happens like this I think:

  1. TB starts up and for some users (including me) HTTPS Everywhere presumably hasn't fully drawn its button yet.
  1. TB Button gets the new window notification, does the relative resize, and all is good until ...
  1. ... HTTPSE fully draws its button. With "Use Small Icons" disabled (= the default), the HTTPSE button + padding + margin + border is 9 px larger vertically than the other toolbar buttons. With "Use Small Icons" enabled, the the HTTPSE button is 1 px larger vertically.
  1. So either 9 or 1 px is stolen vertically from the innerHeight, and e.g. 1000x600 becomes 1000x591 or 1000x599.
  1. If you open a new window manually or through New Identity, HTTPSE is now fully initialized or something and TB Button will do a good stable resize.

Maybe just close the damned first window immediately and open a new one? And could someone ask upstream if there's some kind of "all extensions have initialized" notification?

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

Replying to cypherpunks:

Maybe just close the damned first window immediately and open a new one? And could someone ask upstream if there's some kind of "all extensions have initialized" notification?

The tricky thing is closing the first window before it is visible + getting the resizing done properly. I did not figure out a way to get that working on all supported platforms although I tried for quite a while. And your notification does not help as the window is shown *before* all extensions are initialized/the chrome is overlaid.

comment:10 Changed 5 years ago by cypherpunks

Maybe just close the damned first window immediately and open a new one?

The tricky thing is closing the first window before it is visible

Is that really neccessary? If the window is closed after it's visible then from a user's perspective there'll just be a sort of flicker. Seems okay until a better solution comes along?

(I know next to nothing about JS and Firefox extensions, so my own attempt ended up with an endless loop of windows opening and closing.)

And could someone ask upstream if there's some kind of "all extensions have initialized" notification?

And your notification does not help as the window is shown *before* all extensions are initialized/the chrome is overlaid.

Isn't that good as well? Window opens with a wrong size for a fraction of a second, we get the notification and do the resize, and it's stable because all extensions have been initialized.

The only case not completely fixed would be starting TB with an URL argument.

comment:11 in reply to:  10 Changed 5 years ago by gk

Keywords: tbb-testcase added
Status: newneeds_review

I implemented a fix according to option 3 in comment 3 and tested it on different Windows and Linux systems (Mac is following next Monday). See bug_10095 in my torbutton repo.

comment:12 Changed 5 years ago by cypherpunks

gk: What's the reasoning for the setTimeout(0) in the commit besides introducing a tiny delay for the dispatch? It doesn't work here, but setTimeout(10000) does (until the day when system load happens to be very heavy, I guess).

comment:13 Changed 5 years ago by gk

Status: needs_reviewneeds_revision

It was the suggestion of a Mozilla dev in https://bugzilla.mozilla.org/show_bug.cgi?id=870346 and should be a minimal annoying solution for the users (i.e. the time between having the window visible and resizing it (again) should ideally be non-existent). But it seems that road is blocked as well. Thanks for testing.

comment:14 Changed 5 years ago by cypherpunks

The Mozilla dev was wondering about "a setTimeout(0) that's set from onload", not sure what it means in this context

Sorry, got it.

Last edited 5 years ago by cypherpunks (previous) (diff)

comment:15 Changed 5 years ago by cypherpunks

Inspired by my previous comment I tried to load a modal dialog during startup but without it being visible before it gtes closed again. This works pretty well on Linux (surprisingly) and solves this issue there (tested on different computers with different distros) but alas on Windows the modal dialog is shortly visible.

Could you upload that code to your repo? What you describe sounds pretty good now.

comment:16 Changed 5 years ago by gk

Status: needs_revisionneeds_review

Okay, new approach: mutation observer + setTimeout(0). See bug_10095_v2 in my public repo.
cypherpunks: Does that one work now on your machine? And I don't have the modal-dialog-branch locally anymore.

comment:17 Changed 5 years ago by cypherpunks

Ding dong, the witch is dead! :)

Works perfectly for me, BTW it does so even without the setTimeout(0) wrapper.

comment:18 in reply to:  17 ; Changed 5 years ago by gk

Replying to cypherpunks:

Ding dong, the witch is dead! :)

Nice! But it is a nasty witch which came back several times already. Thus, we need to be cautious here :)

Works perfectly for me, BTW it does so even without the setTimeout(0) wrapper.

Good. I probably like to keep that wrapper for a safety margin, though...

intrigeri: Could you test that patch within your/TAILS' environment?

comment:19 in reply to:  18 ; Changed 5 years ago by intrigeri

Replying to gk:

intrigeri: Could you test that patch within your/TAILS' environment?

Sure.

comment:20 in reply to:  19 ; Changed 5 years ago by cypherpunks

Replying to intrigeri:

  • panopticlick reports screen size = 1000x599


Does the Tails browser "Use Small Icons"?

Last edited 5 years ago by cypherpunks (previous) (diff)

comment:21 in reply to:  20 ; Changed 5 years ago by intrigeri

Replying to cypherpunks:

Does the Tails browser "Use Small Icons"?

No. Once I enable "Use Small Icons", I get 1000x600 in both tests, woo! :)

Should I conclude that the proposed patch depends on this option to be set, and that we have to change it in Tails?

comment:22 in reply to:  21 ; Changed 5 years ago by cypherpunks

Should I conclude that the proposed patch depends on this option to be set, and that we have to change it in Tails?

No no, I just encountered missing 1px when that option is set.

Could you set extensions.torbutton.loglevel to 3 before startup, and show what's logged in these lines

  • "About to resize new window:"
  • "Got max dimensions:"
  • "Resized new window from:"
  • "Mutation observer: Window dimensions are:"
  • "Mutation observer: Window dimensions are (after resizing again):"

one time with small and one time with regular sized icons (small icons would also have to be enabled before startup)?

comment:23 in reply to:  21 ; Changed 5 years ago by cypherpunks

Wait a sec, Tails ships Adblock Plus, right? Maybe retry with that icon removed from the toolbar...

comment:24 in reply to:  19 ; Changed 5 years ago by gk

Replying to intrigeri:

Replying to gk:

intrigeri: Could you test that patch within your/TAILS' environment?

Sure.

Works for me with TAILS (even without the new patch). Thus, I guess your issue might be #9268. Could that be the case? You could test whether the resizing logic works on your machine if you replace

      height = Math.floor(maxHeight/100.0)*100;

with

      height = 500;

in torbutton.js.

comment:25 in reply to:  24 ; Changed 5 years ago by intrigeri

Replying to gk:

Works for me with TAILS (even without the new patch). Thus, I guess your issue might be #9268. Could that be the case?

I've no idea. Could #9268 account for a 1-pixel offset?

You could test whether the resizing logic works on your machine if you replace

      height = Math.floor(maxHeight/100.0)*100;

with

      height = 500;

in torbutton.js.

If I do this, then I get 1000x500 both with and without the new patch.

comment:26 in reply to:  23 Changed 5 years ago by intrigeri

Replying to cypherpunks:

Wait a sec, Tails ships Adblock Plus, right?

Correct.

Maybe retry with that icon removed from the toolbar...

Tails hides the ABP icon with extensions.adblockplus.showintoolbar=false.
We also hide the HTTPS Everywhere and Foxyproxy toolbar icons via userChrome.css.

With normal sized icons:

  • Tails 0.22.1~rc1 + the new patch: fail
  • Tails 0.22.1~rc1 + the new patch; ABP, Cookie Monster and FoxyProxy disabled: fail
  • Tails 0.22.1~rc1 + the new patch; NoScript and HTTPS Everywhere disabled: fail
  • Tails 0.22.1~rc1 + the new patch; ABP, Cookie Monster, FoxyProxy, NoScript and HTTPS Everywhere disabled: success

So indeed, it seems that some extension of ours might be messing up with the toolbar size in a way that Torbutton does not detect.

I've not tried disabling add-ons in small icons mode (since it works fine with all extensions enabled + the new patch).

comment:27 in reply to:  22 ; Changed 5 years ago by intrigeri

Replying to cypherpunks:

Could you set extensions.torbutton.loglevel to 3 before startup, and show what's logged in these lines [...]

If it is still useful that I do this, then I'm happy too. Is it?

Note that all my tests were done with Tails 0.22.1~rc1, in a VM that is given a 1280x768 monitor. My eyes think that the browser window takes all the available vertical space.

Last edited 5 years ago by intrigeri (previous) (diff)

comment:28 in reply to:  27 Changed 5 years ago by cypherpunks

Replying to intrigeri:

Could you set extensions.torbutton.loglevel to 3 before startup, and show what's logged in these lines [...]

If it is still useful that I do this, then I'm happy too. Is it?

I was hoping there's some kind of clue somewhere. But who knows.

Last edited 5 years ago by cypherpunks (previous) (diff)

comment:29 in reply to:  25 ; Changed 5 years ago by gk

Replying to intrigeri:

Replying to gk:

Works for me with TAILS (even without the new patch). Thus, I guess your issue might be #9268. Could that be the case?

I've no idea. Could #9268 account for a 1-pixel offset?

Yes, happened to me. Could you remove the panel/toolbar at the bottom in order to check if that additional space "solves" the problem?

You could test whether the resizing logic works on your machine if you replace

      height = Math.floor(maxHeight/100.0)*100;

with

      height = 500;

in torbutton.js.

If I do this, then I get 1000x500 both with and without the new patch.

Good. So the code is generally working but not in your particular situation.

comment:30 Changed 5 years ago by mcs

Cc: mcs added

comment:31 Changed 5 years ago by gk

Summary: Setting screen resolution to a multiple of 200 x 100 is not working reliably with patch in #8478Setting screen resolution to a multiple of 200 x 100 is not working reliably with overlaid toolbar buttons

FWIW: This patch fixed the problem as well for TBB running on Windows in a Virtualbox environment. But the Ubuntu counterpart is horribly broken which I'll track in an other bug.

Last edited 5 years ago by gk (previous) (diff)

comment:32 in reply to:  31 Changed 5 years ago by gk

Replying to gk:

FWIW: This patch fixed the problem as well for TBB running on Windows in a Virtualbox environment. But the Ubuntu counterpart is horribly broken which I'll track in an other bug.

No need to do so. It seems missing/broken Guest Additions where the reason for this issue. After reinstalling them everything works as expected.

comment:33 in reply to:  29 ; Changed 5 years ago by intrigeri

Replying to gk:

Replying to intrigeri:

Replying to gk:

Thus, I guess your issue might be #9268. Could that be the case?

I've no idea. Could #9268 account for a 1-pixel offset?

Yes, happened to me. Could you remove the panel/toolbar at the bottom in order to check if that additional space "solves" the problem?

It does solve the problem: both with and without the patch I get 1000x500.

comment:34 in reply to:  33 ; Changed 5 years ago by gk

Replying to intrigeri:

Replying to gk:

Replying to intrigeri:

Replying to gk:

Thus, I guess your issue might be #9268. Could that be the case?

I've no idea. Could #9268 account for a 1-pixel offset?

Yes, happened to me. Could you remove the panel/toolbar at the bottom in order to check if that additional space "solves" the problem?

It does solve the problem: both with and without the patch I get 1000x500.

You mean 1000x600? As this (and not the test with height = 500 which was working without removing toolbars) is the one I am interested in.

comment:35 in reply to:  34 ; Changed 5 years ago by intrigeri

Replying to gk:

You mean 1000x600? As this (and not the test with height = 500 which was working without removing toolbars) is the one I am interested in.

OK, let me clarify. On Tails 0.22.1~rc1, without patching Torbutton at all, without changing the browser configuration (icons size, add-ons) at all, I get 1000x600 or 1000x500, depending on the size of the screen given to my VM.

comment:36 in reply to:  35 Changed 5 years ago by gk

Replying to intrigeri:

Replying to gk:

You mean 1000x600? As this (and not the test with height = 500 which was working without removing toolbars) is the one I am interested in.

OK, let me clarify. On Tails 0.22.1~rc1, without patching Torbutton at all, without changing the browser configuration (icons size, add-ons) at all, I get 1000x600 or 1000x500, depending on the size of the screen given to my VM.

Okay, thanks. Your problem seems then indeed to be caused by #9268.

comment:37 Changed 5 years ago by cypherpunks

So why not push the patch as is for TBB 3.5.1 / Tails 0.22.1 (and possibly set small icons on the latter as a workaround for the x768 resolution case)?

Some future logic to fully take toolbars into consideration would be independent of this.

A few weeks after it's integrated in both TBB and Tails, I'll run Panopticlick scrape no. 2, and then another few weeks later scrape no. 3, then subtract 2 from 3 to get some data on how the situation has (hopefully) improved.

comment:38 in reply to:  37 Changed 5 years ago by intrigeri

Replying to cypherpunks:

So why not push the patch as is for TBB 3.5.1 / Tails 0.22.1 (and possibly set small icons on the latter as a workaround for the x768 resolution case)?

Agreed for Tails. I'm on it.

comment:39 Changed 5 years ago by gk

Keywords: MikePerry201401R added

comment:40 Changed 5 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

This is merged in 3.5.2. I still have issues on mac due to a large dock size, possibly also #9268.

comment:41 in reply to:  37 Changed 5 years ago by cypherpunks

Replying to cypherpunks:

A few weeks after it's integrated in both TBB and Tails, I'll run Panopticlick scrape no. 2, and then another few weeks later scrape no. 3, then subtract 2 from 3 to get some data on how the situation has (hopefully) improved.

Okay, two scrapes on 2014-03-02 and 2014-03-12, looking again at Tor-Browser-ish requests with screens between 1000x400x24 and 1000x1400x24.

Good news: 83% of that period's requests were rounded to 100px! (before: 45%)

screenrequests
1000x600x24103
1000x900x2459
1000x700x2430
1000x800x2416
1000x1000x2414
1000x400x247
1000x777x244
1000x878x243
1000x822x243
1000x792x242
1000x711x242
1000x611x242
1000x500x242
1000x687x241
1000x866x241
1000x783x241
1000x877x241
1000x738x241
1000x879x241
1000x702x241
1000x891x241
1000x697x241
1000x691x241
1000x808x241
1000x679x241
1000x1025x241
1000x643x241
1000x635x241
1000x895x241
1000x602x241
1000x938x241
1000x599x241
1000x594x241
1000x589x241
1000x584x241
1000x1035x241
1000x579x241
1000x578x241
1000x577x241
1000x543x241
1000x978x241
1000x992x241
1000x1400x241
1000x1238x241
1000x1044x241
1000x1223x241
1000x1173x241
Note: See TracTickets for help on using tickets.