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

added MikePerry201307 component::torbrowserbutton owner::mikeperry priority::very high resolution::fixed status::closed tbb-2.4-backport tbb-fingerprinting tbb-rebase-regression type::defect labels

Trac:
Owner: mikeperry to erinn
Keywords: N/A deleted, macosx, tbb-fingerprinting, osx, resolution, tbb-rebase-regression, regression added
Summary: Latest TorBrowser on OS X not resizing windows to a multiple of 200x100 to Tor Browser Bundle on OS X 10.6 does not set resolution to a multiple of 200x100
Component: TorBrowserButton to Tor bundles/installation

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

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

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

Trac:
Component: Tor bundles/installation to TorBrowserButton
Status: new to assigned
Keywords: resolution, macosx, regression, osx deleted, N/A added
Owner: erinn to mikeperry

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?

See also #6146 (closed). One fix may solve them both.

Trac:
Keywords: N/A deleted, MikePerry201304 added

Trac:
Priority: major to critical

Trac:
Cc: N/A to gk

Trac:
Keywords: MikePerry201304 deleted, MikePerry201305 added

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.

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...

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.

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

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...

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...

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

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.

Trac:
Status: assigned to needs_review

Trac:
0001-fix-for-bug-8478-which-a-moves-the-resizing-logic-to.patch

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...

Trac:
Keywords: MikePerry201305 deleted, MikePerry201306 added

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. :)

Trac:
Status: needs_review to needs_revision

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?

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

Child items 0

Activity