[tor-bugs] #30429 [Applications/Tor Browser]: Rebase Tor Browser patches for Firefox ESR 68

Tor Bug Tracker & Wiki blackhole at torproject.org
Thu Jul 18 09:47:08 UTC 2019


#30429: Rebase Tor Browser patches for Firefox ESR 68
--------------------------------------+--------------------------------
 Reporter:  gk                        |          Owner:  tbb-team
     Type:  task                      |         Status:  needs_revision
 Priority:  Very High                 |      Milestone:
Component:  Applications/Tor Browser  |        Version:
 Severity:  Normal                    |     Resolution:
 Keywords:  TorBrowserTeam201907      |  Actual Points:
Parent ID:                            |         Points:
 Reviewer:                            |        Sponsor:
--------------------------------------+--------------------------------
Changes (by gk):

 * keywords:  TorBrowserTeam201907R => TorBrowserTeam201907
 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:2 acat]:
 > == [rebased]

 Here are my comments, the hashes are from your `30249` branch:

 9510c9416ddd35a016ee2074dd58f927d97246c7 - not okay

 no need to comment out the maintenance related option, just remove it (and
 while we are at it, please remove the other unused options as well)

 please add `--enable-proxy-bypass-protection` to all mozconfigs

 4aec090126cd79289628b4403366c176714a4c77 - okay
 23b8e34d8fa64affdb265911ff586d8babf1a119 - should we not point to the
 latest Torbutton commit (especially as we have removed all the other
 Torbutton commit updates (rightly so))? otherwise okay

 22088a63f01c5526aedcafb3232f211a78ec9106 - okay (mobile)
 912ed4b07281ceebb67726b1785d12b37ab95b12 - okay (mobile)
 493b664f0fd1867559e51d6015c784e7c21a3259 - okay (mobile)
 34126c910efab560ff0d6923437c50758f8dc03f - we should merge that with the
 patches for #10760 I think (otherwise okay)
 bdf970dcdeeb276eccb9538b0d86dfee07ec5776 - okay
 5d3e47ad112820208dd894aaeb51497ab37caf65 - okay
 27fa31d4350e4248b0bfb35c51918955629a112a - okay
 b7c8b9e0b641cdfeef8ac2adc5188c3411110374 - okay
 f9957d4fd3f164be68adcd32206c09cb6f59a16d - could you do a `git commit
 --squash` here with commit 4aec090126cd79289628b4403366c176714a4c77;
 please add the Trac bug number to this pref flip so we get easily the
 context and move the pref flip to the fingerprinting section of `000-tor-
 browser.js`
 55367b7e2edbaf55f1142140b2cb9ec9b9247bec - okay
 9909eeb95cea7fa84bcd45bcdddd0c4c22d83e17 - okay
 45072c2fea6a535a712ac0888f12f881393cccbd - (mcs/brade should have a second
 look at this patch) I wondered how we ever settled at a `const char16_t*`
 first given the signature of `FormatStringFromName()` but I agree that
 using `const char*` seems more intuitive.

 `+  ProfileStatus profileStatus = PROFILE_STATUS_OK;` <- do we need to
 assign `PROFILE_STATUS_OK` here, wouldn't it be enough to just do
 `ProfileStatus profileStatus;` given that you do `+  aProfileStatus =
 PROFILE_STATUS_OK;` and don't assign that in `SelectUpdateProfile` either?

 There is in `nsToolkitProfileService`:
 {{{
    GetProfileByDir(lf, localDir, getter_AddRefs(profile));

     if (profile && mIsFirstRun && mUseDedicatedProfile) {
 }}}
 Should we have the usual `CheckProfileWriteAccess()` call here as well or
 did you think this is a non-issue as we are there "generally" from an app
 initiated restart, as the comment says.

 a8c48df07cc505bd45c764d223f6ff01de738f31 -- indentation (keep original):
 {{{
 +  nsresult rv = GetOverrideStringBundleForLocale(
 +      aSBS, uriString.get(), userAgentLocale.get(), aResult);
 }}}
 `// Build Torbutton file URI string by starting from the profiles
 directory.` <- It's not the profiles directory anymore

 Making `dirProvider` a `RefPtr` seems okay to me.

 Should `general.useragent.locale` be `intl.locale.requested` (I guess this
 should have been the case for Tor Browser 8 already, but well...)

 2d2a55296e255f9b502d0aa9eb70d4822a1bdd0e - okay
 9ec12a9075a87f1a1d446200ca4f64f88ef8466f - okay [we should upstream that
 one, bug 967812 has code parts]
 5b585b0633d5359504542b0fb05b599cb879a883 - okay
 630b395081dc68828d8f55ea41c71297b123bb87 - okay
 73e8dc78ddb3caf7b7dde8df2ea15dd9898cccac - not needed, see bug 1434772
 8295e48bb7d948fd8e85cbc9f5ffe7e9e0d9ea6b - okay
 d9cc80636e3dfcdf28bb368b416508402e9b9f9a - okay
 237124a540877734cc15a60de613f29b064f3799 - okay
 a4282bea59a32dba0a17f3d31e6f7f6094a98eac - not okay

 There is no docshell/test/mochitest.ini in esr68 and we should not add
 one.

 `+const kTestPath = "/tests/docshell/test/";` needs adaptation to new path

 Why `GetComputedReferrer()` and not `GetOriginalReferrer()`? Could you add
 an
 explaining comment here?

 a3acbf09d562e14f9e67f91db7a12bd185058b18 - not needed anymore with
 `--enable-proxy-bypass-protection` set
 4bd0f7037b7a89a9ec95e540f58ebcb5cd74bd07 - okay
 5011133b08cfdfbb5e6ad3ff03b1d230bb206608 - not okay

 The `AndroidCastProvider` part needs to get ripped out in components.conf
 as well, as things might break otherwise if we don't include it in
 moz.build (which we should not and your patch makes sure)

 You are not including `components.conf` in libmdns `moz.build` instead of
 patching it as you did in the provider case; we should have the same
 approach inboth cases (I am fine with just the `moz.build` one if we think
 that's enough)

 4c93f2c748ba5f6f00a5d5b197716a899798aea2 - okay
 6684321fb901ef1546391e786fca8d2e3ef8b5fc - okay
 a2ade001bdcffa4469baa3ac88a5db19ca8c6e52 - okay
 fd0570fb485cdb5d0327e51745b1ad59ef284240 - okay
 ce0db56b09a3b448508cc619418d84220f8e9acd - not okay

 What is the reason to patch GetUser$Directory now while we did not have
 done so in the esr60 patch? (I guess we should follow Mozilla here and if
 not, please add some comment/explanation for that deviation)

 18a6e88f2c28c4439858476334388de9208ad447 - okay
 d9b20bc60c8167c0ab32b93623beb8968f4b5f07 - test is probably not working
 anymore as we don't have static pins for #29811; we should either drop it
 or fix #29811 and test that the test is still working then (or working
 then again) (I think we should go the fixing route here :) )

 2943fd7440cf90f613ff3a96180cd7d71a3ca483 - okay
 9d8ca4380e947c2097d0fd3554b1f3dad20de634 - not okay
 {{{
 +    removeMessageListener("AboutTBUpdate:Update", this);
 +    removeEventListener("pagehide", this, true);
 }}}
 The respective `add$Listeners` got lost? Or do we get them now via the
 `LEGACY_ACTORS` object? (Does not seem to be the case for me as nothing
 changed in that regard if you look at the restructuring of the
 `AboutReader` page) Maybe those listeners were not needed in the first
 place?

 Does the restructured code not need `isAboutTBUpdate()` anymore?

 We put a lot of the code behind `TOR_BROWSER_UPDATE`, should the inclusion
 of the .jsm file then be behind that as well?

 I guess compilation breaks if we keep `BrowserContentHandler.jsm` where it
 is? (mcs/brade should have a second look here)

 468ccd77da35cb0ea0a3419619d42ce810d00238 - not okay

 Where are all the search engines coming from in that patch which we did
 not have in esr60? I think we don't need those.

 To keep the patch small we should just patch the search engines entries of
 the locales we ship. This holds for mobile as well and we probably should
 address #30017 (and maybe #30606).

 If you look at
 https://hg.mozilla.org/integration/autoland/rev/111b88dd28d6 you see that
 the search plugins are converted to WebExtensions. I wonder

 a) what is the issue with not following Mozilla here? I.e. I am a bit wary
 of deviating from their approach as we want to keep the differences small
 if possible.

 b) that if a) is indeed a thing we should probably make sure to comment
 that in the code and we might want to just not add the search extensions
 in the first place to minimize possible weird interactions.

 4cad2e391d96b7e1c197de5c37408ecd371d6aff - okay
 c1ff0d730d048e115b7d6c551b95a58a50ae8827 - okay
 294445810e60e08f00b8fdf74937664eff5a925d - okay
 da608e51ef73beb1f77fd6852c60b7db270ea31a - okay
 8908dc936bd0ae6f497533d5a69835b81eb242e2 - okay
 c55aea4b373ac4500d1539a8a4c79009fc9c0076 - okay
 b0a1cddc170a51d2b97a7b9c52b4294927c599b9 - okay [upstreaming??]
 1ee3bc541b96729ecb3c41df2fdf2c3bdeb278a4 - okay
 3490265e4331eb77a20eeb169776ddf55d891ecd - okay
 db3a5b7d339bad673a01804ff83fd2f8a79f46d3 - okay
 371f840f3096a9f1cf03d448df0d62569b7a52b9 - okay [upstreaming??]
 3bd36833ce14d3a9ad9d70ac4e6d4f10d5cebdfa - okay [upstreaming??]
 3b6785c5de66cb0f78faf945859e090633e09525 - okay
 df6140a3563f78252721cc3b53bc07ac4f05ca0e - okay
 829a448ca35a6323ce5d7982738c54424292d502 - [probably upstreamed, see
 1561636]
 73569f04f387efd7d7b9e06e5dfadbdfba0a3ee5 - okay
 9d9ca4e994b7a9713153f56a5f93ebc9fa2ed939 - okay
 83459e103450f80b471ecda459a2017857e5d26c - okay
 04d72c21af73359698ccd9f5c6808adb36be816c - not okay

 I think we should squash it with the general `000-tor-browser.js` commit
 (4aec090126cd79289628b4403366c176714a4c77)

 ba383936028e955cef2ced0f98d2f90ca39564de - not okay

 We should fold that into 9510c9416ddd35a016ee2074dd58f927d97246c7;
 additionally it's worth solving #27493 while we are at it.

 9f05eedab888b33f83e97bb3cd870ecb5174ea41 - not okay

 `>  .../fxmonitor/content/img/tor-watermark.svg   |   6 +++`

 It seems you added the .svg to the wrong dir?

 `content/branding/horizontal-lockup.svg` is missing in
 `browser/branding/alpha/content/jar.mn` or maybe we should just remove it
 in the other series? Or replace it with an own icon for
 `newInstallPage.html`? Glancing at bug 1518632 I am tempted to just remove
 that .svg file.

 We should remove the dead app.update.download.backgroundInterval.

 4a8236665a3250705dbab46f6b74a6c0eac1af2b - not okay

 Let's squash it with 4aec090126cd79289628b4403366c176714a4c77

 5269df12586ac7e4e45803a0f1b8c15da9ef529a - looks okay; I guess you need to
 add the EXTRA_PP_JS_MODULES because the build would break otherwise? If
 so, do you know why? mcs/brade should have a second look here.

 d945c68acc68a834f26a89973ab5f728fdbd3e38 - okay
 d84a2fb95136a1728b621a9d4cbe979389db48a0 - not okay

 please include the fixup for the manual page as specified in
 eb5d5dfaae93805baee9e84039e95fca74f9cce2

 c08b58e1d7062b732e39caca8a37b2a52af47249 - not okay, not needed due to
 1520177
 65ec28479828c9bf80f73c0d3d1d5817177c646e - okay
 939c662e69f3cae14e8f5e31b5a75eb6b20fb214 - okay (we should have this early
 on on our final branch so it is easier to bisect problems and still have a
 working Tor Browser experience; that probably includes moving the patch
 exempting our signed extensions to an early place in the branch as well)

 > == [TODO waiting for 1330467 to reland]
 > {{{
 > f47cd2fb5288 Bug 21569: Add first-party domain to Permissions key
 ---[https://bugzilla.mozilla.org/show_bug.cgi?id=1330467]
 > 3298251467df Bug 26670: Make canvas permission respect FPI
 ---[https://bugzilla.mozilla.org/show_bug.cgi?id=1330467]
 > }}}

 We need to backport 1330467 and look closely at the fallout. Might need
 some fix-ups to take this into account.

 > == [TODO not landed firefox patches from #28711 - update and do 'try'
 run?]
 > {{{
 > 7afe16fd6d27 Bug 1474659 Part 2 - Add dedicated AllocKinds just for
 ArrayBufferObjects. r?s.. ---[GC code changed, would be good to update
 patch and do 'try' to check breakage]
 > 2beafe9bd417 Bug 1474659 Part 1 - Add support to EnumSet for more than
 32 values. r?sfink ---[Changes for this part seem not to be needed
 anymore]
 > }}}

 Sounds like a good plan I think.

 > == [DROP not needed]

 Looks good.

 > == [DROP uplifted]
 > {{{
 > 41f620e4a0a3 Bug 13398: at startup, browser gleans user FULL NAME (real
 name, given name) f..
 > 091b41ec2465 Bug 21787: Spoof en-US for date picker
 > 34063061f825 Bug 26540: Enabling pdfjs disableRange option prevents pdfs
 from loading
 > c894688d4924 Bug 25909: disable updater telemetry
 > }}}

 Looks good. 41f620e4a0a3 is actually not needed anymore as the respective
 code got removed in Firefox 68.

 > == [DROP included in 68]

 Looks good.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30429#comment:11>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online


More information about the tor-bugs mailing list