[tor-bugs] #30429 [Applications/Tor Browser]: Rebase Tor Browser patches for Firefox ESR 68
Tor Bug Tracker & Wiki
blackhole at torproject.org
Tue Aug 6 20:46:13 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: tbb-9.0-must-nightly, | Actual Points:
TorBrowserTeam201908 |
Parent ID: | Points:
Reviewer: | Sponsor:
-------------------------------------------------+-------------------------
Changes (by gk):
* status: needs_review => needs_revision
* keywords: tbb-9.0-must-nightly, TorBrowserTeam201908R => tbb-9.0-must-
nightly, TorBrowserTeam201908
Comment:
Replying to [comment:17 acat]:
[snip]
> I also realized that in `Bug 8312: Remove "This plugin is disabled"
barrier.` there is a `doc.getAnonymousElementByAttribute` but I do not see
any defined `doc` variable. It's the same in 52, but I think in the
previous version there was. So I added the `doc` definition, although I
would have to test this to make sure if it works as intended.
Nice catch (and sounds reasonable to me)! Don't spend too much time
testing the Flash use case, though. :)
> A question before comments: would it also make sense to try to upstream
Bug #5741, with a different commit name and behind the --proxy-bypass-
protection flag?
I think it would make sense to make sure configurations with a SOCKS proxy
set and remote resolution of DNS to not be able to bypass that if the flag
is set. So, I am all for it to get that upstreamed.
> Some comments:
> >please add --enable-proxy-bypass-protection to all mozconfigs
> Do you mean `.mozconfig-mac`, `.mozconfig-asan`, `.mozconfig` and
`.mozconfig-mingw`? If so, isn't it already there?
Hrm. I *think* I might have mixed this up with the state of `tor-browser-
build`, sorry.
> > 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
> I think so, but I kept the submodule change in a separate commit since
it's currently it's in my personal branch. Would it be ok to make the last
`WIP torbutton submodule change` commit a fixup when we have an "official"
torbutton "esr68" branch?
Yes, that's cool.
> > a8c48df07cc505bd45c764d223f6ff01de738f31 -- indentation (keep
original):
> >
> >+ nsresult rv = GetOverrideStringBundleForLocale(
> >+ aSBS, uriString.get(), userAgentLocale.get(), aResult);
> `mach clang-format` is changing this one, should we keep the original
indentation anyway?
Let's go with what Mozilla's tool wants to have here (and in other cases).
> >There is no docshell/test/mochitest.ini in esr68 and we should not add
one.
> Thanks. I also realized I removed I accidentally removed a test
`[test_bug1507702.html]`, fixed that too.
>
> >Why GetComputedReferrer() and not GetOriginalReferrer()? Could you add
an explaining comment here?
> I think `GetComputedReferrer()` is the one that preserves the current
patch behaviour (e.g. takes into consideration referrer policy). Actually,
when you mentioned I was not sure whether this was something intended. For
example, with a noreferrer policy navigating through pages of the same
origin will always clear `window.name`. But I saw #3414, so this actually
seems to be the intended behaviour. I added a comment in the code.
Thanks, sounds good.
> >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)
> Ok, I reincluded `components.conf` in `moz.build`, and made the
corresponding components.conf empty. Not sure if you meant this, or that
we actually don't need to touch `libmdns` `components.conf`.
What I meant was to approach the patching in both cases the same way: if
we patch in dom/presentation both `moz.build` and the conf file then it
makes it easier to understand the whole patch set if we do that in the
libmdns case, too. So, I think the result is fine.
> > 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)
> Not sure if you mean changing the static calls by the non-static ones.
If so, this is needed because the patch needs to make several
`GetUser$Directory` non-static, and apparently in esr60 the static
functions were called via object (I would expect C++ to at least warn if
you call a static method via an object?).
Yes, that's what I mean. We should comment the reason for that change at
least, like explaining why they suddenly need to be non-static, in
particular given that Mozilla does not make that change but we have to
now.
> >I guess compilation breaks if we keep BrowserContentHandler.jsm where
it is? (mcs/brade should have a second look here)
> Yes, I think the _PP_ is needed when it needs to go through the
preprocessor (same as 5269df12586ac7e4e45803a0f1b8c15da9ef529a, we added a
macro to check for a build pref).
>
> >It looks like event listeners are handled by via the LEGACY_ACTORS
object. See the large comment near the beginning of
toolkit/modules/ActorManagerParent.jsm. If that is correct, we should
remove the removeMessageListener() and ?>removeEventListener() calls from
browser/actors/AboutTBUpdateChild.jsm.
> True, did it. But I realized that now `onPageHide` does nothing, which
seems wrong. Can we also remove it? I'm not sure if the new patch changes
some behaviour here, since I guess before onpagehide would stop updating
the document?
That's probably a question for mcs and brade and/or trying things out. I
don't know the answer with a bunch of further investigations.
> >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...
>
> If you mean the *.xml, I added those to reuse the old ones without
converting them to WebExtensions. But perhaps you also mean that the
list.json changes are a bit dirty, that we're changing too many engines.
I think I meant both. :)
> In any case, I also think moving to webextensions is the right thing to
do, so I used the script in https://github.com/daleharvey/ffx-opensearch-
to-webext to convert them (it's the script that the mozilla dev used). I
had to modify the yahoo one, since it contained a system param which seems
not to be supported:
> {{{
> {
> "name": "hsimp",
> "condition": "purpose",
> "purpose": "system",
> "value": "yhs-007"
> }
> }}}
>
> I also included 'Google' in the searchEngineOrder field, for some reason
it was not respecting the order we currently have in esr60. We could
perhaps also include other search engines there, and also not sure if the
order prefs are needed anymore.
Let's keep this in mind while testing. I think the less changes we have
here the better.
> This is working fine the first time I open the browser, but
unfortunately for some reason after restarting it the search engine icons
disappear (although they still work). I'll try to find out why, but I
think this should not be a blocker for a nightly. It might also be
possible that it's some issue with my local build, so we can see if it
reproduces.
How did that go? Any further insight here?
> I also removed the changes for mobile `list.json`, as I think they were
only addressing #29798, which was fixed in 65. The mobile search engines
are currently set in `Bug 25741 - TBA: top sites changed, used bookmarks
icon temporarily.`. Should we change that and do all browser search
engines in this patch, or is it fine to do it later for Android
separately?
It's fine having that done later in Android separately (and squash the
mobile changes into this commit)
> >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.
> I reviewed the patch again and changed it a bit. First, is there any
problem with just removing the `tor-watermark.svg`? It's not used (I did
that in the revision).
I think that's okay if we don't use it in the new onboarding code (the
`tor-watermark.svg` was used in the onboarding). We should ask pospeselr
here, though, as the file got added in the work for #25702.
> Then, I realized that this patch actually was not changing the `*/pref
/firefox-branding.js`, but just copying the release one over the alpha and
nightly ones. But the release one is modified in one of the updater
patches (#4234), which has not been applied yet. So I left the
*/pref/firefox/branding.js untouched in this patch, and I plan to modify
it when #4234 is applied. Or we could apply #4234 after this one, and do
all the */pref/firefox/branding.js changes there, not sure.
Sounds good to me. Let's modify it after #4234 got applied.
Here comes another round of review (containing notes and reminders for
myself or anyone else as well). The onboarding code is still in need of
getting reviewed but that should not block nightly builds:
23612cb8e70a72e02e3500d39bb7cf9a9c57e62f - okay (good that all the
.mozconfig files had the proxy-bypass-protection flag set; I might have
been confused with the mozconfig files in `tor-browser-build` not having
this yet)
588d3620f6adc2916929bfbdc2576c35689d6da8 - okay
a33bcc34665727649f7419ba3dc6273f49a1aa36 - okay
0d9178a68150dbabf01f0af5e4fb6d9ea1e4b7da - okay (upstreaming, bug 967812
has code parts)
227c3dace6d6f3a1060f1799c1299d355462c17e - okay
3fe947023d1146e61dfe08f941cc269e2441cd62 - okay
d61046e445419d7c990571aace0616d1663d4a4d - not okay. It's confusing to put
the Torbutton related part in this commit. Let's have it in the respective
integration commit instead (the one for #10760 for example).
7efd3890289371a327154d94aa41d67c304f26a7 - okay (note: fixup commit later
on with esr68 torbutton commit)
7a68d4970b89e3146a4725e494f8b84adf1a2204 - okay
493b664f0fd1867559e51d6015c784e7c21a3259 - okay
1a5bf67fbb4f4a8add8d7e5d23c47d72792eaa34 - okay (but see my review
comments in #10760)
df622b6c8a50a6797711938f857075578fb2aeef - okay
8e7ecb0d9cb9be4bbfd8fbfca1623da9de579499 - okay (nice catch!)
ac911985315e30ab29645f3f7a615789cbc5819d - okay
b7c8b9e0b641cdfeef8ac2adc5188c3411110374 - okay
010d768be882f66df3512c4edaf5b3c88f0c0961 - okay
8c666ebb2d359013f0fe8ed9bb27e10858f1fb03 - okay
2a324a0e0db60fc42d714d73657833d3cdf438cc - okay (mcs/brade second look;
are we good with the "second call earlier"-part?)
6801fb665212d0068009a9a3c718cea4278b7fd6 - okay
3761fccc0a9d0701ae2845cd3fb192ab36052886 - okay
3789f206348dfa8c991e33e90027860743969202 - okay (nice! We should figure
out whether we got/get squashed some of the `tbb-branding` bugs that way
and close them accordingly)
754a163fb5d8d4a12854b11d09f313114de47156 - okay
734a50f6388ca2c11d696e7e6279349d96ef0b38 - okay
0fe1fd8d2966d65991bb33dc797ef4c9541303f7 - okay
4ab0cdd9862b6e079fa94fb4c8f8996b9349b391 - okay
941d6c2037d9bbd6b2c1b933698734c79e90492e - okay
c7e995eedb2a7da9cc6343078776f090ed51246d - okay
7fcd49ef692c635ba6f889c618b7de9df915f8e8 - okay
2131788a493dc7849dd767e981d0b03de4cd92c3 - okay
68072bf454acf8c6e3cbff5bf7531bd4f10b98ea - okay
d03d360bd773c8d8a97ef1f69e565afcc19796e5 - not okay (comment is still
missing)
69c8b59f9cdc792ee533187723286fdd75dbabd0 - okay
9442dda80f9beb90b90934da2bb1a614d39117a4 - not okay. The 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)
f96b46e0884299eee0a0f4bcfe881adf68a2f823 - okay
c6138a75ec817e4d7c1daa3f35959cdf6ba75767 - okay
062794f7cb6819ec1a8a34f3117f8f6b03eaacf8 - not okay (note :ja-JP-mac ->
ja; so mabye we need to adapt tor-browser-build here?)
We just need changes to the locales we support, e.g. not `be` but `es-AR`.
You find the full list e.g. in `tor-browser-build's` `rbm.conf`.
While "reusing" the existing search engines, did you make sure they comply
with what we shipped so far. For example the `ddg.xml` we ship in our
esr60 branches makes sure the requests go out via POST and there is no
further information transmitted to DDG, while the one we reuse in esr68
seems at least to have differentiation about how things got searched
(context menu, searchbar etc.). I think we don't want to have those.
61dcfc0d23a007825a390069de0a401509bad095 - okay
2d4c6343db751a841ad9ddc34dd03b69232e52a8 - okay
9d3401165f54aa99c76700d6a353bd7e73636315 - okay
6328f1b6eaa62f388cae83b8f205ee864bafb054 - okay
910e190a2d9d2fec07835fb026a591752e991852 - okay
2e42869e3aa2ad86b2d36b4128ea264dbb65fafa - okay [upstreaming??]
eb9b740cb74c0ec57ee861ac6fd60e2cfe1c1198 - okay
87fee0c8d5b0e6c365c081d78a73dec11608c5c0 - okay
d90b21dcfcca471900315c97294796364c0f9dd3 - okay
2e42869e3aa2ad86b2d36b4128ea264dbb65fafa - okay [upstreaming??]
f1893bd83085b25f7ee05fa33e368bc063046a32 - okay [upstreaming??]
f8fd2da23c908edc67b1f88f81f12724b4f84855 - okay
32009d2f6d790b4335ec0193519e5f54f1c89672 - okay
9e3c1f576913ae6ad2d805e7130f39e074ac6714 - okay
1a7eae785ecb08bda909d30ead665bea55002990 - okay
7415057d32809e86fe9b33de0f8e83b426e27964 - okay
90a4f393c41313889cbc40c61007028f9814a5ad - not okay (we don't need tor-
watermark.svg for the onboarding branding anymore? we should ask pospeselr
here, see: #25702) Keep track of the missing .js pref changes
887abb83ba0b71590ddea91f55301b7ce603eb26 - okay
b8546d69f9d1035c33927d81d964de63bacf80fb - okay
a03ede0306d96a2c0ba6dd360188d293c48328c2 - okay
546c642194f0434532dbb83389337cbb87d4c86e - okay
7efe2c610e55c7a61cd34c3f7102fd408159601f - not okay
{{{
+ // http onion is secure
}}}
indentation
"pageInfo_EncryptionWithBitsAndProtocol" +
"pageInfo_OnionEncryptionWithBitsAndProtocol" formtatting in security.js
(see previous "hdr =" you have removed)
{{{
if (isHttpScheme &&
IsPotentiallyTrustworthyOrigin(innerContentLocation)) {
*aDecision = ACCEPT;
return NS_OK;
}}}
You copied that block but it needs to get moved (the block before the
comment about the CSP directive should go I think)
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30429#comment:24>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list