Re: [tor-bugs] #25543 [Applications/Tor Browser]: Rebase Tor Browser patches for ESR60

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#25543: Rebase Tor Browser patches for ESR60
--------------------------------------------+------------------------------
 Reporter:  gk                              |          Owner:
                                            |  arthuredelstein
     Type:  task                            |         Status:
                                            |  needs_revision
 Priority:  Very High                       |      Milestone:
Component:  Applications/Tor Browser        |        Version:
 Severity:  Normal                          |     Resolution:
 Keywords:  TorBrowserTeam201805, ff60-esr  |  Actual Points:
Parent ID:  #25741                          |         Points:
 Reviewer:                                  |        Sponsor:
--------------------------------------------+------------------------------

Comment (by arthuredelstein):

 Here's my new branch for review:
 https://github.com/arthuredelstein/tor-browser/commits/25543+13
 (ad5e6f972961abfcb572982231ab40589bfe6450).

 Many thanks to gk for review and to mcs and brade for their work on the
 updater patch. Details follow:

 '''Replying to [comment:22 gk, comment 22]:'''
 > Here is what I have so far. I am looking at 25543+8 and am moving
 upwards the commits Arthur pointed out in comment:6. The full git commit
 hash is the commit I looked at and the abbreviated in parentheses matches
 the one(s) in comment:6.
 >
 > The fixup commits make the reviewing a bit harder: Arthur, could you
 just create new branches next time with stuff squashed (waiting until a
 reasonably large amount of fixups have accumulated is fine, too). At least
 having fixups on the review branch makes things harder for me. Anway, here
 are my notes up to and including 4c9f746f2c19
 >
 > 15aab7d9640bebad5da4fe66535fe2646618cd73 -- good (b7ba24e9438c)
 > bc25026f2b7e77f5bc4b060fff7d52efc96a9286 -- good (3efb1fb5990a)
 > da7624af1f3d7cfd9878d1ef78f8212912bf00f3 -- good (7151b7736fbc)
 > 7da50e7f9f892cb3f3e76d65e73f07051e771f02 -- good (83e40fc55843)
 > c078d465c98e224167c549ea03068e0d7455ad14 -- not okay (d9ffdac205cc)
 >
 > fixed by 7e662c674e6089364aef5db63a24a1a78ea071ee

 I've squashed this now.

 > 423b01125604d1ddcf5f0ddd7a613a9bfeb1969a -- good (7190f7e52771)
 > ef922fccd70ccf4def676097a652cc83d68684e7 -- not okay (6e18348d3fa2)
 >
 > unresolved conflict
 > {{{
 >      // Send tab key events.
 > +<<<<<<< HEAD
 >      synthesizeKey("KEY_Tab", {shiftKey: activateShift});
 > +=======
 > +    var key = SpecialPowers.Ci.nsIDOMKeyEvent.DOM_VK_TAB;
 > +    utils.sendKeyEvent("keydown", key, 0, modifier);
 > +    utils.sendKeyEvent("keypress", key, 0, modifier);
 > +    utils.sendKeyEvent("keyup", key, 0, modifier);
 > +>>>>>>> febbeedade21... Bug 2874: Block Components.interfaces from
 content
 > }}}

 Fixed.

 > c567b8a14692dda0e43f01c9e472eb7b8244bb54 -- good (c91cc92acf64)
 > db22daac349cda4aeb7594451b1a37c4b73e0d5c -- good (05c64bde4a76)
 > 3ac50c1adf35c931cbb89493b38d5a5c9d4662bc -- not okay (4fd7433d2b79)
 >
 > not necessary as ripped out by the patch for 19910 (95ad1e098907), thus
 both should go

 Removed.

 > a2acdd19161c87a6cb39f03ca03dde80c68f8e34 -- good (9a13c4dd4d89)
 > 840833f13ee9cb64c8e2a248ffc43504dc828f09 -- not okay (f05b2599c291)
 > {{{
 > + nsXPIDLString killTitle;
 > +    sb->FormatStringFromName(titleKey, params, 1,
 getter_Copies(killTitle));
 > }}}
 > we should use |nsAutoString| as well and not |nsXPIDLString|
 > |killMessage| is now of type |nsAutoString|, so don't use
 |getter_Copies()|
 > anymore like the unpatched code does

 Fixed.

 > (seems this got fixed up in 0185a3889ea1f2d7e1262eeb5e3a4b04ba350d00,
 though, which is good but confusing, could you fix this up in the commit
 it belongs to?)
 >
 > 0185a3889ea1f2d7e1262eeb5e3a4b04ba350d00 -- okay (c8fbfdb5b0e7)
 > dbc9bd2f05b3728f222bc76b921542837124e237 -- okay (c38fc187252c)
 > 1708d1f2330ee1766fd554841c3e5c054ec6baf2 -- okay (6bbe63c3f3b8)
 > 8a0fca52fafbeaf92198d1daec00cbe1aaaffc59 -- okay (ba2620e0c91d)
 > 1470089f3a2160f03e950536f1f0376b689f4b47 -- okay (73dc870c6712)
 > dad24d92f489ed935c2c79339f015c2ada6ccb4c -- okay (90f3c1b3b687)
 > ec4a23fc9ec9abbd2d6d7f8316ff9fc94bdc9950 -- okay (98966f5b88b5)
 > 57f0a1e2a39e3b4431b3856f64c3877dc71f8dae -- okay (b4981a144854)
 > 579734b9a25db60a7a9f59df198fe8cc2e7367e3 -- okay (d3a986dfb477)
 > a310a95c8dc23d152be27f404495c3e1ca036204 -- okay (ea9c5e94e364)
 > 725b6c8f7d11010e922fe177b1cbda333a423720 -- okay (a71bf76df344)
 > a8add456af7aad04b0f4a402f1e5a8a7114aedec -- okay (5adf623b76f8)
 >
 > [what about the other WebGL things we disable are we good with them wrt
 to https://bugzilla.mozilla.org/show_bug.cgi?id=1217290?]

 I opened a ticket to look into this: #26198

 > 3d287f20252243b5c6cc41841fa2b48f90b3aa14 -- okay (aa65fd2ea82e)
 > 6d25b3a1c946b70935f1df096878f0256292c143 -- okay (ac9bc3723c2b)
 > 664d51393f8422a2b3d8edf5b24ee32b0a5d9b01 -- okay (90e817059ab7)
 > d93edac70a39e9f49555ae5cc96728b85448959a -- not okay (db5663390b3e)
 >
 > fixed by cca5e99eb181e9e120a809e3ef71ea7e73fbec89

 Squashed.

 > 6dbef77faa36e1130612d0f6784b6b05c142dea8 -- okay (6a7ae76e406e)
 >
 > do we still need it as the patch for 17009 got upstreamed?
 >
 > 81f89647957d420d087bbcd5c63104b157a139c9 -- okay (af9e23384692)
 >
 > do we still need it as the patch for 15646 got upstreamed?
 >
 > 74ea0a667a7464d4a6e90cdd7a11ac60e06af649 -- okay (0a2323b8fcaa)
 >
 > do we still need it as the patch for 1517 got upstreamed?

 As we discussed in the Tor Browser team meeting, I've dropped the above 3
 regression test patches because they were upstreamed.

 > 1261430503dd9a37a664e263b7415091cdf1f6a9 -- okay (fb26928c9f6f)
 >
 > U 133a941a72c9 should be O 133a941a72c9 (in comment:6) as <isindex> got
 removed
 >
 > 70c225a4b5f4266de7ce24110f5ef507160e234b -- not okay (b7f33de7c769)
 >
 > there is no
 dom/tests/mochitest/localstorage/test_localStorageByFirstParty.html
 > anymore, thus we can remove tbb-tests-ignore.txt it seems

 Removed.

 > c04c6fd4da01 is not a simple revert of db79c0270d50. The former only
 reverts a part of it. That's due to a mess of this whole e10s compat
 problem we needed to fix and me not properly dealing with the resulting
 patches during rebase. So, we need the resulting patch after reverting
 part of the first one for esr60 as
 https://bugzilla.mozilla.org/show_bug.cgi?id=1305177 is still open.

 OK! I added back a patch which is the combination of those two patches and
 rebased to this branch.

 > 856b810081cfbcd4b3f17ede4d1a95f4c02a14f0 -- not okay (d4da5714eb9d)
 >
 > no need for those `extensions.hotfix` prefs

 OK, I've removed those 4 prefs. There's one pref left.

 > 1cf891b3a783, 043e87d50499, and fdb2ad415cd6 are obsolete because we
 decided we don't want to expose this to users anymore to make it harder
 for them to shoot themselves in the foot?

 Yes, that was my thinking. In any case it's pretty straightforward for
 users to disable the two prefs under about:config.

 > fb7f56c28a9c3a77382361f218ac146c23ef6e30 -- okay (452829d9135f)
 > 80e6743fbf4ceb60b1073e60efd6c67eb2a7fba0 -- okay (04e72287a8c7)
 > a761c12e657389847ff8c29ed836b5c2541c09c9 -- okay (87036e9e33eb)
 >
 > The patch for 19411 is missing (fee72fffc081)

 (Omitted for being obsolete, as determined by mcs and brade.)

 > 5c60f63e322184cfde30256b43ee7875ea2d1df6,
 > aede530c3ec8850224b7f6c49c1d7e1466684639 -- not okay (9ae35ba3c07e)
 >
 > the first commit adds a .rej file;
 > I guess having now 'size: "1234"' is okay even though we had
 > it |null| before; however, I think we should stop patching the tests
 then
 > because there is no partial patch with an invalid hash anymore
 >
 > I see:
 > {{{
 > -                size: "1234",
 > +                hash: "1234cd43a1c77e30191c53a329a3f99d",
 > }}}
 > not sure how effective that is given |getLocalPatchString()|'s changed
 signature
 >
 > there is no SHA512_HASH_SIMPLE_MAR anymore:
 > https://hg.mozilla.org/mozilla-central/rev/e886bf1ba8fc removes it and
 we don't backout that part (in aede530c3ec8850224b7f6c49c1d7e1466684639).
 So, to sum up: just ignore the tests here >
 > cdbdfbf912a00d38754e4da111647f89d0f7b70b,
 > c89b57cea0457ca7fcaa7338b23c57fbef38def2 -- okay (08964d93d418)
 > 48e149a1f5f21aeda8f89d8dc374ede647feda23 -- okay (b0471f5e9e1f)
 > 5c9aa89a778d10d19b55736abfcb63b745e14379 -- not okay (4c9f746f2c19)
 > {{{
 > -  ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-release
 > -  MAR_CHANNEL_ID=firefox-mozilla-release
 > }}}
 > This should be "firefox-mozilla-esr" respectively (I guess you rebased
 the patch before there was a dedicated esr60 branch)
 >
 > aurora|alpha|beta|hardened|release|esr (and at other places) <- we don't
 have the hardened series anymore
 >
 > Do we need
 > {{{
 > -      let prereleaseChannels = ["nightly", "aurora", "beta"];
 > +      let prereleaseChannels = ["nightly", "aurora", "alpha", "beta"];
 > }}}
 > ?
 >
 > The "extensions.lastTorBrowserVersion" part spilled over into
 AddonsManager.jsm from #13052 + adding the AddonManager.jsm in
 toolkit/mozapps/extensions/moz.build
 >
 > Note to Arthur for the patch order: we need the patch for #13052 before
 this one as we use GetTorBrowserUserDataDir()
 > {{{
 > +  done <"${tmpfile}"
 > }}}
 > add a whitespace between "<" and "\""
 >
 > mcs/brade: how do you feel addressing #24476 while you are at it?
 >
 > f8d84e457447ee34bdd4416191404f011fa4459e -- okay (506eb3cbd392 and
 0f7641a6369c)

 mcs and brade fixed the above patches in I included them in the new
 branch.

 '''Replying to [comment:25 gk, comment 25]:'''
 > Some more comments:
 >
 > abdefb33d518b0cab3aef6006a3b4f7e7bd37b37 -- not okay (4564a5f744df)
 > {{{
 > +    LOG(("ApplyUpdate -- appDir->Get(Native)Path() failed (0x%x)\n",
 rv2));
 > }}}
 > should not have "(" and ")" around "Native"

 Fixed by mcs and brade

 > f31404d51d41f2ae65cbc9655630e0bf296bc206 -- not okay (46acba80bdf4)
 >
 > fixed by 2b3836d255481928c9096b72f74cef22fd5086d7,
 b438971ea1567332a3133cf45c92922beaf54e7c,
 9d3a1949117d2e238fadf649188298d0b042bb31, and
 d0d9bcd78e3740d98d2f60115e35a64df923d9e9
 >
 > However, 9d3a1949117d2e238fadf649188298d0b042bb31 should actually be a
 fixup for the patch for bug 4234 and not this one.

 Corrected.

 > dd2e54105436100a82f59f8042122e91ce4c8775 -- not okay (dc4fdd28c696)

 fixed by mcs and brade

 > I think the Tor additions in `old-configure.in` be at the same place as
 those in #4234? Right now they are spread out while they weren't before.
 >
 > What's the reason for replacing `extern NS_METHOD`, `class nsIFile;`
 with `extern nsresult`?

 '''Replying to [comment:26 gk, comment 26]:'''
 > Another bunch of reviewed commits:
 >
 > 65c7670b0cb896ebd083fc81c073db73f7f5d421 -- okay (3a536e56b9f7)
 > e064ba136557870f4f7d14191bfa34c6ca898bcd -- okay (4f7b24106278)
 > 272e53391b920344d741297337e02d7804511946 -- okay (64aed57c7b49)
 > 0f578925daab2e1bc6d5880e82120f53c9f540f2 -- okay (73f02a5f325c)
 > 8dc9616a4a1e372d63d6f8a31ca8cd7b23917805 -- okay (0b00e2ce04e9) (might
 not be needed depending on what we do with CentOS 6)
 > 958666616bfab98ddfd08f4fd3d727b5498eced5 -- not okay (5c25352ec8de)
 >
 > Why is `browser_permissions.js` suddenly deleted?
 > You are adding `^M` characters to `test_permmanager_defaults.js` when
 doing changes.

 Fixed (I have rebased the existing Permissions patch for now.)

 > 5172d9a05ad7bfd92765e9cfeb7de596508d4df8 -- not okay (43c1ed31857d)
 >
 > That's not needed anymore, see bug 1388902 and 1406193.

 Removed.

 '''Replying to [comment:28 gk, comment 28]:'''
 > Careful! We need to keep at least one of the MAR signing keys we are
 currently shipping. Otherwise it is a bit hard with updates. :) So, if I
 see this correctly then #20589 added the one we are currently using in the
 alpha for signing and #23916 added the current backup key. I estimate
 we'll have the rebase ready before I'll add a new signing key. Thus, I
 think we could drop #20589 and add an adapted commit for #23916 that
 basically creates the status quo. And then we'd replace the current backup
 key with the new one using SHA-384.

 I have squashed the #20589 and #23916 patches into one patch.

 '''Replying to [comment:31 gk, comment 31]:'''
 > Alright, here come the remaining bits:
 >
 > 8a17d0bb0f00987477233cdb0103b5cda724a491 -- okay (009934b82a3c)
 > a0fb3774961eacdeaaee6ad5bef1a93c9fb69722 -- not okay (fba536f97fe2)
 >
 > "Only ship the e10srollout system extension and pdfjs." should probably
 be "Only ship pdfjs."

 Fixed.

 > Why do we need `if os.path.exists("$(DIST)/bin/browser/features") else
 []};`? In which case is this available and in which not? Does this affect
 pdf.js bundling?

 I think this directory is empty because we removed all of the extensions
 from it. Therefore os.listdir(...) was throwing an error. I added a check
 to make it behave properly in that case.

 > The fixup (569a412d41a020b8c75ab6d62f4fae42f2802b89 is okay)
 >
 > a30b878113f442895c907066b70941374d2e0c0b -- not okay (c542fb08d725)
 >
 > I think there is no need to take this as a separate commit due to
 https://bugzilla.mozilla.org/show_bug.cgi?id=1433507 and
 https://bugzilla.mozilla.org/show_bug.cgi?id=1433357. So, what we should
 do is adding two fixup commits instead: one for the defense-in-depth pref
 and one for adding the no-proxy-bypass flag to our .mozconfig files.

 Done.

 > aa9b5e2811ab86a66d7ee20eec981c110a23c7a0 -- not okay (c79b911518ed)
 >
 > This got upstreamed in
 https://bugzilla.mozilla.org/show_bug.cgi?id=1382359. We should do a fixup
 commit setting `dom.securecontext.whitelist_onions` to `true` and that's
 it. So "C c79b911518ed" -> "U c79b911518ed"

 I removed this commit. And the pref is there.

 > aba739b085d1499a4ac31f4a1be0235c3c12b093 -- not okay (576f4e90158a)
 >
 > This got upstreamed in
 https://bugzilla.mozilla.org/show_bug.cgi?id=1382359. We should just leave
 this patch I think, until we seriously fix the tests run with our config.
 Then we should uptream that fix. So "C 576f4e90158a" -> "U 576f4e90158a".

 OK, leaving the patch in for now. (Let me know if I didn't understand
 correctly.)

 > d999aa5f22a8081cf01d3a62e0be3cecb71df3df -- okay (f7e646dd976c)
 > 13a0b11f84b776a82d03070215b0e9285be5dce3 -- not okay (fc9f5757efd6)
 >
 > That's not needed anymore/upstreamed. We'd take newer patches tjr is
 making if there are things left. So, I'd say "F[inspect] fc9f5757efd6" ->
 "U fc9f5757efd6".

 OK, removed this patch.

 > 5be9a93363c20a348316e4d6ec10e979255be437 -- not okay (a19fd1255901)
 >
 > Not needed. We'll take the patch for
 https://bugzilla.mozilla.org/show_bug.cgi?id=1460882 once it landed (as
 well as a bunch of other patches to fix compile/runtime issues).

 OK, removed.

 > 8a45f06acad1cf8640e0487f632b5fbd10bdd97d -- okay (87b15309e159)
 > 45571449efe1bb047314dc5d90931666bfc87697 -- okay (6794707e2b3a)
 > ee6b576bc2a9f5c885ae9f027dd1bd4e78ab86ce -- not okay (2646633951fe)
 >
 > That's not needed anymore given that
 https://bugzilla.mozilla.org/show_bug.cgi?id=1418052 landed.

 Removed.

 > 9684e260d0cb2abd4fb62d7960f9f25e60b48c5f -- not okay (009bc0a8f600)
 >
 > I think this could be a fixup to our .mozconfig commit.

 Done.

 > f297c39eb75ffc7bfd71a42916684dc98c904ca0 -- okay (230cb85895bc)
 > b654a7f58a15c500b44f69b81386eca2dc68264a -- not okay (ab9be0575af0)
 >
 > Could we fix up the commit message a bit while we are at it
 (s/bug/big/)? The fixup (commit d8bcb696fddc1d779c40994a96af52f4a8759520)
 looks good.

 Done.

 > 8093f4a07c9b7efc46421dfbf49095128693c9e5 -- okay (b91202db5ef3)
 > 8068477fe20d947b074aee854538b0f6ddf73815 -- okay (6e2c459fa66a)
 > ed8959815ed38d415fe1b71d0fb4e312bffd5b57 -- not okay (f5eebe23eda5)
 >
 > Seems to be a good candidate for a fixup for our prefs file and not an
 own commit.

 Done.

 > a4ae1392fffd7aa6ae1ba28575bad6de3b3489a3 -- not okay (ba141b6054ea)
 >
 > Not needed anymore as `showModalDialog()` is gone (see:
 https://bugzilla.mozilla.org/show_bug.cgi?id=981796)

 Removed.

 > 50f255b69aba42ea05fcc11b315000274500c38d -- not okay (95ad1e098907)
 >
 > As said in a previous comment, this and the commit originally
 implementing this feature should go for now as it is broken and we need a
 replacement.

 Removed.

 > cac8d0158e81c7809afbe922c00c60d418b32850 -- not okay (93999a363c76)
 >
 > Let's use the upstreamed patch in
 https://bugzilla.mozilla.org/show_bug.cgi?id=1441327 instead.

 I've replaced it with the upstreamed patch.

 > 4ac551d9d7d1c3b6aebc5a2fccc7ea5b7aa79da4 -- not okay (c70454fd10ef)
 > {{{
 >  }
 > +static bool
 > +IsSecureHost(nsIURI *aHostURI) {
 > }}}
 > Please add a newline before "+static bool".

 Done.

 > You need to patch the `isSecure` usage in `AddInternal()` as well as in
 the
 > patch on our esr52 branch.

 Fixed.

 > 5b566bf19806ebf1fb56750b549a59cc9c9d3191 -- not okay (82cd8ae9a5de)
 >
 > It's fixed with the fixup commit
 (adb38389878bdcef91919d91ceb61370cb6cbadb).

 Squashed.

 > abf40ca35cf8e42b88d19a915d6f27d3893c399f -- not okay (90e16dd25b6e)
 >
 > I think we should take what we got after some reviews by Moz folks in
 https://bugzilla.mozilla.org/show_bug.cgi?id=859782. If we are lucky we
 can just backport a patch that landed before we ship anything. Still, what
 we have right now in the ticket is preferable to what we have I think.

 OK, I grabbed the patch on the bugzilla ticket and replaced our old one. I
 used the same commit message as before.

 > ca2d7fbabb2b596fd34c14c069affa177d55b2c9 -- okay
 > 03c5dd35093e9b57923ffd7bae684f4410852f4c -- okay
 > 34407382d48ebbbc6deb74dcf5f67e95a74d8346 -- okay
 > 31530f47b84a0df2c1b9371089f7f8211cccd8a9 -- not okay
 >
 > It seems to me with the telemetry pref flip in
 34407382d48ebbbc6deb74dcf5f67e95a74d8346 we don't need the `_enabled`
 check. (see my comment in #25909 as well)

 I left this patch unchanged following comment:34.

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

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs