Opened 4 years ago

Closed 4 years ago

#15493 closed defect (fixed)

Redirects sometimes seem to break circuit status UI

Reported by: mikeperry Owned by: gk
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-circuit-display, tbb-5.0, TorBrowserTeam201508R
Cc: arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Change History (17)

comment:1 Changed 4 years ago by s7r

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.

comment:2 Changed 4 years ago by arthuredelstein

If you do run into this again, please open the (Tools > Web Developer > Browser Console) and see if any errors/exceptions have been reported. Thanks!

comment:3 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201504 added

comment:4 Changed 4 years ago by mikeperry

Keywords: tbb-circuit-display added; tbb-4.5-alpha removed

comment:5 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201505 added; TorBrowserTeam201504 removed

comment:6 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201505 removed

comment:7 Changed 4 years ago by gk

Keywords: tbb-5.0 added
Owner: changed from arthuredelstein to gk
Status: newassigned

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.

comment:8 Changed 4 years ago by 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 the if (credentials) block in the same function; but we don't have something for nodeData yet as credentialsToNodeDataMap is 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 before showCircuitDisplay() is called.
Finally, credentialsToNodeDataMap gets updated:

[06-12 08:09:29] Torbutton INFO: udpating nodeDataForCircuit

This got logged in the if (credentials) block after STREAM SENTCONNECT arrived.

Last edited 4 years ago by gk (previous) (diff)

comment:9 Changed 4 years ago by gk

Cc: arthuredelstein added
Status: assignedneeds_information

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.

comment:10 in reply to:  8 Changed 4 years ago by arthuredelstein

Status: needs_informationneeds_review

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 the if (credentials) block in the same function; but we don't have something for nodeData yet as credentialsToNodeDataMap is 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 before showCircuitDisplay() is called.
Finally, credentialsToNodeDataMap gets updated:

[06-12 08:09:29] Torbutton INFO: udpating nodeDataForCircuit

This got logged in the if (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

comment:11 in reply to:  9 ; Changed 4 years ago by arthuredelstein

Status: needs_reviewneeds_revision

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

comment:12 in reply to:  11 Changed 4 years ago by arthuredelstein

Replying 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:

https://github.com/arthuredelstein/torbutton/commit/15493+1

comment:13 Changed 4 years ago by arthuredelstein

Status: needs_revisionneeds_review

comment:14 Changed 4 years ago by arthuredelstein

Keywords: TorBrowserTeam201508R added

comment:15 Changed 4 years ago by 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/

Last edited 4 years ago by gk (previous) (diff)

comment:16 in reply to:  15 Changed 4 years ago by arthuredelstein

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:

https://github.com/arthuredelstein/torbutton/commit/15493+2

comment:17 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

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

Note: See TracTickets for help on using tickets.