Opened 7 months ago

Closed 4 weeks ago

Last modified 8 days ago

#30429 closed task (fixed)

Rebase Tor Browser patches for Firefox ESR 68

Reported by: gk Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff68-esr, tbb-9.0-must-alpha, TorBrowserTeam201911R, BugSmashFund
Cc: acat, pospeselr, sysrqb, mcs, brade Actual Points: 30
Parent ID: Points: 1
Reviewer: Sponsor:

Description

We need to start rebasing our patches against Firefox 68. This is the ticket that tracks the whole effort.

It's helpful how we did it the last time: comment:6:ticket:25543. As mentioned there https://torpat.ch/ is a very valuable resource. It might need updating, though (which we should do while we are at it, or point Arthur to the things that need to get fixed).

Child Tickets

TicketStatusOwnerSummaryComponent
#28822closedtbb-teamre-implement desktop onboarding for ESR 68Applications/Tor Browser

Change History (98)

comment:1 Changed 7 months ago by gk

Priority: HighVery High

comment:2 Changed 7 months ago by acat

Using commits 47aefe747950..e9b8de8c8d6a from tor-browser-60.7.0esr-9.0.1.

Most of the desktop patches are rebased in https://github.com/acatarineu/tor-browser/commits/30429.

Below you can find the list commits/patches split into several "categories". Order of commits was not preserved in all cases, but the set of the commits of all categories below should be the same as above (47aefe747950..e9b8de8c8d6a).

In that branch there is a WIP #10760 commit adding the browser parts of the torbutton integration, and also temp. moving the torbutton submodule to a WIP branch https://github.com/acatarineu/torbutton/tree/10760. This of course will have to change once/if torbutton migration proposal is approved and #10760 changes accepted. Most (or all?) torbutton features should be working with this.

I'm currently building with mach build && mach package and disabling tor-launcher (--disable-tor-launcher), since AFAIK it would still not work without #29197. I could also not run tbb-tests (although I did not try very hard yet). To launch Tor, I'm doing it manually with

tor HashedControlPassword $(tor --quiet --hash-password mypassword) SocksPort 9150 ControlPort 9151 HTTPTunnelPort 0 DataDirectory /tmp/tortemp

then launching the built browser with

TOR_CONTROL_PASSWD='"mypassword"' ./firefox

NoScript addon can be manually installed (and enabled on private windows, see #28896) and then security levels UI should work fine.

Some TODOs:

  • Port tor-launcher to 68: at least #29197, perhaps more.
  • Integrate in tor-browser-build: is the toolchain ready? would it "just work" to change the firefox repo to point to this one?
  • Be able to run tbb-tests (and make them green).
  • Updater: will mcs/brade work on these?
  • Onboarding: as described in #28822, this needs to be ported, since onboarding (bootstrapped) extension is not there anymore. I could take a look at this one.
  • Onion security expectations: a couple of patches, depending on availability, perhaps pospeselr could work on these?
  • Decide what to do with patches from #28711.
  • Verify 'might not be needed' patches.
  • Backport https://bugzilla.mozilla.org/show_bug.cgi?id=1330467 or wait if it's included in 68.
  • Android: should we do this after desktop patches? here or in a separate ticket?

[rebased]

724044f7deae Bug 28044: Integrate Tor Launcher into tor-browser                                                                                                     
7b9d68ac9f9a Bug 28369: stop shipping pingsender executable                                  
b5ce43598da9 Bug 27503: Disabling accessibility on Windows breaks screen readers             
350fb9de802c Bug 25658: Replace security slider with security level UI                                        
784829dd21ce Bug 29120: Enable media cache in memory                                         
4791a84c69f2 Bug 28885: notify users that update is downloading
0db452ba7d90 Bug 12885: Windows Jump Lists fail for Tor Browser                                       
9a64fd2b6786 Bug 25702: Update Tor Browser icon to follow design guidelines ---[Notes: app.update.download.backgroundInterval is not there anymore; this changes onboarding assets]
000879d8c0d6 Bug 27623 - Export MOZILLA_OFFICIAL during desktop builds                       
cf13c54c3725 Bug 26146: Spoof HTTP User-Agent header for desktop platforms ---[Move to pref overrides?]
ae2a89c7e1e7 Bug 26048: potentially confusing "restart to update" message ---[Did not find a simple way to do this with Fluent, so reused another DTD entitity that is still there...]
6bcc9d3aea2d Bug 24056: Use en-US strings in HTML forms                                                                                       
e8833081b428 Bug 27082: enable a limited UITour                                              
f6cfb16dcbab Bug 26514 - intermittent updater failures on Win64 (Error 19)                                                 
dcb5386e668d Bug 26353: Prevent speculative connect that violated FPI.                                                               
898b402c2458 Bug 26045: Add new MAR signing keys                                             
1ef28dca0a8f Bug 21537: Tests for secure .onion cookies                                      
d5cf5a3f1e89 Bug 21537: Mark .onion cookies as secure                                        
97237186c6b5 Bug 22548: Firefox downgrades VP9 videos to VP8.                                                           
0a793927d7b0 Bug 23104: Add a default line height compensation                               
8ccd532c1007 Bug 21830: Copying large text from web console leaks to /tmp                    
75ea009946e5 Bug 21321: Add test for .onion whitelisting                                     
7fe43a8ecdeb Bug 21431: Clean-up system extensions shipped in Firefox 52                     
a347487a1d1e Bug 16285: Exclude ClearKey system for now ---[There's a new MOZ_EME_WIN32_ARTIFACT, it should not be enabled.]
7162a6dd03b1 Bug 21907: Fix runtime error on CentOS 6 ---[It seems CentOS 6 is supported until Nov 2020, so this is still needed]
e92cd56ed887 Bug 21849: Don't allow SSL key logging                                          
7586806f59e4 Bug #5741: Prevent WebSocket DNS leak. ---[Uplifted, but there are bugzillas 1470411, 1361337, 896206, ...]
08be04a55490 Bug 14970: Don't block our unsigned extensions                                  
169496549650 Omnibox: Add DDG, Startpage, Disconnect, Youtube, Twitter;  remove Amazon, eBa.. ---[Forcing legacy xml in Desktop, instead of new webext]
93c05f56d671 Bug 16940: After update, load local change notes. --- Moved AboutTBUpdate.jsm to AboutTBUpdateChild.jsm, extending ActorChild (followed pattern used internally). This seems to be loading blank about:tor pages and about:tbupdate on startup. Perhaps some missing prefs?
c6843493db5a Bug 21724: Make Firefox and Tor Browser distinct macOS apps                     
85334a982084 Bug 18912: add automated tests for updater cert pinning                                      
cc8dc68c600d Bug 11641: change TBB directory structure to be more like Firefox's             
055498dc6885 Bug 9173: Change the default Firefox profile directory to be TBB-relative.      
1a096da1e527 Bug 19890: Disable installation of system addons                                
2828385c7e2a Bug 19273: Avoid JavaScript patching of the external app helper dialog.         
1f1780afa6c9 Bug 18923: Add a script to run all Tor Browser specific tests                   
e0bf4a7a6e8c Regression tests for #2874: Block Components.interfaces from content            
86bab99e16bf Bug 18821: Disable libmdns for Android and Desktop                              
742d0c9459c8 Bug 18800: Remove localhost DNS lookup in nsProfileLock.cpp                     
6a3afe512b84 Bug 18799: disable Network Tickler                                              
1e91414fac09 Bug 16620: Clear window.name when no referrer sent                              
190e9e67f67e Bug 16441: Suppress "Reset Tor Browser" prompt.                                 
04c01de92591 Bug 14392: Make about:tor behave like other initial pages.                      
548e81f875d1 Bug 2176: Rebrand Firefox to TorBrowser                                         
5d5e7469b211 Bug 18995: Regression test to ensure CacheStorage is disabled in private brows..
d5b09650a06f Regression tests for "Omnibox: Add DDG, Startpage, Disconnect, Youtube, Twitte..
2a7fa9ec659c Regression tests for TB4: Tor Browser's Firefox preference overrides.           
1f9d6925f3d3 Regression tests for Bug #2950: Make Permissions Manager memory-only            
61d4f80ab517 Bug 12620: TorBrowser regression tests folder                                   
5f4a2625e699 Bug 14631: Improve profile access error msgs (strings).                         
568ef8aa5967 Bug 14631: Improve profile access error messages.                                                
b6da0f584d7b Bug 13028: Prevent potential proxy bypass cases.                                
2eb11a17e593 Bug 16439: remove screencasting code. --- [Investigate, is 'secondscreen/RokuApp.jsm' and 'secondscreen/SimpleServiceDiscovery.jsm' only used in Android now? or is there a different mechanism for desktop?]
d243f5affabc Bug 2874: Block Components.interfaces from content --- [Now done via pref.]
38dc17f97c8a Bug 12974: Disable NTLM and Negotiate HTTP Auth --- [Uplifted, but we might still want this.]
da77d1390e7b Bug 10280: Don't load any plugins into the address space.                       
1380ffc86035 Bug 8312: Remove "This plugin is disabled" barrier.                             
07fe87a26867 Bug 3547: Block all plugins except flash.
3bdb29524b80 Bug 26321: New Circuit and New Identity menu items       
05c7fdec8e51 Bug 28640 - Try showing the homepage after the Distribution loads               
e6f469366c70 Bug 28640 - Reload distribution preferences on update                           
aa6704c5c5a7 Bug 28640 - Uninstall torbutton in the user profile on Android                                                                   
d9446943c73a Bug 25013: Add torbutton as a tor-browser submodule                                     
8c146581a741 TB4: Tor Browser's Firefox preference overrides.
47aefe747950 TB3: Tor Browser's official .mozconfigs. --- [commented out --disable-maintenance-service in .mozconfig, only for Windows?]                                       

[TODO updater -> Pearl Crescent?]

f3f4fcf25d43 Bug 13379: Sign our MAR files.
657ffb296498 Bug 4234: Use the Firefox Update Process for Tor Browser.
477911c58292 Bug 19121: reinstate the update.xml hash check                                  
b5be160897af Bug 19121: reinstate the update.xml hash check
29951f9c779c Bug 18900: updater doesn't work on Linux (cannot find  libraries)  
60e7b5b78e87 Bug 13252: Do not store data in the app bundle

[TODO onboarding -> to be ported in 28822 - Note: 25702 also touches some onboarding file]

d92980f74016 Bug 29768: Introduce new features to users
523a66a1affa Bug 27486 Avoid about:blank tabs when opening onboarding pages.
ee4f40d85a8b Bug 26962 - implement new features onboarding (part 1).
98b1707f1930 Bug 26961: New user onboarding.

[TODO onion security expectations]

988d41acfaca Bug 26456: HTTP .onion sites inherit previous page's  certificate information
651e4ef7de3e Bug 23247: Communicating security expectations for .onion

[TODO waiting for 1330467 to reland]

f47cd2fb5288 Bug 21569: Add first-party domain to Permissions key ---[https://bugzilla.mozilla.org/show_bug.cgi?id=1330467]
3298251467df Bug 26670: Make canvas permission respect FPI ---[https://bugzilla.mozilla.org/show_bug.cgi?id=1330467]

[TODO not landed firefox patches from #28711 - update and do 'try' run?]

7afe16fd6d27 Bug 1474659 Part 2 - Add dedicated AllocKinds just for ArrayBufferObjects. r?s.. ---[GC code changed, would be good to update patch and do 'try' to check breakage]
2beafe9bd417 Bug 1474659 Part 1 - Add support to EnumSet for more than 32 values. r?sfink ---[Changes for this part seem not to be needed anymore]

[DROP not needed]

e9b8de8c8d6a Updating Torbutton to 2.1.9                                                     
9e2fca791f62 Pull in latest Torbutton code
19963a38e431 Pulling in new Torbutton release
2170968a8e1e Pulling in latest Torbutton code (2.1.6)
76c0ea886161 Pull in latest Torbutton code
1b10f5a8d841 Picking up latest Torbutton changes
e20342fe5080 Update Torbutton to 2.1.4
67bbd0703b2d Import latest Torbutton commits
44d088130f66 Picking up latest Torbutton commits
67eb148067ba Update Torbutton submodule
40212baaea92 Revert "Bug 29445: Enable support for enterprise policies" 
6643872049b8 Bug 29445: Enable support for enterprise policies
934d12f1de17 Include <cstring> for memcmp in certverifier/Buffer.cpp

[DROP? might not be needed -> check]

8ee45ee52355 Bug 27411: Security Slider breaks on Windows --- [Message passing to noscript webext was changed, might not be an issue now]
65bbebea18a8 Bug 14716: HTTP Basic Authentication prompt only displayed once --- [Could not reproduce, seems to be working now without the patch]
1711b3160e3b Bug 26381: about:tor page does not load on first start on Windows and  browser..
09f0faa4dea6 Bug 26381: about:tor page does not load on first start on Windows ---[In case this is still needed, squash?]
7d0bb93e5c4b Bug 24398: Plugin-container process exhausts memory ---[This was needed because of 24052, but we now have mozilla 1412081 -> check]
603397b01823 Bug 27597: Package dom_bindings_test only with tests enabled                    
0ce33b09b33f Bug 27597: Package layout debugger interface only if tests are enabled 
1d2a9f3f1e8e Bug 29180: MAR download stalls when about dialog is opened ---[Previous logic of pause/restart download is not there or changed, should check if we are good without this patch.]

[DROP uplifted]

41f620e4a0a3 Bug 13398: at startup, browser gleans user FULL NAME (real name, given name) f..
091b41ec2465 Bug 21787: Spoof en-US for date picker
34063061f825 Bug 26540: Enabling pdfjs disableRange option prevents pdfs from loading
c894688d4924 Bug 25909: disable updater telemetry

[DROP included in 68]

d3072ff444c2 Bug 1548634 - Update the default letterboxing behavior to use stepped ranges r..
120eaaa6cff8 Bug 1407366 - Part 5: Reset the Zoom in browser_bug1369357_site_specific_zoom_..
8efd394db5e7 Bug 1407366 - Part 4: Adding a test case for testing letterboxing. r=johannh    
8aea5fe0df54 Bug 1407366 - Part 3: Implementing the window letterboxing. r=johannh           
9d0e645be52e Bug 1447592 Do not reset the Spoof English pref after disabling Resist Fingerp..
dcaabab69579 Bug 1407366 - Part 2: Rearrange RFPHelper for expansion r=johannh               
32eb8a433e88 Bug 1407366 - Part 1: Rename the LanguagePrompt.jsm to RFPHelper.jsm and chang..
6d6441e5a79b Bug 1441327 - Allow for seccomp filtering of socket(AF_INET/AF_INET_6) calls o..
78c2ae1d57f7 Bug 1463509 - SOCKS support for Alternative Services r=valentin
fa1eb8174ad6 Bug 859782 - Firefox cannot start without /proc (chroot). r=sfink,evilpie,jld   
084a76168f4d Bug 1470156 - Part 2: Fixing the crashing problem when using an invalid charac..
fa942f8b1bb6 Bug 1470156 - Part 1: Adding a test case for reassuring mozilla::OriginAttribu..
6e1290cfa0fc Bug 1473247 - Part 2: Add a test case for making sure that IP addresses can wo..
754ba4bf02fb Bug 1473247 - Part 1: Fixing the issue that the IP addresses won't be set for ..
ec18e05896a2 Bug 1459089 - Don't use OS Locale when resistFingerprinting is enabled. When t..
9fc79a537c6a Bug 1455165 - Filter external apps out if needed. r=mcomella                    
68220bda3a25 Bug 1474306 - Fix typo in the extension optionsType handler. r=Mossop           
7992cd827ac0 Bug 1459420 - HLS Player doesn't use the centralized Proxy Selector r=mcomella..
7a794f5f8f1d Bug 1500906 - Suppress FileUriExposedExceptions when launching helper apps. r=..
fa49a114636c Bug 1484472 - Avoid FileUriExposedException in ExternalIntentDuringPrivateBrow..
4eb162e6193a Bug 1450449 - Part 5: Disable file:// URI checks for downloaded files and laun..
3aa1d282f2df Bug 1450449 - Part 4: Starting from Nougat, install updates via content:// URI..
14a82cb003c0 Bug 1450449 - Part 3: Starting from Nougat, share images via content:// URIs. ..
aac358bdcb7f Bug 1450449 - Part 2: Use content:// URI for capturing images from FilePicker...
85d06450d978 Bug 1450449 - Part 1: Add FileProvider. r=jchen                                 
e068c7bad5c4 Bug 1480079 - Add REQUEST_INSTALL_PACKAGES permission for all builds; r=jchen   
27b6ce1f2833 Bug 1504159 - add test to verify we can save a mixed content image from the co..
f2e586f93c2f Bug 1504159 - use TYPE_SAVEAS_DOWNLOAD for data saved through nsIWebBrowserPer..
e806657ea618 Bug 1487263 - set requesting principal for macOS drags, r=mstange               
ea930c8e6643 Bug 1473507 - fix crash in nsILoadInfo::GetOriginAttributes when passing no pr..
bb22e0dc14f0 Bug 1473509 - store principal information with the URIs to avoid having to loc..
b2fc4a61addb Bug 1469916, r=ckerschb,jkt
bff72e9e644a Bug 1479311 - Don't attempt finding and highlighting a tab's base domain withi..
17698434a0b4 Bug 1448305 - Fall back to the memory cache when a shortcut is created. r=JanH  
9540e5fdef8e Bug 1448305 - Avoid disk cache for icons of TwoLinePageRow in private tabs. r=..
15b3db497834 Bug 1448305 - Avoid disk cache for icons of private tabs in the TabsLayoutItem..
9dfbafa2b516 Bug 1448305 - Avoid disk cache for icons of private tab's session history. r=J..
0b1088652aaf Bug 1448305 - Avoid disk cache for icons of private tabs. r=JanH                
f6b82e518ce3 Bug 1441345 - Force the use of the Linux phishing list on Android. r=dimi, a=p..
976a4cbc2b1b Bug 1458905 - Update to FreeType 2.9.1. r=jfkthame                              
861997889d84 Bug 1448014 - avoid needless flattening in AndroidDecoderModule; r=jesup        
74855fb47cf5 Bug 680300 - Part 3: Make the client.navigate() not to reference the baseURL i..
3cd98a889ec7 Bug 680300 - Part 2: Add a test case for ensuring no error reporting when load..
8d4eb7e0ef61 Bug 680300 - Part 1: Stopping reporting errors when loading an unknown externa..
676b2190104d Bug 1472618 - Make navigator.platform return "Win32", even on Win64 OS. r=peterv
eec5a116ea35 Bug 1463936 - Set default security.pki.name_matching_mode to enforce (3) for a..
062853dd3527 Bug 1503354 - Disable background HTTP response throttling for causing visible ..
118d51a33bdf Bug 1474626 - fix timestamp test and values, r=rpl                              
defb067c6cb0 Bug 1471959 - leave keaGroupName and signatureSchemeName undefined if value is..
a0d6857913c4 Bug 1470516 - remove or fix localized values in securityInfo, r=rpl             
e274fb6e4a68 Bug 1464481 - fix and test crash when getting registered channelwrapper, r=kmag 
3c0773d98af7 Bug 1322748 add securityInfo to webRequest listeners, r=keeler,rpl              
a91cf4cd69fd Bug 1322748 add ability to get registered channelwrappers, r=kmag
cf3c165c7d22 Bug 1516642 - Add a function declaration for arc4random_buf in expat. r=peterv                                 
fa7548c07486 Bug 1542309 - Set firstPartyDomain to public suffix if getBaseDomain fails. r=..                                        

[TODO android]

43582f5809cf Bug 29859: Disable HLS support for now                                                                                
34e52459b150 Bug 29982 - Force single-pane UI on Tor Preferences                                                                                
6cba968b5a7e Bug 30136: Use 'Tor Browser' as brand name on mobile, too
321bba05d203 Bug 30239 - Render Fragments after crash                                        
22e72c1e7716 Bug 30214 - Kill background thread when Activity is null
75dcaffedcee Bug 30086 - Prevent Sync-related crashes on Android
545db499b0c7 Bug 28622: Update Tor Browser icon for mobile                                   
b563b207c38c Bug 29238 - Prevent crash on Android after update
323ebe2ca2d4 Bug 28329 - Part 4. Add new Tor Bootstrapping and configuration screens         
79c9b8acba0b Bug 28329 - Part 3. Remove OrbotActivity dependency                             
dd3f22090e52 Bug 28329 - Part 2. Implement checking if the Tor service is running            
fe784a8284e5 Bug 28329 - Part 1. Add new Tor resources
a195c7180350 Bug 28507 - Don't call Push and Sync services during Sanitize
f6f8859b3891 Bug 26690: Port padlock states for .onion services to mobile
722afbb6c5c6 Bug 28051 - Use Orbot's notification-builder wrapper class                      
8436a9443fde Bug 28051 - Stop the background service when we're quitting                     
464c03696814 Bug 28051 - Use our Orbot for proxying our connections                          
a0b886f177d8 Bug 28051 - Launch Orbot if it isn't running in the background                  
a5e24cd02c8e Bug 28051 - Integrate Orbot and add dependencies
39316ccaedbb Bug 28507: Implement fallback to delete private data in the browser startup     
b0b54ec7c7a0 Bug 28507: Add prefs that allow the browser to delete browsing history by defa..
894c64799812 Bug 28507: Parse a set of strings in Android Set Preferences
e860dd77665d Bug 27125 - Move localized Tor Browser for Android strings into separate file
06f9c75d7725 Bug 27111: Configure tor browser for mobile to load about:tor
28a2ea997ba0 Bug 28125 - Prevent non-Necko network connections
878c9536c888 Bug 27472 - Export MOZILLA_OFFICIAL during Android build                        
826e0934adb4 Bug 27400 - Target Android API 26
cb63b9e84081 Bug 27473 - Change app name on Android for Alpha version
6177eef07093 Bug 25696 - Design of alpha onboarding for Tor Browser for Android
8e272a5204e2 Bug 25696 - Implement alpha onboarding for Tor Browser for Android 
2c08bd6d5df5 Bug 24796 - Comment out excess permissions from GeckoView
1bd41f99acd1 Bug 26825 - Delete RECEIVE_BOOT_COMPLETED permission
9023dea11376 Bug 26826 - Disable tab queue and delete SYSTEM_ALERT_WINDOW permission         
350766eecb63 Bug 25906 - Imply false both Adjust and Leanplum configure options              
9312fdf1ac53 Bug 27016 - Create proxy connection during image download
bff13bd887c3 Bug 26528 - Don't allow Fennec to use UpdateService when installed through the..
9b218c05cecc Orfox: hook up default panic trigger to "quit and clear"                        
80b18b10f092 Orfox: quit button added                                                        
f1c13fc12685 Orfox: disable screenshots and prevent page from being in "recent apps"         
e96b86f7a6f6 Bug 25741 - TBA: Disable GeckoNetworkManager                                    
ffcc8cefd762 Bug 25741 - TBA: Adjust the User Agent String so it doesn't leak Android version
0e4ae4cbd11a Bug 25741 - TBA: top sites changed, used bookmarks icon temporarily.            
6e806c3cb9ed Orfox: Centralized proxy applied to AbstractCommunicator and BaseResources.     
433766efba25 Orfox: add BroadcastReceiver to receive Tor status from Orbot                   
41b333e3a9d3 Orfox: NetCipher enabled, checks if orbot is installed                          
d073b92b20ba Bug 25741 - TBA: Neuter Firefox Accounts                                        
f96dc4c9ae4d Bug 25741 - TBA: Only include GCM permissions if we want them                   
e8f22d0b795e Bug 25741 - TBA: Only include Firefox Account permissions if we want them       
6299c89034d3 Bug 25741 - TBA: Always Quit, do not restore the last session                   
a66bd47fd601 Bug 25741 - TBA: Disable all data reporting by default                          
eebbebcb7010 Bug 25741 - TBA: Clear state when the app exits, by default                     
0820ddf07093 Bug 25741 - TBA: Do not import bookmarks and history from native browser by de..
8839bac75801 Bug 25741 - TBA: Do not save browsing history by default                        
3531fb88ab73 Bug 25741 - TBA: Move CAMERA permission within MOZ_WEBRTC                       
c910edef7833 Bug 25741 - TBA: Conditionally require *_LOCATION permissions                   
d2fa5a6928bf Bug 25741 - TBA: Conditionally require WIFI and NETWORK permissions             
f90df9c52d2d Bug 25741 - TBA: Disable QR Code reader by default                              
b6584da608bf Bug 25741 - TBA: Disable the microphone by default                              
92959f9de719 Bug 25741 - TBA: Disable telemetry and experiments                              
e4ea4bf5fbfd Bug 25741 - TBA: Remove sync option from preferences                            
7122131c88d9 Bug 25741 - TBA: Add mobile-override of 000-tor-browser prefs                   
39979b254f2c Bug 25741 - TBA: Add default configure options in dedicated file                
d9b6b1116a67 Bug 25741 - TBA: Do not register Stumbler listener at start up                  
765b0fb6cdee Bug 25741 - TBA: Add an AppConstant for TOR_BROWSER_VERSION                     
de5d9f3c4f83 Bug 25741 - TBA: Disable features at compile-time                               
2e75b845a93f Bug 25741 - TBA: Add mozconfig for Android and pertinent branding files.        
b4aef2103fef Bug 25741 - TBA: Move GCM Push prefs within preprocessor guard                  
db59d0ef5a5b Bug 25741 - TBA: Exclude unwanted Stumbler tests

comment:3 Changed 7 months ago by acat

Until Tor Launcher is integrated, it needs explicit TOR_CONTROL_PORT for New Identity to work:

TOR_CONTROL_PORT=9151 TOR_CONTROL_PASSWD='"mypassword"' ./firefox

comment:4 Changed 7 months ago by gk

Keywords: TorBrowserTeam201905R added; TorBrowserTeam201905 removed
Status: newneeds_review

comment:5 in reply to:  2 ; Changed 7 months ago by gk

Replying to acat:

[snip]

Okay, let me start by replying to the ToDos:

Some TODOs:

[snip]

  • Integrate in tor-browser-build: is the toolchain ready? would it "just work" to change the firefox repo to point to this one?

No, it is not yet. The current branch is linux_esr68_v3 in my repo (with probably a bunch of linux_esr68_v$ to follow) but there are still build requirements missing (nodejs is the next one to add)

[snip]

  • Updater: will mcs/brade work on these?

Yes.

  • Onboarding: as described in #28822, this needs to be ported, since onboarding (bootstrapped) extension is not there anymore. I could take a look at this one.

Sounds good.

  • Onion security expectations: a couple of patches, depending on availability, perhaps pospeselr could work on these?

What are the issues here? Could you file a new bug to track that work?

  • Decide what to do with patches from #28711.

Ideally we could backport them and have some knowledgeable Mozilla person looking over the result.

[snip]

It seems we need to backport them, alas. :( They got a minus for beta (that is esr68) inclusion.

  • Android: should we do this after desktop patches? here or in a separate ticket?

We should do it in parallel or better: not blocking the rebasing work on desktop patches. sysrqb will pick this up and probably decide whether to use a child bug or have the rebase in this ticket.

[snip]

comment:6 Changed 6 months ago by gk

Keywords: TorBrowserTeam201906R added; TorBrowserTeam201905R removed

Moving reviews over to June.

comment:7 Changed 6 months ago by acat

Onion security expectations: a couple of patches, depending on availability, perhaps pospeselr could work on these?

What are the issues here? Could you file a new bug to track that work?

I left them for the end after a quick look, but I just tried again and it seems not so much work. So I can work on these after working on onboarding.

comment:8 Changed 6 months ago by acat

I added some more patches in https://github.com/acatarineu/tor-browser/commits/30429+1. It's onboarding and onion security expectations ones. I think there is one from the "onion security expectations" that is not needed anymore. Changes in the list of commits:

[rebased]

+ d92980f74016 Bug 29768: Introduce new features to users
+ 523a66a1affa Bug 27486 Avoid about:blank tabs when opening onboarding pages.
+ ee4f40d85a8b Bug 26962 - implement new features onboarding (part 1).
+ 98b1707f1930 Bug 26961: New user onboarding.
+ 651e4ef7de3e Bug 23247: Communicating security expectations for .onion

[onboarding]

- d92980f74016 Bug 29768: Introduce new features to users
- 523a66a1affa Bug 27486 Avoid about:blank tabs when opening onboarding pages.
- ee4f40d85a8b Bug 26962 - implement new features onboarding (part 1).
- 98b1707f1930 Bug 26961: New user onboarding.

[onion security expectations]

- 988d41acfaca Bug 26456: HTTP .onion sites inherit previous page's certificate information
- 651e4ef7de3e Bug 23247: Communicating security expectations for .onion

[DROP? might not be needed -> check]

+ 988d41acfaca Bug 26456: HTTP .onion sites inherit previous page's certificate information

comment:9 in reply to:  5 Changed 6 months ago by sysrqb

Replying to gk:

Replying to acat:

  • Android: should we do this after desktop patches? here or in a separate ticket?

We should do it in parallel or better: not blocking the rebasing work on desktop patches. sysrqb will pick this up and probably decide whether to use a child bug or have the rebase in this ticket.

I opened #31010 for Android. I'll update that ticket and let this one remain focused on the desktop and shared code patches.

comment:10 Changed 5 months ago by gk

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201906R removed

No reviews in June 2019 anymore, moving them.

comment:11 in reply to:  2 Changed 5 months ago by gk

Keywords: TorBrowserTeam201907 added; TorBrowserTeam201907R removed
Status: needs_reviewneeds_revision

Replying to acat:

[rebased]

Here are my comments, the hashes are from your 30249 branch:

9510c9416ddd35a016ee2074dd58f927d97246c7 - not okay

no need to comment out the maintenance related option, just remove it (and while we are at it, please remove the other unused options as well)

please add --enable-proxy-bypass-protection to all mozconfigs

4aec090126cd79289628b4403366c176714a4c77 - okay
23b8e34d8fa64affdb265911ff586d8babf1a119 - should we not point to the latest Torbutton commit (especially as we have removed all the other Torbutton commit updates (rightly so))? otherwise okay

22088a63f01c5526aedcafb3232f211a78ec9106 - okay (mobile)
912ed4b07281ceebb67726b1785d12b37ab95b12 - okay (mobile)
493b664f0fd1867559e51d6015c784e7c21a3259 - okay (mobile)
34126c910efab560ff0d6923437c50758f8dc03f - we should merge that with the patches for #10760 I think (otherwise okay)
bdf970dcdeeb276eccb9538b0d86dfee07ec5776 - okay
5d3e47ad112820208dd894aaeb51497ab37caf65 - okay
27fa31d4350e4248b0bfb35c51918955629a112a - okay
b7c8b9e0b641cdfeef8ac2adc5188c3411110374 - okay
f9957d4fd3f164be68adcd32206c09cb6f59a16d - could you do a git commit --squash here with commit 4aec090126cd79289628b4403366c176714a4c77; please add the Trac bug number to this pref flip so we get easily the context and move the pref flip to the fingerprinting section of 000-tor-browser.js
55367b7e2edbaf55f1142140b2cb9ec9b9247bec - okay
9909eeb95cea7fa84bcd45bcdddd0c4c22d83e17 - okay
45072c2fea6a535a712ac0888f12f881393cccbd - (mcs/brade should have a second look at this patch) I wondered how we ever settled at a const char16_t* first given the signature of FormatStringFromName() but I agree that using const char* seems more intuitive.

+ ProfileStatus profileStatus = PROFILE_STATUS_OK; <- do we need to assign PROFILE_STATUS_OK here, wouldn't it be enough to just do ProfileStatus profileStatus; given that you do + aProfileStatus = PROFILE_STATUS_OK; and don't assign that in SelectUpdateProfile either?

There is in nsToolkitProfileService:

   GetProfileByDir(lf, localDir, getter_AddRefs(profile));

    if (profile && mIsFirstRun && mUseDedicatedProfile) {

Should we have the usual CheckProfileWriteAccess() call here as well or did you think this is a non-issue as we are there "generally" from an app initiated restart, as the comment says.

a8c48df07cc505bd45c764d223f6ff01de738f31 -- indentation (keep original):

+  nsresult rv = GetOverrideStringBundleForLocale(
+      aSBS, uriString.get(), userAgentLocale.get(), aResult);

// Build Torbutton file URI string by starting from the profiles directory. <- It's not the profiles directory anymore

Making dirProvider a RefPtr seems okay to me.

Should general.useragent.locale be intl.locale.requested (I guess this should have been the case for Tor Browser 8 already, but well...)

2d2a55296e255f9b502d0aa9eb70d4822a1bdd0e - okay
9ec12a9075a87f1a1d446200ca4f64f88ef8466f - okay [we should upstream that one, bug 967812 has code parts]
5b585b0633d5359504542b0fb05b599cb879a883 - okay
630b395081dc68828d8f55ea41c71297b123bb87 - okay
73e8dc78ddb3caf7b7dde8df2ea15dd9898cccac - not needed, see bug 1434772
8295e48bb7d948fd8e85cbc9f5ffe7e9e0d9ea6b - okay
d9cc80636e3dfcdf28bb368b416508402e9b9f9a - okay
237124a540877734cc15a60de613f29b064f3799 - okay
a4282bea59a32dba0a17f3d31e6f7f6094a98eac - not okay

There is no docshell/test/mochitest.ini in esr68 and we should not add one.

+const kTestPath = "/tests/docshell/test/"; needs adaptation to new path

Why GetComputedReferrer() and not GetOriginalReferrer()? Could you add an
explaining comment here?

a3acbf09d562e14f9e67f91db7a12bd185058b18 - not needed anymore with --enable-proxy-bypass-protection set
4bd0f7037b7a89a9ec95e540f58ebcb5cd74bd07 - okay
5011133b08cfdfbb5e6ad3ff03b1d230bb206608 - not okay

The AndroidCastProvider part needs to get ripped out in components.conf as well, as things might break otherwise if we don't include it in moz.build (which we should not and your patch makes sure)

You are not including components.conf in libmdns moz.build instead of patching it as you did in the provider case; we should have the same approach inboth cases (I am fine with just the moz.build one if we think that's enough)

4c93f2c748ba5f6f00a5d5b197716a899798aea2 - okay
6684321fb901ef1546391e786fca8d2e3ef8b5fc - okay
a2ade001bdcffa4469baa3ac88a5db19ca8c6e52 - okay
fd0570fb485cdb5d0327e51745b1ad59ef284240 - okay
ce0db56b09a3b448508cc619418d84220f8e9acd - not okay

What is the reason to patch GetUser$Directory now while we did not have done so in the esr60 patch? (I guess we should follow Mozilla here and if not, please add some comment/explanation for that deviation)

18a6e88f2c28c4439858476334388de9208ad447 - okay
d9b20bc60c8167c0ab32b93623beb8968f4b5f07 - test is probably not working anymore as we don't have static pins for #29811; we should either drop it or fix #29811 and test that the test is still working then (or working then again) (I think we should go the fixing route here :) )

2943fd7440cf90f613ff3a96180cd7d71a3ca483 - okay
9d8ca4380e947c2097d0fd3554b1f3dad20de634 - not okay

+    removeMessageListener("AboutTBUpdate:Update", this);
+    removeEventListener("pagehide", this, true);

The respective add$Listeners got lost? Or do we get them now via the LEGACY_ACTORS object? (Does not seem to be the case for me as nothing changed in that regard if you look at the restructuring of the AboutReader page) Maybe those listeners were not needed in the first place?

Does the restructured code not need isAboutTBUpdate() anymore?

We put a lot of the code behind TOR_BROWSER_UPDATE, should the inclusion of the .jsm file then be behind that as well?

I guess compilation breaks if we keep BrowserContentHandler.jsm where it is? (mcs/brade should have a second look here)

468ccd77da35cb0ea0a3419619d42ce810d00238 - not okay

Where are all the search engines coming from in that patch which we did not have in esr60? I think we don't need those.

To keep the patch small we should just patch the search engines entries of the locales we ship. This holds for mobile as well and we probably should address #30017 (and maybe #30606).

If you look at https://hg.mozilla.org/integration/autoland/rev/111b88dd28d6 you see that the search plugins are converted to WebExtensions. I wonder

a) what is the issue with not following Mozilla here? I.e. I am a bit wary of deviating from their approach as we want to keep the differences small if possible.

b) that if a) is indeed a thing we should probably make sure to comment that in the code and we might want to just not add the search extensions in the first place to minimize possible weird interactions.

4cad2e391d96b7e1c197de5c37408ecd371d6aff - okay
c1ff0d730d048e115b7d6c551b95a58a50ae8827 - okay
294445810e60e08f00b8fdf74937664eff5a925d - okay
da608e51ef73beb1f77fd6852c60b7db270ea31a - okay
8908dc936bd0ae6f497533d5a69835b81eb242e2 - okay
c55aea4b373ac4500d1539a8a4c79009fc9c0076 - okay
b0a1cddc170a51d2b97a7b9c52b4294927c599b9 - okay [upstreaming??]
1ee3bc541b96729ecb3c41df2fdf2c3bdeb278a4 - okay
3490265e4331eb77a20eeb169776ddf55d891ecd - okay
db3a5b7d339bad673a01804ff83fd2f8a79f46d3 - okay
371f840f3096a9f1cf03d448df0d62569b7a52b9 - okay [upstreaming??]
3bd36833ce14d3a9ad9d70ac4e6d4f10d5cebdfa - okay [upstreaming??]
3b6785c5de66cb0f78faf945859e090633e09525 - okay
df6140a3563f78252721cc3b53bc07ac4f05ca0e - okay
829a448ca35a6323ce5d7982738c54424292d502 - [probably upstreamed, see 1561636]
73569f04f387efd7d7b9e06e5dfadbdfba0a3ee5 - okay
9d9ca4e994b7a9713153f56a5f93ebc9fa2ed939 - okay
83459e103450f80b471ecda459a2017857e5d26c - okay
04d72c21af73359698ccd9f5c6808adb36be816c - not okay

I think we should squash it with the general 000-tor-browser.js commit (4aec090126cd79289628b4403366c176714a4c77)

ba383936028e955cef2ced0f98d2f90ca39564de - not okay

We should fold that into 9510c9416ddd35a016ee2074dd58f927d97246c7; additionally it's worth solving #27493 while we are at it.

9f05eedab888b33f83e97bb3cd870ecb5174ea41 - not okay

> .../fxmonitor/content/img/tor-watermark.svg | 6 +++

It seems you added the .svg to the wrong dir?

content/branding/horizontal-lockup.svg is missing in
browser/branding/alpha/content/jar.mn or maybe we should just remove it in the other series? Or replace it with an own icon for newInstallPage.html? Glancing at bug 1518632 I am tempted to just remove that .svg file.

We should remove the dead app.update.download.backgroundInterval.

4a8236665a3250705dbab46f6b74a6c0eac1af2b - not okay

Let's squash it with 4aec090126cd79289628b4403366c176714a4c77

5269df12586ac7e4e45803a0f1b8c15da9ef529a - looks okay; I guess you need to add the EXTRA_PP_JS_MODULES because the build would break otherwise? If so, do you know why? mcs/brade should have a second look here.

d945c68acc68a834f26a89973ab5f728fdbd3e38 - okay
d84a2fb95136a1728b621a9d4cbe979389db48a0 - not okay

please include the fixup for the manual page as specified in eb5d5dfaae93805baee9e84039e95fca74f9cce2

c08b58e1d7062b732e39caca8a37b2a52af47249 - not okay, not needed due to 1520177
65ec28479828c9bf80f73c0d3d1d5817177c646e - okay
939c662e69f3cae14e8f5e31b5a75eb6b20fb214 - okay (we should have this early on on our final branch so it is easier to bisect problems and still have a working Tor Browser experience; that probably includes moving the patch exempting our signed extensions to an early place in the branch as well)

[TODO waiting for 1330467 to reland]

f47cd2fb5288 Bug 21569: Add first-party domain to Permissions key ---[https://bugzilla.mozilla.org/show_bug.cgi?id=1330467]
3298251467df Bug 26670: Make canvas permission respect FPI ---[https://bugzilla.mozilla.org/show_bug.cgi?id=1330467]

We need to backport 1330467 and look closely at the fallout. Might need some fix-ups to take this into account.

[TODO not landed firefox patches from #28711 - update and do 'try' run?]

7afe16fd6d27 Bug 1474659 Part 2 - Add dedicated AllocKinds just for ArrayBufferObjects. r?s.. ---[GC code changed, would be good to update patch and do 'try' to check breakage]
2beafe9bd417 Bug 1474659 Part 1 - Add support to EnumSet for more than 32 values. r?sfink ---[Changes for this part seem not to be needed anymore]

Sounds like a good plan I think.

[DROP not needed]

Looks good.

[DROP uplifted]

41f620e4a0a3 Bug 13398: at startup, browser gleans user FULL NAME (real name, given name) f..
091b41ec2465 Bug 21787: Spoof en-US for date picker
34063061f825 Bug 26540: Enabling pdfjs disableRange option prevents pdfs from loading
c894688d4924 Bug 25909: disable updater telemetry

Looks good. 41f620e4a0a3 is actually not needed anymore as the respective code got removed in Firefox 68.

[DROP included in 68]

Looks good.

comment:12 Changed 5 months ago by gk

Keywords: tbb-9.0-must-nightly added

Starting with 9.0 keywords

comment:13 Changed 5 months ago by gk

mcs/brade: to not lose this in the previous wall of text: I thought it could be helpful if you'd had a second look at commits

45072c2fea6a535a712ac0888f12f881393cccbd
9d8ca4380e947c2097d0fd3554b1f3dad20de634
5269df12586ac7e4e45803a0f1b8c15da9ef529a

on acat's 30429 branch.

comment:14 in reply to:  13 Changed 5 months ago by mcs

Replying to gk:

mcs/brade: to not lose this in the previous wall of text: I thought it could be helpful if you'd had a second look at commits

45072c2fea6a535a712ac0888f12f881393cccbd

R.e. the char16_t question: a while ago, Mozilla changed FormatStringFromName() to take a
const char * so the new code is good.

R.e. gk's second comment: Kathy and I would prefer to always initialize profileStatus (seems safer).

R.e. gk's last comment about possibly adding an another call to CheckProfileWriteAccess(): we think it is OK to skip this case.

In the ProfileErrorDialog() function, please reinstate the rv check for the call to sb->FormatStringFromName() (I think it was lost during a previous rebase).

In nsToolkitProfileService::SelectStartupProfile() the first call to GetProfileByName() is wrong. We should not be passing a path to that function (it looks like the ESR60 patch is wrong too). Instead, once we obtain lf, we should call CheckProfileWriteAccess(lf). In other words, we can remove the entire chunk that contains the first call to GetProfileByName() as well as a call to CheckProfileWriteAccess(profile) and
instead make the second call earlier.

Also, Mozilla has made several changes to these files since Alex started the ESR68 TB rebasing work. Kathy and I would like to take another look at the patch after those changes have been accounted for.

9d8ca4380e947c2097d0fd3554b1f3dad20de634

It looks like event listeners are handled by via the LEGACY_ACTORS object. See the large comment near the beginning of toolkit/modules/ActorManagerParent.jsm. If that is correct, we should remove the removeMessageListener() and removeEventListener() calls from browser/actors/AboutTBUpdateChild.jsm.

R.e. isAboutTBUpdate(), that is replaced by the matches property within the LEGACY_ACTORS object.

R.e. BrowserContentHandler.jsm being moved to EXTRA_PP_COMPONENTS: that was part of the #4234 patch.

Otherwise, this looks good. We will need to test it once the other updater patches have been rebased.

5269df12586ac7e4e45803a0f1b8c15da9ef529a

This looks good to us, although testing is required. The reason the EXTRA_PP_JS_MODULES change appears in this patch is because a similar change is included in the #4234 patch (which was applied before this patch).

comment:15 Changed 5 months ago by mcs

Alex, we have a couple of questions about the rebasing process:

  1. Do you want to rebase your rebased patches onto the tip of Mozilla's esr68 branch (or a recent tag) before Kathy and I do our rebasing work? I think that makes sense unless it turns out to be very time consuming.
  1. Previous attempts to reorder the patches while rebasing have not gone well; it would be better if we preserved the order so for example the #4234 patch comes before the other updater-related patches such as the #16940 one. Is it OK if Kathy and I create a new branch with the rebased patches for the following tickets inserted in a similar place within the commit stream as for the ESR60 Tor Browser branches: #18900, #13252, #19121, #4234, #13379? The only downside that I see is that if you are working in parallel with us someone will need to cherry pick some patches from our branch along with all of your patches to create a consolidated branch (or do something else magical with git).

comment:16 in reply to:  15 Changed 5 months ago by acat

Replying to mcs:

Alex, we have a couple of questions about the rebasing process:

  1. Do you want to rebase your rebased patches onto the tip of Mozilla's esr68 branch (or a recent tag) before Kathy and I do our rebasing work? I think that makes sense unless it turns out to be very time consuming.

I will do that, after I address the requested changes here. Since I assume this is blocking for you, I will try to do it soon.

  1. Previous attempts to reorder the patches while rebasing have not gone well; it would be better if we preserved the order so for example the #4234 patch comes before the other updater-related patches such as the #16940 one. Is it OK if Kathy and I create a new branch with the rebased patches for the following tickets inserted in a similar place within the commit stream as for the ESR60 Tor Browser branches: #18900, #13252, #19121, #4234, #13379? The only downside that I see is that if you are working in parallel with us someone will need to cherry pick some patches from our branch along with all of your patches to create a consolidated branch (or do something else magical with git).

I think this is fine, I don't see any issue with cherry-picking the updater patches to a more up to date consolidated branch later, if needed.

comment:17 Changed 5 months ago by acat

Thanks for the reviews, GeKo, mcs/brade.

I took branch https://github.com/acatarineu/tor-browser/commits/30429+2 (which is the one resulting of #10760 changes) and addressed the review comments there, resulting on https://github.com/acatarineu/tor-browser/commits/30429+3. Then I rebased those commits on top of latest esr68 branch, that's https://github.com/acatarineu/tor-browser/commits/30429+4. And finally, reordered some commits to address some review remaining comments and also fixed the search engines patch (Omnibox: Add DDG, Startpage, Disconnect, Youtube, Twitter...), that's branch https://github.com/acatarineu/tor-browser/commits/30429+5. I left the search engine fix for the end, and then I realized that there were some conflicts with esr68 branch, so that's why I only fixed it in the last branch (30429+5).

While rebasing to esr68 branch (30429+3 -> 30429+4) there were several conflicts, although most of them were due to style changes. But I think it would be good to review them, for example:

  • Bug 21849: Don't allow SSL key logging (gyp_vars['enable_sslkeylogfile'] was added)

I also realized that in Bug 8312: Remove "This plugin is disabled" barrier. there is a doc.getAnonymousElementByAttribute but I do not see any defined doc variable. It's the same in 52, but I think in the previous version there was. So I added the doc definition, although I would have to test this to make sure if it works as intended.

A question before comments: would it also make sense to try to upstream Bug #5741, with a different commit name and behind the --proxy-bypass-protection flag?

Some comments:

please add --enable-proxy-bypass-protection to all mozconfigs

Do you mean .mozconfig-mac, .mozconfig-asan, .mozconfig and .mozconfig-mingw? If so, isn't it already there?

23b8e34d8fa64affdb265911ff586d8babf1a119 - should we not point to the latest Torbutton commit (especially as we have removed all the other Torbutton commit updates (rightly so))? otherwise okay

I think so, but I kept the submodule change in a separate commit since it's currently it's in my personal branch. Would it be ok to make the last WIP torbutton submodule change commit a fixup when we have an "official" torbutton "esr68" branch?

a8c48df07cc505bd45c764d223f6ff01de738f31 -- indentation (keep original):

+ nsresult rv = GetOverrideStringBundleForLocale(
+ aSBS, uriString.get(), userAgentLocale.get(), aResult);

mach clang-format is changing this one, should we keep the original indentation anyway?

There is no docshell/test/mochitest.ini in esr68 and we should not add one.

Thanks. I also realized I removed I accidentally removed a test [test_bug1507702.html], fixed that too.

Why GetComputedReferrer() and not GetOriginalReferrer()? Could you add an explaining comment here?

I think GetComputedReferrer() is the one that preserves the current patch behaviour (e.g. takes into consideration referrer policy). Actually, when you mentioned I was not sure whether this was something intended. For example, with a noreferrer policy navigating through pages of the same origin will always clear window.name. But I saw #3414, so this actually seems to be the intended behaviour. I added a comment in the code.

You are not including components.conf in libmdns moz.build instead of patching it as you did in the provider case; we should have the same approach inboth cases (I am fine with just the moz.build one if we think that's enough)

Ok, I reincluded components.conf in moz.build, and made the corresponding components.conf empty. Not sure if you meant this, or that we actually don't need to touch libmdns components.conf.

What is the reason to patch GetUser$Directory now while we did not have done so in the esr60 patch? (I guess we should follow Mozilla here and if not, please add some comment/explanation for that deviation)

Not sure if you mean changing the static calls by the non-static ones. If so, this is needed because the patch needs to make several GetUser$Directory non-static, and apparently in esr60 the static functions were called via object (I would expect C++ to at least warn if you call a static method via an object?).

9d8ca4380e947c2097d0fd3554b1f3dad20de634 - not okay
I guess compilation breaks if we keep BrowserContentHandler.jsm where it is? (mcs/brade should have a second look here)

Yes, I think the _PP_ is needed when it needs to go through the preprocessor (same as 5269df12586ac7e4e45803a0f1b8c15da9ef529a, we added a macro to check for a build pref).

It looks like event listeners are handled by via the LEGACY_ACTORS object. See the large comment near the beginning of toolkit/modules/ActorManagerParent.jsm. If that is correct, we should remove the removeMessageListener() and ?>removeEventListener() calls from browser/actors/AboutTBUpdateChild.jsm.

True, did it. But I realized that now onPageHide does nothing, which seems wrong. Can we also remove it? I'm not sure if the new patch changes some behaviour here, since I guess before onpagehide would stop updating the document?

Where are all the search engines coming from in that patch which we did not have in esr60? I think we don't need those.
To keep the patch small we should just patch the search engines entries of the locales we ship. This holds for mobile as well and we probably should address #30017 (and maybe #30606).
If you look at ​https://hg.mozilla.org/integration/autoland/rev/111b88dd28d6 you see that the search plugins are converted to WebExtensions...

If you mean the *.xml, I added those to reuse the old ones without converting them to WebExtensions. But perhaps you also mean that the list.json changes are a bit dirty, that we're changing too many engines.
In any case, I also think moving to webextensions is the right thing to do, so I used the script in https://github.com/daleharvey/ffx-opensearch-to-webext to convert them (it's the script that the mozilla dev used). I had to modify the yahoo one, since it contained a system param which seems not to be supported:

		{
		  "name": "hsimp",
		  "condition": "purpose",
		  "purpose": "system",
		  "value": "yhs-007"
		}

I also included 'Google' in the searchEngineOrder field, for some reason it was not respecting the order we currently have in esr60. We could perhaps also include other search engines there, and also not sure if the order prefs are needed anymore.

This is working fine the first time I open the browser, but unfortunately for some reason after restarting it the search engine icons disappear (although they still work). I'll try to find out why, but I think this should not be a blocker for a nightly. It might also be possible that it's some issue with my local build, so we can see if it reproduces.

I also removed the changes for mobile list.json, as I think they were only addressing #29798, which was fixed in 65. The mobile search engines are currently set in Bug 25741 - TBA: top sites changed, used bookmarks icon temporarily.. Should we change that and do all browser search engines in this patch, or is it fine to do it later for Android separately?

As I said before, These changes were done only on 30429+5 branch.

ba383936028e955cef2ced0f98d2f90ca39564de - not okay

We should fold that into 9510c9416ddd35a016ee2074dd58f927d97246c7; additionally it's worth solving #27493 while we are at it.

Done, removed mk_add_options MOZILLA_OFFICIAL=1 and also mk_add_options BUILD_OFFICIAL=1, since it's not used anymore.

9f05eedab888b33f83e97bb3cd870ecb5174ea41 - not okay

.../fxmonitor/content/img/tor-watermark.svg | 6 +++

It seems you added the .svg to the wrong dir?
content/branding/horizontal-lockup.svg is missing in
browser/branding/alpha/content/jar.mn or maybe we should just remove it in the other series? Or replace it with an own icon for newInstallPage.html? Glancing at bug 1518632 I am tempted to just remove that .svg file.
We should remove the dead app.update.download.backgroundInterval.

I reviewed the patch again and changed it a bit. First, is there any problem with just removing the tor-watermark.svg? It's not used (I did that in the revision).
Then, I realized that this patch actually was not changing the */pref/firefox-branding.js, but just copying the release one over the alpha and nightly ones. But the release one is modified in one of the updater patches (#4234), which has not been applied yet. So I left the */pref/firefox/branding.js untouched in this patch, and I plan to modify it when #4234 is applied. Or we could apply #4234 after this one, and do all the */pref/firefox/branding.js changes there, not sure.
I also updated some locale files. For example, brand.ftl is now used extensively in the code, and has more strings. That means that I also needed to modify patch #2176 to replace brand.ftl -> tor-browser-brand.ftl, and to add the missing strings.

comment:18 Changed 5 months ago by acat

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201907 removed
Status: needs_revisionneeds_review

comment:19 Changed 4 months ago by acat

Not so important, but I was wondering whether it would be ok to do a last pass to fix style issues whenever we think we have a branch ready for nightly. I can do this, and it cannot take more than 1 day.

I think almost all C++ style should be fine according to ./mach clang-format, but I was thinking to also make the JavaScript code we add or modify obey mozilla style (ESLint rules). Many of the style issues can be fixed automatically. The rules are quite advanced and tuned to Firefox codebase, and sometimes they help fixing bugs (deprecated things in XPCOM, etc.). I saw this while working on torbutton integration.

comment:20 in reply to:  19 Changed 4 months ago by gk

Replying to acat:

Not so important, but I was wondering whether it would be ok to do a last pass to fix style issues whenever we think we have a branch ready for nightly. I can do this, and it cannot take more than 1 day.

I think almost all C++ style should be fine according to ./mach clang-format, but I was thinking to also make the JavaScript code we add or modify obey mozilla style (ESLint rules). Many of the style issues can be fixed automatically. The rules are quite advanced and tuned to Firefox codebase, and sometimes they help fixing bugs (deprecated things in XPCOM, etc.). I saw this while working on torbutton integration.

Good idea, let's do it. I am fine not blocking on our nightly releases for this but commit a fixup commit which then gets resolved during a later rebase (just if we don't get to it in time).

comment:21 Changed 4 months ago by gk

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201907R removed

No July any longer.

comment:22 Changed 4 months ago by acat

I realized that the changes for the security slider translations deduplication got accidentally lost when moving from 30429+2 to 30429+3. But maybe we should actually keep it this way and do the full change in #24653 (in tor-browser and torbutton).

comment:23 in reply to:  22 Changed 4 months ago by gk

Replying to acat:

I realized that the changes for the security slider translations deduplication got accidentally lost when moving from 30429+2 to 30429+3. But maybe we should actually keep it this way and do the full change in #24653 (in tor-browser and torbutton).

Yes, that's a good idea.

comment:24 in reply to:  17 ; Changed 4 months ago by gk

Keywords: TorBrowserTeam201908 added; TorBrowserTeam201908R removed
Status: needs_reviewneeds_revision

Replying to acat:

[snip]

I also realized that in Bug 8312: Remove "This plugin is disabled" barrier. there is a doc.getAnonymousElementByAttribute but I do not see any defined doc variable. It's the same in 52, but I think in the previous version there was. So I added the doc definition, although I would have to test this to make sure if it works as intended.

Nice catch (and sounds reasonable to me)! Don't spend too much time testing the Flash use case, though. :)

A question before comments: would it also make sense to try to upstream Bug #5741, with a different commit name and behind the --proxy-bypass-protection flag?

I think it would make sense to make sure configurations with a SOCKS proxy set and remote resolution of DNS to not be able to bypass that if the flag is set. So, I am all for it to get that upstreamed.

Some comments:

please add --enable-proxy-bypass-protection to all mozconfigs

Do you mean .mozconfig-mac, .mozconfig-asan, .mozconfig and .mozconfig-mingw? If so, isn't it already there?

Hrm. I *think* I might have mixed this up with the state of tor-browser-build, sorry.

23b8e34d8fa64affdb265911ff586d8babf1a119 - should we not point to the latest Torbutton commit (especially as we have removed all the other Torbutton commit updates (rightly so))? otherwise okay

I think so, but I kept the submodule change in a separate commit since it's currently it's in my personal branch. Would it be ok to make the last WIP torbutton submodule change commit a fixup when we have an "official" torbutton "esr68" branch?

Yes, that's cool.

a8c48df07cc505bd45c764d223f6ff01de738f31 -- indentation (keep original):

+ nsresult rv = GetOverrideStringBundleForLocale(
+ aSBS, uriString.get(), userAgentLocale.get(), aResult);

mach clang-format is changing this one, should we keep the original indentation anyway?

Let's go with what Mozilla's tool wants to have here (and in other cases).

There is no docshell/test/mochitest.ini in esr68 and we should not add one.

Thanks. I also realized I removed I accidentally removed a test [test_bug1507702.html], fixed that too.

Why GetComputedReferrer() and not GetOriginalReferrer()? Could you add an explaining comment here?

I think GetComputedReferrer() is the one that preserves the current patch behaviour (e.g. takes into consideration referrer policy). Actually, when you mentioned I was not sure whether this was something intended. For example, with a noreferrer policy navigating through pages of the same origin will always clear window.name. But I saw #3414, so this actually seems to be the intended behaviour. I added a comment in the code.

Thanks, sounds good.

You are not including components.conf in libmdns moz.build instead of patching it as you did in the provider case; we should have the same approach inboth cases (I am fine with just the moz.build one if we think that's enough)

Ok, I reincluded components.conf in moz.build, and made the corresponding components.conf empty. Not sure if you meant this, or that we actually don't need to touch libmdns components.conf.

What I meant was to approach the patching in both cases the same way: if we patch in dom/presentation both moz.build and the conf file then it makes it easier to understand the whole patch set if we do that in the libmdns case, too. So, I think the result is fine.

What is the reason to patch GetUser$Directory now while we did not have done so in the esr60 patch? (I guess we should follow Mozilla here and if not, please add some comment/explanation for that deviation)

Not sure if you mean changing the static calls by the non-static ones. If so, this is needed because the patch needs to make several GetUser$Directory non-static, and apparently in esr60 the static functions were called via object (I would expect C++ to at least warn if you call a static method via an object?).

Yes, that's what I mean. We should comment the reason for that change at least, like explaining why they suddenly need to be non-static, in particular given that Mozilla does not make that change but we have to now.

I guess compilation breaks if we keep BrowserContentHandler.jsm where it is? (mcs/brade should have a second look here)

Yes, I think the _PP_ is needed when it needs to go through the preprocessor (same as 5269df12586ac7e4e45803a0f1b8c15da9ef529a, we added a macro to check for a build pref).

It looks like event listeners are handled by via the LEGACY_ACTORS object. See the large comment near the beginning of toolkit/modules/ActorManagerParent.jsm. If that is correct, we should remove the removeMessageListener() and ?>removeEventListener() calls from browser/actors/AboutTBUpdateChild.jsm.

True, did it. But I realized that now onPageHide does nothing, which seems wrong. Can we also remove it? I'm not sure if the new patch changes some behaviour here, since I guess before onpagehide would stop updating the document?

That's probably a question for mcs and brade and/or trying things out. I don't know the answer with a bunch of further investigations.

Where are all the search engines coming from in that patch which we did not have in esr60? I think we don't need those.
To keep the patch small we should just patch the search engines entries of the locales we ship. This holds for mobile as well and we probably should address #30017 (and maybe #30606).
If you look at ​https://hg.mozilla.org/integration/autoland/rev/111b88dd28d6 you see that the search plugins are converted to WebExtensions...

If you mean the *.xml, I added those to reuse the old ones without converting them to WebExtensions. But perhaps you also mean that the list.json changes are a bit dirty, that we're changing too many engines.

I think I meant both. :)

In any case, I also think moving to webextensions is the right thing to do, so I used the script in https://github.com/daleharvey/ffx-opensearch-to-webext to convert them (it's the script that the mozilla dev used). I had to modify the yahoo one, since it contained a system param which seems not to be supported:

		{
		  "name": "hsimp",
		  "condition": "purpose",
		  "purpose": "system",
		  "value": "yhs-007"
		}

I also included 'Google' in the searchEngineOrder field, for some reason it was not respecting the order we currently have in esr60. We could perhaps also include other search engines there, and also not sure if the order prefs are needed anymore.

Let's keep this in mind while testing. I think the less changes we have here the better.

This is working fine the first time I open the browser, but unfortunately for some reason after restarting it the search engine icons disappear (although they still work). I'll try to find out why, but I think this should not be a blocker for a nightly. It might also be possible that it's some issue with my local build, so we can see if it reproduces.

How did that go? Any further insight here?

I also removed the changes for mobile list.json, as I think they were only addressing #29798, which was fixed in 65. The mobile search engines are currently set in Bug 25741 - TBA: top sites changed, used bookmarks icon temporarily.. Should we change that and do all browser search engines in this patch, or is it fine to do it later for Android separately?

It's fine having that done later in Android separately (and squash the mobile changes into this commit)

9f05eedab888b33f83e97bb3cd870ecb5174ea41 - not okay

.../fxmonitor/content/img/tor-watermark.svg | 6 +++

It seems you added the .svg to the wrong dir?
content/branding/horizontal-lockup.svg is missing in
browser/branding/alpha/content/jar.mn or maybe we should just remove it in the other series? Or replace it with an own icon for newInstallPage.html? Glancing at bug 1518632 I am tempted to just remove that .svg file.
We should remove the dead app.update.download.backgroundInterval.

I reviewed the patch again and changed it a bit. First, is there any problem with just removing the tor-watermark.svg? It's not used (I did that in the revision).

I think that's okay if we don't use it in the new onboarding code (the tor-watermark.svg was used in the onboarding). We should ask pospeselr here, though, as the file got added in the work for #25702.

Then, I realized that this patch actually was not changing the */pref/firefox-branding.js, but just copying the release one over the alpha and nightly ones. But the release one is modified in one of the updater patches (#4234), which has not been applied yet. So I left the */pref/firefox/branding.js untouched in this patch, and I plan to modify it when #4234 is applied. Or we could apply #4234 after this one, and do all the */pref/firefox/branding.js changes there, not sure.

Sounds good to me. Let's modify it after #4234 got applied.

Here comes another round of review (containing notes and reminders for myself or anyone else as well). The onboarding code is still in need of getting reviewed but that should not block nightly builds:

23612cb8e70a72e02e3500d39bb7cf9a9c57e62f - okay (good that all the .mozconfig files had the proxy-bypass-protection flag set; I might have been confused with the mozconfig files in tor-browser-build not having this yet)
588d3620f6adc2916929bfbdc2576c35689d6da8 - okay
a33bcc34665727649f7419ba3dc6273f49a1aa36 - okay
0d9178a68150dbabf01f0af5e4fb6d9ea1e4b7da - okay (upstreaming, bug 967812 has code parts)
227c3dace6d6f3a1060f1799c1299d355462c17e - okay
3fe947023d1146e61dfe08f941cc269e2441cd62 - okay
d61046e445419d7c990571aace0616d1663d4a4d - not okay. It's confusing to put the Torbutton related part in this commit. Let's have it in the respective integration commit instead (the one for #10760 for example).

7efd3890289371a327154d94aa41d67c304f26a7 - okay (note: fixup commit later on with esr68 torbutton commit)
7a68d4970b89e3146a4725e494f8b84adf1a2204 - okay
493b664f0fd1867559e51d6015c784e7c21a3259 - okay
1a5bf67fbb4f4a8add8d7e5d23c47d72792eaa34 - okay (but see my review comments in #10760)
df622b6c8a50a6797711938f857075578fb2aeef - okay
8e7ecb0d9cb9be4bbfd8fbfca1623da9de579499 - okay (nice catch!)
ac911985315e30ab29645f3f7a615789cbc5819d - okay
b7c8b9e0b641cdfeef8ac2adc5188c3411110374 - okay
010d768be882f66df3512c4edaf5b3c88f0c0961 - okay
8c666ebb2d359013f0fe8ed9bb27e10858f1fb03 - okay
2a324a0e0db60fc42d714d73657833d3cdf438cc - okay (mcs/brade second look; are we good with the "second call earlier"-part?)
6801fb665212d0068009a9a3c718cea4278b7fd6 - okay
3761fccc0a9d0701ae2845cd3fb192ab36052886 - okay
3789f206348dfa8c991e33e90027860743969202 - okay (nice! We should figure out whether we got/get squashed some of the tbb-branding bugs that way and close them accordingly)
754a163fb5d8d4a12854b11d09f313114de47156 - okay
734a50f6388ca2c11d696e7e6279349d96ef0b38 - okay
0fe1fd8d2966d65991bb33dc797ef4c9541303f7 - okay
4ab0cdd9862b6e079fa94fb4c8f8996b9349b391 - okay
941d6c2037d9bbd6b2c1b933698734c79e90492e - okay
c7e995eedb2a7da9cc6343078776f090ed51246d - okay
7fcd49ef692c635ba6f889c618b7de9df915f8e8 - okay
2131788a493dc7849dd767e981d0b03de4cd92c3 - okay
68072bf454acf8c6e3cbff5bf7531bd4f10b98ea - okay
d03d360bd773c8d8a97ef1f69e565afcc19796e5 - not okay (comment is still missing)
69c8b59f9cdc792ee533187723286fdd75dbabd0 - okay
9442dda80f9beb90b90934da2bb1a614d39117a4 - not okay. The test is probably not working anymore as we don't have static pins for #29811; we should either drop it or fix #29811 and test that the test is still working then (or working then again)

f96b46e0884299eee0a0f4bcfe881adf68a2f823 - okay
c6138a75ec817e4d7c1daa3f35959cdf6ba75767 - okay
062794f7cb6819ec1a8a34f3117f8f6b03eaacf8 - not okay (note :ja-JP-mac -> ja; so mabye we need to adapt tor-browser-build here?)

We just need changes to the locales we support, e.g. not be but es-AR. You find the full list e.g. in tor-browser-build's rbm.conf.

While "reusing" the existing search engines, did you make sure they comply with what we shipped so far. For example the ddg.xml we ship in our esr60 branches makes sure the requests go out via POST and there is no further information transmitted to DDG, while the one we reuse in esr68 seems at least to have differentiation about how things got searched (context menu, searchbar etc.). I think we don't want to have those.

61dcfc0d23a007825a390069de0a401509bad095 - okay
2d4c6343db751a841ad9ddc34dd03b69232e52a8 - okay
9d3401165f54aa99c76700d6a353bd7e73636315 - okay
6328f1b6eaa62f388cae83b8f205ee864bafb054 - okay
910e190a2d9d2fec07835fb026a591752e991852 - okay
2e42869e3aa2ad86b2d36b4128ea264dbb65fafa - okay [upstreaming??]
eb9b740cb74c0ec57ee861ac6fd60e2cfe1c1198 - okay
87fee0c8d5b0e6c365c081d78a73dec11608c5c0 - okay
d90b21dcfcca471900315c97294796364c0f9dd3 - okay
2e42869e3aa2ad86b2d36b4128ea264dbb65fafa - okay [upstreaming??]
f1893bd83085b25f7ee05fa33e368bc063046a32 - okay [upstreaming??]
f8fd2da23c908edc67b1f88f81f12724b4f84855 - okay
32009d2f6d790b4335ec0193519e5f54f1c89672 - okay
9e3c1f576913ae6ad2d805e7130f39e074ac6714 - okay
1a7eae785ecb08bda909d30ead665bea55002990 - okay
7415057d32809e86fe9b33de0f8e83b426e27964 - okay
90a4f393c41313889cbc40c61007028f9814a5ad - not okay (we don't need tor-watermark.svg for the onboarding branding anymore? we should ask pospeselr here, see: #25702) Keep track of the missing .js pref changes
887abb83ba0b71590ddea91f55301b7ce603eb26 - okay
b8546d69f9d1035c33927d81d964de63bacf80fb - okay
a03ede0306d96a2c0ba6dd360188d293c48328c2 - okay
546c642194f0434532dbb83389337cbb87d4c86e - okay
7efe2c610e55c7a61cd34c3f7102fd408159601f - not okay

+         // http onion is secure

indentation

"pageInfo_EncryptionWithBitsAndProtocol" + "pageInfo_OnionEncryptionWithBitsAndProtocol" formtatting in security.js (see previous "hdr =" you have removed)

   if (isHttpScheme && IsPotentiallyTrustworthyOrigin(innerContentLocation)) {
     *aDecision = ACCEPT;
     return NS_OK;

You copied that block but it needs to get moved (the block before the comment about the CSP directive should go I think)

comment:25 in reply to:  24 Changed 4 months ago by mcs

Replying to gk:

True, did it. But I realized that now onPageHide does nothing, which seems wrong. Can we also remove it? I'm not sure if the new patch changes some behaviour here, since I guess before onpagehide would stop updating the document?

That's probably a question for mcs and brade and/or trying things out. I don't know the answer with a bunch of further investigations.

Kathy and I think that onPageHide() can be removed since its purpose was to remove the listeners that presumably are now removed for us via the LEGACY_ACTORS mechanism. We only need a "one time" update at page load time. But it would be good to test things by loading about:tbupdate and making sure the version and release date are filled in.

2a324a0e0db60fc42d714d73657833d3cdf438cc - okay (mcs/brade second look; are we good with the "second call earlier"-part?)

This looks good to us. We tested the "access denied" case on macOS and saw an error alert as expected. We also tried to test the "read only file" system case but we did not seen an alert. I made a note to test that scenario again when Kathy and I are in a better position to debug it (we are in the middle of rebasing updater patches at the moment). I think we should use this patch for now and fix it up later if necessary,

comment:26 Changed 4 months ago by acat

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

---

d61046e445419d7c990571aace0616d1663d4a4d - not okay. It's confusing to put the Torbutton related part in this commit. Let's have it in the respective integration commit instead (the one for #10760 for example).

Thanks, this must have slipped in one of the conflicts while rebasing. Note I also fixed a bug in each commit I noticed: loc.installer.uninstallAddon instead of loc.uninstallAddon.

---

What is the reason to patch GetUser$Directory now while we did not have done so in the esr60 patch? (I guess we should follow Mozilla here and if not, please add some comment/explanation for that deviation)

Not sure if you mean changing the static calls by the non-static ones. If so, this is needed because the patch needs to make several GetUser$Directory non-static, and apparently in esr60 the static functions were called via object (I would expect C++ to at least warn if you call a static method via an object?).

Yes, that's what I mean. We should comment the reason for that change at least, like explaining why they suddenly need to be non-static, in particular given that Mozilla does not make that change but we have to now.
...
d03d360bd773c8d8a97ef1f69e565afcc19796e5 - not okay (comment is still missing)

Ok, added a comment in the declaration of the function which causes all the static->non-static changes. Please let me know if you were thinking of something else.

---

9442dda80f9beb90b90934da2bb1a614d39117a4 - not okay. The test is probably not working anymore as we don't have static pins for #29811; we should either drop it or fix #29811 and test that the test is still working then (or working then again)

I dropped it, i can add a comment to #29811 to add it again when it is fixed.

---

062794f7cb6819ec1a8a34f3117f8f6b03eaacf8 - not okay (note :ja-JP-mac -> ja; so mabye we need to adapt tor-browser-build here?)
We just need changes to the locales we support, e.g. not be but es-AR. You find the full list e.g. in tor-browser-build's rbm.conf.

Sorry, I looked at the wrong source I guess, should be fixed.

---

This is working fine the first time I open the browser, but unfortunately for some reason after restarting it the search engine icons disappear (although they still work). I'll try to find out why, but I think this should not be a blocker for a nightly. It might also be possible that it's some issue with my local build, so we can see if it reproduces.

How did that go? Any further insight here?

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

---

While "reusing" the existing search engines, did you make sure they comply with what we shipped so far. For example the ddg.xml we ship in our esr60 branches makes sure the requests go out via POST and there is no further information transmitted to DDG, while the one we reuse in esr68 seems at least to have differentiation about how things got searched (context menu, searchbar etc.). I think we don't want to have those.

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

---

"pageInfo_EncryptionWithBitsAndProtocol" + "pageInfo_OnionEncryptionWithBitsAndProtocol" formtatting in security.js (see previous "hdr =" you have removed)

Done, the second hdr = formatting was fixed by eslint when trying to do the same as the first one, so I assume it's ok.

if (isHttpScheme && IsPotentiallyTrustworthyOrigin(innerContentLocation)) {
	 *aDecision = ACCEPT;
	 return NS_OK;

---

You copied that block but it needs to get moved (the block before the comment about the CSP directive should go I think)

Oops, nice catch. Fixed.

comment:27 Changed 4 months ago by cypherpunks

Nice work, acat. If you want another review, set needs_review. To get it form the tbb-team, set TorBrowserTeam201908R.

comment:28 Changed 4 months ago by acat

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201908 removed
Status: needs_revisionneeds_review

Thanks.

comment:29 Changed 4 months ago by mcs

Here is a rebased patch for #13252:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug30429-pc-02&id=ba40aaeb975ed6680c79dca8f30b8285ec3578d4

Even though Kathy and I are not finished with the updater patches yet, it would be good to include this patch in the nightly builds soon. It should be inserted into the sequence of patches after the #11641 patch (commit 7521e628b071c61b65bcb9fa5e99c820369183c8 on acat's 30429+6 branch). Some notes about the rebased patch:

  1. We removed the code that copied files from distribution/preferences to the user's profile (XPIProvider.jsm). This was not needed even for ESR60.
  1. We removed the old Mac profile migration code. This is no longer needed because (a) everyone should have have upgraded (and therefore migrated) years ago and (b) Tor Browser 8.0 was a watershed release, so someone who does update a pre-8.0 Tor Browser to 9.x will have their profile migrated by the 8.0 code.

acat and/or gk, please review.

comment:30 Changed 4 months ago by acat

The patch looks good to me, also tested in Linux.

comment:31 Changed 4 months ago by mcs

The updater patches are ready for review now. Please examine the following commits on our bug30429-pc-03 branch within the brade tor-browser.git repo (https://gitweb.torproject.org/user/brade/tor-browser.git/log/?h=bug30429-pc-03):

9f9017a63b156e11af23b55fde36223b74d859e4
    Bug 13379: Sign our MAR files.

9aa2a90005dde6a7ea2bf58b63241d27912a78eb
    Bug 4234: Use the Firefox Update Process for Tor Browser.

afb98b58e51c64f4a5a8f51ff68cbd4a801dc831
    Bug 19121: reinstate the update.xml hash check

bff9eb4e1fc5244b53533f4fdbbc43a0ffbd43fd
    Bug 19121: reinstate the update.xml hash check

88bc9543973ac089fcb9a3c5c9aae2295086e99b
    Bug 13252: Do not store data in the app bundle

Also, that branch includes a couple of fixup commits that will be needed:

89177dd51ae361d5360abae00387a1d75c15a1be
    fixup! TB4: Tor Browser's Firefox preference overrides.

55b4bcecc587bb03ce32933096b12fbfb5ceb146
    fixup! Bug 16940: After update, load local change notes.

We inserted them into the patch sequence in a logical place (similar to where they are in the ESR60-based Tor Browser).

We did some testing of all of these patches together on macOS and Linux, but not on Windows.

If you compare the ESR60 and ESR68 patches for #4234, you will notice that we omitted the AdjustPathForUpdater() code inside toolkit/xre/nsUpdateDriver.cpp. The #13379 contains the changes that are needed (look for AppendToLibPath()) and in ESR60 that #13379 patch just removes the AdjustPathForUpdater() code that is added by the #4234 patch (which seems silly since we don't plan to ever use the #4234 patch without the #13379 one).

We confirmed that the #29180 patch is no longer needed.

Finally, we noticed a few miscellaneous issues while testing in ESR68 and we will open new tickets for them. For example, our Linux alpha build wants to create a new profile directory named RANDOM.default-alpha which is not good for Tor Browser.

comment:32 in reply to:  26 Changed 4 months ago by gk

Status: needs_reviewneeds_revision

Replying to acat:

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

Looks mostly good now (I still need to look at the onboarding stuff, though):

[snip]

9442dda80f9beb90b90934da2bb1a614d39117a4 - not okay. The test is probably not working anymore as we don't have static pins for #29811; we should either drop it or fix #29811 and test that the test is still working then (or working then again)

I dropped it, i can add a comment to #29811 to add it again when it is fixed.

Sounds good, please do.

[snip]

Could you adapt the commit message for 3fe947023d1146e61dfe08f941cc269e2441cd62? You could just drop the ESR45 paragraph or change it in a way that reflects what we actually block with the patch.

comment:33 Changed 4 months ago by acat

Some comments on the updater patches:

9f9017a63b156e11af23b55fde36223b74d859e4 (Bug 13379: Sign our MAR files.) -

  • In the original esr60 patch, AppendToLibPath is called for Windows, Mac and Linux, while in the new patch it's only used for Mac. Is this not needed anymore for Linux and Windows? In #29818 it says that in Linux now -rpath=$ORIGIN is used, but I did not see anything about Windows.

9aa2a90005dde6a7ea2bf58b63241d27912a78eb (Bug 4234: Use the Firefox Update Process for Tor Browser.)

  • TOR_BROWSER_VERSION is now mandatory, if I understand it correctly. But I still see some cases that are checking whether TOR_BROWSER_VERSION_QUOTED is defined:
+#ifdef TOR_BROWSER_VERSION_QUOTED
+  nsAutoCString tbVersion(TOR_BROWSER_VERSION_QUOTED);
+  rv = parser.GetString("Compatibility", "LastTorBrowserVersion", buf);
+  if (NS_FAILED(rv) || !tbVersion.Equals(buf)) return false;
+#endif
+#ifdef TOR_BROWSER_VERSION_QUOTED
+  nsAutoCString tbVersion(TOR_BROWSER_VERSION_QUOTED);
+  static const char kTorBrowserVersionHeader[] =
+      NS_LINEBREAK "LastTorBrowserVersion=";
+  PR_Write(fd, kTorBrowserVersionHeader, sizeof(kTorBrowserVersionHeader) - 1);
+  PR_Write(fd, tbVersion.get(), tbVersion.Length());
+#endif
+

Can these checks be removed?

  • AdjustPathForUpdater in toolkit/xre/nsUpdateDriver.cpp is removed, similar to comment above, is this not needed anymore for Windows?

afb98b58e51c64f4a5a8f51ff68cbd4a801dc831 (Bug 19121: reinstate the update.xml hash check) - ok

bff9eb4e1fc5244b53533f4fdbbc43a0ffbd43fd (Bug 19121: reinstate the update.xml hash check) - ok

88bc9543973ac089fcb9a3c5c9aae2295086e99b (Bug 13252: Do not store data in the app bundle) - ok, already reviewed

89177dd51ae361d5360abae00387a1d75c15a1be (fixup! TB4: Tor Browser's Firefox preference overrides.)

  • Since TOR_BROWSER_VERSION is now mandatory, can the ifdef check be removed?

55b4bcecc587bb03ce32933096b12fbfb5ceb146 (fixup! Bug 16940: After update, load local change notes.)

  • Since TOR_BROWSER_VERSION is now mandatory, can the ifdef check be removed?


comment:34 in reply to:  30 Changed 4 months ago by gk

Replying to acat:

The patch looks good to me, also tested in Linux.

Looks good to me, too. It's commit 752fcc36bde612c146adae002b8ef9e1e1a119ac on tor-browser-68.0esr-9.0-2 and should be available with tomorrow's nightly builds.

Last edited 4 months ago by gk (previous) (diff)

comment:35 Changed 4 months ago by acat

Could you adapt the commit message for 3fe947023d1146e61dfe08f941cc269e2441cd62? You could just drop the ESR45 paragraph or change it in a way that reflects what we actually block with the patch.

One question regarding that commit, will the meek-http-helper@bamsoftware.com exemption still be needed? I think that is a bootstrapped extension, which will not work in ESR68, and from what I see in #29430 the plan is to replace it with something else. Should we also remove this exemption here?

comment:36 in reply to:  35 Changed 4 months ago by gk

Replying to acat:

Could you adapt the commit message for 3fe947023d1146e61dfe08f941cc269e2441cd62? You could just drop the ESR45 paragraph or change it in a way that reflects what we actually block with the patch.

One question regarding that commit, will the meek-http-helper@bamsoftware.com exemption still be needed? I think that is a bootstrapped extension, which will not work in ESR68, and from what I see in #29430 the plan is to replace it with something else. Should we also remove this exemption here?

Up to you. :) I am fine seeing the necessary bits in a fixup commit done in #29430 or just taking action right now in this bug while rebasing everything.

comment:37 Changed 4 months ago by acat

Ok, I ended up removing it and changing the commit message here: https://github.com/acatarineu/tor-browser/commits/30429+7

comment:38 Changed 4 months ago by gk

Keywords: ff68-esr added

comment:39 in reply to:  37 Changed 4 months ago by gk

Replying to acat:

Ok, I ended up removing it and changing the commit message here: https://github.com/acatarineu/tor-browser/commits/30429+7

Looks good to me. Could you go over the latest commits that landed on tor-browser-60.8.0esr-9.0-1 while we did our back and forth with the rebase review and pick those necessary for esr68 up? (To make sure we don't have anything that meanwhile landed forgotten)

comment:40 in reply to:  33 ; Changed 4 months ago by mcs

Replying to acat:

Some comments on the updater patches:

9f9017a63b156e11af23b55fde36223b74d859e4 (Bug 13379: Sign our MAR files.) -

  • In the original esr60 patch, AppendToLibPath is called for Windows, Mac and Linux, while in the new patch it's only used for Mac. Is this not needed anymore for Linux and Windows? In #29818 it says that in Linux now -rpath=$ORIGIN is used, but I did not see anything about Windows.

We do not think it will be needed on Windows because Mozilla no longer copies updater.exe to a new location and the DLLs it depends on are in the same directory as that exe. But we will find out for sure when we are able to test the ESR68-based updater on Windows (we are waiting for Windows nightlies to be available before we do that testing).

9aa2a90005dde6a7ea2bf58b63241d27912a78eb (Bug 4234: Use the Firefox Update Process for Tor Browser.)

  • TOR_BROWSER_VERSION is now mandatory, if I understand it correctly. But I still see some cases that are checking whether TOR_BROWSER_VERSION_QUOTED is defined:

...

We will remove the #ifdefs.

+#ifdef TOR_BROWSER_VERSION_QUOTED
+  nsAutoCString tbVersion(TOR_BROWSER_VERSION_QUOTED);
+  rv = parser.GetString("Compatibility", "LastTorBrowserVersion", buf);
+  if (NS_FAILED(rv) || !tbVersion.Equals(buf)) return false;
+#endif
+#ifdef TOR_BROWSER_VERSION_QUOTED
+  nsAutoCString tbVersion(TOR_BROWSER_VERSION_QUOTED);
+  static const char kTorBrowserVersionHeader[] =
+      NS_LINEBREAK "LastTorBrowserVersion=";
+  PR_Write(fd, kTorBrowserVersionHeader, sizeof(kTorBrowserVersionHeader) - 1);
+  PR_Write(fd, tbVersion.get(), tbVersion.Length());
+#endif
+

Can these checks be removed?

  • AdjustPathForUpdater in toolkit/xre/nsUpdateDriver.cpp is removed, similar to comment above, is this not needed anymore for Windows?

It should not be needed. Also see what I said about AdjustPathForUpdater() in comment:31.

...
89177dd51ae361d5360abae00387a1d75c15a1be (fixup! TB4: Tor Browser's Firefox preference overrides.)

  • Since TOR_BROWSER_VERSION is now mandatory, can the ifdef check be removed?

55b4bcecc587bb03ce32933096b12fbfb5ceb146 (fixup! Bug 16940: After update, load local change notes.)

  • Since TOR_BROWSER_VERSION is now mandatory, can the ifdef check be removed?

Yes and yes.

We will revise the patches.

comment:41 in reply to:  40 Changed 4 months ago by mcs

Replying to mcs:

We will revise the patches.

The revised commits are on the bug30429-pc-04 branch within brade's tor-browser repo (https://gitweb.torproject.org/user/brade/tor-browser.git/log/?h=bug30429-pc-04). Here are the commit hashes:

441c06803de45ef4361b73d8a89dc157ac36b931 // #13252
ce12bc2c589e3afee005d21390a5a8fdc5dba839 // #19121 part 1
1ce0013447ad026bd9a63423e6a4c8fa3a8e55a1 // #19121 part 2
2333721bb8db38a04edc40ed9896723f57c283b6 // #4234
0e5f7c647affc5951d125193f05ba1330b4e8167 // #13379

And the two fixups:

68f67b9d87c9b7a0952b4b777532748370c0442d // fixup for #16940
2e6c050c1c44bd36ec7c8747dd66ef8d4cf146b3 // fixup for "TB4: Tor Browser's Firefox preference overrides."

comment:42 Changed 4 months ago by acat

These look good to me.

comment:43 Changed 4 months ago by pili

Sponsor: Sponsor44-can

Tagging with Sponsor 44

comment:44 Changed 4 months ago by acat

New branch: https://github.com/acatarineu/tor-browser/commits/30429+8

It has latest missing desktop patches from tor-browser-60.8.0esr-9.0-1, the latest updater patches, backported #27601 (https://bugzilla.mozilla.org/show_bug.cgi?id=1330467) and #28711 (https://bugzilla.mozilla.org/show_bug.cgi?id=1474659) and I also added the missing changes to the patch for #25702, which was to copy the release pref/firefox-branding.js over the nightly and alpha ones, now that the updater patches are there.

comment:45 Changed 4 months ago by acat

Status: needs_revisionneeds_review

comment:46 in reply to:  42 Changed 4 months ago by gk

Replying to acat:

These look good to me.

Alright, I pushed tor-browser-68.0esr-9.0-3 and enabled .mar file generation on tor-browser-build's master again (536715a6c5d134bc25ebb07147c81c422c11c3b3) after pointing to the new browser branch (commit c54f90e102124dcd77d6efb9e186a4e3215972a8). We'll see how it goes in tomorrow's nightly build.

comment:47 in reply to:  44 ; Changed 4 months ago by gk

Keywords: TorBrowserTeam201908 added; TorBrowserTeam201908R removed
Status: needs_reviewneeds_revision

Replying to acat:

New branch: https://github.com/acatarineu/tor-browser/commits/30429+8

It has latest missing desktop patches from tor-browser-60.8.0esr-9.0-1, the latest updater patches, backported #27601 (https://bugzilla.mozilla.org/show_bug.cgi?id=1330467) and #28711 (https://bugzilla.mozilla.org/show_bug.cgi?id=1474659) and I also added the missing changes to the patch for #25702, which was to copy the release pref/firefox-branding.js over the nightly and alpha ones, now that the updater patches are there.

I'll comment separately on the onboarding patches in #28822. Here come the relevant comment for commits besides them:

0c19ff1358931bfa1be03dca261ec4d84a9826ee -- okay

The backported patches for bug 1330467 look mostly good. Part 7 as tiny mistake. You end up with:

-  // make sure principals with userContextId or firstPartyDomain use the same permissions
+  // make sure principals with a firstPartyDomain use different permissions
   Assert.equal(
     Ci.nsIPermissionManager.ALLOW_ACTION,
     pm.testPermissionFromPrincipal(principal6, TEST_PERMISSION)
   );
   Assert.equal(
-    Ci.nsIPermissionManager.ALLOW_ACTION,
+    Ci.nsIPermissionManager.UNKNOWN_ACTION,
     pm.testPermissionFromPrincipal(principal7, TEST_PERMISSION)
   );

but the original is

-  // make sure principals with userContextId or firstPartyDomain use the same permissions
+  // make sure principals with userContextId use the same permissions
   Assert.equal(Ci.nsIPermissionManager.ALLOW_ACTION,
                pm.testPermissionFromPrincipal(principal6, TEST_PERMISSION));
-  Assert.equal(Ci.nsIPermissionManager.ALLOW_ACTION,
+  // make sure principals with a firstPartyDomain use different permissions
+  Assert.equal(Ci.nsIPermissionManager.UNKNOWN_ACTION,
                pm.testPermissionFromPrincipal(principal7, TEST_PERMISSION));

cbe8a7e484d4217be86edb0e5cf81709bfe9df68 - okay (mabye squash with #25658?)
1d4f025ccbefad61498044a1e7e1040e935fc736 - okay (mabye squash with #25658?)
fb50127d4f55e8ec102be1a3f7316f96ae2fb578 - okay
a96dc99a94a687cf4ed6f39ecb4bd09f60e63734 - okay
849d7121914c595e78e35f068b099c46ba1c6e9a - okay

I am still debating taking the permission API patches out of the final alpha branch. But for now (one or two nightlies) I let them in. So, fixing up the tiny mistake above should be a fixup then as I am about to push a 68.1.0esr branch for further testing.

comment:48 Changed 4 months ago by gk

Alright, I just pushed tor-browser-68.1.0esr-9.0-1 to our tor-browser repo. Please use that one for basing your branches and fixup commits on.

comment:49 Changed 3 months ago by mcs

Here is a fixup for the #4234 patch which contains fixes for some Windows updater issues that Kathy and I found:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug30429-fixup-01&id=f607d5bb45e536dc7094999252dc89c6bc84884b

Mozilla moved the app.update.auto setting to a new JSON file that is designed to be shared across all Firefox profiles. Firefox also stores some update-related data (on Windows only) in a system location and we do not want either of these things.

Please let me know if we need to create a new ticket for these; for now I am using this ticket because it is still open.

comment:50 in reply to:  49 Changed 3 months ago by gk

Replying to mcs:

Here is a fixup for the #4234 patch which contains fixes for some Windows updater issues that Kathy and I found:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug30429-fixup-01&id=f607d5bb45e536dc7094999252dc89c6bc84884b

Mozilla moved the app.update.auto setting to a new JSON file that is designed to be shared across all Firefox profiles. Firefox also stores some update-related data (on Windows only) in a system location and we do not want either of these things.

Please let me know if we need to create a new ticket for these; for now I am using this ticket because it is still open.

Here is fine, thanks. The patch looks good to me. Merged to tor-browser-68.1.0esr-9.0-1 (commit f607d5bb45e536dc7094999252dc89c6bc84884b).

Last edited 3 months ago by gk (previous) (diff)

comment:51 Changed 3 months ago by cypherpunks

Mozilla moved the app.update.auto setting to a new JSON file that is designed to be shared across all Firefox profiles. Firefox also stores some update-related data (on Windows only) in a system location and we do not want either of these things.

No. We want either of these things. Because it's "the right thing" (part of sandboxing). But CommonAppData should point to e.g. C:\Tor Browser\Browser\TorBrowser\Data\Browser\Mozilla instead of C:\ProgramData\Mozilla.

Please let me know if we need to create a new ticket for these

Yes, please.

comment:52 in reply to:  51 Changed 3 months ago by mcs

Replying to cypherpunks:

No. We want either of these things. Because it's "the right thing" (part of sandboxing). But CommonAppData should point to e.g. C:\Tor Browser\Browser\TorBrowser\Data\Browser\Mozilla instead of C:\ProgramData\Mozilla.

Maybe, but we do not have time to patch or test such a scheme right now. I think we are better off without the complexity that this adds for developers and users of Tor Browser.

comment:53 Changed 3 months ago by gk

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

Move must-nightly items to must-alpha ones.

comment:54 Changed 3 months ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201908 removed

Moving must-alpha tickets to September.

comment:55 Changed 3 months ago by pili

Points: 1

comment:56 in reply to:  47 ; Changed 3 months ago by acat

Replying to gk:

Replying to acat:

New branch: https://github.com/acatarineu/tor-browser/commits/30429+8

It has latest missing desktop patches from tor-browser-60.8.0esr-9.0-1, the latest updater patches, backported #27601 (https://bugzilla.mozilla.org/show_bug.cgi?id=1330467) and #28711 (https://bugzilla.mozilla.org/show_bug.cgi?id=1474659) and I also added the missing changes to the patch for #25702, which was to copy the release pref/firefox-branding.js over the nightly and alpha ones, now that the updater patches are there.

I'll comment separately on the onboarding patches in #28822. Here come the relevant comment for commits besides them:

0c19ff1358931bfa1be03dca261ec4d84a9826ee -- okay

The backported patches for bug 1330467 look mostly good. Part 7 as tiny mistake. You end up with:

-  // make sure principals with userContextId or firstPartyDomain use the same permissions
+  // make sure principals with a firstPartyDomain use different permissions
   Assert.equal(
     Ci.nsIPermissionManager.ALLOW_ACTION,
     pm.testPermissionFromPrincipal(principal6, TEST_PERMISSION)
   );
   Assert.equal(
-    Ci.nsIPermissionManager.ALLOW_ACTION,
+    Ci.nsIPermissionManager.UNKNOWN_ACTION,
     pm.testPermissionFromPrincipal(principal7, TEST_PERMISSION)
   );

but the original is

-  // make sure principals with userContextId or firstPartyDomain use the same permissions
+  // make sure principals with userContextId use the same permissions
   Assert.equal(Ci.nsIPermissionManager.ALLOW_ACTION,
                pm.testPermissionFromPrincipal(principal6, TEST_PERMISSION));
-  Assert.equal(Ci.nsIPermissionManager.ALLOW_ACTION,
+  // make sure principals with a firstPartyDomain use different permissions
+  Assert.equal(Ci.nsIPermissionManager.UNKNOWN_ACTION,
                pm.testPermissionFromPrincipal(principal7, TEST_PERMISSION));

cbe8a7e484d4217be86edb0e5cf81709bfe9df68 - okay (mabye squash with #25658?)
1d4f025ccbefad61498044a1e7e1040e935fc736 - okay (mabye squash with #25658?)
fb50127d4f55e8ec102be1a3f7316f96ae2fb578 - okay
a96dc99a94a687cf4ed6f39ecb4bd09f60e63734 - okay
849d7121914c595e78e35f068b099c46ba1c6e9a - okay

I am still debating taking the permission API patches out of the final alpha branch. But for now (one or two nightlies) I let them in. So, fixing up the tiny mistake above should be a fixup then as I am about to push a 68.1.0esr branch for further testing.

A fixup for the Part 7 is in https://github.com/acatarineu/tor-browser/commit/30429+9

comment:57 in reply to:  56 Changed 3 months ago by gk

Replying to acat:

[snip]

A fixup for the Part 7 is in https://github.com/acatarineu/tor-browser/commit/30429+9

Thanks. Looks good. I cherry-picked the fixup onto tor-browser-68.1.0esr-9.0-2 (commit a8c2fe1a361abb926e6dbbce91940a1c56b0b9bf).

comment:58 Changed 3 months ago by gk

FWIW I just filed #31598 as we seem to have missed at least the letterboxing pref in the rebase.

Last edited 3 months ago by gk (previous) (diff)

comment:59 Changed 3 months ago by gk

I took a look at commit 14df73ecfdd88db89f196cecfb934dab7e552969 (aka the rebased patch for #4234):

Do we really want to disable staged updates for macOS? I feel if we don't have good reasons like we do for Windows and Linux we should a) not degrade the user experience here and b) not deviate from the update code path Mozilla is using (assuming that the alternative path is less tested).

We need an updated logo for aboutdebugging-firefox-release.svg at least, right? Do we have a bug for that on file? (one can see the issue after flipping devtools.aboutdebugging.new-enabled and then opening about:debugging)

Why do we suddenly need the #ifdef XP_WIN blocks around closeHandle() and friends? Just to make it abundantly clear that this is only used for Windows as is the ctypes inclusion? I wonder if we could that somehow upstream or make it clear upstream in a way that we don't have to carry that part of the patch around every time.

comment:60 in reply to:  59 ; Changed 3 months ago by pili

We need an updated logo for aboutdebugging-firefox-release.svg at least, right? Do we have a bug for that on file? (one can see the issue after flipping devtools.aboutdebugging.new-enabled and then opening about:debugging)

Do we have something we can use for this already or do we need antonela to make something?

comment:61 in reply to:  59 Changed 3 months ago by mcs

Replying to gk:

I took a look at commit 14df73ecfdd88db89f196cecfb934dab7e552969 (aka the rebased patch for #4234):

Do we really want to disable staged updates for macOS? I feel if we don't have good reasons like we do for Windows and Linux we should a) not degrade the user experience here and b) not deviate from the update code path Mozilla is using (assuming that the alternative path is less tested).

As we discussed on IRC, Kathy and I will create a fixup patch that keeps staged updates as the default on macOS.

We need an updated logo for aboutdebugging-firefox-release.svg at least, right? Do we have a bug for that on file? (one can see the issue after flipping devtools.aboutdebugging.new-enabled and then opening about:debugging)

As far as I know there is no open ticket for this... but my fuzzy memory is that it was mentioned previously somewhere.

Why do we suddenly need the #ifdef XP_WIN blocks around closeHandle() and friends? Just to make it abundantly clear that this is only used for Windows as is the ctypes inclusion?

Yes.

I wonder if we could that somehow upstream or make it clear upstream in a way that we don't have to carry that part of the patch around every time.

We could just remove the added #ifdef XP_WIN lines. They are not essential. On the other hand, including them in our patch makes it more likely that we will notice any additions that Mozilla makes to their use of ctypes (which seems like a good thing, but does mean potentially more work during the routine rebasing that happens for point releases).

comment:62 Changed 3 months ago by gk

Here comes feedback for commit 21066b41d8844805876fa71e1ea562c5dfaac439:

"This is used so that updater can find libmozsqlite3.so" s/updater/the updater/

and not .so but .dylib in that comment as this is for macOS only.

"can located" -> can locate

Any reason you omitted !strstr(pathValue, pathToAppend) in AppendToLibPath()? It might not be likely that this is an issue but having that check seems to be correct to me: there is no need to add the path if it's already there for whatever reason.

comment:63 in reply to:  60 Changed 3 months ago by gk

Replying to pili:

We need an updated logo for aboutdebugging-firefox-release.svg at least, right? Do we have a bug for that on file? (one can see the issue after flipping devtools.aboutdebugging.new-enabled and then opening about:debugging)

Do we have something we can use for this already or do we need antonela to make something?

I think we can use the firefox.svg we have in the respective branding folders, like browser/branding/official. The issue just needs a fixup patch.

comment:64 in reply to:  8 ; Changed 3 months ago by gk

Replying to acat:

[snip]

[DROP? might not be needed -> check]

+ 988d41acfaca Bug 26456: HTTP .onion sites inherit previous page's certificate information

That's not clear yet, probably we don't need it. However, the current state of our rebased .onion security expectations needs improvements. Right now if you load an http:// .onion (you could pick one from https://onion.torproject.org) the proper icon is shown in the URL bar. But: clicking on the info box shows that the connection is not secure which is a regression to the stable series. URICanBeConsideredSecure() (in security/manager/ssl/nsSecureBrowserUIImpl.cpp) seems to be suspicious here as it does not care about .onion or not.

comment:65 in reply to:  62 ; Changed 3 months ago by mcs

Replying to gk:

Here comes feedback for commit 21066b41d8844805876fa71e1ea562c5dfaac439:

"This is used so that updater can find libmozsqlite3.so" s/updater/the updater/

and not .so but .dylib in that comment as this is for macOS only.

Thanks. We will produce a fixup for this. I think we will mention libnss3.dylib (as we do elsewhere in the patch).

"can located" -> can locate

We will fix this too.

Any reason you omitted !strstr(pathValue, pathToAppend) in AppendToLibPath()? It might not be likely that this is an issue but having that check seems to be correct to me: there is no need to add the path if it's already there for whatever reason.

You have forgotten about #18900 where we had to back out that part of Mozilla's code. With the ESR68 patches, we had to resurrect the entire AppendToLibPath() function (since Mozilla removed it completely) so we just omitted the problematic strstr() call in our implementation. We will add a mention of #18900 to the #13379 commit message.

comment:66 in reply to:  65 ; Changed 3 months ago by gk

Replying to mcs:

Replying to gk:

Any reason you omitted !strstr(pathValue, pathToAppend) in AppendToLibPath()? It might not be likely that this is an issue but having that check seems to be correct to me: there is no need to add the path if it's already there for whatever reason.

You have forgotten about #18900 where we had to back out that part of Mozilla's code. With the ESR68 patches, we had to resurrect the entire AppendToLibPath() function (since Mozilla removed it completely) so we just omitted the problematic strstr() call in our implementation. We will add a mention of #18900 to the #13379 commit message.

Well, indeed I had forgotten about that. However, why does this issue apply to macOS which is the only platform that part of the patch is concerned about? I mean we set LD_LIBRARY_PATH on Linux with the start script, so I see why we ran into #18900 but we don't touch the respective env variable on macOS, no?

comment:67 in reply to:  66 ; Changed 3 months ago by mcs

Replying to gk:

Well, indeed I had forgotten about that. However, why does this issue apply to macOS which is the only platform that part of the patch is concerned about? I mean we set LD_LIBRARY_PATH on Linux with the start script, so I see why we ran into #18900 but we don't touch the respective env variable on macOS, no?

On macOS we do have the tor script, although it does not currently modify DYLD_LIBRARY_PATH in a way that could cause a problem like #18900. However, the strstr() code is wrong and it seems risky to include it given the potential for future problems. Also, none of our recent Tor Browser releases have included the strstr() code so Kathy and I are not comfortable adding it this close to the 9.0 release.

comment:68 Changed 3 months ago by mcs

Here is a patch that re-enables staged updates on macOS:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug30429-fixup-02&id=87b3cdb0862a4912b0c3318ad63954dbf42bc774

Here is a fix for the dev tools branding:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug30429-fixup-02&id=d41c386fa1fc365e4962d96f5d99df063ba72dba

Note that devtools/client/themes/images/aboutdebugging-firefox-logo.svg is a color image and it should be a black one that can be colorized via code. We filed #31803 to track that issue (but the color icon does not look terrible).

Finally, here is a fix for the #13379 issues you mentioned in comment:62:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug30429-fixup-02&id=74c3ea2d5152e3687cb9026c93cdd043e90beb76

All three patches are on the bug30429-fixup-02 branch within the brade tor-browser repo.

comment:69 Changed 3 months ago by mcs

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed
Status: needs_revisionneeds_review

comment:70 in reply to:  67 Changed 3 months ago by gk

Replying to mcs:

Replying to gk:

Well, indeed I had forgotten about that. However, why does this issue apply to macOS which is the only platform that part of the patch is concerned about? I mean we set LD_LIBRARY_PATH on Linux with the start script, so I see why we ran into #18900 but we don't touch the respective env variable on macOS, no?

On macOS we do have the tor script, although it does not currently modify DYLD_LIBRARY_PATH in a way that could cause a problem like #18900. However, the strstr() code is wrong and it seems risky to include it given the potential for future problems. Also, none of our recent Tor Browser releases have included the strstr() code so Kathy and I are not comfortable adding it this close to the 9.0 release.

That's perfectly fine. I am interested in getting the reasons for our omission right, though, to make it easier for future readers (both us and anybody else) to understand why we did what.

Last edited 3 months ago by gk (previous) (diff)

comment:71 in reply to:  68 Changed 3 months ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201909R removed
Status: needs_reviewneeds_revision

Replying to mcs:

[snip]

All three patches are on the bug30429-fixup-02 branch within the brade tor-browser repo.

Looks good. I guess I'll adapt the commit message regarding the included fix for #18900 slightly when rebasing to take comment:67 into account but that's nothing we need to change right now. tor-browser-68.1.0esr-9.0-2 has the three fixups (commit 87b3cdb0862a4912b0c3318ad63954dbf42bc774, d41c386fa1fc365e4962d96f5d99df063ba72dba, and 74c3ea2d5152e3687cb9026c93cdd043e90beb76).

Setting the state of this ticket back to needs_revision to address comment:64.

comment:72 in reply to:  64 Changed 3 months ago by acat

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed
Status: needs_revisionneeds_review

Replying to gk:

Replying to acat:

[snip]

[DROP? might not be needed -> check]

+ 988d41acfaca Bug 26456: HTTP .onion sites inherit previous page's certificate information

That's not clear yet, probably we don't need it. However, the current state of our rebased .onion security expectations needs improvements. Right now if you load an http:// .onion (you could pick one from https://onion.torproject.org) the proper icon is shown in the URL bar. But: clicking on the info box shows that the connection is not secure which is a regression to the stable series. URICanBeConsideredSecure() (in security/manager/ssl/nsSecureBrowserUIImpl.cpp) seems to be suspicious here as it does not care about .onion or not.

Fixup in https://github.com/acatarineu/tor-browser/commit/30429+10. Not sure what I saw while rebasing, but clearly changing URICanBeConsideredSecure was necessary but not sufficient :)

I also realized that the "mixed onion" icon was not being shown properly and marked as secure, so I also had to change nsDocShell::GetAllowMixedContentAndConnectionData.

comment:73 Changed 3 months ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201909R removed
Status: needs_reviewneeds_revision

So, it seems we have two new checks for "is this a .onion and should we treat this as trustworthy?". Could we use the one that actually is available in the tree instead (nsMixedContentBlocker::IsPotentiallyTrustworthyOnion())? That would at least make in the nsDocShell sense given the mixed content context your changes are in. But in the others as well I feel. We should adapt URICanBeConsideredSecure(), too (which had the .onion check already. Not sure what *I* saw while reviewing previously ;) ).

   // If the security state is STATE_IS_INSECURE, the TLS handshake never
   // completed. Don't set any further state.

Does not make much sense anymore with the code changes. Could you add a different comment explaining e.g. why we check for != INSECURE here after making sure there is actually a securityInfo available, which might not be obvious at first glance.

comment:74 Changed 3 months ago by acat

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed
Status: needs_revisionneeds_review

Fixed in https://github.com/acatarineu/tor-browser/commits/30429+11. I also changed the last check for the .onion case to if ((mState & STATE_IS_SECURE) == 0) {, because I think the previous if (mState != STATE_IS_INSECURE) { may have erased some flags in case of a https onion.

I changed the comment, but not completely sure if you meant that or something else :)

comment:75 in reply to:  74 ; Changed 3 months ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201909R removed
Status: needs_reviewneeds_revision

Replying to acat:

Fixed in https://github.com/acatarineu/tor-browser/commits/30429+11. I also changed the last check for the .onion case to if ((mState & STATE_IS_SECURE) == 0) {, because I think the previous if (mState != STATE_IS_INSECURE) { may have erased some flags in case of a https onion.

I changed the comment, but not completely sure if you meant that or something else :)

I meant something else but I was wrong. However, the changed comment *does* add value, so thanks. The patch looks good to me. I'd like to have another reviewer here (as this is a C++ patch), likely pospeselr. Menawhile, though, just a small nit to fix up:

+    // router over tor (.onion).

s/router/routed/

comment:76 Changed 3 months ago by cypherpunks

+      mState = STATE_IS_SECURE;

to erase all other flags?

comment:77 in reply to:  75 ; Changed 3 months ago by gk

Replying to gk:

Replying to acat:

Fixed in https://github.com/acatarineu/tor-browser/commits/30429+11. I also changed the last check for the .onion case to if ((mState & STATE_IS_SECURE) == 0) {, because I think the previous if (mState != STATE_IS_INSECURE) { may have erased some flags in case of a https onion.

I changed the comment, but not completely sure if you meant that or something else :)

I meant something else but I was wrong. However, the changed comment *does* add value, so thanks. The patch looks good to me. I'd like to have another reviewer here (as this is a C++ patch), likely pospeselr. Menawhile, though, just a small nit to fix up:

+    // router over tor (.onion).

s/router/routed/

Additionally, it seems that somehow your patch is breaking the mobile experience. With the latest nightly I can see an onion icon and the session is marked as secure. However, testing your patch with my 30429_test (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=30429_test) just gives the regular globe and an insecure connection setting.

comment:78 Changed 2 months ago by pili

Keywords: TorBrowserTeam201910 added

comment:79 Changed 2 months ago by pili

Keywords: TorBrowserTeam201909 removed

comment:80 in reply to:  77 ; Changed 2 months ago by acat

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: needs_revisionneeds_review

Replying to gk:

Replying to gk:

Replying to acat:

Fixed in https://github.com/acatarineu/tor-browser/commits/30429+11. I also changed the last check for the .onion case to if ((mState & STATE_IS_SECURE) == 0) {, because I think the previous if (mState != STATE_IS_INSECURE) { may have erased some flags in case of a https onion.

I changed the comment, but not completely sure if you meant that or something else :)

I meant something else but I was wrong. However, the changed comment *does* add value, so thanks. The patch looks good to me. I'd like to have another reviewer here (as this is a C++ patch), likely pospeselr. Menawhile, though, just a small nit to fix up:

+    // router over tor (.onion).

s/router/routed/

Additionally, it seems that somehow your patch is breaking the mobile experience. With the latest nightly I can see an onion icon and the session is marked as secure. However, testing your patch with my 30429_test (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=30429_test) just gives the regular globe and an insecure connection setting.

I verified that the fixup in https://trac.torproject.org/projects/tor/ticket/31010#comment:35 fixes the mobile issue.

Fixed the typo: https://www.github.com/acatarineu/tor-browser/commit/30429+12.

comment:81 in reply to:  2 Changed 2 months ago by gk

Replying to acat:

[snip]

2eb11a17e593 Bug 16439: remove screencasting code. --- [Investigate, is 'secondscreen/RokuApp.jsm' and 'secondscreen/SimpleServiceDiscovery.jsm' only used in Android now? or is there a different mechanism for desktop?]

Desktop support just got ripped out, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1393582. We should probably adjust the commit message of that commit accordingly while we are at it (we forgot to do so for esr60 it seems).

[snip]

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

comment:82 in reply to:  80 Changed 2 months ago by gk

Replying to acat:

Replying to gk:

Replying to gk:

Replying to acat:

Fixed in https://github.com/acatarineu/tor-browser/commits/30429+11. I also changed the last check for the .onion case to if ((mState & STATE_IS_SECURE) == 0) {, because I think the previous if (mState != STATE_IS_INSECURE) { may have erased some flags in case of a https onion.

I changed the comment, but not completely sure if you meant that or something else :)

I meant something else but I was wrong. However, the changed comment *does* add value, so thanks. The patch looks good to me. I'd like to have another reviewer here (as this is a C++ patch), likely pospeselr. Menawhile, though, just a small nit to fix up:

+    // router over tor (.onion).

s/router/routed/

Additionally, it seems that somehow your patch is breaking the mobile experience. With the latest nightly I can see an onion icon and the session is marked as secure. However, testing your patch with my 30429_test (https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=30429_test) just gives the regular globe and an insecure connection setting.

I verified that the fixup in https://trac.torproject.org/projects/tor/ticket/31010#comment:35 fixes the mobile issue.

Fixed the typo: https://www.github.com/acatarineu/tor-browser/commit/30429+12.

Thanks. Looks good to me. I cherry-picked the fix to tor-browser-68.1.0esr-9.0-2 (commit dece7a15a8703596366c54f4420bd7286c66b10f).

comment:83 Changed 2 months ago by gk

We are close with this ticket. Things I still want to see/figure out are

1) Addressing comment:81
2) Check whether/why 988d41acfaca can actually get dropped
3) Check whether/why 65bbebea18a8 can actually get dropped
4) Second reviewer for code we have for #23247 now

Last edited 4 weeks ago by gk (previous) (diff)

comment:84 in reply to:  83 Changed 2 months ago by gk

Replying to gk:

We are close with this ticket. Things I still want to see/figure out are

1) Addressing comment:81

That's done on tor-browser-68.1.0esr-9.0-3 (commit ef66e9cb42a21dc55b5aee75450fb70e2cd1b77a).

comment:85 Changed 8 weeks ago by pospeselr

dece7a15a8703596366c54f4420bd7286c66b10f looks good to me and from what I can tell through examination 988d41acfaca can most likely be entirely dropped (seeings how none of the code it touches exists anymore). I can do some testing on a build next week to make sure we don't have any edge cases, pretty sure I have a list of test cases from the original ticket in my notes somewhere.

comment:86 in reply to:  85 Changed 8 weeks ago by gk

Replying to pospeselr:

dece7a15a8703596366c54f4420bd7286c66b10f looks good to me and from what I can tell through examination 988d41acfaca can most likely be entirely dropped (seeings how none of the code it touches exists anymore). I can do some testing on a build next week to make sure we don't have any edge cases, pretty sure I have a list of test cases from the original ticket in my notes somewhere.

Thanks. Having some edge-case tests run would indeed be helpful.

comment:87 Changed 6 weeks ago by pili

Keywords: TorBrowserTeam201911 added

Moving tickets to November 2019

comment:88 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201910R removed

There is no way to do reviews in October 2019 anymore.

comment:89 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201911 removed

comment:90 Changed 4 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed
3) Check whether/why 65bbebea18a8 can actually get dropped

is still open but I don't have time to investigate why this can get dropped now. If someone wants to dig a bit that would be highly appreciated, though.

acat, et.al.: please add your points spent here (I know it's hard :) ).

Last edited 4 weeks ago by gk (previous) (diff)

comment:91 in reply to:  90 ; Changed 4 weeks ago by cypherpunks

Replying to gk:

3) Check whether/why 7d0bb93e5c4b can actually get dropped

is still open but I don't have time to investigate why this can get dropped now. If someone wants to dig a bit that would be highly appreciated, though.

Because it was a fix for your workaround for #24052, no? Now that Mozilla fixed that bug in esr68, and you removed any ability to run plug-ins, it is no longer needed.

comment:92 in reply to:  91 Changed 4 weeks ago by gk

Replying to cypherpunks:

Replying to gk:

3) Check whether/why 7d0bb93e5c4b can actually get dropped

is still open but I don't have time to investigate why this can get dropped now. If someone wants to dig a bit that would be highly appreciated, though.

Because it was a fix for your workaround for #24052, no? Now that Mozilla fixed that bug in esr68, and you removed any ability to run plug-ins, it is no longer needed.

Yeah, that's right. I was actually mentioning the wrong commit, sorry. I meant commit 65bbebea18a8 which is the patch for #14716.

comment:93 Changed 4 weeks ago by cypherpunks

I meant commit 65bbebea18a8 which is the patch for #14716.

Oh, that one. Maybe, because nsLoginManager.js was removed in Firefox 68? :)
The last related change was https://hg.mozilla.org/mozilla-central/rev/f52b086cbd9d
If "no prompt after cancelling it 3 times" from STR of #14716 is a desired behavior, then we are good here.

comment:94 Changed 4 weeks ago by acat

Actual Points: 30

acat, et.al.: please add your points spent here (I know it's hard :) ).

It is :)

comment:95 in reply to:  93 ; Changed 3 weeks ago by gk

Replying to cypherpunks:

I meant commit 65bbebea18a8 which is the patch for #14716.

Oh, that one. Maybe, because nsLoginManager.js was removed in Firefox 68? :)

More "moved": https://searchfox.org/mozilla-esr68/source/toolkit/components/passwordmgr/LoginManager.jsm has still almost all the code we patched for #14716. Yet, still...

comment:96 in reply to:  95 Changed 3 weeks ago by cypherpunks

Replying to gk:

Replying to cypherpunks:

I meant commit 65bbebea18a8 which is the patch for #14716.

Oh, that one. Maybe, because nsLoginManager.js was removed in Firefox 68? :)

More "moved": https://searchfox.org/mozilla-esr68/source/toolkit/components/passwordmgr/LoginManager.jsm has still almost all the code we patched for #14716. Yet, still...

Smiley means it was a joke ;)
Mozilla fixed https://bugzilla.mozilla.org/show_bug.cgi?id=1216882 during switching to SQL backend.
Detailed description: https://bugzilla.mozilla.org/show_bug.cgi?id=783994#c19
https://bugzilla.mozilla.org/show_bug.cgi?id=1389664 is where to start from.

comment:97 Changed 8 days ago by pili

Keywords: BugSmashFund added

BugSmashFund can be used for the ESR work done so far

comment:98 Changed 8 days ago by pili

Sponsor: Sponsor44-can

Sponsor 44 only covered PM and Team Lead work

Note: See TracTickets for help on using tickets.