Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#21547 closed defect (fixed)

e10s and 52esr compatibility for tor circuit display

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-torbutton, TorBrowserTeam201703R, ff52-esr, tbb-7.0-must
Cc: Actual Points:
Parent ID: #21201 Points:
Reviewer: Sponsor:

Description

mcs and brade noticed a crash in the circuit display. Also first party key is now part of origin attributes, so that probably needs to operate differently as well.

Child Tickets

Change History (9)

comment:1 Changed 2 years ago by gk

Component: Applications/TorbuttonApplications/Tor Browser
Keywords: tbb-torbutton added
Owner: set to tbb-team
Status: newassigned

comment:2 Changed 2 years ago by gk

Keywords: TorBrowserTeam201703 ff52-esr tbb-7.0-must added

comment:3 Changed 2 years ago by arthuredelstein

Here's a patch for review:
https://github.com/arthuredelstein/torbutton/commit/21547
It's on top of some other #21201 patches, but only the top patch is relevant to this ticket.

comment:4 Changed 2 years ago by mcs

Keywords: TorBrowserTeam201703R added; TorBrowserTeam201703 removed
Status: assignedneeds_review

comment:5 Changed 2 years ago by mcs

Kathy and I reviewed the code and ran it with a browser built from your tor-browser.git 20680+4 branch. Circuit display seems to work, but we see quite a few messages like this:

Torbutton INFO: Failed to get credentials or browser from channel

We modified the code to log the exception and chan.URI.spec. Here is an example:

[Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]" nsresult: "0x80004002 (NS_NOINTERFACE)" location: "JS frame :: chrome://torbutton/content/tor-circuit-display.js :: browserForChannel :: line 163" data: no]
uri: https://check.torproject.org/torcheck/img/tor-on.png

The above error occurred while loading https://check.torproject.org/?lang=en_US; we also see similar messages for many "internal" fetches such as OSCP which is not as surprising.

We should figure out a way to suppress the logging for errors that are expected. When we do log, we may want to include the exception as well as chan.URI.spec to make debugging easier.

comment:6 in reply to:  5 Changed 2 years ago by arthuredelstein

Replying to mcs:

[snip]

We should figure out a way to suppress the logging for errors that are expected. When we do log, we may want to include the exception as well as chan.URI.spec to make debugging easier.

Thanks for reviewing this and you make very good points. Here's my new version, applying your suggestions:

https://github.com/arthuredelstein/torbutton/commits/21547+2

comment:7 Changed 2 years ago by mcs

r=brade, r=mcs
Thanks for making the improvements. This looks good to us now.

comment:8 in reply to:  7 Changed 2 years ago by arthuredelstein

Resolution: fixed
Status: needs_reviewclosed

Replying to mcs:

r=brade, r=mcs
Thanks for making the improvements. This looks good to us now.

Thanks for the review. Added to my #21201 branch.

comment:9 Changed 2 years ago by gk

Looking over that patch it seems to me we only care about HTTP traffic nowadays at least if the circuit display concerned. Am I correct? While I am not a big fan of FTP, showing the corresponding circuit still works in current Tor Browser versions.

Note: See TracTickets for help on using tickets.