Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#8478 closed defect (fixed)

Tor Browser Bundle on OS X 10.6 does not set resolution to a multiple of 200x100

Reported by: cypherpunks Owned by: mikeperry
Priority: Very High Milestone:
Component: TorBrowserButton Version:
Severity: Keywords: tbb-fingerprinting, tbb-rebase-regression, MikePerry201307, tbb-2.4-backport
Cc: gk, intrigeri@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Panopticlick and http://browserspy.dk/screen.php report a width of 1000 pixels and height of 571 pixels.

This behavior is observed immediately after unzipping TorBrowser into a clean directory and launching it.

Mac OS X 10.6.8, TorBrowser-2.3.25-5-osx-x86_64-en-US.zip

Child Tickets

Change History (34)

comment:1 Changed 7 years ago by cypherpunks

Component: TorBrowserButtonTor bundles/installation
Keywords: resolution macosx osx regression tbb-fingerprinting tbb-rebase-regression added
Owner: changed from mikeperry to erinn
Summary: Latest TorBrowser on OS X not resizing windows to a multiple of 200x100Tor Browser Bundle on OS X 10.6 does not set resolution to a multiple of 200x100

comment:2 Changed 7 years ago by mikeperry

Component: Tor bundles/installationTorBrowserButton
Keywords: resolution macosx osx regression removed
Owner: changed from erinn to mikeperry
Status: newassigned

This is not a bundles issue. You almost caused this to get ignored forever.

See also #6146 and #7255 (especially https://bugzilla.mozilla.org/show_bug.cgi?id=357725).

For the record, I also observe the same x71 resolution on my Mac.

comment:3 Changed 7 years ago by cypherpunks

Whoops. Too many things abbreviated TBB.

The link to the torbutton_set_window_size() function in one of the other bugs is outdated. This permalink points to the source for torbutton_set_window_size() as it looks right now: https://gitweb.torproject.org/torbutton.git/blob/98da166d26c6d79dbf89ed716425e5a9373f6aab:/src/chrome/content/torbutton.js#l2153

I browsed the commit history for torbutton.js. torbutton_do_resize() and torbutton_check_round() were removed. No recent changes to torbutton_set_window_size() though. I assume this bug comes from changes made in Firefox.

Where should we start debugging this?

comment:4 Changed 7 years ago by mikeperry

Keywords: MikePerry201304 added

See also #6146. One fix may solve them both.

comment:5 Changed 7 years ago by mikeperry

Priority: majorcritical

comment:6 Changed 7 years ago by mikeperry

Cc: gk added

comment:7 Changed 7 years ago by mikeperry

Keywords: MikePerry201305 added; MikePerry201304 removed

comment:8 Changed 7 years ago by gk

Just as a short work-in-progress report: If I take the current torbutton master and only change

if (!m_tb_tbb && m_tb_prefs.getBoolPref("extensions.torbutton.prompt_torbrowser")) {

to

if (true)) {

(which shows an "Important Torbutton Information" popup on start-up) then everything is fine. While this is not a viable option it might shed some light on the issue and help finding a fix...
If I open a new window in the same session (which does not trigger such a popup) then the weird height values appear in it.

comment:9 Changed 7 years ago by gk

The findings in the latter comment are only true for the TBB. Interestingly, the strategy outlined there fails if I just try the latest Torbutton in a clean new profile (Firefox version does not matter, I tested (with some modifications from 10 - current Nightly)). That popup is always shown there but still the height values are weird. I tracked that issue down to rev dbe7b143eeb9c3d181f329a2dd312ee6065ae433. Rev 19fb6d91c2610e988f90dbb3c08fa45fb573e2db is fine and if I reverted the changes in torbutton.js, crash-observer.js and preferences.js (in defaults/preferences) in the broken revision everything was working fine again. Somewhat surprisingly reverting the changes in crash-observer.js was necessary...

comment:10 Changed 7 years ago by gk

Having

setTimeout(function() {torbutton_set_window_size(browser.contentWindow);}, 0);

instead of

torbutton_set_window_size(browser.contentWindow);

helps as well. While this is much better than the popup mentioned in comment 8 the resizing is visible.

comment:11 Changed 7 years ago by gk

Two further failures: 1) David Baron's idea to cut the setTimeout lag further down does not seem to work here (see: http://dbaron.org/log/20100309-faster-timeouts). 2) xul-window-visible is fired too late (the window which is not properly resized yet is visible shortly).

comment:12 Changed 7 years ago by gk

Resizing the window in onStateChange() (with a STATE_STOP flag) (WebProgressListener) works for me on Linux and Mac OS X reliably without the resizing being visible ("xul-window-visible" is broadcasted in SetVisibility() in nsXULWindow.cpp which gets called by OnChromeLoaded() which in turn gets called in onStateChange() (see: nsWebShellWindow.cpp for the latter). Only on Windows the height is still smaller (2px!!, wtf) as expected. If I can't come up with a cleaner solution, resizing again with setTimeout(0) (still in onStateChange()) might be good enough as I doubt increasing the height by 2px once is annoying users. Doing that additional resizing with setTimeout(0) might be a good idea in general, as a "defense in depth", if the other hackery is still to racy or gets broken by some changes in underlying Mozilla code...

comment:13 Changed 7 years ago by gk

I finally found the reason for the 2px deviation on Windows: the Torbutton button on the toolbar is causing the re-rendering of the UI (neither the NoScript nor the HTTPS Everywhere button behave in this way). One can see this effect "live" if one removes and adds the icon from/to the currentSet. That might give us some new options to solve this issue in a "clean" way...

comment:14 Changed 7 years ago by gk

Until #8941 gets fixed I think I can mitigate that problem with a slightly different CSS rule for each affected icon (ugh). I'll prepare a patch with that in mind...

comment:15 Changed 7 years ago by gk

Status: assignedneeds_review

I tested the patch on Debian, Ubuntu, Mac OS X 10.6.8, Win 7 and Win 8 and with opening new windows, new identity and "normal" start-up. It works so far. It would be good if somebody could test on newer Macs, though.

Some things to note: 1) We might still think about an additional setTimeout(0) (which is not implemented yet). 2) I took the "P0-tag" seriously and hijacked the old webprogress listener. Not sure whether there were other preparations necessary before that should have happened. 3) The torbutton-disabled icon on the toolbar is a little bit cropped due to its 18x18 size while we only should use a height of 16px.

comment:16 Changed 7 years ago by mikeperry

Keywords: MikePerry201306 added; MikePerry201305 removed

Thanks a lot for this gk. However, I am a little concerned that you've switched us from resizing on new window events to the nsIWebProgress events with flag STATE_STOP. From your comments, it sounds like you're making use of the fact that XUL loading is triggering these progress events before content?

If so, that might be OK.. But otherwise it makes me a little nervous if we're depending on the content to finish loading before resizing.

I am also a little sad about adding a listener to listen to *all* nsIWebProgress events. If we could avoid the content-related ones somehow, that might be better for performance...

comment:17 in reply to:  16 Changed 7 years ago by gk

Status: needs_reviewneeds_revision

Replying to mikeperry:

Thanks a lot for this gk. However, I am a little concerned that you've switched us from resizing on new window events to the nsIWebProgress events with flag STATE_STOP. From your comments, it sounds like you're making use of the fact that XUL loading is triggering these progress events before content?

Yes.

If so, that might be OK.. But otherwise it makes me a little nervous if we're depending on the content to finish loading before resizing.

We don't. Looking at the URL aProgress.DOMWindow gives us it shows chrome://browser/content/browser.xul. My naive interpretation here would be that our resizing logic got called as browser.xul (i.e. the chrome) got loaded which is way before the update check is started or about:blank is loaded.

I am also a little sad about adding a listener to listen to *all* nsIWebProgress events. If we could avoid the content-related ones somehow, that might be better for performance...

Well, NOTIFY_STATE_ALL is not necessary, indeed. But NOTIFY_STATE_NETWORK fires too late (and probably NOTIFY_STATE_REQUEST as well although I have not tested that one). Thus, we won't avoid using NOTIFY_STATE_DOCUMENT. But that is not too problematic here as testing showed that onStateChange()| is called only once after we registered the listener. The performance impact should therefore be minimal if existing at all. I'll change the patch to use NOTIFY_STATE_DOCUMENT.

Another idea I got while thinking about the issues you raised: We could test the following: Instead of specifying torbutton.xul as the overlay in the chrome.manifest we could specify a dummy XUL file which actually does not contain any XUL at all but only includes a JS file which loads torbutton.xul dynamically (i.e. with |document.loadOverlay()|). This would give us the option to register an overlay observer that fires "xul-overlay-merged" where we could call our resizing logic. If that works out we might avoid patching torbutton.css as the 18x18 icons should already be merged before the resizing logic kicks in and don't need webprogress listeners. Maybe that approach is preferable for you. :)

comment:18 Changed 7 years ago by mikeperry

NOTIFY_STATE_DOCUMENT will also get called for web content too though, right? Probably multiple times for a page if there are lots of frames/iframes?

If that is the case, then I think we do want to go with the overlay route. It also sounds more narrowly scoped to exactly what we need?

comment:19 in reply to:  18 Changed 7 years ago by gk

Replying to mikeperry:

NOTIFY_STATE_DOCUMENT will also get called for web content too though, right? Probably multiple times for a page if there are lots of frames/iframes?

No, no. To avoid exactly that problem

progress.removeProgressListener(this);

exists. In theory, this should remove the progress listener after |onStateChange()| got called _once_ (which should be after the chrome got loaded but before the XUL window is made visible) and our resizing code got executed. It turns out that happens in practice as well. :)

If that is the case, then I think we do want to go with the overlay route. It also sounds more narrowly scoped to exactly what we need?

I'd love to go that road as well for the reason you mention. But unfortunately 'xul-overlay-merged' is broadcasted after the XUL window is already visible.

comment:20 Changed 7 years ago by gk

Status: needs_revisionneeds_review

Second shot. 1) in comment 15 is still worth considering as a fallback especially for users that customize their Torbrowser with additional addons.

comment:21 Changed 6 years ago by mikeperry

Status: needs_reviewneeds_revision

Hrmm. I was just about to apply this when I noticed that it looks like it removes the window.name clearing observer, so we either need to make a separate observer for that, find a different way to clear window.name, or use the XUL binding approach instead of this.

comment:22 Changed 6 years ago by gk

Status: needs_revisionneeds_review

Looks like that answered my second point in comment 15. Anyway, I chose the simplest approach for the time being and did not touch the other webprogress listener in the new patch.

comment:23 Changed 6 years ago by mikeperry

Keywords: MikePerry201307 added; MikePerry201306 removed

comment:24 Changed 6 years ago by mikeperry

Keywords: tbb-2.4-backport added
Resolution: fixed
Status: needs_reviewclosed

Ok, I've merged this. I'll be testing it this week in a new gitian build, and if all goes well, it should appear in 3.0alpha3, and we'll consider backporting it to Torbutton 1.5 and TBB-2.4.

Thanks, gk!

comment:25 Changed 6 years ago by intrigeri

We at Tails have backported the relevant commit (5e5c5bd41) into Torbutton 1.5.2. Without the patch, we get 1000x573. With the patch, one of us gets 1000x599, and the other gets 1000x594. Is this expected?

comment:26 Changed 6 years ago by intrigeri

Cc: intrigeri@… added

comment:27 in reply to:  25 ; Changed 6 years ago by gk

Replying to intrigeri:

We at Tails have backported the relevant commit (5e5c5bd41) into Torbutton 1.5.2. Without the patch, we get 1000x573. With the patch, one of us gets 1000x599, and the other gets 1000x594. Is this expected?

No. What happens if both use the vanilla TBB on the same computers?

comment:28 Changed 6 years ago by gk

I meant vanilla 3.0a4, btw.

comment:29 in reply to:  27 ; Changed 6 years ago by intrigeri

Replying to gk:

What happens if both use the vanilla TBB on the same computers?

Exactly the same for me: 1000x599. In case it matters, this happens in a libvirt+qemu VM, with QXL graphics and using Spice (spice-vdagent is running). The display inside the VM is set to 1280x768.

comment:30 in reply to:  29 ; Changed 6 years ago by gk

Replying to intrigeri:

Replying to gk:

What happens if both use the vanilla TBB on the same computers?

Exactly the same for me: 1000x599. In case it matters, this happens in a libvirt+qemu VM, with QXL graphics and using Spice (spice-vdagent is running). The display inside the VM is set to 1280x768.

What happens if you are using the small icons in the toolbar?

comment:31 in reply to:  30 Changed 6 years ago by intrigeri

Replying to gk:

What happens if you are using the small icons in the toolbar?

My current results (on Tails 0.21, in a libvirt+qemu VM, with QXL graphics and using Spice (spice-vdagent is running) with 3.0a4 are:

  • default toolbar icon size:
    • ip-check: screen = 1000 x 561, window = 1000 x 561 (changes to 1000 x 594 after closing the "plugin missing" bar)
    • panopticlick: screen size = 1000x600x24
  • small toolbar icons:
    • ip-check: screen = 1000 x 567, window = 1000 x 567 (changes to 1000 x 600 after closing "plugin missing" bar)
    • panopticlick: screen size = 1000x600x24
Note: See TracTickets for help on using tickets.