Update Tor Button for ESR 60

added TorBrowserTeam201805R component::applications/tor browser ff60-esr owner::tbb-team priority::very high resolution::fixed severity::normal status::closed tbb-torbutton type::defect labels

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

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

Trac:
Priority: Medium to Very High
Keywords: N/A deleted, ff60-esr, tbb-torbutton, TorBrowserTeam201805 added

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.

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.

Trac:
Cc: arthuredelstein to arthuredelstein, sukhbir

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

Trac:
Status: new to needs_review

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:

Speaks anything against having that in utils.js instead of creating a new module?
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.

Trac:
Status: needs_review to needs_revision

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

FWIW, I opened #26189 (moved) 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.

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.

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:

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

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.

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:

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.

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.

Trac:
Status: needs_revision to needs_review

Trac:
Keywords: TorBrowserTeam201805 deleted, TorBrowserTeam201805R added

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.

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

closed

mentioned in issue #26186 (moved)

mentioned in issue #26219 (moved)

moved to tpo/applications/tor-browser#26100 (closed)

mentioned in issue tpo/applications/tor-browser#26186 (closed)

mentioned in issue tpo/applications/tor-browser#26219 (closed)

Update Tor Button for ESR 60

Child items 0

Activity