Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#20680 closed defect (fixed)

Rebase Tor Browser patches to 52 ESR

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff52-esr, TorBrowserTeam201704, tbb-7.0-must-alpha
Cc: gk, brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor4

Description

Here is FIREFOX_AURORA_52_BASE:
https://github.com/mozilla/gecko-dev/commit/29475fa4c5a45574e57d8f14e5bde8261e482c35

Important dates:
Firefox Beta 52: 2017-01-24
Firefox Release 52: 2017-03-07
Firefox 45 ESR End-of-life: 2017-06-13

Child Tickets

TicketStatusOwnerSummaryComponent
#21201closedtbb-teamAdapt torbutton to TBB/FF52ESRApplications/Tor Browser
#21309closedtbb-teamFix Omnibox for TBB/52ESRApplications/Tor Browser
#21546closedtbb-teamAdapt Tor Launcher to TBB/FF52ESRApplications/Tor Browser
#21712closedtbb-teamIs our patch for #19212 still necessary in ESR52?Applications/Tor Browser

Attachments (3)

0001-fixup-Bug-4234-Use-the-Firefox-Update-Process-for-To.patch (1.9 KB) - added by mcs 2 years ago.
fixup for the ESR52 #4234 patch
browser-xul-fixup.txt (1.8 KB) - added by mcs 2 years ago.
another fixup! (for the rebased #6253 patch)
0001-fixup-Bug-18821-Disable-libmdns-for-Android-and-Desk.patch (1.2 KB) - added by mcs 2 years ago.
#18821 fixup for Linux and Windows

Download all attachments as: .zip

Change History (67)

comment:1 Changed 3 years ago by gk

Cc: gk added

comment:2 Changed 3 years ago by gk

Keywords: ff52-esr added

comment:3 Changed 3 years ago by mcs

Cc: brade mcs added

comment:4 Changed 3 years ago by gk

Keywords: TorBrowserTeam201612 added

comment:5 Changed 3 years ago by gk

Keywords: TorBrowserTeam201701 added; TorBrowserTeam201612 removed

Moving our tickets to January 2017

comment:6 Changed 2 years ago by arthuredelstein

Here's my current branch (updated on 27 January):

https://github.com/arthuredelstein/tor-browser/commits/20680+2

And here is a table tracking where each patch from TBB/45ESR went. Note there are three additional patches needed here, for which I have opened tickets. I'm also still working on testing and inspecting these patches -- any findings are very welcome.

Rebasing TBB/45ESR to TBB/52ESR

Key:
A = Already in ESR52 (had been backported to TBB/ESR45)
B = Replaced by backport from FF53 or later
D = Dropped commit (because of Reverts)
O = Obsolete because of other changes
I = Incoming Firefox patch that we could still backport in the near future (would be relabeled B).
P = Rebased from TBB/ESR56 to TBB/ESR52 by Pearl Crescent (mcs and brade)
R = Rebased from TBB/ESR45 to TBB/ESR52
U = Uplifted/replaced in Firefox and therefore already in ESR52
W = Patch re-written (see child bugs for review)
* = More work needed

R bde5dc5 Bug 20589: Adding new MAR signing key
R e90690e Bug 13252: Do not store data in the app bundle
R 90cb545 Bug #10281: Use jemalloc4 and abort on redzone corruption
A[3445ad74] 4b51be9 Bug 1277704 - Update jemalloc 4 to version 4.3.1. r=glandium
A[662ef756] 89d17cb Bug 1269959 - Update jemalloc 4 to version 4.1.1. r=glandium
A[8170c2d9] 98c0053 Bug 1254850 - Update jemalloc 4 to version 4.1.0. r=njn
A[1ef4f451] d303a01 Bug 1186934 - update jemalloc to upstream HEAD; r=glandium
R c9cf878 Bug 16622: Pref to spoof time zone as UTC
R 66a6826 Bug 20707: Avoid localization failure in about:preferences
R a926b2b Bug 19459: Size new windows to 1000x1000 or nearest 200x100
A[42404707] c6d2b47 Bug 1311275 - use protocol service directly instead of NS_GetFileFromURLSpec; r=mayhemer
A[d7672f77] c64ea49 Bug 1273371, don't use the searchbar for this test, instead use a separate textbox, r=gijs
A[780d816c] 226549c Bug 1270277, HasDataMatchingFlavors should only return true for text/unicode, r=snorp
A[a4ee9d8d] fe6b667 Bug 1249522, when a file is present, only specify file type, r=smaug
A[27d39ba9] d0dc268 Bug 1311044 - show error when connection to domain socket is failed; r=bagder
U[2151007a] d150c8f Bug 20304: SOCKS socket does not support spaces and other special characters
R 605c5e5 Bug 20244.2: Add "privacy.thirdparty.isolate" checkbox
R 796c0b5 Bug 20244.1: Add "privacy.resistFingerprinting" checkbox
U[see d087a35e] 54a14f6 Bug 20043: Isolate SharedWorker script requests to first party
A[63c4f33f] f54d277 Bug 1070710 - Use ViewRegion for window dragging. r=spohl
A[f1138d1e] f805bd1 Bug 1070710 - Use ViewRegion for vibrant areas in VibrancyManager. r=spohl
A[5ee44d89] 4454b6e Bug 1070710 - Add mozilla::ViewRegion which assembles a LayoutDeviceIntRegion as NSViews. r=spohl
A[92fabd41] a6e755e Bug 1291543 - [1.1] Accept partial information from VBR headers. r=jya
A[e1bbdff4] 7a30be5 Bug 1263334 - Check VBR header is valid before using it for duration calculations. r=esawin
A[d69c074e] 5894fef Bug 1236639 - [1.2] Avoid division by zero in MP3Demuxer. r=gerald
R b0c0a61 Bug 20123: Always block remote jar files
R 6767d56 Bug 17334: Spoof referrer when leaving a .onion domain
R 18db5c1 Bug 17858: Cannot create incremental MARs for hardened builds.
R 8cbed5e4 Bug 19890: Disable installation of system addons
R 1240853 Bug 19273: Avoid JavaScript patching of the external app helper dialog.
R 0f5d15f Bug 19417: Disable asmjs for now
R 70e290b Bug 18923: Add a script to run all Tor Browser specific tests
D 558f719 Revert "Bug 18923: Add a script to run all Tor Browser specific tests"
D 5475dc3 Bug 18923: Add a script to run all Tor Browser specific tests
U[bgz.la/1304219] e3aae80 Bug 16998: Isolate link rel=preconnect to first party
D 8e2ac91 Revert "Bug 16998: Disable link rel=preconnect"
R 5d60090 Bug 19411: Update icon shows up even if partial updates are failing.
R 7432546 Regression tests for Bug 1517: Reduce precision of time for Javascript.
R 10a70ab Bug 19212: SIGSEGV with developer tools open
O 17b0875 Bug 18884: Add --disable-loop flag
R 6dd286e Bug 18914: Use English-only label in <isindex/> tags
R 4f6d3ec Bug 18912: add automated tests for updater cert pinning
R 1b612be Bug 19121: reinstate the update.xml hash check
A[b565a3d4] b79ca4f Bug 18885: Disable possible logging of TLS key material
R d491d26 Regression tests for Bug 15646: Prevent keyboard layout fingerprinting in KeyboardEvent
R d816be5 Regression tests for Bug 17009: Pref to suppress some modifier key events
O[eabb5f64] 14fcdbf Bug 18886: Hide pocket menu items when Pocket is disabled
R 4b78eb5 Bug 18619: If indexedDB disabled, use in-memory db for asyncStorage.js
U[54c8149d] 44d8ac6 Bug 18958: Spoof screen.orientation values
R 9a58c59 Bug 18995: Regression test to ensure CacheStorage is disabled in private browsing
R 7525830 Bug 18900: updater doesn't work on Linux (cannot find libraries)
D f6a772e Bug 16998: Disable link rel=preconnect
R 1982608 Bug 18821: Disable libmdns for Android and Desktop
R 271699e Bug 18800: Remove localhost DNS lookup in nsProfileLock.cpp
U[a934a3b7] 794c4a7 Bug 13419: Fix ICU cross-compilation for Windows
R 6ebbc50 Bug 14970: Don't block our unsigned extensions
R 794d6e1 Bug 18799: disable Network Tickler
R 2aa8106 Bug 6786: Do not expose system colors to CSS or canvas.
P 2581fe5 Bug 13252 - Do not store data in the app bundle
R a576dc8 Bug 18292: Staged updates fail on Windows
P 8a77ff2 Bug 16940: After update, load local change notes.
R b264be6 Bug 18008: Create a new MAR Signing key
P db78778 Bug 13379: Sign our MAR files.
P ac912c2 Bug #4234: Use the Firefox Update Process for Tor Browser.
R ce73edb Bug 18170: After update, only changelog tab shown
R 0525158 Bug #11641: change TBB directory structure to be more like Firefox's
R bb70648 Bug #9173: Change the default Firefox profile directory to be TBB-relative.
U[bgz.la/1277803] df5c185 Bug #13670.1: Isolate favicon requests by first party
U[b003df4b] f6a31c4 Bug 16300: Isolate Broadcast Channels to first party.
U[33d9942f] 9f80f4d Regression tests for Bug 15564: Isolate SharedWorker by first party domain
U[dfebfaa3] 1392761 Bug 15564: Isolate SharedWorker by first party domain
U[bgz.la/1264595] 5b9b5c7 Bug #15703: Regression tests for isolation of mediasource URI
U[bd3c0cc8] e6d5488 Bug #15502, Part 2: Regression tests for blob URL isolation
U[bgz.la/1260931] 43785cf Bug #15502. Isolate blob, mediasource & mediastream URLs to first party
U[bgz.la/1264562, bgz.la/1312794] 4751d0e Bug 13670.2: Isolate OCSP requests by first party domain
U[2b1661df] c6c578d Bug #13749.1: regression tests for first party isolation of localStorage
U[bgz.la/1260931] a60ca50 Bug #6564: Isolate DOM storage to first party URI.
U[d087a35e] b07443b Bug #13749.2: Regression tests for first-party isolation of cache
U[bgz.la/1270680] 7843363 Bug #6539: Isolate the Image Cache per url bar domain.
U[bgz.la/1260931]] 66f87b3 Bug 13742: Isolate cache to URL bar domain.
U[a8b4c2a9] eb04eeb Bug 13900: Remove 3rd party HTTP auth tokens.
O[first-party isolation] 7dde6e5 Bug #5742: API allows you to get the url bar URI for a channel or nsIDocument.
R 7b9e7f1 Bug 16620: Clear window.name when no referrer sent
R*(#18599) 1a64b63 Bug #6253: Add canvas image extraction prompt.
R e08ad00 Bug 18297: Use separate Noto JP,KR,SC,TC fonts
U[2fefe85c] 196a0c3 Regression tests for Bug #17207: Hide mime types and plugins when resisting fingerprinting
U[2fefe85c] 74b1f7c Bug #17207: Hide mime types and plugins when resisting fingerprinting
U[cdccbe2a] ef49977 Bug #13313: Pref 'font.system.whitelist' restricts set of permitted fonts
R 39cddae Bug 17009: Pref to suppress some modifier key events
R 3246840 Bug 15646: Prevent keyboard layout fingerprinting in KeyboardEvent
R 68f324f Bug #16005: Relax minimal mode.
R 6a871dd Bug 1517: Reduce precision of time for Javascript.
A[3345f3b6] 8b9f5c4 Bug 867501 - Pref allows JS locale to be set to US English/C. r=khuey
R 218728b Regression tests for #5856: Do not expose physical screen info via window & window.screen.
R 87105f1 Regression tests for #2875: Limit device and system specific CSS Media Queries.
R 4668a00 Regression tests for #4755: Return client window coordinates for mouse event screenX/Y (for dragend, 0,0 is returned).
R e386200 Bug 16441: Suppress "Reset Tor Browser" prompt.
R 129c3f4 Bug 14392: Make about:tor behave like other initial pages.
R 10a7cd9 Bug #2176: Rebrand Firefox to TorBrowser
R e0eb3f3 Regression tests for "Omnibox: Add DDG, Startpage, Disconnect, Youtube, Twitter; remove Amazon, eBay, bing"
*(#21309) 911d56f Omnibox: Add DDG, Startpage, Disconnect, Youtube, Twitter;  remove Amazon, eBay, bing
R 1ab9ef0 Regression tests for TB4: Tor Browser's Firefox preference overrides.
R 1d1df84 Regression tests for Bug #2950: Make Permissions Manager memory-only
R dd55334 Regression tests for #2874: Block Components.interfaces from content
R f2a0d52 Bug #12620: TorBrowser regression tests folder
R 656b1e2 Bug 14631: Improve profile access error msgs (strings).
R a72a74d Bug 14631: Improve profile access error messages.
O[456e54eb3] 9f284eb Bug #16855: Allow blobs to be downloaded on first-party pages
*(#21308) c2d877c Bug 16528: Prevent indexedDB Modernizr breakage (e10s highrisk).
R 8c9ad0a Bug 14716: HTTP Basic Authentication prompt only displayed once
R 515daac Bug #3875: Use Optimistic Data SOCKS variant.
R c7b0a03 Bug #5282: Randomize HTTP request order and pipeline depth.
R fe45c436d Bug 13028: Prevent potential proxy bypass cases.
U[0e9470fe, ef52c3bbf] 05dc6ad Bug #5741: Prevent WebSocket DNS leak.
R 9baae2e Bug 16488:  Remove "Sign in to Sync" from the menu.
R 5e39125 Bug 16439: remove screencasting code.
U[91d0ac11] 602ee90 Bug 17502: Add a pref hiding the "Open with" option
R,I 4a3629a Bug 12827: Create preference to disable SVG.
U[556ed991] 41073c0 Bug 13548: Create preference to disable MathML.
R 7271e80 Bug #2874: Block Components.interfaces from content
R 4425a1b Bug #12974: Disable NTLM and Negotiate HTTP Auth
R 2d728f7 Bug 10280: Don't load any plugins into the address space.
R 4173f95 Bug #8312: Remove "This plugin is disabled" barrier.
R de2eb8f Bug #3547: Block all plugins except flash.
O [loop removed] 9adf819 Bug 16863: console.error on new Tor Browser window
R d0fff8c TB4: Tor Browser's Firefox preference overrides.
R,A [94fa8fd7] 9b466e4 Don't package things we don't build
A[7041992f] e89d0bf Bug 1211567 - Enable domain socket support for SOCKS; r=bagder
O 83c294c Revert "Bug 1229855: Fix miscompilation of uint8_t enum class with gcc4.8.2; r=luke a=lizzard"
A[b093982d] b1b7c16 Bug 1238694 - Limit the number of asm.js/wasm code allocations to avoid running into Linux kernel limits. r=luke
A[1d92294b] 81a0560 Bug 1234246 - Don't reprotect JIT code more than once when linking. r=nbp
A[0db5d8b5] 399e261 Bug 1215479 - Turn on W^X JIT code by default. r=luke
A[e2fe0b8f] 956bfb8 Bug 1233328 - Part 2: Use SHA-256 StaticFingerprints directly instead of StaticPinset since the SHA-1 StaticFingerprints entry will always be null. r=keeler
A[638ba07a] 7da7afe Bug 1233328 - Part 1: Ignore SHA-1 pins in PublicKeyPinningService.cpp. r=keeler
A[05919374] 8d6f636 Bug 1229284 - Remove support for SHA-1 hashes in genHPKPStaticPins.js. r=keeler
A[5d2aea87] f39769b Bug 1266963, stop propagation before other steps, r=masayuki
A[a815bdb8] a73119f Bug 1246614 - Check if system add-ons directory exists before trying to clean it. r=mossop
A[a3ad2879] 255a977 Bug 1250046 - Remove Shumway references from telemetry. r=gfritzsche
A[347e3720] 0928713 Bug 1250046 - Remove Shumway references from IPC. r=jmathies
A[d3e1f744] 730552f Bug 1250046 - Remove Shumway core files. r=till
A[687d9646] e162f31 Bug 1233963 - Work around recent GNU gold behavior with segments starting before the first section they contain
O bc348b2 Revert "Bug 856404 - Enable libraries folding on mingw. r=glandium"
A[c1230235] 00808ec Don't use -Werror in mingw builds
O[dd664443] 1186ff4 Disabling view management for mingw-w64 builds
A[9e4a3887] 82f4abf Bug 1240589 - Cross compilation fixup.
A[65aeb7ca] 223ec27 Bug 1167248 - Cross compilation fixup.
R 5fb68cb TB3: Tor Browser's official .mozconfigs.
Last edited 2 years ago by arthuredelstein (previous) (diff)

comment:7 Changed 2 years ago by mcs

Kathy and I slightly messed up the #4234 patch. Mozilla removed support for the
--enable-update-packaging configure option (but we don't think it is needed anyway). I will attach a fixup patch.

Changed 2 years ago by mcs

fixup for the ESR52 #4234 patch

Changed 2 years ago by mcs

Attachment: browser-xul-fixup.txt added

another fixup! (for the rebased #6253 patch)

comment:8 Changed 2 years ago by gk

Thanks for the rebase work, Arthur.

Two questions before looking at the code:

1) "R[a934a3b7] 794c4a7 Bug 13419: Fix ICU cross-compilation for Windows" should be "U[a934a3b7] 794c4a7 Bug 13419: Fix ICU cross-compilation for Windows"?

2) What is the "I" in "R,I 4a3629a Bug 12827: Create preference to disable SVG."?

FWIW: We start the switch to ESR 52 with 77 rebased patches while we started the switch to ESR 38 and 45 with 82 each.

comment:9 Changed 2 years ago by gk

Arthur: you have "O 17b0875 Bug 18884: Add --disable-loop flag" yet that flag is still in the .mozconfig files.

comment:10 in reply to:  7 Changed 2 years ago by arthuredelstein

Replying to mcs:

Kathy and I slightly messed up the #4234 patch. Mozilla removed support for the
--enable-update-packaging configure option (but we don't think it is needed anyway). I will attach a fixup patch.

Thanks! I added your two fixup patches to the same branch.

comment:11 in reply to:  9 Changed 2 years ago by arthuredelstein

Replying to gk:

Arthur: you have "O 17b0875 Bug 18884: Add --disable-loop flag" yet that flag is still in the .mozconfig files.

Good point. I added a fixup.

comment:12 in reply to:  8 Changed 2 years ago by arthuredelstein

Replying to gk:

Thanks for the rebase work, Arthur.

Two questions before looking at the code:

1) "R[a934a3b7] 794c4a7 Bug 13419: Fix ICU cross-compilation for Windows" should be "U[a934a3b7] 794c4a7 Bug 13419: Fix ICU cross-compilation for Windows"?

You are right. I will fix in comment:6.

2) What is the "I" in "R,I 4a3629a Bug 12827: Create preference to disable SVG."?

I forgot to add this to the key. There is an incoming patch (now landed) that we can potentially backport and replace our existing SVG patch with. There a few edge cases that Jonathan is still dealing with, however.

FWIW: We start the switch to ESR 52 with 77 rebased patches while we started the switch to ESR 38 and 45 with 82 each.

That's exciting! Most of the work is in first-party isolation patches upstreamed and adapted to Firefox by Mozilla's Tor uplift team. That saved a lot of rebasing work.

Last edited 2 years ago by arthuredelstein (previous) (diff)

comment:13 Changed 2 years ago by gk

mcs/brade: the patch for #15640 got finally included in the big canvas one. Do you think it would be worthwhile noting this in the commit message as well? I saw we mentioned a bunch of other bugs there too and thought it might help clarifying things in case one is wondering where that patch is gone (as I did while updating the design doc).

comment:14 in reply to:  13 ; Changed 2 years ago by gk

Replying to gk:

mcs/brade: the patch for #15640 got finally included in the big canvas one. Do you think it would be worthwhile noting this in the commit message as well? I saw we mentioned a bunch of other bugs there too and thought it might help clarifying things in case one is wondering where that patch is gone (as I did while updating the design doc).

I feel mentioning our adapted logic from #17446 would be good as well.

comment:15 in reply to:  14 Changed 2 years ago by mcs

Replying to gk:

Replying to gk:

mcs/brade: the patch for #15640 got finally included in the big canvas one. Do you think it would be worthwhile noting this in the commit message as well? I saw we mentioned a bunch of other bugs there too and thought it might help clarifying things in case one is wondering where that patch is gone (as I did while updating the design doc).

I feel mentioning our adapted logic from #17446 would be good as well.

Yes, we should mention both of those tickets in the new #6253 commit message.
Arthur, can you add them when you create a new ESR52 branch or is that not something you will need to do soon?

comment:16 Changed 2 years ago by gk

Keywords: TorBrowserTeam201702 added; TorBrowserTeam201701 removed

Moving our tickets to Feb 2017.

comment:17 Changed 2 years ago by gk

Sponsor: Sponsor4

This is Sponsor4 work

Changed 2 years ago by mcs

#18821 fixup for Linux and Windows

comment:18 Changed 2 years ago by mcs

I just attached a fix that Kathy and I needed to make in order to build successfully on Linux (probably also needed for Windows).

comment:19 in reply to:  18 ; Changed 2 years ago by arthuredelstein

Replying to mcs:

Replying to gk:

Replying to gk:

mcs/brade: the patch for #15640 got finally included in the big canvas one. Do you think it would be worthwhile noting this in the commit message as well? I saw we mentioned a bunch of other bugs there too and thought it might help clarifying things in case one is wondering where that patch is gone (as I did while updating the design doc).

I feel mentioning our adapted logic from #17446 would be good as well.

Yes, we should mention both of those tickets in the new #6253 commit message.
Arthur, can you add them when you create a new ESR52 branch or is that not something you will need to do soon?

I have added mention of both of these tickets in our #6253 commit message.

Replying to mcs:

I just attached a fix that Kathy and I needed to make in order to build successfully on Linux (probably also needed for Windows).

Thanks -- added to my branch.

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

comment:20 in reply to:  19 ; Changed 2 years ago by gk

Replying to arthuredelstein:

Replying to mcs:

Replying to gk:

Replying to gk:

mcs/brade: the patch for #15640 got finally included in the big canvas one. Do you think it would be worthwhile noting this in the commit message as well? I saw we mentioned a bunch of other bugs there too and thought it might help clarifying things in case one is wondering where that patch is gone (as I did while updating the design doc).

I feel mentioning our adapted logic from #17446 would be good as well.

Yes, we should mention both of those tickets in the new #6253 commit message.
Arthur, can you add them when you create a new ESR52 branch or is that not something you will need to do soon?

I have added mention of both of these tickets in our #6253 commit message.

Replying to mcs:

I just attached a fix that Kathy and I needed to make in order to build successfully on Linux (probably also needed for Windows).

Thanks -- added to my branch.

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

Could you adapt the commit message for the patch for #1517 as well? We don't round keyboard events to 250ms, see #19186.

While looking a bit over the patches it occurred to me we want to test whether we still need the patch for #19212.

comment:21 in reply to:  20 Changed 2 years ago by arthuredelstein

Replying to gk:

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

Could you adapt the commit message for the patch for #1517 as well? We don't round keyboard events to 250ms, see #19186.

Done. https://github.com/arthuredelstein/tor-browser/commits/20680+4

While looking a bit over the patches it occurred to me we want to test whether we still need the patch for #19212.

Still looking into this. So far I tried reverting the patch and saw no crash. But I need to be able to reproduce the crash in TBB/ESR45 and haven't done that yet.

comment:22 Changed 2 years ago by gk

Keywords: tbb-7.0-must added

comment:23 Changed 2 years ago by gk

Keywords: TorBrowserTeam201703 added; TorBrowserTeam201702 removed

Moving tickets to March

comment:24 Changed 2 years ago by gk

I guess for our jemalloc hardening we need updated jemalloc code in our alphas and the .mozconfig file needs to get amended:

# Use jemalloc 4.x. In 52ESR, we should use
# the --enable-jemalloc=4 flag instead:
export MOZ_JEMALLOC4=1

comment:25 in reply to:  24 Changed 2 years ago by arthuredelstein

Replying to gk:

I guess for our jemalloc hardening we need updated jemalloc code in our alphas and the .mozconfig file needs to get amended:

# Use jemalloc 4.x. In 52ESR, we should use
# the --enable-jemalloc=4 flag instead:
export MOZ_JEMALLOC4=1

Fixed.

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

I opened a ticket to see if we still need our #19212 patch: #21712.

comment:26 Changed 2 years ago by gk

ESR 52 is out. Could you rebase the patches once more to the up-to-date Mozilla branch? FWIW: we have an official branch now in gecko-dev for that. There are a bunch of rebase conflicts and I'd like to review the final patches.

comment:27 in reply to:  26 Changed 2 years ago by arthuredelstein

Replying to gk:

ESR 52 is out. Could you rebase the patches once more to the up-to-date Mozilla branch? FWIW: we have an official branch now in gecko-dev for that. There are a bunch of rebase conflicts and I'd like to review the final patches.

Yes -- for the record, here's the branch rebased to gecko-dev/esr52:
https://github.com/arthuredelstein/tor-browser/commits/20680+6

And here is the same branch with fixups squashed:
https://github.com/arthuredelstein/tor-browser/commits/20680+7

comment:28 Changed 2 years ago by arthuredelstein

comment:29 Changed 2 years ago by gk

In 20680+7 (and 20680+8) ac_add_options --enable-update-packaging is back again -- which is not available anymore.

comment:30 Changed 2 years ago by cypherpunks

O [removed in esr17] Bug #6210: set plugin.expose_full_path to false
O [removed in esr38] Bug #9012: Do not display the missing plugin information bar
D [broken] Bug #9867: Flash is not click-to-play
O* [removed in esr31] Bug #10703: Fallback charset enables fingerprinting of bundle localization
O [FF has] Bug #11253: Turn on TLS 1.1 and 1.2 in TorBrowser
O [removed in esr31] Bug #12212: Disable deprecated Audio Data API
O [removed in esr38] Bug #15029: Tor Browser 4.5a4 prompts to install Flash
O [FF has] Bug #17369: The RC4 cipher flags in TBB must be set to "false" by default

comment:31 in reply to:  29 Changed 2 years ago by arthuredelstein

Replying to gk:

In 20680+7 (and 20680+8) ac_add_options --enable-update-packaging is back again -- which is not available anymore.

Thanks for catching that. I added a fixup to 20680+8.

comment:32 Changed 2 years ago by gk

Keywords: tbb-7.0-must-nightly added

We want those tickets for our first ESR52 nightlies.

comment:34 Changed 2 years ago by 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
2) f161c394e049a440637f06ba87dd6be6f73479bb should get merged keeping the bug 17858... in the commit message
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

7a2928fb53ee51714a825cdcadc749274495a9ab:
1) Merge e44e6f529e10468c2694b889a70b9b85f109f949 + mention bug in commit message
2) Merge c7113c8588eba3de80819090c38145d08d9eea0a + mention bug in commit message
3) Merge f3ec07bff9a96ffbfe9c3d03c1725a557097cc61 + mention bug in commit message
4) pref("gfx.xrender.enabled",false); is already included in ESR52, no need to set it again

e0213fac01fb1fc9f0949490d652c6ae979d6bd4: not needed anymore

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?

Last edited 2 years ago by gk (previous) (diff)

comment:35 Changed 2 years ago by 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?

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.

800a82cf4133635e6b709013bbb95ccc3ae1a5e7:

1) Why is MOZ_UTF16() not good enough anymore? Does the code not build with it anymore?

comment:36 Changed 2 years ago by 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?)
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);

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

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

fd11d2ad97ea828f9e68750165de70cb34e3a7e0: good
d882e68b91a8a9ac1b6656bec5c38a2a7514115d: good
d85df6ecd6f8de4ff718b3dc85882686f94488a9: good

Last edited 2 years ago by gk (previous) (diff)

comment:37 Changed 2 years ago by 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.

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!?)
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 ?
8387adbc333c6502e098eb21f8a1934e91a7c8d4: good

comment:38 Changed 2 years ago by gk

Another bunch of comments:

b0b8846de6ff4764b47035c72faf7764df29c5ea: good
e5669287f7218a7f97c2bf4895524e9a6dbfc9df: good
88b6156799a5e6d7f8f0de10c3e06dc3f668a3da: good (update to newer jemalloc4?)
5d5007a994f6f0965f4dbd0355919002384deb55: good
219ef733285a0c9a40955104354deb0ae8bab55e:

"Do not accept or send remote commands;" -> "(default) Do not accept or send remote commands;"

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

Last edited 2 years ago by gk (previous) (diff)

comment:39 in reply to:  35 Changed 2 years ago by mcs

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

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

comment:40 Changed 2 years ago by 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.

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

dea0055d7dabfbe23fe8191b42dbf4ac0a9c02d0: good
d6a9e29b04760f56bf7f4d82798d915da5a28c2c: good
015699fe2b5fb82884e900901d7648727a720e06 (mozilla backport): good
697a2218e7c6511174e2946137d4f2d62aeca8c5 (mozilla backport): good (use fix for 1348841 instead or get it uplifted)
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?)

comment:41 in reply to:  36 ; Changed 2 years ago by mcs

Replying to gk:

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

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

The old API is gone. Kathy and I agree that a NULL pointer check should be added.

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

Kathy and I agree that it should be topLevelDocURI.

Here are the additional things that we noticed:

  • The second call to CanvasPermissionPromptHelper.init(); should be .uninit();
  • In browser/base/content/browser.xul, the class canvas-icon was added but Kathy and I do not see why it is needed.
  • In dom/canvas/OffscreenCanvas.cpp, maybe add a reference to #18599 and also mention that for now placeholder image data is always returned to users of OffscreenCanvas.

comment:42 in reply to:  41 ; Changed 2 years ago by mcs

Replying to mcs:

Replying to gk:

013d0cd1f153626cb7f40cc39288300ee55e100e: (mcs/brade could you have a second look here as well?)

One more thing: on OSX at least, the canvas permission prompt is not displayed (access is always blocked). It looks like the canvas-permissions-prompt observer notification is not received. This seems to be an e10s problem, because the prompt is correctly displayed if I disable multiprocess mode.

comment:43 in reply to:  42 Changed 2 years ago by mcs

Replying to mcs:

One more thing: on OSX at least, the canvas permission prompt is not displayed (access is always blocked). It looks like the canvas-permissions-prompt observer notification is not received. This seems to be an e10s problem, because the prompt is correctly displayed if I disable multiprocess mode.

I just realized that this issue is already being tracked in #21778.

comment:44 Changed 2 years ago by arthuredelstein

Replying to 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 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 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 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 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 mcs:

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

comment:45 in reply to:  41 Changed 2 years ago by arthuredelstein

Replying to mcs:
Sorry, I missed this part:

Here are the additional things that we noticed:

  • The second call to CanvasPermissionPromptHelper.init(); should be .uninit();

Fixed.

  • In browser/base/content/browser.xul, the class canvas-icon was added but Kathy and I do not see why it is needed.

Removed.

  • In dom/canvas/OffscreenCanvas.cpp, maybe add a reference to #18599 and also mention that for now placeholder image data is always returned to users of OffscreenCanvas.

Added.

These are a fixup patch appended to
https://github.com/arthuredelstein/tor-browser/commits/20680+10

comment:46 Changed 2 years ago by gk

Thanks, I prepared tor-browser-52.0.2esr-7.0-1 which I am going to push once I have proper Internet again (in about 4 hours). I'll add #21849 as a child ticket as this is rebase related.

comment:47 Changed 2 years ago by gk

Status: newneeds_information

Arthur, one thing I forgot: what happend to our DNS leak prevention patch? It seems it did not get rebased? Looking above you seem to imply it got upstreamed. I found you pointing to bug 751465 but what is the other commit you had in mind.

I think I am inclined to take that patch again until we can be sure Mozilla is taking our proxy bypass safety requirements pretty seriously.

comment:48 in reply to:  47 ; Changed 2 years ago by arthuredelstein

Replying to gk:

Arthur, one thing I forgot: what happend to our DNS leak prevention patch? It seems it did not get rebased? Looking above you seem to imply it got upstreamed. I found you pointing to bug 751465 but what is the other commit you had in mind.

Turns out I inadvertently deleted the first digit in the hash (I edited the comment above to fix this). I intended to point to the test for 751465 you provided at ef52c3bbf96f317d461ad652970f817e78923531. Do these two patches cover the problem reported in #5741, or do we need the Tor Browser patch as well?

I think I am inclined to take that patch again until we can be sure Mozilla is taking our proxy bypass safety requirements pretty seriously.

Here's a rebased version, in case we still need it:
https://github.com/arthuredelstein/tor-browser/commit/20680+10_5741

Last edited 2 years ago by arthuredelstein (previous) (diff)

comment:49 in reply to:  46 ; Changed 2 years ago by arthuredelstein

Replying to gk:

Thanks, I prepared tor-browser-52.0.2esr-7.0-1 which I am going to push once I have proper Internet again (in about 4 hours). I'll add #21849 as a child ticket as this is rebase related.

Thanks. Note there is an extra fixup here (thanks to mcs and brade for noticing this error):
https://github.com/arthuredelstein/tor-browser/commit/3d38cd9dd7596872778c6e6573ac748650e31598

comment:50 in reply to:  48 Changed 2 years ago by gk

Replying to arthuredelstein:

Replying to gk:

Arthur, one thing I forgot: what happend to our DNS leak prevention patch? It seems it did not get rebased? Looking above you seem to imply it got upstreamed. I found you pointing to bug 751465 but what is the other commit you had in mind.

Turns out I inadvertently deleted the first digit in the hash (I edited the comment above to fix this). I intended to point to the test for 751465 you provided at ef52c3bbf96f317d461ad652970f817e78923531. Do these two patches cover the problem reported in #5741, or do we need the Tor Browser patch as well?

That specific problem should be fixed by that, yes. However, the patch we had so far is more general than that as it prevents any DNS leaks not only the WebSocket related one.

I think I am inclined to take that patch again until we can be sure Mozilla is taking our proxy bypass safety requirements pretty seriously.

Here's a rebased version, in case we still need it:
https://github.com/arthuredelstein/tor-browser/commit/20680+10_5741

Thanks, I picked that one and applied it on tor-browser-52.0.2esr-7.0-2 (e81b5bba67dd3c08f76b4d8e76d04743db202011).

comment:51 in reply to:  49 Changed 2 years ago by gk

Replying to arthuredelstein:

Replying to gk:

Thanks, I prepared tor-browser-52.0.2esr-7.0-1 which I am going to push once I have proper Internet again (in about 4 hours). I'll add #21849 as a child ticket as this is rebase related.

Thanks. Note there is an extra fixup here (thanks to mcs and brade for noticing this error):
https://github.com/arthuredelstein/tor-browser/commit/3d38cd9dd7596872778c6e6573ac748650e31598

Included into the rebased tor-browser-52.0.2esr-7.0-2.

comment:52 Changed 2 years ago by gk

Keywords: TorBrowserTeam201704 added; TorBrowserTeam201703 removed

Moving tickets over to April

comment:53 Changed 2 years ago by gk

Keywords: tbb-7.0-must removed

No need to use somewhat duplicated keywords.

comment:54 Changed 2 years ago by gk

Keywords: tbb-7.0-must-alpha added; tbb-7.0-must-nightly removed

Things to tag for the alphas (as the remaining tasks in the tickets are no nightly bloclers).

comment:55 Changed 2 years ago by gk

Status: needs_informationassigned

Arthur, could you look at the child tickets so we are done here for when the alpha is due?

comment:56 Changed 2 years ago by gk

Resolution: fixed
Status: assignedclosed

We are done, thanks all.

comment:57 Changed 2 years ago by cypherpunks

So you've ignored my comment:30. OK.

comment:58 in reply to:  57 ; Changed 2 years ago by arthuredelstein

Replying to cypherpunks:

So you've ignored my comment:30. OK.

Can you please explain what you are trying to say there? I don't understand it.

comment:59 in reply to:  58 ; Changed 2 years ago by cypherpunks

Replying to arthuredelstein:

Replying to cypherpunks:

So you've ignored my comment:30. OK.

Can you please explain what you are trying to say there? I don't understand it.

  • O [removed in esr38] Bug #9012: Do not display the missing plugin information bar:

O - obsolete patch, because it is for something which was removed in esrXX.

  • D [broken] Bug #9867: Flash is not click-to-play

D - drop this patch, because it's broken.

  • O* [removed in esr31] Bug #10703: Fallback charset enables fingerprinting of bundle localization

O* - the same as O, but the new proper fix is needed.

comment:60 in reply to:  59 ; Changed 2 years ago by arthuredelstein

Replying to cypherpunks:

Replying to arthuredelstein:

Replying to cypherpunks:

So you've ignored my comment:30. OK.

Can you please explain what you are trying to say there? I don't understand it.

  • O [removed in esr38] Bug #9012: Do not display the missing plugin information bar:

O - obsolete patch, because it is for something which was removed in esrXX.

  • D [broken] Bug #9867: Flash is not click-to-play

D - drop this patch, because it's broken.

  • O* [removed in esr31] Bug #10703: Fallback charset enables fingerprinting of bundle localization

O* - the same as O, but the new proper fix is needed.

These patches are already not in our current branch, at https://gitweb.torproject.org/tor-browser.git/log/?h=tor-browser-52.0.2esr-7.0-2

Are you requesting some further action?

Last edited 2 years ago by arthuredelstein (previous) (diff)

comment:61 in reply to:  60 ; Changed 2 years ago by cypherpunks

Replying to arthuredelstein:

Replying to cypherpunks:

Replying to arthuredelstein:

Replying to cypherpunks:

So you've ignored my comment:30. OK.

Can you please explain what you are trying to say there? I don't understand it.

  • O [removed in esr38] Bug #9012: Do not display the missing plugin information bar:

O - obsolete patch, because it is for something which was removed in esrXX.

  • D [broken] Bug #9867: Flash is not click-to-play

D - drop this patch, because it's broken.

  • O* [removed in esr31] Bug #10703: Fallback charset enables fingerprinting of bundle localization

O* - the same as O, but the new proper fix is needed.

These patches are already not in our current branch, at https://gitweb.torproject.org/tor-browser.git/log/?h=tor-browser-52.0.2esr-7.0-2

All these 8 patches are (squashed?) present in https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.0.2esr-7.0-2&id=6623ae8f65bba3e7c1f117d2b4915584709af891, even with regression tests in https://gitweb.torproject.org/tor-browser.git/log/?h=tor-browser-52.0.2esr-7.0-2&ofs=100 and refs to their bug numbers.

Are you requesting some further action?

For needs proper fix?

comment:62 in reply to:  61 ; Changed 2 years ago by arthuredelstein

Replying to cypherpunks:

All these 8 patches are (squashed?) present in https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.0.2esr-7.0-2&id=6623ae8f65bba3e7c1f117d2b4915584709af891, even with regression tests in https://gitweb.torproject.org/tor-browser.git/log/?h=tor-browser-52.0.2esr-7.0-2&ofs=100 and refs to their bug numbers.

I see. Are you suggesting these settings should be removed?

comment:63 in reply to:  62 ; Changed 2 years ago by cypherpunks

Replying to arthuredelstein:

Replying to cypherpunks:

All these 8 patches are (squashed?) present in https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.0.2esr-7.0-2&id=6623ae8f65bba3e7c1f117d2b4915584709af891, even with regression tests in https://gitweb.torproject.org/tor-browser.git/log/?h=tor-browser-52.0.2esr-7.0-2&ofs=100 and refs to their bug numbers.

I see. Are you suggesting these settings should be removed?

Should you remove settings that do nothing, duplicate upstream or are wrong?

comment:64 in reply to:  63 Changed 2 years ago by arthuredelstein

Replying to cypherpunks:

Replying to arthuredelstein:

Replying to cypherpunks:

All these 8 patches are (squashed?) present in https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.0.2esr-7.0-2&id=6623ae8f65bba3e7c1f117d2b4915584709af891, even with regression tests in https://gitweb.torproject.org/tor-browser.git/log/?h=tor-browser-52.0.2esr-7.0-2&ofs=100 and refs to their bug numbers.

I see. Are you suggesting these settings should be removed?

Should you remove settings that do nothing, duplicate upstream or are wrong?

I have opened a ticket to look into this: #21916

Note: See TracTickets for help on using tickets.