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

Wed Aug 7 22:34:42 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:
-------------------------------------------------+-------------------------

Comment (by acat):

 Thanks again for the reviews. A new branch addressing your comments and
 some small issues I found: https://github.com/acatarineu/tor-
 browser/commits/30429+6

 ---

 > 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).
 Thanks, this must have slipped in one of the conflicts while rebasing.
 Note I also fixed a bug in each commit I noticed:
 `loc.installer.uninstallAddon` instead of `loc.uninstallAddon`.

 ---

 >>>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.
 >...
 >d03d360bd773c8d8a97ef1f69e565afcc19796e5 - not okay (comment is still
 missing)
 Ok, added a comment in the declaration of the function which causes all
 the static->non-static changes. Please let me know if you were thinking of
 something else.

 ---

 >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)
 I dropped it, i can add a comment to #29811 to add it again when it is
 fixed.

 ---

 >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.
 Sorry, I looked at the wrong source I guess, should be fixed.

 ---

 >>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 still did not find the cause, but I tested reverting this patch and the
 issue still happens to me with the Firefox default search engines, so it's
 either an issue with my local build that only happens to me, or caused by
 some other patch. But I think we can see in the nightly.

 ---

 >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.
 Ok, I changed the default ddg to use POST as before, removed the params,
 and also changed the icon for the current one, so that it can be
 distinguised from the .onion one (I guess that's the reason why they are
 different). Also removed these params from the yahoo search, even though I
 think they were already in esr60 yahoo.xml. Also changed some search
 extensions so that they are not localized, took the strings from the
 english version and replaced them in the manifests. Then I removed the
 _locales folder for these search extensions, since they were not used
 anymore. Removing these is probably not strictly needed, but I did just in
 case, to make sure that we have the same search extensions for all shipped
 locales.

 ---

 >"pageInfo_EncryptionWithBitsAndProtocol" +
 "pageInfo_OnionEncryptionWithBitsAndProtocol" formtatting in security.js
 (see previous "hdr =" you have removed)
 Done, the second hdr = formatting was fixed by eslint when trying to do
 the same as the first one, so I assume it's ok.

 {{{
 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)
 Oops, nice catch. Fixed.

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