[tor-bugs] #20680 [Applications/Tor Browser]: Rebase Tor Browser patches to 52 ESR

Tue Apr 4 07:51:36 UTC 2017

#20680: Rebase Tor Browser patches to 52 ESR
-------------------------------------------------+-------------------------
 Reporter:  arthuredelstein                      |          Owner:  tbb-
                                                 |  team
     Type:  defect                               |         Status:  new
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  ff52-esr, tbb-7.0-must,              |  Actual Points:
  TorBrowserTeam201703, tbb-7.0-must-nightly     |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
                                                 |  Sponsor4
-------------------------------------------------+-------------------------

Comment (by arthuredelstein):

 Replying to [comment:34 gk]:
 > Let's start the review:
 >
 > `aacb4ae907f93c8ad07b4ac5141a181348a2530c`:
 > 1) There is "Bug 18884: Disable Loop extension" mentioned in the commit
 message but there is no sign of that one in the `.mozconfig files anymore

 "Reworded."

 > 2) f161c394e049a440637f06ba87dd6be6f73479bb should get merged keeping
 the bug 17858... in the commit message

 Squashed.

 > 3) I cleaned up my extension signing patch a bit and moved the ICU
 support for Windows out; it should be in the `.mozconfig-mingw` directly
 (I have no clue which I thought that would be a good idea to add that
 piece to the extension signing patch, it is not); see my
 `bug_20680_icu_fixup` branch (https://gitweb.torproject.org/user/gk/tor-
 browser.git/log/?h=bug_20680_icu_fixup) for splitting up that patch

 I have cherry-picked these patches.

 > `7a2928fb53ee51714a825cdcadc749274495a9ab`:
 > 1) Merge e44e6f529e10468c2694b889a70b9b85f109f949 + mention bug in
 commit message

 Squashed.

 > 2) Merge c7113c8588eba3de80819090c38145d08d9eea0a + mention bug in
 commit message

 Squashed.

 > 3) Merge f3ec07bff9a96ffbfe9c3d03c1725a557097cc61 + mention bug in
 commit message

 Squashed.

 > 4) `pref("gfx.xrender.enabled",false);` is already included in ESR52, no
 need to set it again

 Fixed.

 > `e0213fac01fb1fc9f0949490d652c6ae979d6bd4`: not needed anymore

 Removed.

 > `046b74f2dfd53b29b3e4515c2533244b6924f69e`: good
 > `0bd71cd9f84185c71d91784467293585d46e6367`: good
 > `0eb5695b4a48b94ac98c076d4640fcfa64fdc832`: good
 > `44755f47a01eb3623227df9124a93226d034b75e`: good
 > `746cfdbfabce6ed31bbd58d46deaefa0ccabd1fd`: looks okay; I was wondering
 what to do with the remaining `COMPONENTS_SHIM_ACCESSED_BY_CONTENT`
 places. I guess keeping them is fine as this is Telemetry related which we
 disable anyway?

 Yeah, I'd be inclined to leave this out. And I wonder again if we should
 remove this patch entirely.

 Replying to [comment:35 gk]:
 > Here is the next batch:
 >
 > `43d4fc730a6216433c059345111c5fe9d11deae6`: looks good to me. mcs/brade
 could you have a second look?
 > `2b52030695f445b2f924a4442efeb6d49ee9fde2`: good
 > `8faf75d71705cdf891e6bedfc10ef8e93429d7fa`: good
 > `88e6c45cd16afdc26c1ec0ed7c887e5844c0374e`: good
 > `7ba9b10dab17c534933ec3441ff646b236445fbf`: good
 > `e8503c86d6ce4c286f54488b95f63447f9184c97`: good
 >
 > `1f1afb99079173c4ceff978cdb17ec74e8c39af4`:
 >
 > 1) the last bit in setLoginSavingEnabled() seems to be missing?

 Fixed.

 > `facd4d52edcbe48478f4b27f8f59b6579b02cc67`:
 >
 > 1) Why do we have:
 >
 > {{{
 > +    nsCOMPtr<nsIToolkitProfile> profile;
 > +    rv = aProfileSvc->GetProfileByName(nsDependentCString(arg),
 > +                                       getter_AddRefs(profile));
 > +    if (NS_SUCCEEDED(rv)) {
 > +      ProfileStatus status = CheckProfileWriteAccess(profile);
 > +      if (PROFILE_STATUS_OK != status)
 > +        return ProfileErrorDialog(profile, status, nullptr, aNative,
 aResult);
 > +    }
 > }}}
 >
 > now? It seems the relevant code around it does not have changed enough
 that this is self-explaining to me.

 Nothing much has changed here except that the code for declaring and
 instantiating `profile` had been removed in ESR52, so I had to restore it
 in order to pass it to `CheckProfileWriteAccess()`.

 > `800a82cf4133635e6b709013bbb95ccc3ae1a5e7`:
 >
 > 1) Why is MOZ_UTF16() not good enough anymore? Does the code not build
 with it anymore?

 Yes, this macro was removed from the codebase in
 https://bugzilla.mozilla.org/1277106

 Replying to [comment:36 gk]:
 > Here comes another batch:
 >
 > `2c0fdc9fb55dc4f28edb96c2a69a1451bcf8dcf3`: good
 > `1e1736ebc1a35427d1c1738d199b9c2ecca6373e`: good
 > `0e58aa9e4028085038827a583f12ea943fa2405e`: good
 > `8de96436b99518e947c6dedf3019d1df83714985`: good
 > `230c803c10f8c0aedc8beaaf18d13e92e5d95259`: good
 > `22508fe47768201b37ae86b2d995b14394727882`: good
 > `414a6ce893d50c1374968e485113ac21dfb0b5dd`: good
 > `85311e454060b97bc83494e6b59fb99e42b5f778`: good
 > `1985add6bc2fcc8d3167b1381b985d543bd80998`: good
 > `2399284cd6a7eaf4f21e01ce5d7d04b6297876f5`: good
 > `b379130d85235ea6395ac36ad6e82eff4ea15359`: good
 > `5c0f41dc2b317dcf1f4934c7cbc34a1de88e666b`: good
 > `20dfcf1c67fbeeebd1b580fc59b83baf99bb66c6`: good
 > `441f91e03424305e978bf27ca8b479c5929d9594`: good
 > `106f19b01457ffd88273cea1e0ef39caa779a298`: good
 > `b946f6cbe6cdecc3925e044c586810c7e48fcbc0`: good (should we merge that
 with TB4 commit?)

 Yes; squashed.

 > `013d0cd1f153626cb7f40cc39288300ee55e100e`: (mcs/brade could you have a
 second look here as well?)
 >
 > in `IsImageExtractionAllowed` why did you replace the old getting-the-
 first-party-code with:
 > {{{
 > +    nsIDocument* topLevelDocument =
 aDocument->GetTopLevelContentDocument();
 > +    nsIURI *topLevelDocURI = topLevelDocument ?
 topLevelDocument->GetDocumentURI() : nullptr;
 > +    nsCString topLevelDocURISpec;
 > +    topLevelDocURI->GetSpec(topLevelDocURISpec);
 > }}}

 Because we dropped the #5742 patch and so the GetFirstPartyURI API is not
 longer available.

 > It seems you are not guarding against a possible null-pointer-deref
 there?

 Good point. Fixed.

 > {{{
 > +    rv = permissionManager->TestPermission(docURI,
 > +
 PERMISSION_CANVAS_EXTRACT_DATA, &permission);
 > +    NS_ENSURE_SUCCESS(rv, false);
 > }}}
 > Why not `topLevelDocURI` instead of `docURI`? in 45.8.0 it is
 `firstPartyURI` that gets tested.

 Fixed.

 > `fd11d2ad97ea828f9e68750165de70cb34e3a7e0`: good
 > `d882e68b91a8a9ac1b6656bec5c38a2a7514115d`: good
 > `d85df6ecd6f8de4ff718b3dc85882686f94488a9`: good
 >

 Replying to [comment:37 gk]:
 > Here comes another batch:
 >
 > `0e0368701c236f103218cce56b7de22bd364e633`:
 > 1)
 > {{{
 > +  // Ensure that we allow torbutton, tor-launcher, and https-everywhere
 > }}}
 > should be
 > {{{
 > // Ensure that we allow Torbutton, TorLauncher, HTTPS-Everywhere, and
 meek.
 > }}}

 Fixed.

 > 2) see comment:34 and my bug_20680_icu_fixup)
 >
 > `07b1bd53e8802bab19947b23177805636513ebc6`: good
 > `b2b1409e8ee349c059da9d2bffbf43ca2fffd89c`: good
 > `54603a99ddf0796457d428d19fedfa2d9c532984`: good
 > `98bb714d7ca671dbff48bd4c00251ff691c2a349`: good
 > `5c0cdcab7c396a0a6bc1e112990266a0481f518f`: good
 > `5e0b61b2ff09ddbc71e4544ec5346c050ff1700c`: good
 > `3eb3616578465c2079caf5210e3e89770e21004c`: good
 > `8392e637ba1925606d6f5016e091022e8a1713ed`: good
 > `87a339023519749b736fde0cb5367a5decd74215`: good (but still needed!?)

 I don't know if it is still needed; see #21712.

 > `49e660828cc33168915edcb7ab5afe84620a8d9b`: good
 > `c7113c8588eba3de80819090c38145d08d9eea0a`: good (but see comment:34)
 > `1184271cd3ea12a0bae3c45e9817a2d6b6423e4e`: good
 > `f1ed90364cd203a5cb65f07f86eb30674f4b39f8`: good
 > `f161c394e049a440637f06ba87dd6be6f73479bb`: good
 > `f3ec07bff9a96ffbfe9c3d03c1725a557097cc61`: good (but see comment:34)
 > `dddcf25888d4eb7ee829ec5939876420fd4b005e`: good
 > `f392b99d215ba50727eb8b23823811c9ab079104` +
 `cda80ad28fa7c5508ae5686a6c0819fddc4cc595`: good
 > `977cf8724cd8a847f68248656e86a7c31c2c30bb`: take patch from
 https://bugzilla.mozilla.org/show_bug.cgi?id=1330882 ?

 Good idea; done.

 > `8387adbc333c6502e098eb21f8a1934e91a7c8d4`: good

 Replying to [comment:38 gk]:
 > Another bunch of comments:
 >
 > `b0b8846de6ff4764b47035c72faf7764df29c5ea`: good
 > `e5669287f7218a7f97c2bf4895524e9a6dbfc9df`: good
 > `88b6156799a5e6d7f8f0de10c3e06dc3f668a3da`: good (update to newer
 jemalloc4?)

 We could perhaps -- I opened a ticket: #21852.

 > `5d5007a994f6f0965f4dbd0355919002384deb55`: good
 > `219ef733285a0c9a40955104354deb0ae8bab55e`:
 >
 > "Do not accept or send remote commands;" -> "(default) Do not accept or
 send remote commands;"

 Fixed.

 > `ecbe2ed5a640738473c9f1b0936532b5b8c1f89e`: good
 > `6c9b2590ec741122413db9f99f4e5663935e6fc0`: good
 > `e44e6f529e10468c2694b889a70b9b85f109f949`: good (but see comment:34)
 > `98d3dfbce9af151d037cce74325a989a4fc1cf35`: good
 > `26acfcf65151ea6051646548c52c9be1c1158ab0`: good
 >
 > We need a rebased patch for #18885 due to
 https://bugzilla.mozilla.org/show_bug.cgi?id=1188657

 I opened #21849.

 Replying to [comment:39 mcs]:
 > Replying to [comment:35 gk]:
 > > Here is the next batch:
 > >
 > > `43d4fc730a6216433c059345111c5fe9d11deae6`: looks good to me.
 mcs/brade could you have a second look?
 >
 > We have a couple of small comments:
 > * In `dom/base/nsObjectLoadingContent.cpp`, the `SVGs load as documents,
 but are their own capability` comment should not be added (that comment
 was removed from the Mozilla code because it is no longer applicable).

 Fixed.

 > * In `parser/html/nsHtml5DocumentBuilder.cpp`, the old patch removed the
 `NS_ASSERTION(ssle, "Node didn't QI to style.");` statement. It seems like
 we still want to remove that since the patched code uses `if (ssle)` to
 handle the now expected case where `ssle` is NULL.

 Fixed.

 > Also, we could instead backport the fix that Mozilla landed for Firefox
 53: https://bugzilla.mozilla.org/show_bug.cgi?id=1216893
 > One problem we see with that approach is that the favicon portion is
 missing upstream (#18602).

 OK -- I'll hold off backporting for now.

 Replying to [comment:40 gk]:
 > The final trove:
 >
 > `7a094d1375d3f127b23427362a1d22424ac3cfe3`: good
 > `3882cea932e6b035120de45ca09d14cdd7314867`: good
 > `70ed7f54da8a431970a8a35d6f6f2e3b7ff69a4d`:
 > {{{
 > +ac_add_options --enable-signmar
 > +ac_add_options --enable-verify-mar
 > }}}
 > These are in the commit message for
 `16c4e654f2096b82a0e8922e29ee26a9f81b1ef0` and it seems to me enabling the
 signing things in all .mozconfig files does fit there better.

 Fixed.

 > `16c4e654f2096b82a0e8922e29ee26a9f81b1ef0`: good (see previous comment)
 > `44316a8f135da5085111cdff409151282997d023`:

 > 1) In the second whitelist there is only "|welcomeback" but it seems
 "|tor" is missing (the esr45 patch has it)

 Fixed.

 > 2)
 > {{{
 > +  // When electrolysis is enabled we will need to adopt an architecture
 that is
 > +  // more similar to the one that is used for about:home (see
 AboutHomeListener
 > +  // in this file and browser/modules/AboutHome.jsm).
 > }}}
 >
 > Has this been done? Or is there at least a ticket filed somewhere?

 #21850.

 > `dea0055d7dabfbe23fe8191b42dbf4ac0a9c02d0`: good
 > `d6a9e29b04760f56bf7f4d82798d915da5a28c2c`: good
 > `015699fe2b5fb82884e900901d7648727a720e06` (mozilla backport): good
 > `697a2218e7c6511174e2946137d4f2d62aeca8c5` (mozilla backport): good (use
 fix for 1348841 instead or get it uplifted)

 I have now backported 1348841. I suspect that ticket is independent so I
 will try to get 697a2218e7c6511174e2946137d4f2d62aeca8c5 / 1344613
 uplifted as well.

 > `af782600b9ea29529a74d06e5c5cff6f4dfed0ad`: good
 > `636eddeeb0ab2354689068257ff00b80199338b8`: good (changes due to #21309
 I guess; do we have a follow-up ticket taking care of getting rid of the
 tor-browser-bundle changes needed in #18915?)

 I opened one: #21851.

 Here's the new branch:
 https://github.com/arthuredelstein/tor-browser/commits/20680+10

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