Opened 4 months ago

Closed 4 months ago

#26100 closed defect (fixed)

Update Tor Button for ESR 60

Reported by: igt0 Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff60-esr, tbb-torbutton, TorBrowserTeam201805R
Cc: arthuredelstein, sukhbir Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Update Tor Button to make it compatible with the Firefox ESR 60 code.

Child Tickets

Change History (15)

comment:1 Changed 4 months ago by igt0

An initial work has been made in the following branch: https://github.com/igortoliveira/torbutton/tree/torbutton-esr60

comment:2 Changed 4 months ago by igt0

Summary: Update Tor Launcher for ESR 60Update Tor Button for ESR 60

comment:3 Changed 4 months ago by gk

Keywords: ff60-esr tbb-torbutton TorBrowserTeam201805 added
Priority: MediumVery High

comment:4 in reply to:  1 ; Changed 4 months ago by arthuredelstein

Replying to igt0:

An initial work has been made in the following branch: https://github.com/igortoliveira/torbutton/tree/torbutton-esr60

Thanks, Igor! I ran into a couple of issues and made some patches to merge with your branch.

https://github.com/arthuredelstein/torbutton/commits/26100+1

The first patch make sure that nodeDataForCircuit returns a value. The second patch fixes an issue where the browser could not load any remote pages -- it would get stuck after I entered the page. I tracked the problem down to the pref-loading code added in f4ed4ecd5c705e684046ef418f4c519f2c4ce915 and found a fix that seems to work.

Also regarding f4ed4ecd5c705e684046ef418f4c519f2c4ce915, I have a few suggestions.

  • I think it would be better to move the pref-loading code into its own module, if possible, because it doesn't have any connection to logging. Maybe under src/modules/ ?
  • I noticed some other changes in files outside of torbutton-logger.js. These seem to be unrelated to the purpose given in the commit message, so I am inclined to think we should maybe move these to a separate patch if they are necessary.
  • In several places that patch changes code to use Services.prefs.getDefaultBranch(null). This seems to be unnecessary when Services.prefs already works as far as I can tell. So maybe we could change these back? It might even be sensible to change all prefs operations to use Services.prefs.
Last edited 4 months ago by arthuredelstein (previous) (diff)

comment:5 in reply to:  4 Changed 4 months ago by igt0

Replying to arthuredelstein:

Replying to igt0:

An initial work has been made in the following branch: https://github.com/igortoliveira/torbutton/tree/torbutton-esr60

Thanks, Igor! I ran into a couple of issues and made some patches to merge with your branch.

https://github.com/arthuredelstein/torbutton/commits/26100+1

The first patch make sure that nodeDataForCircuit returns a value. The second patch fixes an issue where the browser could not load any remote pages -- it would get stuck after I entered the page. I tracked the problem down to the pref-loading code added in f4ed4ecd5c705e684046ef418f4c519f2c4ce915 and found a fix that seems to work.

Also regarding f4ed4ecd5c705e684046ef418f4c519f2c4ce915, I have a few suggestions.

  • I think it would be better to move the pref-loading code into its own module, if possible, because it doesn't have any connection to logging. Maybe under src/modules/ ?

Indeed, it should be separated, the trick part is because several components need the preferences and they are loaded in the profile-after-change notification, in parallel. What do you think besides adding its own module, creating a single point of initialization for tor button? something similar to the tl-process.js(Tor Launcher)?

  • I noticed some other changes in files outside of torbutton-logger.js. These seem to be unrelated to the purpose given in the commit message, so I am inclined to think we should maybe move these to a separate patch if they are necessary.

+1

  • In several places that patch changes code to use Services.prefs.getDefaultBranch(null). This seems to be unnecessary when Services.prefs already works as far as I can tell. So maybe we could change these back? It might even be sensible to change all prefs operations to use Services.prefs.

Cool, i will give a try.

comment:6 Changed 4 months ago by sukhbir

Cc: sukhbir added

comment:7 Changed 4 months ago by igt0

Status: newneeds_review

Hi team,

We finally have a branch stable enough, you can find about it here:

https://github.com/igortoliveira/torbutton/tree/torbutton-esr60%2B9

bug 26100: Migrate general.useragent.locale to intl.locale.requested
https://github.com/igortoliveira/torbutton/commit/1eafafefd2dfb6006f9f2686e6ecb591da8bc434

bug 26100: Update deprecated JS code
https://github.com/igortoliveira/torbutton/commit/629f58cfffefb3f58ceab7275e0f1e1194a6f012

bug 26100: Update XPCOM calls after changes in the interfaces
https://github.com/igortoliveira/torbutton/commit/9c3e58597f3f3c41af2d44ae809a652c3a7d91a7

bug 26100: Load extension preferences during initialization
https://github.com/igortoliveira/torbutton/commit/ec0e146668a817954ee75b78216151830e626ebc

bug 26100: Switch from Task.spawn to async/await
https://github.com/igortoliveira/torbutton/commit/5e59e4c941e1b8a0c7b3ac868ca4e8417dcfd4f8

Bug 26100: Use new asynchronous applyFilter
https://github.com/igortoliveira/torbutton/commit/862ed4fb7b5a1709c59cbef25038b37673e8546f

bug 26100 - baseMenuOverlay.xul was removed by Mozilla
https://github.com/igortoliveira/torbutton/commit/e90ff32809cae6800b919b9b7e30d9b5b65ee3b9

bug 26100: Update about:tor code after changes in the resource and js
https://github.com/igortoliveira/torbutton/commit/c1f2c1f79d334c43ffdbd598312d5de2e3b54a34

Bug 26100: Use inputStream.asyncWait instead of nsIInputStreamPump
https://github.com/igortoliveira/torbutton/commit/75505702f257c0bd4230025ce826797157436427

Bug 26100: Don't use console object for logging
https://github.com/igortoliveira/torbutton/commit/674e336f587d687934b74b37244353a27fa95a18

comment:8 Changed 4 months ago by gk

Status: needs_reviewneeds_revision

Okay, this works for me, nice! Here comes a first batch of review comments (sorry, I have to split this up as I must switch context but wanted you to give some feedback to already work on):

commit 1eafafefd2dfb6006f9f2686e6ecb591da8bc434

Adding a hint to the underlying Mozilla bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1414390) to the commit message would be good I think.

commit 629f58cfffefb3f58ceab7275e0f1e1194a6f012

Is that new that catch statements can't have if statements anymore?

commit 9c3e58597f3f3c41af2d44ae809a652c3a7d91a7

Could you move the moz- removals in this bug to the previous commit?

I don't understand your plugin.disable related commit/code change. It's not set by Mozilla code in ESR52 either. We are setting it in our own pref file we ship with the browser. Thus, it seems to me setting it here is not necessary?

What speaks against using getBrowser()? It's cheaper to use window.gBrowser?

Starting with "1." in the commit message without a "2." seems weird. :) Could you add more stuff as you seemed to want or reword that part?

commit ec0e146668a817954ee75b78216151830e626ebc

Please do a

const Cu = Components.utils;

and use Cu in cookie-jar-selector.js

+  //Services.console.logStringMessage(`getPrefValue(${prefName})`);

No need for this commented out log statement

+	  this.logmethod = 2;

Why is that needed?

There are two additional new lines in startup-observer.js which are not needed.

Re the pref loading:

1) Speaks anything against having that in utils.js instead of creating a new module?

2) I think we should set the prefs on the default prefs branch as it is done right now. That allows the user easily see which preferences got modified by themselves/the browser and more importantly there is the option to return to save defaults. tor-launcher's 949379555010a04633c64b08ed52896a86c2cce6 might give some inspiration.

comment:9 Changed 4 months ago by gk

Okay I looked over the remaining commits. There is just one additional thing to change I think:

5e59e4c941e1b8a0c7b3ac868ca4e8417dcfd4f8

lgtm

862ed4fb7b5a1709c59cbef25038b37673e8546f

lgtm

e90ff32809cae6800b919b9b7e30d9b5b65ee3b9

lgtm

c1f2c1f79d334c43ffdbd598312d5de2e3b54a34

"tor button" -> "Torbutton"

75505702f257c0bd4230025ce826797157436427

lgtm

674e336f587d687934b74b37244353a27fa95a18

lgtm

comment:10 Changed 4 months ago by gk

FWIW, I opened #26189 for removing our content-policy.js component. We should do this in a separate ticket and not during the general update for ESR60 done here.

comment:11 in reply to:  9 Changed 4 months ago by gk

Replying to gk:

Okay I looked over the remaining commits. There is just one additional thing to change I think:

Oh, and another nit: Some commits start with "bug 26100" and some with "Bug 26100". I think all of them should start with "Bug 26100" if they are related to this bug.

comment:12 in reply to:  8 Changed 4 months ago by igt0

Replying to gk:

Okay, this works for me, nice! Here comes a first batch of review comments (sorry, I have to split this up as I must switch context but wanted you to give some feedback to already work on):

commit 1eafafefd2dfb6006f9f2686e6ecb591da8bc434

Adding a hint to the underlying Mozilla bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1414390) to the commit message would be good I think.

OK!

commit 629f58cfffefb3f58ceab7275e0f1e1194a6f012

Is that new that catch statements can't have if statements anymore?

Yes, conditional clause was removed.

In the page https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch
it says:

Reminder: this functionality is not part of the ECMAScript specification and has been removed in Firefox 59. It's not supported in any current browser anymore.

commit 9c3e58597f3f3c41af2d44ae809a652c3a7d91a7

Could you move the moz- removals in this bug to the previous commit?

I don't understand your plugin.disable related commit/code change. It's not set by Mozilla code in ESR52 either. We are setting it in our own pref file we ship with the browser. Thus, it seems to me setting it here is not necessary?

What speaks against using getBrowser()? It's cheaper to use window.gBrowser?

Starting with "1." in the commit message without a "2." seems weird. :) Could you add more stuff as you seemed to want or reword that part?

commit ec0e146668a817954ee75b78216151830e626ebc

Please do a

const Cu = Components.utils;

and use Cu in cookie-jar-selector.js

+  //Services.console.logStringMessage(`getPrefValue(${prefName})`);

No need for this commented out log statement

+	  this.logmethod = 2;

Why is that needed?

There are two additional new lines in startup-observer.js which are not needed.

Re the pref loading:

1) Speaks anything against having that in utils.js instead of creating a new module?

2) I think we should set the prefs on the default prefs branch as it is done right now. That allows the user easily see which preferences got modified by themselves/the browser and more importantly there is the option to return to save defaults. tor-launcher's 949379555010a04633c64b08ed52896a86c2cce6 might give some inspiration.

comment:13 in reply to:  9 Changed 4 months ago by arthuredelstein

Status: needs_revisionneeds_review

Here's a revised branch for review: https://github.com/arthuredelstein/torbutton/commits/26100+11 (482abfb95de85bce98043338d8f7fcad9f6b7845)

Details of the revision follow:

Replying to gk:

Okay, this works for me, nice! Here comes a first batch of review comments (sorry, I have to split this up as I must switch context but wanted you to give some feedback to already work on):

commit 1eafafefd2dfb6006f9f2686e6ecb591da8bc434

Adding a hint to the underlying Mozilla bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1414390) to the commit message would be good I think.

Done.

commit 629f58cfffefb3f58ceab7275e0f1e1194a6f012

Is that new that catch statements can't have if statements anymore?

Yes -- I added a link to the bugzilla bug.

commit 9c3e58597f3f3c41af2d44ae809a652c3a7d91a7

Could you move the moz- removals in this bug to the previous commit?

Done.

I don't understand your plugin.disable related commit/code change. It's not set by Mozilla code in ESR52 either. We are setting it in our own pref file we ship with the browser. Thus, it seems to me setting it here is not necessary?

This was a leftover from mobile work and has been removed.

What speaks against using getBrowser()? It's cheaper to use window.gBrowser?

igt0 noticed in base/content/browser.js:

function getBrowser() {
   return gBrowser;
}
/* DEPRECATED */

Starting with "1." in the commit message without a "2." seems weird. :) Could you add more stuff as you seemed to want or reword that part?

Reworded.

commit ec0e146668a817954ee75b78216151830e626ebc

Please do a

const Cu = Components.utils;

and use Cu in cookie-jar-selector.js

Done.

+  //Services.console.logStringMessage(`getPrefValue(${prefName})`);

No need for this commented out log statement

Removed.

+       this.logmethod = 2;

Why is that needed?

Removed.

There are two additional new lines in startup-observer.js which are not needed.

Removed.

Re the pref loading:

1) Speaks anything against having that in utils.js instead of creating a new module?

I'm inclined to have a separate module because, unlike the function in utils.js, this function uniquely needs to run before most other code. But I don't feel too strongly.

2) I think we should set the prefs on the default prefs branch as it is done right now. That allows the user easily see which preferences got modified by themselves/the browser and more importantly there is the option to return to save defaults. tor-launcher's 949379555010a04633c64b08ed52896a86c2cce6 might give some inspiration.

Good point! I think I have fixed this.

Replying to gk:
[snip]

c1f2c1f79d334c43ffdbd598312d5de2e3b54a34

"tor button" -> "Torbutton"

[snip]

Fixed.

comment:14 Changed 4 months ago by arthuredelstein

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201805 removed

comment:15 Changed 4 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good to me, thanks for all the work. I merged this to master as commits
399d2ba81505e4a74535b61f60563f4a9c2a8f6d
3047f3fcb9c606a659c6e7ffa7e4d97e33cd0ebe
91f1c5deae6fb8415e0c22a99d4c0c3eac7db7b5
e5ecb22e0f4bfec84935f8266cc1520642301660
c4a44294290fc96471646ae1ff39fb55d2dedf92
d07608265a3720f6f0a35f1e1bfa3c4d08706861
5905b318d348f608f6c8e59dccac28015aea6672
e37b97e4af14fdbccfcb485f6b4ecf136aeb2e18
2764f9a0019b1eb8704e6748bfa46b9f58aaf74a
482abfb95de85bce98043338d8f7fcad9f6b7845.

Note: See TracTickets for help on using tickets.