Opened 8 months ago

Closed 6 months ago

Last modified 6 months ago

#25543 closed task (fixed)

Rebase Tor Browser patches for ESR60

Reported by: gk Owned by: arthuredelstein
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201805R, ff60-esr
Cc: tbb-team, franklin, igt0, intrigeri Actual Points:
Parent ID: #25741 Points:
Reviewer: Sponsor:

Description

This is the ticket to discuss and review the rebase of our patches for ESR60

Child Tickets

Attachments (3)

tb-patches-2018-04-20.zip (15.3 KB) - added by mcs 7 months ago.
#13252 patch plus a few fixups
tb-patches-2018-04-24.zip (2.8 KB) - added by mcs 7 months ago.
more fixup patches
0001-fixup-Bug-4234-Use-the-Firefox-Update-Process-for-To.patch (1.9 KB) - added by mcs 6 months ago.
remove &mig64=1 code

Download all attachments as: .zip

Change History (50)

comment:1 Changed 8 months ago by gk

Cc: tbb-team added; arthuredelstein removed
Owner: changed from tbb-team to arthuredelstein
Status: newassigned

comment:2 Changed 8 months ago by franklin

Cc: franklin added

comment:3 Changed 7 months ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201803 removed

Moving our tickets to April.

comment:4 Changed 7 months ago by gk

Priority: HighVery High

comment:5 Changed 7 months ago by gk

Parent ID: #25741

comment:6 Changed 7 months ago by arthuredelstein

Here is my current version of a rebase branch:
https://github.com/arthuredelstein/tor-browser/commits/25543+6 (b4907074cbe48d46a621fa8ad2b0e4b29c7041de)

It contains nearly all desktop patches rebased to mozilla-beta (those labeld C and F). It does not yet include a few updater patches (labeled P) and mobile patches. See https://torpat.ch for further reference. The branch builds and seems to run OK on Linux 64.

Here's what happened to each patch:

? = more investigation needed
B = already included in Firefox 60
C = cherry-picked
D = Delete
F = fixed up
K = Broken
O = obsolete
P = Pearl Crescent (in progress)
U = Upstreamed

F 90e16dd25b6e Bug 20283: Tor Browser should run without a `/proc` filesystem.
F 82cd8ae9a5de Bug 21537: Tests for secure .onion cookies
F c70454fd10ef Bug 21537: Mark .onion cookies as secure
F 7719a132533d fixup! Bug 16940: After update, load local change notes.
U 901380f79a74 Bug 23439: Exempt .onion domains from mixed content warnings
U 314e5b4a08d3 Bug 23439: Exempt .onion domains from mixed content warnings
B 0fb51b9375f6 Bug 25147: Sanitize HTML fragments created for chrome-privileged documents
O 74b92f0512e8 Bug 25112: Tor Browser 7.5 is not working on Windows Vista 64bit
B 0d3da213dc86 Bug 1370027: Part 1 - Cleanly handle a subprocess child being reaped by NSPR. r=aswan
D 76b6a5dc0859 Revert "Bug 18619: If indexedDB disabled, use in-memory db for asyncStorage.js"
C 93999a363c76 Bug 22794: Don't open AF_INET/AF_INET6 sockets when AF_LOCAL is configured
C 95ad1e098907 Bug 19910: Rip out optimistic data socks handshake variant (#3875)
C ba141b6054ea Bug 22614: Make e10s/non-e10s Tor Browsers indistinguishable
B 01b8fa23b26a Bug 1005640 - Flush StringBundle cache when app-locales change. r=valentin
C f5eebe23eda5 Bug 13575: Disable randomised Firefox HTTP cache decay user tests.
F 6e2c459fa66a Bug 23916: Add new MAR signing key
B 5e53cbb2d63c Bug 1403412 - disable VP9 estimizer on Mac; r=jya
C b91202db5ef3 Bug 22548: Firefox downgrades VP9 videos to VP8.
U 031dba9cfdf3 Allow std::unordered_*.
U 848e862614a1 Bug 24197: fix uppercase/lowercase issue in Wow64.h include
B 52781b3a80f4 Bug 23970: Printing to a file is broken with Linux content sandboxing enabled
B ab8aca382251 Bug 23970: Printing to a file is broken with Linux content sandboxing enabled
B c96c64300d52 Bug 23970: Printing to a file is broken with Linux content sandboxing enabled
B 5d36dc9a3d5b Bug 23970: Printing to a file is broken with Linux content sandboxing enabled
B cfe5bda0cec0 Bug 23970: Printing to a file is broken with Linux content sandboxing enabled
O d6131d2157a1 Bug 23016: "Print to File" does not create the expected file in non-English locales
B a0382e7bc741 Bug 1372072 - Part 2: Add a test case for check whether network information API has been spoofed correctly when 'privacy.resistFingerprinting' is true. r=arthuredelstein,baku
B 3841170c74d8 Bug 1372072 - Part 1: Spoofing network information API and blocking ontypechange event when 'privacy.resistFingerprinting' is true. r=arthuredelstein,baku
C ab9be0575af0 Bug 24398: Plugin-container process exhausts memory
C 230cb85895bc Bug 23104: Add a default line height compensation
C 009bc0a8f600 Bug 24478: Enable debug assertions and tests in our ASan builds
C 2646633951fe Bug 21925: Don't compile with ASan and FORTIFY_SOURCE
C 6794707e2b3a Bug 24052: Handle redirects by blocking them early
K 2e0a54b89593 Bug 24052: Streamline handling of file:// resources
B 2270fb027a31 Bug 1305396 - Replace memmove with std::copy_backward in a file that doesn't include cstring explicitly. r=keeler
D e7fc8cfbe27d Revert "Bug 21308: Set indexedDB->null when dom.indexeddb.enabled=false"
D ca8fa1fb280c Revert "bug 23104 - Add a default line height compensation"
C 87b15309e159 Bug 13398: at startup, browser gleans user FULL NAME (real name, given name) from O/S
B 8c0c1a4d6469 Bug 366945 - Disable middlemouse.contentLoadURL by default on UNIX and Android, r=gijs
D 478a8ccce85b bug 23104 - Add a default line height compensation
C a19fd1255901 We don't take the SANDBOX_EXPORTS path and fix compile issues along our way
F[inspect] fc9f5757efd6 Bug 16010: Fixing sandbox compile issues
B fe5c1809487e Bug 1386279 - Renovate Linux sandbox file broker handling of access(). r=gcp
B f99102a4c3d4 Bug 1374281. r=jld
B 43247a6b0732 Bug 1344106 - Remove Linux todos() now that Linux sandboxing is riding the trains. r=haik
B 08edba4a1f7a Bug 1317802 - don't stop for SIGSYS in .gdbinit; r=jld
B aab5c2714555 Bug 1337162 - Enable the Linux content sandbox for non-Nightly builds. r=ted
B bed2159de684 Bug 1355274 - Polyfill SOCK_DGRAM socketpairs with SOCK_SEQPACKET, for libasyncns. r=gcp
B 4e8bfae856e9 Bug 1361238 - Re-allow accept4, used by accessibility. r=gcp
B 7dbf00b82e6a Bug 1358647 - Disallow bind/listen/accept for Linux content processes. r=gcp
B 0232c989f8ea Bug 1286865 - Step 0: Turn off crash-on-seccomp-fail by default on non-nightly. r=gcp
B 6c802b3741c9 Bug 1320085 - Allow the getrlimit-equivalent subset of prlimit64. r=tedd
U 2e72b91df3e5 Bug 18101: Suppress upload file dialog proxy bypass (linux)
B 201df98d032e Bug 1365047 Turn on the Windows DLL Blocklist in MinGW r=aklotz
B 4d27bc319f9d Bug 1368406 Use non-Windows Printf Format Specifiers in MinGW r=froydnj
U c773ce1f161f Bug 23230: Fix build error on Windows 64
D c04c6fd4da01 Revert "Bug 19273: Avoid JavaScript patching of the external app helper dialog."
C f7e646dd976c Bug 21830: Copying large text from web console leaks to /tmp
C 576f4e90158a Bug 21321: Add test for .onion whitelisting
C c79b911518ed Bug 21321: .onion domains are shown as non-secure
U 6214b3a48f36 Don't break accessibility support for Windows
D 2aadce237574 Revert "Getting Tor Browser to build with accessibility enabled on Windows"
F c542fb08d725 Bug 23044: Don't allow GIO supported protocols by default
U 67d6461d58a6 Bug 16485: Improve about:cache page
O? 019cfd615d7f Bug 21862: Rip out potentially unsafe rust code
U 5a812a560343 Bug 1329521 - GetLoadContextInfo() should not compare originAttributes and privateBrowsing boolean when docShell is chrome type, r=smaug
U 1e44ba71702e Bug 22452: Isolate tab list menuitem favicons to content first party
U 671e4be2682f Bug 22327: Isolate Page Info media previews to content first party
U a49b1a4d604a Bug 1319908 - Load the menu icons for the bookmarks menu with the correct content type and principal on OSX; r=baku
U 08391e69ed95 Bug 21972: about:support is partially broken
U a48b75ea65c0 Bug 21684: Don't expose navigator.AddonManager to content
U 177805982c2b Bug 22320: Use pref name 'referer.hideOnionSource' everywhere
F fba536f97fe2 Bug 21431: Clean-up system extensions shipped in Firefox 52
F 009934b82a3c Bug 16285: Exclude ClearKey system for now
U 6018c8682553 Bug 22165: Block DoListAddresses when resisting fingerprinting
U 1fc107434bd9 Bug 10286: Regression tests for Touch API fingerprinting resistance
U 4cd7a879addc Bug 10286: Touch API fingerprinting resistance
C 43c1ed31857d Bug 13612: Disable Social API
F* 5c25352ec8de Bug 21569: Add first-party domain to Permissions key
U 3d7920974fa7 Bug 16337: Round times exposed by Animation API to nearest 100ms
U c991664faabc Bug 21792: Suppress MediaError.message when privacy.resistFingerprinting = true
B 3d55d320d172 Bug 1282655 - Test if site permissions are universal across origin attributes. r=tanvi
B 472166860594 Bug 1274020 - Tests that shows the Cache Web API is separated by origin attributes. r=baku
B 5a8d26d0cc01 Bug 1315602 - Remove the assertion of FirstPartyDomain should be empty in HTTP redirect. r=smaug
B 84c976d6c191 Bug 1351071: Get rid of pre-generated startup cache r=glandium
B 0b9734f23584 Bug 1342887 - Detect and log failures to dispatch SetupMacCommandLine to the main thread. r=rstrong
B 16d29020cd2a Bug 1335916 - Make sure the update driver only calls SetupMacCommandLine from the main thread. r=rstrong
F 0b00e2ce04e9 Bug 21907: Fix runtime error on CentOS 6
B 452a464d126f Bug 1352305 - Part2: Add a test case for making sure dialog windows will not be enforced to rounded sizes when fingerprinting resistance is enabled. r?ehsan
B dd2efe4502f7 Bug 1352305 - Part 1:  Making the XULWindow will not be enforecd to be rounded dimensions if it is a window without a primary content when fingerprinting resistance is enabled. r?ehsan
O? 98ee0302a49d Bug 21876: Always use esr policies for e10s.
F 73f02a5f325c Bug 21849: Don't allow SSL key logging
D 75c7cfcb68e1 Getting Tor Browser to build with accessibility enabled on Windows
U ad7ff6542560 Backport of tjr's patch for bug 1331349
U 9ea59d59ffa6 Backport of tjr's patch for bug 1314979
B c640867a52d2 Bug 805173 - Enable HeapEnableTerminationOnCorruption for chrome processes on Windows. r=mhowell,tjr
C? 64aed57c7b49 Bug #5741: Prevent WebSocket DNS leak.
U cef74a746683 Bug 21723: Fix inconsistent generation of MOZ_MACBUNDLE_ID
O? dc0210891a9e Workaround for broken ASan builds (bug 1272498)
F 4f7b24106278 Bug 14970: Don't block our unsigned extensions
B 3555582727db Bug 1330882 - Part 5: Add more test cases for rounded windows test. r=arthuredelstein,smaug
B 04f0a2bb4696 Bug 1330882 - Part 4: Making the window.open() can only open rounded windows and the inner window will be automatically rounded after setting size through innerWidth/Height and outerWidth/Height when fingerprinting resistance is enabled. r=smaug
B 6c0ecaa44d1b Bug 1330882 - Part 3: Add a test case for opening new windows as rounded size when fingerprinting resistance is enabled. r=arthuredelstein,smaug
B d362791d8e53 Bug 1330882 - Part 2: Disallow the session restore to modify window size when fingerprinting resistance is enabled. r=arthuredelstein,mikedeboer
B 75691f7a6e30 Bug 1330882 - Part 1: Making new windows to be rounded size when fingerprinting resistance is enabled (adopt from Tor #19459). r=arthuredelstein,smaug
F 3a536e56b9f7 Omnibox: Add DDG, Startpage, Disconnect, Youtube, Twitter;  remove Amazon, eBay, bing
D c40c21632973 Bug 21308: Set indexedDB->null when dom.indexeddb.enabled=false
B ffcb66f639f4 Bug 1344613 - Prevent null pointer crash in nsSOCKSIOLayer.cpp
B cebb513dc6aa Bug 1305144 - Option to hide referrer when leaving a .onion domain. r=mcmanus
F 506eb3cbd392 Bug 20589: Adding new MAR signing key
P dc4fdd28c696 Bug 13252: Do not store data in the app bundle
F 46acba80bdf4 Bug 16940: After update, load local change notes.
P 4564a5f744df Bug 13379: Sign our MAR files.
P 4c9f746f2c19 Bug 4234: Use the Firefox Update Process for Tor Browser.
F b0471f5e9e1f Bug 21724: Make Firefox and Tor Browser distinct macOS apps
C 08964d93d418 Bug 18912: add automated tests for updater cert pinning
P? 9ae35ba3c07e Bug 19121: reinstate the update.xml hash check
O? fee72fffc081 Bug 19411: Update icon shows up even if partial updates are failing.
F 87036e9e33eb Bug 18900: updater doesn't work on Linux (cannot find libraries)
F 0f7641a6369c Bug 18008: Create a new MAR Signing key
U 5f189ecd2805 Bug 18170: After update, only changelog tab shown
F 04e72287a8c7 Bug 11641: change TBB directory structure to be more like Firefox's
F 452829d9135f Bug 9173: Change the default Firefox profile directory to be TBB-relative.
U? e9be3f9dff33 Bug 20981: On Windows, check TZ for timezone first
U? 142c643b4cff Bug 16622: Pref to spoof time zone as UTC
O fdb2ad415cd6 Bug 20707: Avoid localization failure in about:preferences
O 043e87d50499 Bug 20244.2: Add "privacy.firstparty.isolate" checkbox
O 1cf891b3a783 Bug 20244.1: Add "privacy.resistFingerprinting" checkbox
C d4da5714eb9d Bug 19890: Disable installation of system addons
D db79c0270d50 Bug 19273: Avoid JavaScript patching of the external app helper dialog.
C b7f33de7c769 Bug 18923: Add a script to run all Tor Browser specific tests
U 133a941a72c9 Bug 18914: Use English-only label in <isindex/> tags
C fb26928c9f6f Regression tests for #2874: Block Components.interfaces from content
C 0a2323b8fcaa Regression tests for Bug 1517: Reduce precision of time for Javascript.
F af9e23384692 Regression tests for Bug 15646: Prevent keyboard layout fingerprinting in KeyboardEvent
F 6a7ae76e406e Regression tests for Bug 17009: Pref to suppress some modifier key events
D 53531cf002aa Bug 18619: If indexedDB disabled, use in-memory db for asyncStorage.js
F db5663390b3e Bug 18821: Disable libmdns for Android and Desktop
F 90e817059ab7 Bug 18800: Remove localhost DNS lookup in nsProfileLock.cpp
F ac9bc3723c2b Bug 18799: disable Network Tickler
U 88e5ed76f941 Bug 6786: Do not expose system colors to CSS or canvas.
F aa65fd2ea82e Bug 16620: Clear window.name when no referrer sent
U 72998c7d5064 Bug 6253: Add canvas image extraction prompt.
U c9c82d317082 Bug 17009: Pref to suppress some modifier key events
U cbad7a986dcb Bug 15646: Prevent keyboard layout fingerprinting in KeyboardEvent
U f6683c586a30 Bug 16005: Relax minimal mode.
U 03f286aa425e Bug 1517: Reduce precision of time for Javascript.
C 5adf623b76f8 Bug 16441: Suppress "Reset Tor Browser" prompt.
C a71bf76df344 Bug 14392: Make about:tor behave like other initial pages.
F ea9c5e94e364 Bug 2176: Rebrand Firefox to TorBrowser
C d3a986dfb477 Bug 18995: Regression test to ensure CacheStorage is disabled in private browsing
C b4981a144854 Regression tests for #5856: Do not expose physical screen info via window & window.screen.
C 98966f5b88b5 Regression tests for #2875: Limit device and system specific CSS Media Queries.
C 90f3c1b3b687 Regression tests for #4755: Return client window coordinates for mouse event screenX/Y (for dragend, 0,0 is returned).
C 73dc870c6712 Regression tests for "Omnibox: Add DDG, Startpage, Disconnect, Youtube, Twitter; remove Amazon, eBay, bing"
C ba2620e0c91d Regression tests for TB4: Tor Browser's Firefox preference overrides.
C 6bbe63c3f3b8 Regression tests for Bug #2950: Make Permissions Manager memory-only
C c38fc187252c Bug 12620: TorBrowser regression tests folder
F c8fbfdb5b0e7 Bug 14631: Improve profile access error msgs (strings).
F f05b2599c291 Bug 14631: Improve profile access error messages.
F 9a13c4dd4d89 Bug 14716: HTTP Basic Authentication prompt only displayed once
C 4fd7433d2b79 Bug 3875: Use Optimistic Data SOCKS variant.
O 2c74c1e6b2c7 Bug 5282: Randomize HTTP request order and pipeline depth.
C 05c64bde4a76 Bug 13028: Prevent potential proxy bypass cases.
O[Bug 18743] fd4a8863a4c3 Bug 16488:  Remove "Sign in to Sync" from the menu.
F c91cc92acf64 Bug 16439: remove screencasting code.
U 478ee75278f0 Bug 12827: Create preference to disable SVG.
F 6e18348d3fa2 Bug 2874: Block Components.interfaces from content
C 7190f7e52771 Bug 12974: Disable NTLM and Negotiate HTTP Auth
F d9ffdac205cc Bug 10280: Don't load any plugins into the address space.
C 83e40fc55843 Bug 8312: Remove "This plugin is disabled" barrier.
C 7151b7736fbc Bug 3547: Block all plugins except flash.
F 3efb1fb5990a TB4: Tor Browser's Firefox preference overrides.
C b7ba24e9438c TB3: Tor Browser's official .mozconfigs.
Last edited 7 months ago by arthuredelstein (previous) (diff)

comment:7 Changed 7 months ago by gk

Okay, I'll skimmed the list and have some comments:

N 2c74c1e6b2c7 Bug 5282: Randomize HTTP request order and pipeline depth.

That should be an "O" instead of an "N". There is no pipelining code anymore in ESR60, and thus the patch is obsolete.

K 2e0a54b89593 Bug 24052: Streamline handling of file:// resources

What does "K" mean here? It makes me a bit nervous given that this was one of our fixes to close a cricital hole in Firefox.

I assume "F*" means "still work in progress but essentially done"?

comment:8 in reply to:  7 Changed 7 months ago by arthuredelstein

Replying to gk:

Okay, I'll skimmed the list and have some comments:

N 2c74c1e6b2c7 Bug 5282: Randomize HTTP request order and pipeline depth.

That should be an "O" instead of an "N". There is no pipelining code anymore in ESR60, and thus the patch is obsolete.

OK! I will edit the list in comment:6 to fix that.

K 2e0a54b89593 Bug 24052: Streamline handling of file:// resources

What does "K" mean here? It makes me a bit nervous given that this was one of our fixes to close a cricital hole in Firefox.

Including this patch causes a runtime error that prevents the browser from starting up properly. We will need to debug this problem.

I assume "F*" means "still work in progress but essentially done"?

Yes, the patch seems to work, but there is a problem with the unit test that I would like to fix to be more sure.

comment:9 Changed 7 months ago by mcs

Thanks for posting all of the patch details.

I just attached a couple of patches that are fixups: libmdns.patch​ fixes a compile error on macOS, and packaging.patch​ fixes errors that prevented ./mach package from finishing. Kathy and I are still working off your 25543_volatile+4 branch for the moment, but it looks like your 25543+6 branch needs these changes too.

Changed 7 months ago by mcs

Attachment: tb-patches-2018-04-20.zip added

#13252 patch plus a few fixups

comment:10 Changed 7 months ago by mcs

I removed the individual "fixup" patches that I attached to this ticket a couple of days ago and I replaced them with a .zip archive which contains proper git am formatted patches. The .zip also contains a rebased patch for #13252.

Kathy and I are still working on rebasing these two patches:
P 4564a5f744df Bug 13379: Sign our MAR files.
P 4c9f746f2c19 Bug 4234: Use the Firefox Update Process for Tor Browser.

We will also take care of this one:
P? 9ae35ba3c07e Bug 19121: reinstate the update.xml hash check
For this patch we need to resurrect more code that Mozilla has removed (see https://bugzil.la/1373267).

comment:11 in reply to:  10 Changed 7 months ago by arthuredelstein

Replying to mcs:

I removed the individual "fixup" patches that I attached to this ticket a couple of days ago and I replaced them with a .zip archive which contains proper git am formatted patches. The .zip also contains a rebased patch for #13252.

Thank you for these! I have added those patches to the branch and rebased to the latest mozilla-beta (Firefox 60).

https://github.com/arthuredelstein/tor-browser/commits/25543+7 (b67b8dc99c9a31121a5bb1204634c833e74fb63f)

comment:12 Changed 7 months ago by igt0

Cc: igt0 added

Changed 7 months ago by mcs

Attachment: tb-patches-2018-04-24.zip added

more fixup patches

comment:13 Changed 7 months ago by mcs

I attached a zip archive that contains a few more fixup patches. There are probably more places in the Tor Browser patches where we need to remove the JS version from the MIME type when referencing from an XHTML file or HTML file (see 0001-fixup-Bug-16940-After-update-load-local-change-notes.patch within the zip archive).

Also, Kathy and I finished our initial rebasing of the updater patches and are in the process of testing them. I assume you would rather wait and receive tested patches, but let us know if you would rather have them sooner (we will probably not finish the updater testing until next week).

comment:14 in reply to:  6 ; Changed 7 months ago by sysrqb

Replying to arthuredelstein:

F 3efb1fb5990a TB4: Tor Browser's Firefox preference overrides.

Friendly reminder to update general.useragent.override before merging this. (59d6b454184b3 on 25543+7)

comment:15 in reply to:  13 Changed 7 months ago by arthuredelstein

Replying to mcs:

I attached a zip archive that contains a few more fixup patches.

Thanks for these -- I have added them to https://github.com/arthuredelstein/tor-browser/commits/25543+7 (HEAD is now fe38c8a83d8e90a99f6d04fb19a1dc085a19a766).

There are probably more places in the Tor Browser patches where we need to remove the JS version from the MIME type when referencing from an XHTML file or HTML file (see 0001-fixup-Bug-16940-After-update-load-local-change-notes.patch within the zip archive).

I did a search to find any examples of "version=" and found them in the following files:

docshell/test/test_tor_bug16620.html  
docshell/test/tor_bug16620.html       
docshell/test/tor_bug16620_form.html  
dom/events/test/test_tor_bug15646.html
tbb-tests/test_tor_bug1517.html       
tbb-tests/test_tor_bug23104.html      
tbb-tests/test_tor_bug2875.html       

These are all tests we have added, and it seems that the scripts still run, so I'm inclined not to patch them, although it would do no harm to do so.

Also, Kathy and I finished our initial rebasing of the updater patches and are in the process of testing them. I assume you would rather wait and receive tested patches, but let us know if you would rather have them sooner (we will probably not finish the updater testing until next week).

Waiting for your tests sounds good to me. Thank you!

comment:16 in reply to:  14 Changed 7 months ago by arthuredelstein

Replying to sysrqb:

Replying to arthuredelstein:

F 3efb1fb5990a TB4: Tor Browser's Firefox preference overrides.

Friendly reminder to update general.useragent.override before merging this. (59d6b454184b3 on 25543+7)

Thanks! I added a fixup commit: 947eba86aeed38727605e9cb875c9a5ddb05b7a4

comment:17 Changed 7 months ago by intrigeri

Cc: intrigeri added

comment:18 Changed 7 months ago by mcs

The updater patches are available here:

https://people.torproject.org/~brade/tmp/25543/

Kathy and I thought it would be a good idea to include a signature.
We included the patches for #25909 as well.

comment:19 Changed 6 months ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201804 removed

Move our roadmap tickets to May.

Changed 6 months ago by mcs

remove &mig64=1 code

comment:20 Changed 6 months ago by mcs

I just attached a fixup patch 0001-fixup-Bug-4234-Use-the-Firefox-Update-Process-for-To.patch that removes some code that hunts around in the Windows registry to decide whether to append &mig64=1 to the update check URL. Arthur, please include this fixup when you create a new branch.

For Firefox, the registry keys that are checked are set by Mozilla's installer. This means that they will never be set for Tor Browser, which also means it is a waste of time to look for them.

comment:21 Changed 6 months ago by arthuredelstein

Thanks to mcs and brade for the updater patches and fixups. Here's a new branch including them all, rebased to the latest Firefox 60 (which is now in mozilla/release).

https://github.com/arthuredelstein/tor-browser/commits/25543+10

Last edited 6 months ago by arthuredelstein (previous) (diff)

comment:22 Changed 6 months ago by gk

Status: assignedneeds_revision

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

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

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

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

(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?]

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

fixed by cca5e99eb181e9e120a809e3ef71ea7e73fbec89

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?

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

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.

856b810081cfbcd4b3f17ede4d1a95f4c02a14f0 -- not okay (d4da5714eb9d)

no need for those extensions.hotfix prefs

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?

fb7f56c28a9c3a77382361f218ac146c23ef6e30 -- okay (452829d9135f)
80e6743fbf4ceb60b1073e60efd6c67eb2a7fba0 -- okay (04e72287a8c7)
a761c12e657389847ff8c29ed836b5c2541c09c9 -- okay (87036e9e33eb)

The patch for 19411 is missing (fee72fffc081)

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 I think

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)

comment:23 in reply to:  22 Changed 6 months ago by mcs

Replying to gk:

...
The patch for 19411 is missing (fee72fffc081)

Kathy and I looked at #19411 and have determined that the patch is obsolete. Code examination shows that when Mozilla switched to the doorhanger-based UI they also rewrote the code that adds the badges to the hamburger menu. We will comment in the corresponding Bugzilla bug.

aede530c3ec8850224b7f6c49c1d7e1466684639 -- not okay (9ae35ba3c07e)

Kathy and I will handle this one.

5c9aa89a778d10d19b55736abfcb63b745e14379 -- not okay (4c9f746f2c19)

Kathy and I will handle this one too. I made a few comments below.

-  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)

Yes, that is what happened.

aurora|alpha|beta|hardened|release|esr (and at other places) <- we don't have the hardened series anymore

We left that in "just in case" but will leave it out.

Do we need

-      let prereleaseChannels = ["nightly", "aurora", "beta"];
+      let prereleaseChannels = ["nightly", "aurora", "alpha", "beta"];

?

This is not really needed since it is in telemetry code. We will leave it out.

The "extensions.lastTorBrowserVersion" part spilled over into AddonsManager.jsm from #13052 + adding the AddonManager.jsm in toolkit/mozapps/extensions/moz.build

You meant #13252. After talking to Kathy, I remember that we intentionally moved that code from #13252 to #4234 (but we can move it back if you think that makes more sense).

+  done <"${tmpfile}"

add a whitespace between "<" and "\""

We followed the format that Mozilla used just above in list_dirs.

mcs/brade: how do you feel addressing #24476 while you are at it?

Sure, we can do that.

comment:24 Changed 6 months ago by gk

Keywords: ff60-esr added

comment:25 Changed 6 months ago by gk

Some more comments:

abdefb33d518b0cab3aef6006a3b4f7e7bd37b37 -- not okay (4564a5f744df)

+    LOG(("ApplyUpdate -- appDir->Get(Native)Path() failed (0x%x)\n", rv2));

should not have "(" and ")" around "Native"

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.

dd2e54105436100a82f59f8042122e91ce4c8775 -- not okay (dc4fdd28c696)

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?

comment:26 Changed 6 months ago by gk

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.

5172d9a05ad7bfd92765e9cfeb7de596508d4df8 -- not okay (43c1ed31857d)

That's not needed anymore, see bug 1388902 and 1406193.

comment:27 Changed 6 months ago by mcs

Kathy and I agree that the patch order is important. We looked at the #13252 patch as well as the updater patches, and I propose that Kathy and I will produce revised patches that apply in the following order:

TicketDescriptionHashes from Arthur's 25543+10 branch
#13252Do not store data in the app bundle2921e01e5533264e8cfac92fe2d295149098e9b6
#19121Revert Bug 1373267 Remove hashFunction and hashValue861ec411a79124d5fffcd5c8f5d51f078ec5f21b
#19121reinstate the update.xml hash checkd7252e51dba825a0051c20beb0192066c7a7bc88
#25909disable updater telemetry226e54bc49c6e30e961566fac95e8ccc8e8ee86a
#4234main TB updater patch80955896a5b447ea2908f562bbefdef9072c0be1 a81aa97d552918424c51a268e52454349169d6e9
#13379sign our MAR files4a12c75f47d951186db9822fc425a3655d30fd26
#16940about:tbupdate61aaeccacd01b036b4368f9b4f1204bd97cc0951 91282e9a83b0f427495f52a2e3cdde58d2a2493a 86f03e70ecd31d7b1905a62562ad11835a4bf0ec 6a3713c9aabd1067f7e51550ec34980541b94753 1d965d29b043484ed7c56be03f2229123d772cf5

This will place all of the updater patches together in a logical order.

Also, we will merge the fixup patches for the above so there will only be one patch for each.

Finally, we propose dropping the following two patches because we need to use a new SHA384 MAR signing key. Is there a ticket for that task?

TicketDescriptionHashes from Arthur's 25543+10 branch
#20589Adding new MAR signing keya90ef29f2b35068fd76d34b6e14d5a5a96fc81a5
#23916Add new MAR signing key289569ffa97760cca8e8b8d9d4163868bb8f6472

Georg and Arthur: Please let Kathy and me know if you agree with this plan.

comment:28 in reply to:  27 ; Changed 6 months ago by gk

Replying to mcs:

Kathy and I agree that the patch order is important. We looked at the #13252 patch as well as the updater patches, and I propose that Kathy and I will produce revised patches that apply in the following order:

TicketDescriptionHashes from Arthur's 25543+10 branch
#13252Do not store data in the app bundle2921e01e5533264e8cfac92fe2d295149098e9b6
#19121Revert Bug 1373267 Remove hashFunction and hashValue861ec411a79124d5fffcd5c8f5d51f078ec5f21b
#19121reinstate the update.xml hash checkd7252e51dba825a0051c20beb0192066c7a7bc88
#25909disable updater telemetry226e54bc49c6e30e961566fac95e8ccc8e8ee86a
#4234main TB updater patch80955896a5b447ea2908f562bbefdef9072c0be1 a81aa97d552918424c51a268e52454349169d6e9
#13379sign our MAR files4a12c75f47d951186db9822fc425a3655d30fd26
#16940about:tbupdate61aaeccacd01b036b4368f9b4f1204bd97cc0951 91282e9a83b0f427495f52a2e3cdde58d2a2493a 86f03e70ecd31d7b1905a62562ad11835a4bf0ec 6a3713c9aabd1067f7e51550ec34980541b94753 1d965d29b043484ed7c56be03f2229123d772cf5

This will place all of the updater patches together in a logical order.

Also, we will merge the fixup patches for the above so there will only be one patch for each.

Sounds good to me.

Finally, we propose dropping the following two patches because we need to use a new SHA384 MAR signing key. Is there a ticket for that task?

Yes, #26045.

TicketDescriptionHashes from Arthur's 25543+10 branch
#20589Adding new MAR signing keya90ef29f2b35068fd76d34b6e14d5a5a96fc81a5
#23916Add new MAR signing key289569ffa97760cca8e8b8d9d4163868bb8f6472

Georg and Arthur: Please let Kathy and me know if you agree with this plan.

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.

comment:29 in reply to:  25 Changed 6 months ago by mcs

Replying to gk:

Some more comments:

abdefb33d518b0cab3aef6006a3b4f7e7bd37b37 -- not okay (4564a5f744df)

+    LOG(("ApplyUpdate -- appDir->Get(Native)Path() failed (0x%x)\n", rv2));

should not have "(" and ")" around "Native"

OK. We will add platform-specific log messages (within the #ifdef's).

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.

Yes. That got switched around because of the order of the patches, but Kathy and I are fixing it.

dd2e54105436100a82f59f8042122e91ce4c8775 -- not okay (dc4fdd28c696)

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.

Good catch. Kathy and I messed that up and will fix it.

What's the reason for replacing extern NS_METHOD, class nsIFile; with extern nsresult?

Mozilla has eliminated NS_METHOD. See https://bugzilla.mozilla.org/show_bug.cgi?id=1295053
As for class nsIFile, we found that is not needed because it is defined in nsIFile.h (maybe that was always the case, or maybe things changes since we originally wrote that code).

comment:30 in reply to:  28 ; Changed 6 months ago by mcs

Replying to gk:

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. :)

The new updater needs MAR files in the new format, so I don't see why we need to keep one of the old signing keys. But it is OK to do so for now.

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.

For now, that is what I have done in the local tree that Kathy and I are working on; hopefully we will be done soon and make the patches available here.

comment:31 Changed 6 months ago by gk

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."

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?

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.

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"

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".

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".

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).

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.

9684e260d0cb2abd4fb62d7960f9f25e60b48c5f -- not okay (009bc0a8f600)

I think this could be a fixup to our .mozconfig commit.

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.

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.

a4ae1392fffd7aa6ae1ba28575bad6de3b3489a3 -- not okay (ba141b6054ea)

Not needed anymore as showModalDialog() is gone (see: https://bugzilla.mozilla.org/show_bug.cgi?id=981796)

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.

cac8d0158e81c7809afbe922c00c60d418b32850 -- not okay (93999a363c76)

Let's use the upstreamed patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1441327 instead.

4ac551d9d7d1c3b6aebc5a2fccc7ea5b7aa79da4 -- not okay (c70454fd10ef)

 }
+static bool
+IsSecureHost(nsIURI *aHostURI) {

Please add a newline before "+static bool".
You need to patch the isSecure usage in AddInternal() as well as in the
patch on our esr52 branch.

5b566bf19806ebf1fb56750b549a59cc9c9d3191 -- not okay (82cd8ae9a5de)

It's fixed with the fixup commit (adb38389878bdcef91919d91ceb61370cb6cbadb).

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.

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)

comment:32 in reply to:  30 Changed 6 months ago by gk

Replying to mcs:

Replying to gk:

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. :)

The new updater needs MAR files in the new format, so I don't see why we need to keep one of the old signing keys. But it is OK to do so for now.

Aha, yes, I think I was confused in thinking we need ship at least one know key when doing an update but the important part is rather: the MAR files need to be *signed* with one known key. So, as long as we sign the first update using the new MAR format and shipping the new key(s) with a known key (to the user) we should be good. And as this is a watershed update we can ship two keys right away not bothering that the update after the first one is only using new keys. So assuming I got it right this time, I agree, we can start with two new keys and just leave the previous MAR key patches out.

comment:33 Changed 6 months ago by mcs

The revised patches that Kathy and I worked on are available here:

https://people.torproject.org/~brade/tmp/25543/

There are 7 patches; see comment:27.
We started with Arthur's 25543+10 branch, removed patches until we got to the #21724 patch, and then created these patches starting at that point.

comment:34 in reply to:  31 Changed 6 months ago by gk

Replying to gk:

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)

Actually, I think that's the better fix, see: comment:7:ticket:25909. So, we are good with that one.

comment:35 in reply to:  20 Changed 6 months ago by gk

Replying to mcs:

I just attached a fixup patch 0001-fixup-Bug-4234-Use-the-Firefox-Update-Process-for-To.patch that removes some code that hunts around in the Windows registry to decide whether to append &mig64=1 to the update check URL. Arthur, please include this fixup when you create a new branch.

For Firefox, the registry keys that are checked are set by Mozilla's installer. This means that they will never be set for Tor Browser, which also means it is a waste of time to look for them.

Looks good to me (I checked commit d2fd026be23ba97ba4826dedb2197c778d0315b1 on Arthur's 25543+10).

comment:36 in reply to:  28 ; Changed 6 months ago 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 patches. Details follow:

Replying to 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 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 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 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 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.

Last edited 6 months ago by arthuredelstein (previous) (diff)

comment:37 Changed 6 months ago by arthuredelstein

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201805 removed
Status: needs_revisionneeds_review

comment:38 Changed 6 months ago by gk

Marked #22564 as a duplicate as we are disabling Sync during the rebase by setting identity.fxaccounts.enabled to false.

comment:39 in reply to:  36 ; Changed 6 months ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201805R removed
Status: needs_reviewneeds_revision

Replying to arthuredelstein:

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

Thanks, we are getting closer! Could you rebase the next branch onto the commit used for 60.0.1esr (it won't have the bulk of the Windows build fixes, but that's okay as we'll cherry-pick those once we get to the Windows builds)?

Here is what I did: I created two branches, one with the patches from 25543+8 and one with the patches from 25543+13, rebased both to the latest esr60 commit and diffed them with git diff then I compared the result to my notes from the 25543+8 review and tried to make sure everything that should have been changed got changed and everything that should have been gone was gone (and that the remaining things staid as they were).

I've yet to give the permissions related patch a second look and have not looked at the external helper app patch.

mcs/brade could you help with the latter? I think it's commit 50f1dc0847958df657529be88b32410c57969a9e (feel free to have another look at the permissions one, too, if you like (54e14170fe07a214d3a27d813d111c174737df5d)).

Alright, here comes the feedback:

1) f89d5a6b2775cfb9a0f45669640c73c9ef9855a6 needs to go, it is disabling our hash check for the downloaded MAR files.

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.)

I still see those characters. I think you can get rid of them by converting that file using dos2unix or some similar tool.

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.

No, it is gone on your branch, please add it.

ee6b576bc2a9f5c885ae9f027dd1bd4e78ab86ce -- not okay (2646633951fe)

That's not needed anymore given that https://bugzilla.mozilla.org/show_bug.cgi?id=1418052 landed.

Removed.

No, the patch is still there (commit 6a5d740cd85de6e2ea97e33f705cb0633893bff5).

a4ae1392fffd7aa6ae1ba28575bad6de3b3489a3 -- not okay (ba141b6054ea)

Not needed anymore as showModalDialog() is gone (see: https://bugzilla.mozilla.org/show_bug.cgi?id=981796)

Removed.

No, the preference is still there.

comment:40 Changed 6 months ago by gk

#24476 is a duplicate of this one as using set -e in the scripts generating the update files is done during this rebase.

comment:41 Changed 6 months ago by gk

commit 5d8d6b14fba6607e29199bfc18f8e9310dcd4525 looks good to me.

comment:42 Changed 6 months ago by gk

The external helper app patch looks good to me, too (I've not tested it, though).

comment:43 in reply to:  39 ; Changed 6 months ago by mcs

Replying to gk:

I've yet to give the permissions related patch a second look and have not looked at the external helper app patch.

mcs/brade could you help with the latter? I think it's commit 50f1dc0847958df657529be88b32410c57969a9e (feel free to have another look at the permissions one, too, if you like (54e14170fe07a214d3a27d813d111c174737df5d)).

Unfortunately, we won't have time to look at this today (we are not really around). What is needed? Review of the rebased patch?

comment:44 in reply to:  43 Changed 6 months ago by gk

Replying to mcs:

Replying to gk:

I've yet to give the permissions related patch a second look and have not looked at the external helper app patch.

mcs/brade could you help with the latter? I think it's commit 50f1dc0847958df657529be88b32410c57969a9e (feel free to have another look at the permissions one, too, if you like (54e14170fe07a214d3a27d813d111c174737df5d)).

Unfortunately, we won't have time to look at this today (we are not really around). What is needed? Review of the rebased patch?

Yes, but it is not urgent. It looked good to me and it seemed to be pretty straightforward. We can move on without your review for now.

comment:45 in reply to:  39 ; Changed 6 months ago by arthuredelstein

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201805 removed
Status: needs_revisionneeds_review

Here's the new branch: https://github.com/arthuredelstein/tor-browser/commits/25543+15 (fe68460a72cddd936a5a313a2f986bafd9e7e7ef)

Replying to gk:

1) f89d5a6b2775cfb9a0f45669640c73c9ef9855a6 needs to go, it is disabling our hash check for the downloaded MAR files.

Removed.

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.)

I still see those characters. I think you can get rid of them by converting that file using dos2unix or some similar tool.

I'm not seeing the characters -- maybe I'm missing something? The new commit is fe68460a72cddd936a5a313a2f986bafd9e7e7ef

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.

No, it is gone on your branch, please add it.

Oops -- added back.

ee6b576bc2a9f5c885ae9f027dd1bd4e78ab86ce -- not okay (2646633951fe)

That's not needed anymore given that https://bugzilla.mozilla.org/show_bug.cgi?id=1418052 landed.

Removed.

No, the patch is still there (commit 6a5d740cd85de6e2ea97e33f705cb0633893bff5).

Ok, actually removed.

a4ae1392fffd7aa6ae1ba28575bad6de3b3489a3 -- not okay (ba141b6054ea)

Not needed anymore as showModalDialog() is gone (see: https://bugzilla.mozilla.org/show_bug.cgi?id=981796)

Removed.

No, the preference is still there.

Removed properly now.

comment:46 in reply to:  45 Changed 6 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to arthuredelstein:

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.)

I still see those characters. I think you can get rid of them by converting that file using dos2unix or some similar tool.

I'm not seeing the characters -- maybe I'm missing something? The new commit is fe68460a72cddd936a5a313a2f986bafd9e7e7ef

I see those by doing git show -p -1 extensions/cookie/test/unit/test_permmanager_defaults.js. The problem seems actually to be that the patch for bug 1421992 is causing this. Thus, if Mozilla thinks this is okay, then be it so.

This looks fine to me now. I added just one fixup commit to remove a superfluous newline that got added during the last revision and pushed tor-browser-60.0.1esr-8.0-1 to our tor-browser repo. \o/

comment:47 in reply to:  42 Changed 6 months ago by mcs

Replying to gk:

The external helper app patch looks good to me, too (I've not tested it, though).

To close the loop on this one: Kathy and I reviewed and tested the patch (on macOS), and it looks good to us.

Note: See TracTickets for help on using tickets.