Redirects sometimes seem to break circuit status UI
If you go to https://bugs.torproject.org, you will be redirected to https://trac.torproject.org, and in some cases the circuit status UI will not display a circuit for you. It seems that even subsequent visits to trac.torproject.org will not display a circuit. Others have reported similar issues with google.com country redirects.
This happens with TBB 4.5a5, but is not reliably reproducible. Sometimes the UI actually displays a circuit after the redirect.
Activity
I've seen this too, in 4.5a4 on Debian. I went to https://www.google.com/ and got redirected to https://www.google.de (German exit relay selected). Tor Button was not showing circuit info for this tab. I had a hidden service opened in another tab, there Tor Button was showing circuit info just fine. Refreshed / reconnected few times to google, still couldn't see circuit data. Restarted Tor Browser, and it worked.
Tried again and I couldn't reproduce it again. Tried on websites which use all sorts of redirects but just couldn't reproduce.
Something confusing could be with providers like google who ship multiple services with one account. For example, I go to gmail.com and I am redirected to enter credentials to accounts.google.com/whatever. After that, I am redirected again to mail.google.com. From here I can navigate to other services related to the same account, such as drive.google.com or calendar or maps. Each will have a different circuit in this case. Currently it works, but there are some services which log you out if IP address changes during a session.
I've recorded quite some debug logs over the past weeks and think we can do something about 99% of the cases. There are race conditions at work here which I am not done tracking down the reason for yet. For those running into it it often helps to switch active tabs (getting a different tab to the foreground and then the tab in question again).
Nevertheless, we might want to sneak some easy workaround into 5.0 as we got numerous reports back by confused users.
Trac:
Status: new to assigned
Keywords: N/A deleted, tbb-5.0 added
Owner: arthuredelstein to gk-
The problem for the majority of the failures I've seen is that watching for the STREAM event is racing with updating the circuit display resulting sometimes in logs like the following:
[06-12 08:09:29] Torbutton INFO: updating circuit displayThis is right at the beginning of
updateCircuitDisplay()[06-12 08:09:29] Torbutton INFO: got credentials zeit.de:2This got logged right at the beginning of the
if (credentials)block in the same function; but we don't have something fornodeDatayet ascredentialsToNodeDataMapis not ready yet, hence[06-12 08:09:29] Torbutton INFO: about to show the display with: undefinedwhich is logged by
logger.eclog(3, "about to show the display with: " + (credentials && nodeData));immediately before
showCircuitDisplay()is called. Finally,credentialsToNodeDataMapgets updated:[06-12 08:09:29] Torbutton INFO: udpating nodeDataForCircuitThis got logged in the
if (credentials)block after STREAM SENTCONNECT arrived. -
Thinking a bit about it what speaks against updating the circuit info/making the decision whether to show the circuit or not only when the user is clicking on the green onion and opening the Torbutton menu? There seems to be no need to make that call already once a tab gets selected for instance. Putting that into the onpopupshowing handler or something similar should cope with the possible race conditions and avoid any code execution wrt the display until a user really wants to see it. Not sure if there would be some lag while updating the display but looking at the involved operations a doubt that.
Trac:
Cc: N/A to arthuredelstein
Status: assigned to needs_information Replying to gk:
The problem for the majority of the failures I've seen is that watching for the STREAM event is racing with updating the circuit display resulting sometimes in logs like the following: {{{ [06-12 08:09:29] Torbutton INFO: updating circuit display }}} This is right at the beginning of
updateCircuitDisplay(){{{ [06-12 08:09:29] Torbutton INFO: got credentials zeit.de:2 }}} This got logged right at the beginning of theif (credentials)block in the same function; but we don't have something fornodeDatayet ascredentialsToNodeDataMapis not ready yet, hence {{{ [06-12 08:09:29] Torbutton INFO: about to show the display with: undefined }}} which is logged by {{{ logger.eclog(3, "about to show the display with: " + (credentials && nodeData)); }}} immediately beforeshowCircuitDisplay()is called. Finally,credentialsToNodeDataMapgets updated: {{{ [06-12 08:09:29] Torbutton INFO: udpating nodeDataForCircuit }}} This got logged in theif (credentials)block after STREAM SENTCONNECT arrived.Thanks for tracking that down. Here's a possible fix for review: https://github.com/arthuredelstein/torbutton/commit/15493
Trac:
Status: needs_information to needs_reviewReplying to gk:
Thinking a bit about it what speaks against updating the circuit info/making the decision whether to show the circuit or not only when the user is clicking on the green onion and opening the Torbutton menu? There seems to be no need to make that call already once a tab gets selected for instance. Putting that into the onpopupshowing handler or something similar should cope with the possible race conditions and avoid any code execution wrt the display until a user really wants to see it. Not sure if there would be some lag while updating the display but looking at the involved operations a doubt that.
That's a good suggestion. I think we would also like the display to update if new circuit info arrives while the menu is being held open by the user, however. I'll look into how this might be done cleanly. (Probably the patch in my previous comment can be postponed while we think about this.)
Trac:
Status: needs_review to needs_revisionReplying to arthuredelstein:
Replying to gk:
Thinking a bit about it what speaks against updating the circuit info/making the decision whether to show the circuit or not only when the user is clicking on the green onion and opening the Torbutton menu? There seems to be no need to make that call already once a tab gets selected for instance. Putting that into the onpopupshowing handler or something similar should cope with the possible race conditions and avoid any code execution wrt the display until a user really wants to see it. Not sure if there would be some lag while updating the display but looking at the involved operations a doubt that.
That's a good suggestion. I think we would also like the display to update if new circuit info arrives while the menu is being held open by the user, however. I'll look into how this might be done cleanly. (Probably the patch in my previous comment can be postponed while we think about this.)
OK, here's a patch that updates the circuit display just before the user opens the menu, and also when new data arrives:
-
This looks good. I only found some nits:
// __collectIsolationData(aController)__.This one has now a second argument. Could you add as well a comment why it was necessary maybe pointing to this ticket?
s/Whenever user starts/Whenever a user starts/ (or "Whenever the user starts")
s/while popup menu is open/while the popup menu is open/
s/before popup menu is shown/before the popup menu is shown/
Replying to gk:
This looks good. I only found some nits: {{{ // collectIsolationData(aController). }}} This one has now a second argument. Could you add as well a comment why it was necessary maybe pointing to this ticket?
s/Whenever user starts/Whenever a user starts/ (or "Whenever the user starts")
s/while popup menu is open/while the popup menu is open/
s/before popup menu is shown/before the popup menu is shown/
Hm, I seem to have been in the mood to drop definite articles.
Thanks for the review. Here's the new version with fixes as suggested:
-
Looks good. This is commit 1d4e0280b6dfca79bbd7e204d588dc4432832728. Note I saw one case where I did not get circuit info at all. Alas, this was just once without the proper logging enabled. I guess this can be handled in a different bug in case I (or someone else) can reproduce this (again).
Trac:
Status: needs_review to closed
Resolution: N/A to fixed mentioned in issue #16990 (moved)
