Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18466 closed defect (fixed)

Fix torbutton for ESR45

Reported by: arthuredelstein Owned by: gk
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Critical Keywords: ff45-esr, TorBrowserTeam201604R, GeorgKoppen201604 tbb-6.0a5
Cc: brade, mcs, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by arthuredelstein)

domain-isolator.js has a couple of minor breakages in Firefox 45.

  1. A new ReferenceError is thrown by things like let mozilla = mozilla || {};. We can change it to let mozilla = {};.
  1. The method newSOCKSProxyInfo was available in our SOCKS proxy patch. Since we landed that patch with some minor changes, we should now make an equivalent call to built-in Firefox method newProxyInfoWithAuth.

Child Tickets

Change History (16)

comment:1 Changed 3 years ago by arthuredelstein

Description: modified (diff)

comment:2 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201603R added; TorBrowserTeam201603 removed
Status: newneeds_review

Here's my patch, for review:

https://github.com/arthuredelstein/torbutton/commit/18466

(This patch will break torbutton for TBB/FF38, so we will likely need a separate branch.)

comment:3 Changed 3 years ago by arthuredelstein

Description: modified (diff)
Summary: Fix domain-isolator for ESR45Fix torbutton for ESR45

comment:4 Changed 3 years ago by gk

Keywords: ff45-esr added

comment:5 Changed 3 years ago by mcs

Cc: brade mcs added

Kathy and I looked at c55c1ef497f7b2a97ff7c9b0af351a065df3b477 and 594b2bbc55a0bbc2f548356f44ea191d4e2466e2. Your changes look good. We have two questions:
Can you give us some background on why statements such as let mozilla = mozilla || {}; were needed previously? Or maybe they were not needed?

I assume that the newProxyInfoWithAuth() change is what breaks in ESR38. Can we avoid the need for a branch by using a runtime check such as:
if ("newProxyInfoWithAuth" in mozilla.protocolProxyService) ... ?

comment:6 Changed 3 years ago by gk

Status: needs_reviewneeds_information

comment:7 Changed 3 years ago by gk

Keywords: TorBrowserTeam201604R added; TorBrowserTeam201603R removed

No easy way to do reviews in March anymore.

comment:8 Changed 3 years ago by gk

Keywords: TorBrowserTeam201604 GeorgKoppen201604 added; TorBrowserTeam201604R removed
Owner: changed from tbb-team to gk
Parent ID: #15197
Priority: MediumHigh
Severity: NormalCritical
Status: needs_informationassigned

There is more we need to change for fixing Torbutton. I am working on an additional patch

comment:9 Changed 3 years ago by gk

Keywords: tbb-6.0a5 added

comment:11 Changed 3 years ago by gk

Cc: arthuredelstein added
Keywords: TorBrowserTeam201604R added; TorBrowserTeam201604 removed

See bug_18466_v2 (https://gitweb.torproject.org/user/gk/torbutton.git/log/?h=bug_18466_v2) in my public torbutton repo for review. I added an additional commit to Arthur's two which looked good to me fwiw. mcs: I don't think we need to do something special to avoid a branch here. master will be used for the next alpha and that one is going to be ESR45 based. So, everything is fine. Regarding your other question I defer to Arthur.

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

comment:12 Changed 3 years ago by gk

Status: assignedneeds_review

comment:13 in reply to:  10 ; Changed 3 years ago by mcs

Replying to gk:

Part of the pain here is due to https://bugzilla.mozilla.org/show_bug.cgi?id=589199 (see: https://blog.mozilla.org/addons/2015/10/14/breaking-changes-let-const-firefox-nightly-44/).

Yes, these incompatible changes seem to be causing a lot of pain across the Firefox extension universe. The patch that you point to in comment:11 looks fine. I wonder if we missed any other things like this in Tor Launcher or Torbutton. How did you find these ones? I assume you noticed something was not functioning correctly?

comment:14 in reply to:  13 Changed 3 years ago by gk

Replying to mcs:

Replying to gk:

Part of the pain here is due to https://bugzilla.mozilla.org/show_bug.cgi?id=589199 (see: https://blog.mozilla.org/addons/2015/10/14/breaking-changes-let-const-firefox-nightly-44/).

Yes, these incompatible changes seem to be causing a lot of pain across the Firefox extension universe. The patch that you point to in comment:11 looks fine. I wonder if we missed any other things like this in Tor Launcher or Torbutton. How did you find these ones? I assume you noticed something was not functioning correctly?

Yes, I kicked Tor Browser until the issues went away and I had all the functionality back we currently have with the ESR38. I can try to test Tor Launcher a bit more but am pretty confident that it is fine.

comment:15 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

I tested Tor Launcher and it works fine for me. Thus, I think only Torbutton was affected. I've applied bug_18466_v2 to master (e6939a0043b8fc944ecfa4559efbd503d1d758c1, d454bd4ac3eb71b3945ff64704bb87fdb211d711 and e4f7079dd5d4ae44b8d10b48889d165eba20a660).

comment:16 in reply to:  5 Changed 3 years ago by arthuredelstein

Replying to mcs:

Kathy and I looked at c55c1ef497f7b2a97ff7c9b0af351a065df3b477 and 594b2bbc55a0bbc2f548356f44ea191d4e2466e2. Your changes look good. We have two questions:
Can you give us some background on why statements such as let mozilla = mozilla || {}; were needed previously? Or maybe they were not needed?

These were not needed. I used this to avoid accidentally destroying a mozilla object (for example) if it had already been defined. But these JS modules are isolated from all other code, so there is no risk of that.

Note: See TracTickets for help on using tickets.