Write (C++) patch for window resizing parts

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.

added TorBrowserTeam201611R component::applications/tor browser owner::arthuredelstein priority::medium resolution::fixed severity::normal sponsor::U status::closed tbb-fingerprinting tbb-torbutton-conversion type::task labels

Trac:
Owner: tbb-team to arthuredelstein
Status: new to assigned

Getting important SponsorU things on our August radar.

Trac:
Keywords: N/A deleted, TorBrowserTeam201608 added

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.

Trac:
Status: assigned to needs_review

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.

Trac:
Status: needs_review to needs_revision

It looks like https://github.com/arthuredelstein/tor-browser/commit/546fc5f6fc23b5bf89ea404cce632ec4a20ff9f7 ;)

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?

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.

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

Trac:
Keywords: TorBrowserTeam201608 deleted, TorBrowserTeam201609R added
Status: needs_revision to needs_review

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 (moved))

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.

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 (moved))

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

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 (moved))

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

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 (moved) and #20076 (moved)). The extensions are in Contents/Resources/distribution/extensions. Then I started Tor Browser.

Trac:
Status: needs_review to needs_revision

I cheated a bit because NEW IDENTITY should not work with your Torbutton patch just on top of master (as #14271 (moved) 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

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

Trac:
Keywords: TorBrowserTeam201609R deleted, TorBrowserTeam201609 added

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

Trac:
Keywords: TorBrowserTeam201609 deleted, TorBrowserTeam201609R added
Status: needs_revision to needs_review

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 (moved) 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.

Trac:
Keywords: TorBrowserTeam201609R deleted, TorBrowserTeam201609 added
Status: needs_review to needs_revision

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 (moved) 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.