[tbb-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 tbb-bugs
mailing list