[tor-bugs] #30429 [Applications/Tor Browser]: Rebase Tor Browser patches for Firefox ESR 68
Tor Bug Tracker & Wiki
blackhole at torproject.org
Tue Jul 30 10:05:07 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, tbb-9.0-must- | Actual Points:
nightly |
Parent ID: | Points:
Reviewer: | Sponsor:
-------------------------------------------------+-------------------------
Comment (by acat):
Thanks for the reviews, GeKo, mcs/brade.
I took branch https://github.com/acatarineu/tor-browser/commits/30429+2
(which is the one resulting of #10760 changes) and addressed the review
comments there, resulting on https://github.com/acatarineu/tor-
browser/commits/30429+3. Then I rebased those commits on top of latest
esr68 branch, that's https://github.com/acatarineu/tor-
browser/commits/30429+4. And finally, reordered some commits to address
some review remaining comments and also fixed the search engines patch
(Omnibox: Add DDG, Startpage, Disconnect, Youtube, Twitter...), that's
branch https://github.com/acatarineu/tor-browser/commits/30429+5. I left
the search engine fix for the end, and then I realized that there were
some conflicts with esr68 branch, so that's why I only fixed it in the
last branch (30429+5).
While rebasing to esr68 branch (30429+3 -> 30429+4) there were several
conflicts, although most of them were due to style changes. But I think it
would be good to review them, for example:
- Bug 21849: Don't allow SSL key logging
(`gyp_vars['enable_sslkeylogfile']` was added)
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.
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?
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?
> 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?
> 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?
>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.
>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 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?).
>9d8ca4380e947c2097d0fd3554b1f3dad20de634 - not okay
>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?
>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.
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.
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.
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?
As I said before, These changes were done only on 30429+5 branch.
> ba383936028e955cef2ced0f98d2f90ca39564de - not okay
> We should fold that into 9510c9416ddd35a016ee2074dd58f927d97246c7;
additionally it's worth solving #27493 while we are at it.
Done, removed `mk_add_options MOZILLA_OFFICIAL=1` and also `mk_add_options
BUILD_OFFICIAL=1`, since it's not used anymore.
>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).
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.
I also updated some locale files. For example, brand.ftl is now used
extensively in the code, and has more strings. That means that I also
needed to modify patch #2176 to replace brand.ftl -> tor-browser-
brand.ftl, and to add the missing strings.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30429#comment:17>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list