Opened 4 years ago

Closed 4 years ago

#18884 closed task (fixed)

Rip Firefox Hello Beta / Loop extension in ESR45 based Tor Browser

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

Description (last modified by gk)

We should think about getting rid of the Hello/Loop extension. As gacar mentioned (https://lists.torproject.org/pipermail/tor-qa/2016-April/000809.html) it is quite large (1.6 MB) and is probably not running anyway as we are disable WebRTC at compile time and Firefox Hello is not active either.

Child Tickets

Change History (18)

comment:1 Changed 4 years ago by gk

Description: modified (diff)

comment:2 Changed 4 years ago by mcs

Cc: brade mcs added

Also see https://bugzilla.mozilla.org/show_bug.cgi?id=1266358 (which I happened to see today in a list of recent bugs found by the Firefox desktop QA team).

comment:3 Changed 4 years ago by gk

Priority: MediumHigh
Severity: NormalMajor

Yes, I think we should rip that out (+ the bits depending on it) to make sure it is off.

comment:4 Changed 4 years ago by bugzilla

Summary: Think about getting rid of the loop extension in ESR45 based Tor BrowserRip Firefox Hello Beta / Loop extension in ESR45 based Tor Browser

Good that all issues about it that were found in 6.0a5 were reported to Mozilla. So it's not worth mentioning them in #18937.

comment:5 Changed 4 years ago by gk

Keywords: TorBrowserTeam201605 added

Dragging into May to have it on our 6.0 radar.

comment:6 Changed 4 years ago by arthuredelstein

Owner: changed from tbb-team to arthuredelstein
Status: newassigned

comment:7 Changed 4 years ago by gk

Keywords: tbb-6.0-must added

comment:8 Changed 4 years ago by arthuredelstein

Keywords: TorBrowserTeam201605R added; TorBrowserTeam201605 removed
Status: assignedneeds_review

Here's a branch with two commits:
https://github.com/arthuredelstein/tor-browser/commits/18884+6

  • 941c9a27bc5dfa5ac8be9cf6d6c4d9d06c41fbbb introduces a build flag, --disable-loop, to prevent bundling the Loop extension.
  • 3790ded147c3dcf5f12e65f5ae8a4cc7e987f204 adds --disable-loop to Tor Browser's mozconfig files.
Last edited 4 years ago by arthuredelstein (previous) (diff)

comment:9 Changed 4 years ago by mcs

Status: needs_reviewneeds_revision

When testing with a standalone (non-gitian) build on MacOS, we had to add this to the patch:

diff --git a/browser/locales/Makefile.in b/browser/locales/Makefile.in
index be9454e..ec54b63 100644
--- a/browser/locales/Makefile.in
+++ b/browser/locales/Makefile.in
@@ -133,7 +133,9 @@ ifdef MOZ_WEBAPP_RUNTIME
        @$(MAKE) -C ../../webapprt/locales AB_CD=$* XPI_NAME=locale-$*
 endif
        @$(MAKE) -C ../../extensions/spellcheck/locales AB_CD=$* XPI_NAME=locale-$*
+ifdef MOZ_LOOP
        @$(MAKE) -C ../extensions/loop/chrome/locale AB_CD=$* XPI_NAME=locale-$*
+endif
        @$(MAKE) -C ../../intl/locales AB_CD=$* XPI_NAME=locale-$*
        @$(MAKE) -C ../../devtools/client/locales AB_CD=$* XPI_NAME=locale-$* XPI_ROOT_APPID='$(XPI_ROOT_APPID)'
        @$(MAKE) -B searchplugins AB_CD=$* XPI_NAME=locale-$*

Also, our packaged build still includes the loop extension (but we haven't figured out why).

ls -l obj-macos/dist/firefox/TorBrowserDebug.app/Contents/Resources/browser/features/
total 3312
-rw-r--r--  1 brade  staff  1691779 May 24 11:39 loop@mozilla.org.xpi

comment:10 Changed 4 years ago by gk

Okay, I am waiting with the 6.0 build for this one. I guess we can get it going tomorrow then.

comment:11 Changed 4 years ago by gk

I decided to postpone this one for 6.0.1 which seems to be okay to me given that there will only be about one week between it and 6.0 (the latter is planned for May 30 now). Apart from the need to get it reviewed again I think testing the fix in a nightly at least would be beneficial. But starting the build for 6.0 is kind of urgent as we want to have a somewhat meaningful QA before.

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

Status: needs_revisionneeds_review

Replying to mcs:

When testing with a standalone (non-gitian) build on MacOS, we had to add this to the patch:

diff --git a/browser/locales/Makefile.in b/browser/locales/Makefile.in
index be9454e..ec54b63 100644
--- a/browser/locales/Makefile.in
+++ b/browser/locales/Makefile.in
@@ -133,7 +133,9 @@ ifdef MOZ_WEBAPP_RUNTIME
        @$(MAKE) -C ../../webapprt/locales AB_CD=$* XPI_NAME=locale-$*
 endif
        @$(MAKE) -C ../../extensions/spellcheck/locales AB_CD=$* XPI_NAME=locale-$*
+ifdef MOZ_LOOP
        @$(MAKE) -C ../extensions/loop/chrome/locale AB_CD=$* XPI_NAME=locale-$*
+endif
        @$(MAKE) -C ../../intl/locales AB_CD=$* XPI_NAME=locale-$*
        @$(MAKE) -C ../../devtools/client/locales AB_CD=$* XPI_NAME=locale-$* XPI_ROOT_APPID='$(XPI_ROOT_APPID)'
        @$(MAKE) -B searchplugins AB_CD=$* XPI_NAME=locale-$*

Thanks for finding this issue. I've added your fix, as well as a second one that fixes a problem I ran into with make package.

https://github.com/arthuredelstein/tor-browser/commits/18884+7
Hash f2c8fe1176794a453318e359b08fbf1d618f45d4
There are two patches as before. I haven't yet tested the new revised patches in a full gitian build for 3 platforms, but I'm posting what is working for me on OS X.

Also, our packaged build still includes the loop extension (but we haven't figured out why).

ls -l obj-macos/dist/firefox/TorBrowserDebug.app/Contents/Resources/browser/features/
total 3312
-rw-r--r--  1 brade  staff  1691779 May 24 11:39 loop@mozilla.org.xpi

I'm not seeing this issue. Did you try running ./mach clobber before running ./mach build?

comment:13 in reply to:  12 ; Changed 4 years ago by mcs

Replying to arthuredelstein:

...
Thanks for finding this issue. I've added your fix, as well as a second one that fixes a problem I ran into with make package.

https://github.com/arthuredelstein/tor-browser/commits/18884+7
Hash f2c8fe1176794a453318e359b08fbf1d618f45d4
There are two patches as before. I haven't yet tested the new revised patches in a full gitian build for 3 platforms, but I'm posting what is working for me on OS X.

Did you push that branch?

Also, our packaged build still includes the loop extension (but we haven't figured out why).

ls -l obj-macos/dist/firefox/TorBrowserDebug.app/Contents/Resources/browser/features/
total 3312
-rw-r--r--  1 brade  staff  1691779 May 24 11:39 loop@mozilla.org.xpi

I'm not seeing this issue. Did you try running ./mach clobber before running ./mach build?

I thought we had clobbered everything, but I will double check.

comment:14 in reply to:  13 ; Changed 4 years ago by mcs

Replying to mcs:

...
I thought we had clobbered everything, but I will double check.

You were right. After fully clobbering and putting in a fix for make package (probably the same one you added), things look OK. Sorry for the confusion.

Please push your 18884+7 branch and we will review it.

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

Replying to mcs:

Replying to mcs:

...
I thought we had clobbered everything, but I will double check.

You were right. After fully clobbering and putting in a fix for make package (probably the same one you added), things look OK. Sorry for the confusion.

Please push your 18884+7 branch and we will review it.

Oops, I meant to do that before. It's pushed now. Thanks for having a look. I haven't managed to run the full gitian build still, so it's possible you may run into issues.

comment:16 Changed 4 years ago by mcs

r=brade, r=mcs
This looks good to us.

comment:17 Changed 4 years ago by arthuredelstein

FWIW, I built all 3 platforms with gitian and there were no errors. And I manually ran the OS X version and observed no problems.

comment:18 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good to me. And given that this feature is not enabled in Tor Browser anyway we can put it into 6.0.1 too. Commit 61a4d57b1cc573b85baef431b277149cbf740c74 and 9a9765a3cb73e7341b7c2494c167725b4f688c07 on tor-browser-45.1.1esr-6.0-1 have the fixes.

Note: See TracTickets for help on using tickets.