Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#19459 closed task (fixed)

Write (C++) patch for window resizing parts

Reported by: gk Owned by: arthuredelstein
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-torbutton-conversion, TorBrowserTeam201611R, tbb-fingerprinting
Cc: arthuredelstein, brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorU

Description

Resizing the window to multiples of 200x100 on start-up is one of the features in Torbutton we should get out of it and into Firefox. Getting away from the constraints if extension JS code might give us the means to finally solve this properly.

Child Tickets

Change History (53)

comment:1 Changed 3 years ago by arthuredelstein

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

comment:2 Changed 3 years ago by gk

Keywords: TorBrowserTeam201608 added

Getting important SponsorU things on our August radar.

comment:3 Changed 3 years ago by arthuredelstein

Status: assignedneeds_review

Here's my current patch: https://github.com/arthuredelstein/tor-browser/commit/19459

It's in JavaScript, rather than C++, mainly because the window-sizing startup code in Firefox is already in JavaScript.

I'm not entirely happy with the patch, because it uses a MutationObserver to alter the new window's dimensions after the dimensions are otherwise stabilized, but before the window is made visible. Unfortunately I wasn't able to find a more straightforward way to interject the resizing code. Perhaps during an uplift we can get advice on how to improve this approach.

Nonetheless, I do think the code is in approximately the right place (browser.js) and should be a workable substitute for our torbutton code.

Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:4 Changed 3 years ago by gk

Status: needs_reviewneeds_revision

So, the link gives me a 404. Thus, it is hard to say what the patch looks like :). That said, it seems there is at least the corresponding Torbutton one missing that backs the code we have out and just uses the Firefox one.

That said, I was under the impression we can't fix the resizing problems on start-up at least from JS land. Thus, I bet Mozilla won't be happy with it which is why I thought we should try the C++ road.

The needs_revision is at least for the missing Torbutton patch. Not sure about the JS vs. C++ issue.

comment:6 Changed 3 years ago by gk

Oh, another thing while looking at the code: we have a max width and max height of 1000 and we round to multiples of 200x100. What were you reasons for changing this?

comment:7 in reply to:  4 Changed 3 years ago by arthuredelstein

Replying to gk comment 4:

So, the link gives me a 404. Thus, it is hard to say what the patch looks like :).

Sorry! I've fixed the URL above.

That said, it seems there is at least the corresponding Torbutton one missing that backs the code we have out and just uses the Firefox one.

Yes, I'll submit that patch as well.

That said, I was under the impression we can't fix the resizing problems on start-up at least from JS land. Thus, I bet Mozilla won't be happy with it which is why I thought we should try the C++ road.

I couldn't rule out a C++ solution, but all the indications to me point to an entirely (or mostly) JS approach, because nearly all the existing code in Firefox for setting the dimensions of a new window are in JavaScript (browser/base/content/browser.js and friends).

Replying to gk comment 6:

Oh, another thing while looking at the code: we have a max width and max height of 1000 and we round to multiples of 200x100. What were you reasons for changing this?

The max width and height I chose to match as closely as possible Mozilla's latest preferred dimensions:
https://dxr.mozilla.org/mozilla-central/rev/1a5b53a831e5a6c20de1b081c774feb3ff76756c/browser/base/content/browser.js#963
but if we want to stick to 1000x1000, I'm also fine with that. I will also change the rounding to 200x100, which I had misremembered.

I'm working on a new (hopefully improved) version of the patch, so no need to review the last one further.

comment:8 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201609R added; TorBrowserTeam201608 removed
Status: needs_revisionneeds_review

Here's my new version:
https://github.com/arthuredelstein/tor-browser/commit/19459+5
I no longer need to use a MutationObserver or anything too nasty. Basically I found that if we set the tabBrowser's flex attribute to "", then the XULWindow adjusts its size to conform with whatever tabBrowser width and height we specify. Then we can set flex back to "1" after the window is first painted.

And here's the new-window-sizing code removed from torbutton:
https://github.com/arthuredelstein/torbutton/commit/19459

Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:9 Changed 3 years ago by gk

Just two quick questions regarding the Torbutton patch:

1) You remove the defense against an OS maximizing the window on start-up because it thinks the browser window is taking more space on the screen than a particular threshold determines (this happened with Ubuntu Unity on displays with low resolution IIRC). Is that a non-issue anymore? (was fixed in #7255)

2) You remove the option to customize the maxWidth and maxHeight. Do you think this is not necessary anymore as it was originally used for users that ran into corner cases we fixed? If so, I wonder whether it would be smart to keep those prefs and checks anyway to allow users a better option than to maximize.

comment:10 in reply to:  9 ; Changed 3 years ago by arthuredelstein

Replying to gk:

Just two quick questions regarding the Torbutton patch:

1) You remove the defense against an OS maximizing the window on start-up because it thinks the browser window is taking more space on the screen than a particular threshold determines (this happened with Ubuntu Unity on displays with low resolution IIRC). Is that a non-issue anymore? (was fixed in #7255)

Good question. I'm not certain, but I'm inclined to think that this issue was caused by the Firefox code, here:
https://gitweb.torproject.org/tor-browser.git/tree/browser/base/content/browser.js?h=tor-browser-45.3.0esr-6.5-1#n1012

This patch prevents that maximization case from running entirely when the "privacy.resistFingerprinting" pref is enabled.

(In the latest versions of mozilla-central, the maximization case was removed, which is why I am linked to the tor-browser repo instead.)

2) You remove the option to customize the maxWidth and maxHeight. Do you think this is not necessary anymore as it was originally used for users that ran into corner cases we fixed? If so, I wonder whether it would be smart to keep those prefs and checks anyway to allow users a better option than to maximize.

Yes, we can add that option to the browser patch, although I think we should always round the window size when fingerprinting resistance is enabled. Here's a new version:

https://github.com/arthuredelstein/tor-browser/commit/19459+6

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

Replying to arthuredelstein:

Replying to gk:

Just two quick questions regarding the Torbutton patch:

1) You remove the defense against an OS maximizing the window on start-up because it thinks the browser window is taking more space on the screen than a particular threshold determines (this happened with Ubuntu Unity on displays with low resolution IIRC). Is that a non-issue anymore? (was fixed in #7255)

Good question. I'm not certain, but I'm inclined to think that this issue was caused by the Firefox code, here:
https://gitweb.torproject.org/tor-browser.git/tree/browser/base/content/browser.js?h=tor-browser-45.3.0esr-6.5-1#n1012

This patch prevents that maximization case from running entirely when the "privacy.resistFingerprinting" pref is enabled.

(In the latest versions of mozilla-central, the maximization case was removed, which is why I am linked to the tor-browser repo instead.)

Interested. I'll double-check that but I think the case I have in mind is the underlying OS causing this.

2) You remove the option to customize the maxWidth and maxHeight. Do you think this is not necessary anymore as it was originally used for users that ran into corner cases we fixed? If so, I wonder whether it would be smart to keep those prefs and checks anyway to allow users a better option than to maximize.

Yes, we can add that option to the browser patch, although I think we should always round the window size when fingerprinting resistance is enabled. Here's a new version:

https://github.com/arthuredelstein/tor-browser/commit/19459+6

comment:12 Changed 3 years ago by gk

Status: needs_reviewneeds_revision

So, I tested this on my OS X testbox and I get always on a very first start 789x600. After New Identity 1200x600 shows up which is okay. After restarting I get most of the time 1200x600 but sometimes I end up with 335[sic!]x600. I've uploaded everything for reproducing the bug:

https://people.torproject.org/~gk/testbuilds/19459/TorBrowser-tbb-nightly-osx64_ALL_19459.dmg
https://people.torproject.org/~gk/testbuilds/19459/TorBrowser-tbb-nightly-osx64_ALL_19459.dmg.asc
https://people.torproject.org/~gk/testbuilds/19459/tor-launcher-0.2.8.5.xpi
https://people.torproject.org/~gk/testbuilds/19459/tor-launcher-0.2.8.5.xpi.asc
https://people.torproject.org/~gk/testbuilds/19459/torbutton-1.9.6.1.xpi
https://people.torproject.org/~gk/testbuilds/19459/torbutton-1.9.6.1.xpi.asc

I extracted the .dmg file on my Desktop, made sure there is no old TorBrowser-Data dir and copied the extensions over (Torbutton contains your patch on top of master and TorLauncher contains fixes for #20075 and #20076). The extensions are in Contents/Resources/distribution/extensions. Then I started Tor Browser.

comment:13 Changed 3 years ago by gk

I cheated a bit because NEW IDENTITY should not work with your Torbutton patch just on top of master (as #14271 is not merged yet).

Here comes an updated .xpi which still shows the same issue as in comment:12 for me:

https://people.torproject.org/~gk/testbuilds/19459/torbutton-1.9.6.1-new.xpi
https://people.torproject.org/~gk/testbuilds/19459/torbutton-1.9.6.1-new.xpi.asc

comment:14 Changed 3 years ago by gk

On a Windows 7 box I get 1200x762. On my work computer (Linux) it works fine.

comment:15 Changed 3 years ago by gk

Keywords: TorBrowserTeam201609 added; TorBrowserTeam201609R removed

comment:16 in reply to:  12 Changed 3 years ago by arthuredelstein

Replying to gk:

So, I tested this on my OS X testbox and I get always on a very first start 789x600. After New Identity 1200x600 shows up which is okay. After restarting I get most of the time 1200x600 but sometimes I end up with 335[sic!]x600. I've uploaded everything for reproducing the bug:

Thanks for testing this. I don't know why, but it seems Tor Browser builds the chrome window differently on first launch, before a profile has been created. Also, weirdly, while I could see the same first-launch window sizing behavior using your TorBrowser.app build, I couldn't reproduce it with my own tor-browser.git ./mach build. I was able to experiment by editing the browser.js inside your TorBrowser.app and find a way to accommodate both modes of building the window.

Also I now realize I don't need to use the flex attribute at all, so that simplifies things a little further.

Here is the new version. I have tested on OS X and Linux, but not Windows yet:
https://github.com/arthuredelstein/tor-browser/commit/19459+7

comment:17 Changed 3 years ago by gk

Keywords: TorBrowserTeam201609R added; TorBrowserTeam201609 removed
Status: needs_revisionneeds_review

comment:18 Changed 3 years ago by gk

Keywords: TorBrowserTeam201609 added; TorBrowserTeam201609R removed
Status: needs_reviewneeds_revision

Okay, I gave v7 another round of testing. On OS X my issue is gone, thanks! On Windows it basically remains: The width is good but the height is off (I get 730px or 710px).

Two additional things I'd like to see addressed:

1) When maximizing (e.g. by chance) and restarting the window is not resized to sane defaults. We should do that, though, at least until something for #14429 lands.

2) I realized you set maxWidth to 1200. While I generally think it could be smart to revisit the decision to cap that value at 1000, I'd like to see this happening in a different ticket. There we could think about a new maxWidth value based on real data. This would help making this move more transparent as well.

comment:19 in reply to:  18 ; Changed 3 years ago by arthuredelstein

Replying to gk:

Okay, I gave v7 another round of testing. On OS X my issue is gone, thanks! On Windows it basically remains: The width is good but the height is off (I get 730px or 710px).

Two additional things I'd like to see addressed:

1) When maximizing (e.g. by chance) and restarting the window is not resized to sane defaults. We should do that, though, at least until something for #14429 lands.

2) I realized you set maxWidth to 1200. While I generally think it could be smart to revisit the decision to cap that value at 1000, I'd like to see this happening in a different ticket. There we could think about a new maxWidth value based on real data. This would help making this move more transparent as well.

Here's a new patch that fixes these two issues:
https://github.com/arthuredelstein/tor-browser/commit/19459+8

I have partially isolated the Windows problem, but unfortunately I'm still struggling with it. When I set the gBrowser dimensions, like this:

gBrowser.flex = "";
gBrowser.height = 1000;

the window gets an incorrect extra margin at the bottom (on Windows only). I'm in the process of trying to debug to find the cause of the extra margin. It looks like it will need a C++ patch to fix this incorrect behavior.

comment:20 Changed 3 years ago by bugzilla

The problem seems to be not with JS and gBrowser, but +30px in container:

Torbutton INFO:  chromeWin 1016x725 container 1000x630 gBrowser.fullZoom 1X targetContent 1000x600 zoom 1X targetBrowser 1000x600 gBrowser 1000x600 content 1000x600

Subsequent resizing attempts are successful on Win 7.

comment:21 in reply to:  19 Changed 3 years ago by arthuredelstein

Replying to arthuredelstein:

Replying to gk:

Okay, I gave v7 another round of testing. On OS X my issue is gone, thanks! On Windows it basically remains: The width is good but the height is off (I get 730px or 710px).

Two additional things I'd like to see addressed:

1) When maximizing (e.g. by chance) and restarting the window is not resized to sane defaults. We should do that, though, at least until something for #14429 lands.

2) I realized you set maxWidth to 1200. While I generally think it could be smart to revisit the decision to cap that value at 1000, I'd like to see this happening in a different ticket. There we could think about a new maxWidth value based on real data. This would help making this move more transparent as well.

Here's a new patch that fixes these two issues:
https://github.com/arthuredelstein/tor-browser/commit/19459+8

I have partially isolated the Windows problem, but unfortunately I'm still struggling with it. When I set the gBrowser dimensions, like this:

gBrowser.flex = "";
gBrowser.height = 1000;

the window gets an incorrect extra margin at the bottom (on Windows only). I'm in the process of trying to debug to find the cause of the extra margin. It looks like it will need a C++ patch to fix this incorrect behavior.

I'm getting closer to a solution. The incorrect extra margin goes away when "browser.tabs.drawInTitlebar" is true. It appears that the sizing code is somehow not correctly taking the effect of a custom titlebar on the overall height into account.

comment:22 Changed 3 years ago by gk

Keywords: TorBrowserTeam201610 added; TorBrowserTeam201609 removed

Moving SponsorU items to October.

comment:23 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201610 removed
Status: needs_revisionneeds_review

Ouch, I'm admitting a humiliating defeat and switching to a C++ patch. :( I honestly thought a JavaScript patch was the right way to go, but it was not to be.

I tested this patch on Windows, Debian/xfce4 and OS X. It seems to be working on all 3.

https://github.com/arthuredelstein/tor-browser/commit/19459+10

I've left in some commented-out printf statements because they are highly useful for debugging. But we can remove them in the final version if it all looks OK.

comment:24 Changed 3 years ago by mcs

Cc: brade mcs added

comment:25 Changed 3 years ago by arthuredelstein

I forgot to mention that the torbutton patch remains the same, ​https://github.com/arthuredelstein/torbutton/commit/19459. Also I'm leaving out the maxWidth and maxHeight options for now, because I don't think they are likely to be needed.

comment:26 Changed 3 years ago by gk

Status: needs_reviewneeds_revision

Okay, testing on Linux I realized that the window does not get properly sized again after maximizing it once. Neither NEW IDENTITY nor restarting give me a rounded window again: I always end up with a maximized window. Resizing (but not maximizing) does not show the same problem.

Another thing somewhat related to 2) in comment:18: let's just port the current behavior and worry about adjusting the granularity of the window height in a different ticket.

comment:27 Changed 3 years ago by gk

Keywords: TorBrowserTeam201610 added; TorBrowserTeam201610R removed

comment:28 in reply to:  26 Changed 3 years ago by bugzilla

How about to make the window properly sized (keep the proper size) after adding/removing toolbars?

comment:29 in reply to:  26 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201610 removed
Status: needs_revisionneeds_review

Replying to gk:

Okay, testing on Linux I realized that the window does not get properly sized again after maximizing it once. Neither NEW IDENTITY nor restarting give me a rounded window again: I always end up with a maximized window. Resizing (but not maximizing) does not show the same problem.

Thanks for finding this. Here's a new version that I believe fixes this problem:

https://github.com/arthuredelstein/tor-browser/commit/19459+11

And here is the torbutton patch (same as before):
https://github.com/arthuredelstein/torbutton/commit/19459

comment:30 Changed 3 years ago by gk

Keywords: TorBrowserTeam201610 added; TorBrowserTeam201610R removed
Status: needs_reviewneeds_revision

The second part of comment:26 seems still to be unaddressed, no? I.e. let's keep the multiple of 200x100 for now and decide in a separate bug if we need to adjust that and if so how.

comment:31 in reply to:  30 Changed 3 years ago by arthuredelstein

Status: needs_revisionneeds_review

Replying to gk:

The second part of comment:26 seems still to be unaddressed, no? I.e. let's keep the multiple of 200x100 for now and decide in a separate bug if we need to adjust that and if so how.

Sorry! I had misinterpreted your comment. My mistake in any case. Here's the new version.
https://github.com/arthuredelstein/tor-browser/commit/19459+12

comment:32 Changed 3 years ago by gk

Okay, this looks mostly good to me I think. Two comments to the C++ code

1) We might want to add an NS_ENSURE_STATE(shellWindow); after and an NS_ENSURE_STATE(mPrimaryContentShell); before
nsCOMPtr<nsIBaseWindow> shellWindow(do_QueryInterface(mPrimaryContentShell));. There is code that does this in r306252 and this seems reasonable to me.

2) We might want to check the return value of GetAvailScreenSize() called in ResizeToBoundedDimensions() as well (as is done at the other place you use it)? Not sure what should happen in case this fails, though.

Regarding my testing: it looks good on all machines I tried it. I encounerted an issue I mentioned in comment:18 (1)) again but after digging a bit deeper it seems older versions have this problem, too. So, no regression at least. This is #18175 fwiw.

I think we can leave the debugging statements in for now.

The Torbutton patch looks good to me as well. Nice work!

mcs/brade could you have a look at the C++ portion as well?

comment:33 in reply to:  32 ; Changed 3 years ago by arthuredelstein

Replying to gk:

Thanks as always for testing and reviewing!

1) We might want to add an NS_ENSURE_STATE(shellWindow); after and an NS_ENSURE_STATE(mPrimaryContentShell); before
nsCOMPtr<nsIBaseWindow> shellWindow(do_QueryInterface(mPrimaryContentShell));. There is code that does this in r306252 and this seems reasonable to me.

Added.

2) We might want to check the return value of GetAvailScreenSize() called in ResizeToBoundedDimensions() as well (as is done at the other place you use it)? Not sure what should happen in case this fails, though.

I have added this as well. For now it causes ResizeToBoundedDimensions to abort.

Regarding my testing: it looks good on all machines I tried it. I encounerted an issue I mentioned in comment:18 (1)) again but after digging a bit deeper it seems older versions have this problem, too. So, no regression at least. This is #18175 fwiw.

I was able to reproduce this problem on a Windows machine and fixed it by adding a little piece of the old JS patch. It's annoying to me to have to add special-case code to two different files, but there is so much magic in Firefox window sizing that I don't know how else to do it. I wonder if this patch fixes the problem on your test system as well.

Here's the new version:
https://github.com/arthuredelstein/tor-browser/commit/19459+13

comment:34 Changed 3 years ago by gk

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201610 removed

comment:35 in reply to:  33 Changed 3 years ago by gk

Replying to arthuredelstein:

I was able to reproduce this problem on a Windows machine and fixed it by adding a little piece of the old JS patch. It's annoying to me to have to add special-case code to two different files, but there is so much magic in Firefox window sizing that I don't know how else to do it. I wonder if this patch fixes the problem on your test system as well.

Here's the new version:
https://github.com/arthuredelstein/tor-browser/commit/19459+13

It does, neat! One nit: could you put a comment above the JS code change explaining what is going on? (maybe pointing to #18175 additionally?)

comment:36 Changed 3 years ago by mcs

Kathy and I reviewed both patches. Here are a few comments on the Torbutton patch:

  • In torbutton_resizelistener.onStateChange, the container variable can be removed.
  • The extensions.torbutton.startup_resize_period pref (aka k_tb_tor_resize_warn_pref) is never set to false and its value is never read, so it seems like it can be removed too.
  • Since the call to quantizeBrowserSize() was removed, should the entire src/chrome/content/content-sizer.js file be removed? Or will that functionality be reinstated?

And here are our comments on the browser (C++) patch:

  • Please add a brief comment above nsXULWindow::ResizeToRoundedDimensions() to explain what it does (1000 x 1000 maximum, width constrained to a multiple of 200, etc.)
  • nsXULWindow::ResizeToRoundedDimensions() should have return NS_OK at the end.
  • Are we sure that it is okay to remove support for the extensions.torbutton.window.innerWidth / innerHeight prefs? With the new patches, there is no way for the user to override the initial window size unless they disable fingerprinting protection. That might be okay, but I think people who have set these prefs in the past will be surprised.

A related issue (but not really for this ticket) is that constraining the width to a multiple of 200 might be annoying on Android devices that have narrow screens (old phones)?

comment:37 Changed 3 years ago by gk

Keywords: TorBrowserTeam201610 added; TorBrowserTeam201610R removed

comment:38 Changed 3 years ago by bugzilla

Status: needs_reviewneeds_revision

comment:39 in reply to:  36 ; Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201610 removed
Status: needs_revisionneeds_review

Thanks, gk, mcs and brade for your reviews!

Replying to gk:

It does, neat! One nit: could you put a comment above the JS code change explaining what is going on? (maybe pointing to #18175 additionally?)

Good idea -- added.

Replying to mcs:

Kathy and I reviewed both patches. Here are a few comments on the Torbutton patch:

  • In torbutton_resizelistener.onStateChange, the container variable can be removed.

Done.

  • The extensions.torbutton.startup_resize_period pref (aka k_tb_tor_resize_warn_pref) is never set to false and its value is never read, so it seems like it can be removed too.

Removed.

  • Since the call to quantizeBrowserSize() was removed, should the entire src/chrome/content/content-sizer.js file be removed? Or will that functionality be reinstated?

Oops, I restored that function call.

And here are our comments on the browser (C++) patch:

  • Please add a brief comment above nsXULWindow::ResizeToRoundedDimensions() to explain what it does (1000 x 1000 maximum, width constrained to a multiple of 200, etc.)

Added.

  • nsXULWindow::ResizeToRoundedDimensions() should have return NS_OK at the end.

Fixed.

  • Are we sure that it is okay to remove support for the extensions.torbutton.window.innerWidth / innerHeight prefs? With the new patches, there is no way for the user to override the initial window size unless they disable fingerprinting protection. That might be okay, but I think people who have set these prefs in the past will be surprised.

I have added two prefs: privacy.window.maxInnerWidth and privacy.window.maxInnerHeight. Hopefully these will be enough to solve any such problems.

A related issue (but not really for this ticket) is that constraining the width to a multiple of 200 might be annoying on Android devices that have narrow screens (old phones)?

Good point. We could consider allowing smaller quantizations for small window sizes.

Here's the new version:

https://github.com/arthuredelstein/tor-browser/commit/19459+14
https://github.com/arthuredelstein/torbutton/commit/19459+1

comment:40 in reply to:  39 ; Changed 3 years ago by mcs

Replying to arthuredelstein:

I have added two prefs: privacy.window.maxInnerWidth and privacy.window.maxInnerHeight. Hopefully these will be enough to solve any such problems.

Kathy and I were actually more concerned about removal of extensions.torbutton.window.innerWidth / innerHeight. Without those prefs, there should be no way to get a window that is wider than 1000 pixels at startup, for example. I guess someone needs to make the call on whether it is okay to remove this functionality.

Here's the new version:

https://github.com/arthuredelstein/tor-browser/commit/19459+14
https://github.com/arthuredelstein/torbutton/commit/19459+1

The changes looked okay to Kathy and me, but when we tested on a Retina Mac with Sierra (OSX 10.12.1), we saw incorrect results (content window size was 1300 x 700 at startup). Here is the output from the nsXULWindow::ResizeToRoundedDimensions() printf logging:

scaling factor: 2.000000
window size: 2000 x 1558
avail screen size: 2880 x 1746
primary content shell: 200 x 200
target content size: 800, 200

window size: 2600 x 1558
avail screen size: 1440 x 873
primary content shell: 200 x 200

(notice the primary content shell output). If we comment out the call to quantizeBrowserSize() inside torbutton.js, the output looks correct (and the content window size is 1000 x 700):

scaling factor: 2.000000
window size: 2400 x 1604
avail screen size: 2880 x 1746
primary content shell: 2400 x 1400
target content size: 2000, 1400

window size: 2000 x 1604
avail screen size: 1440 x 873
primary content shell: 2000 x 1400

comment:41 in reply to:  40 ; Changed 3 years ago by arthuredelstein

Replying to mcs:

Thanks again for the review.

Kathy and I were actually more concerned about removal of extensions.torbutton.window.innerWidth / innerHeight. Without those prefs, there should be no way to get a window that is wider than 1000 pixels at startup, for example. I guess someone needs to make the call on whether it is okay to remove this functionality.

I have altered privacy.window.maxInnerWidth and privacy.window.maxInnerHeight so that they now replace the default 1000x1000. So the user can specify that the window be smaller or larger than the default, but it will also stay within the availScreen and always be rounded.

The changes looked okay to Kathy and me, but when we tested on a Retina Mac with Sierra (OSX 10.12.1), we saw incorrect results (content window size was 1300 x 700 at startup).

I can't exactly reproduce this result (maybe because I don't have Sierra), but I also saw an incorrect result on another version of OS X. It turns out that the code inside quantizeBrowserSize() was running too early and interfering with the startup resizing. So I added a setTimeout(..., 0) and the problem went away for me.

This revision:
https://github.com/arthuredelstein/tor-browser/commit/19459+15
https://github.com/arthuredelstein/torbutton/commit/19459+2

comment:42 Changed 3 years ago by gk

Keywords: TorBrowserTeam201611R added; TorBrowserTeam201610R removed

Moving review tickets to November.

comment:43 in reply to:  41 ; Changed 3 years ago by mcs

Replying to arthuredelstein:

I have altered privacy.window.maxInnerWidth and privacy.window.maxInnerHeight so that they now replace the default 1000x1000. So the user can specify that the window be smaller or larger than the default, but it will also stay within the availScreen and always be rounded.

That looks good; thanks!

The changes looked okay to Kathy and me, but when we tested on a Retina Mac with Sierra (OSX 10.12.1), we saw incorrect results (content window size was 1300 x 700 at startup).

I can't exactly reproduce this result (maybe because I don't have Sierra), but I also saw an incorrect result on another version of OS X. It turns out that the code inside quantizeBrowserSize() was running too early and interfering with the startup resizing. So I added a setTimeout(..., 0) and the problem went away for me.

Unfortunately, Kathy and I still see this problem. We also tested on an older OSX 10.11.6 system to remove Sierra and the Retina display from the equation, but the problem still occurred. It definitely looks like quantizeBrowserSize() running too early is causing the problem, because if I change the setTimeout() delay to 5000ms the window is sized correctly.

One more data point: if I start fresh (after removing the TorBrowser-Data directory), the window is correctly sized to 1000 x 1000 pixels the first time I start the browser but it is wrong on subsequent runs.

Kathy and I also tried applying your Torbutton patch to master to ensure that everything in our test setup was reasonably in sync (e.g., Torbutton and Tor Launcher) but that did not help.

comment:44 Changed 3 years ago by gk

Keywords: TorBrowserTeam201611 added; TorBrowserTeam201611R removed
Status: needs_reviewneeds_revision

comment:45 in reply to:  43 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201611R added; TorBrowserTeam201611 removed

Replying to mcs:

Thanks for your help working out these issues!

Unfortunately, Kathy and I still see this problem. We also tested on an older OSX 10.11.6 system to remove Sierra and the Retina display from the equation, but the problem still occurred. It definitely looks like quantizeBrowserSize() running too early is causing the problem, because if I change the setTimeout() delay to 5000ms the window is sized correctly.

Strange -- I can't seem to reproduce this on my Mac. I don't have Sierra, but as you say, you also saw the problem on an old system. Here's another version of the torbutton patch (rebased onto master) where I use a "xul-window-visible" observer instead of setTimeout(...,0). Maybe this will work on your system? Hopefully it's easy to test because you can keep the C++ build and just install the new xpi.

https://github.com/arthuredelstein/torbutton/commit/19459+3

If this doesn't work, I can also post a patch that retains a MutationObserver from the old torbutton code. It's an awkward hack so I'd rather figure out how to do this correctly without it.

comment:46 Changed 3 years ago by arthuredelstein

Status: needs_revisionneeds_review

comment:47 Changed 3 years ago by gk

mcs/brade: could you give the new patch a test? I am thinking of getting the fix for this ticket into 6.5a4.

comment:48 in reply to:  47 ; Changed 3 years ago by mcs

Replying to gk:

mcs/brade: could you give the new patch a test? I am thinking of getting the fix for this ticket into 6.5a4.

We did not forget but we were distracted with ESR 45.5 updater changes. In any case, we have now tested the latest torbutton patch and it fixes the problem we saw. Kathy and I think it is okay to include this in 6.5a4.

Here are a few things to consider for the future:

  • The prefs extensions.torbutton.window.maxWidth and extensions.torbutton.window.maxHeight should be removed from torbutton. We could include default values for privacy.window.maxInnerWidth and privacy.window.maxInnerHeight` in the browser, but if we do not want to encourage people to use them we should leave them out.
  • I don't remember why we limit the window width and height to 95% of the available screen dimensions. This leads to surprising behavior such as the window being 200 pixels less than it could be when I set privacy.window.maxInnerWidth to a large number such as 5000. If we just want to avoid opening with a full screen window, we could subtract a constant number of pixels (e.g, 20) instead of taking away 5% of the available space.
  • I am not sure if this is a Firefox bug or not, but it seems as though the EXPORTED_SYMBOLS are all available even if you do not include them on the left side of a Cu.import() statement. I notice this because you forgot to add observe. This leads me to think that some of the names in the utils.js module are too generic, e.g., observe and showDialog. In the long run we may want to move everything into a container object such as TorButtonUtil.

comment:49 in reply to:  48 Changed 3 years ago by arthuredelstein

Replying to mcs:

Replying to gk:

mcs/brade: could you give the new patch a test? I am thinking of getting the fix for this ticket into 6.5a4.

We did not forget but we were distracted with ESR 45.5 updater changes. In any case, we have now tested the latest torbutton patch and it fixes the problem we saw.

That's a relief! Thanks for testing.

  • The prefs extensions.torbutton.window.maxWidth and extensions.torbutton.window.maxHeight should be removed from torbutton. We could include default values for privacy.window.maxInnerWidth and privacy.window.maxInnerHeight` in the browser, but if we do not want to encourage people to use them we should leave them out.

Good point. I have removed them for now.

  • I am not sure if this is a Firefox bug or not, but it seems as though the EXPORTED_SYMBOLS are all available even if you do not include them on the left side of a Cu.import() statement. I notice this because you forgot to add observe. This leads me to think that some of the names in the utils.js module are too generic, e.g., observe and showDialog. In the long run we may want to move everything into a container object such as TorButtonUtil.

I wasn't aware of this behavior of Cu.import(), but after some research I found out that the canonical way to avoid polluting the namespace is to add an empty {} as a second argument:

let { controller } = Cu.import("resource://torbutton/modules/tor-control-port.js", {});

Here's a torbutton branch with a couple of followup patches to handle these things:
https://github.com/arthuredelstein/torbutton/commits/19459+4

  • I don't remember why we limit the window width and height to 95% of the available screen dimensions. This leads to surprising behavior such as the window being 200 pixels less than it could be when I set privacy.window.maxInnerWidth to a large number such as 5000. If we just want to avoid opening with a full screen window, we could subtract a constant number of pixels (e.g, 20) instead of taking away 5% of the available space.

I think the problem is (or was) that some linux flavors incorrectly report available width/height, ignoring the presence of a toolbar or the window's titlebar. Since we don't really know what these numbers, a percentage seemed to make sense. We could also subtract a constant number, but the problem is we don't know in advance how many pixels it should be.

comment:50 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

This looks good to me. Thanks for the quick follow-up, Arthur (I was about to open a new bug for the fixups) and for the thorough review mcs/brade. I pushed the Torbutton commits to master (105b155787944799a38ad2c48f0bd015b1fa032e, e33d7c3ae6752ca7b4d5a266a3424f59f483b26f, and 516ada586ebbceb736e9d40ff28425aeb0b226ae). The tor-browser changes are commits c8724d473603d63af82b1cf01d22dd5f7e4a8279 and 9d8658aed5e5cf6a1ed4b8cdbf1fcaa96bb875c7 on tor-browser-45.5.0esr-6.5-1.

comment:51 Changed 2 years ago by joebt

gk, not sure if this has landed in TBB 6.08 final (Win), but it's not working (100%) for me in Vista x64 hm. prem. There are so many bugs on "screen size not being reported / rounded correctly," I have no idea where to post my latest findings. Sorry if this isn't the best place (let me know & I'll move it).

Suffice to say, if Windows system DPI is NOT default 96, then TBB's height is never an increment of 100 or 200. Never has been, for yrs & yrs.

As of TBB 6.08, if Windows is 96 DPI, then test sites report 1000 w x 900 h, on a 4:3 widescreen, 1920 x 1080 monitor. If the goal is a (nearly, not quite) square TBB, it's sort of happening (at 96 DPI).

If Windows' DPI is say 110, then somehow TBB still rounds the width to 1000, but the height's never a multiple of 100. Height's always 729 or an odd number. Why can TBB still round the width correctly when Windows' DPI is changed, but not the height?

MANY people older / younger than 60 can't correct vision to 20/20 or have problems focusing on the default small font in Windows & all apps, if DPI is 96.
The default font (New Times Roman), 16 size in Firefox -when Windows DPI=96 is simply too small for many folks - young & old.

We can CHANGE the font in TBB/ Firefox, but AFAIK, that will also make you different - if sites are checking it - correct? Zooming a page is instantly detectable.

Thanks.

comment:52 in reply to:  51 Changed 2 years ago by gk

Replying to joebt:

gk, not sure if this has landed in TBB 6.08 final (Win), but it's not working (100%) for me in Vista x64 hm. prem. There are so many bugs on "screen size not being reported / rounded correctly," I have no idea where to post my latest findings. Sorry if this isn't the best place (let me know & I'll move it).

This fix has not landed in the 6.0.x series but I would like to know whether it solves your problem. Could you try the current alpha and report back? See: https://www.torproject.org/projects/torbrowser.html.en#downloads-alpha. I was about to reply to your mail to tor-talk. If you could report back in that thread this would be helpful. We'll see we might move the discussion to a different bug in this bug tracker then in case your problem is still not fixed in the alphas.

As of TBB 6.08, if Windows is 96 DPI, then test sites report 1000 w x 900 h, on a 4:3 widescreen, 1920 x 1080 monitor. If the goal is a (nearly, not quite) square TBB, it's sort of happening (at 96 DPI).

The goal is to have the width a multiple of 200 and the height a multiple of 100.

comment:53 Changed 2 years ago by arthuredelstein

Keywords: tbb-fingerprinting added
Note: See TracTickets for help on using tickets.