Opened 4 years ago

Closed 3 years ago

#15197 closed task (fixed)

Rebase Tor Browser patches to ESR 45

Reported by: gk Owned by: arthuredelstein
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Critical Keywords: ff45-esr, TorBrowserTeam201604, tbb-6.0a5
Cc: mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorU

Description (last modified by arthuredelstein)

Once Firefox 45 ESR reaches beta state we should start rebasing our existing patches. This ticket is tracking that progress + the upcoming issues.

For reference:

  • Firefox 45 Alpha: 2015-12-14
  • Firefox 45 Beta: 2016-01-26
  • Firefox 45 Release (and ESR): 2016-03-08
  • Firefox 38 ESR end-of-life: 2016-05-31

See also Mozilla's Merge/Release schedule, Release overview, and Rapid Release Calendar.

Here are TBB5.5a patches upstreamed to Firefox. These will be present in ESR45 and so we won't need to rebase them from TBB 5.5a to TBB 6.0a:

Child Tickets

TicketStatusOwnerSummaryComponent
#18602closedmcsMake sure our SVG disabling patches work with SVG faviconsApplications/Tor Browser
#18631closedtbb-teamRewritten #1517 patch for TBB/ESR45Applications/Tor Browser
#18632closedtbb-teamRewritten Image Cache isolation patch for TBB/ESR45Applications/Tor Browser
#18777closedmcsrestore "black on black constrast fix" to ESR45Applications/Tor Browser
#18791closedmcsFix for #13252 does not compile on ESR 45Applications/Tor Browser

Attachments (2)

0001-Bug-12827-Create-preference-to-disable-SVG.patch.gz (9.7 KB) - added by mcs 3 years ago.
revised patch for #12827 (replaces f1a5dfae111d9ab281e766a67ae00559cc13578a)
0001-fixup-Bug-6253-Add-canvas-image-extraction-prompt.patch (894 bytes) - added by mcs 3 years ago.
#6253 fixup patch

Download all attachments as: .zip

Change History (63)

comment:1 Changed 4 years ago by arthuredelstein

Description: modified (diff)

comment:2 Changed 4 years ago by arthuredelstein

Description: modified (diff)

comment:3 Changed 4 years ago by arthuredelstein

Description: modified (diff)

comment:4 Changed 4 years ago by arthuredelstein

Description: modified (diff)

comment:5 Changed 3 years ago by gk

Sponsor: SponsorU

These are SponsorU tickets

comment:6 Changed 3 years ago by arthuredelstein

Description: modified (diff)
Severity: Normal

comment:7 Changed 3 years ago by arthuredelstein

Description: modified (diff)

comment:8 Changed 3 years ago by gk

Keywords: TorBrowserTeam201602 added

comment:9 Changed 3 years ago by cypherpunks

Priority: MediumHigh

comment:10 Changed 3 years ago by gk

Keywords: TorBrowserTeam201603 added; TorBrowserTeam201602 removed

comment:11 Changed 3 years ago by mcs

Kathy and I finished rebasing the updater patches. You can grab our set of six 'am' patches here:
https://pearlcrescent.com/tor/esr45/esr45-patches-mcs-brade-2016-03-10.zip

So far we only built and tested these on Mac OS, but we believe they are correct.

Also, the patch for #18008 (already applied by Arthur) should probably be folded into the #13379 patch someday. In other words, we should add our current signing certificates in one commit. In our rebased #13379 patch, Kathy and I did not touch the toolkit/mozapps/update/updater/release_secondary.der file.

comment:12 Changed 3 years ago by gk

Priority: HighVery High
Severity: NormalCritical
Type: defecttask

comment:13 Changed 3 years ago by gk

One of the things I remembered while looking at 15197+5 was that we should probably put the patches we need to build the ESR45 based Tor Browser at all right at the beginning followed by all the .mozconfigs. Which in turn should be followed by our custom prefs. This way we can be quite sure that bisecting works as expected and is not breaking due to missing prefs/.mozconfig entries.

comment:14 Changed 3 years ago by arthuredelstein

Here is my rebased branch to TBB/ESR45: https://github.com/arthuredelstein/tor-browser/commits/15197+7
(Hash 3934bfd814b74d0e7eec382ab126ef83107b23ab)

I tried to re-order commits to group them by category and also to follow gk's suggestions in comment:13.

The following shows what happened to each commit on the most recent TBB/ESR38-alpha branch,
tor-browser-38.7.1esr-6.0-1:

Key:
A = Already in ESR45 (had been backported to TBB/ESR38)
B = Replaced by backport from FF46 or later
D = Dropped commit (because of Reverts)
R = Rebased from TBB/ESR38 to TBB/ESR45
P = Rebased from TBB/ESR38 to TBB/ESR45 by Pearl Crescent (mcs and brade)
U = Upstreamed by us and therefore already in ESR45
W = Patch re-written (see child bugs for review)

Commits:
R c1b7d7e fixup! Bug 16940: After update, load local change notes.
R 38e31a6 fixup! Bug #4234: Use the Firefox Update Process for Tor Browser.
R a9354c8 Bug 13252 - Do not store data in the app bundle
P 9729ce1 Bug 18292: Staged updates fail on Windows
R 09e1a0e Bug 18170: After update, only changelog tab shown
R beb375e Bug 18297: Use separate Noto JP,KR,SC,TC fonts
A 9abdcf8 Bug 1202266 - Suppress '-Wformat-security' in libstagefright CXXFLAGS. r=kentuckyfriedtakahe
R 78e86ce Bug 16322: Update DuckDuckGo search engine
R e38d0e1 Bug 18008: Create a new MAR Signing key
U 23e813e Bug 17931: Use a non-format argument in LogMessageToConsole
U c758f98 Bug 17931: Update GonkGPSGeolocationProvider.cpp to use B2G-style logging
R 0c04c060 Regression tests for Bug 15564: Isolate SharedWorker by first party domain
R c1c8969 Bug 15564: Isolate SharedWorker by first party domain
R 345666e Bug 17009: Pref to suppress some modifier key events
R 6e30c72 Bug 16441: Suppress "Reset Tor Browser" prompt.
R 1a86d45 Bug 16863: console.error on new Tor Browser window
R 24f9e73 Bug 12516: Compile hardenend Tor Browser with -fwrapv
R c629f33 Bug 17502: Add a pref hiding the "Open with" option
P 79324a1 Bug 16940: After update, load local change notes.
A 1fb0575 Bug 1151485 - Disable app update xml certificate checks on Linux now that there is mar signing on Linux. r=bbondy
A 54c9f73 Bug 920750 - Disable update xml certificate checks on Mac OS X. r=bbondy
R fe91f17 Bug 16620: Clear window.name when no referrer sent
R 2c935ce Regression tests for Bug #17207: Hide mime types and plugins when resisting fingerprinting
R 78688df Bug #17207: Hide mime types and plugins when resisting fingerprinting
R 75fe2a3 Updating .mozconfig-asan
A 206cb87 Bug 1196381 - Eliminate breakpad dependency in ThreadStackHelper; r=nfroyd r=snorp The breakpad dependency in ThreadStackHelper is preventing us from upgrading our in-tree copy to a newer version (bug 1069556). This patch gets rid of that dependency. This makes native stack frames not work for BHR, but because of the ftp.m.o decommissioning, native symbolication was already broken and naive stack frames already don't work, so we don't really lose anything from this patch.
A e3d793a Bug 1147248 - GCC 4.9 needs this patch to use address sanitizer. r=glandium
D 9b0aed1 Revert "Back out changes for bug 16909"
D f5928cb Back out changes for bug 16909
U dab1267 Bug 16906: Don't depend on Windows crypto DLLs
A 1516b26 Bug 16906: Fix MinGW compilation breakage.
R 5899d2a Bug #16855: Allow blobs to be downloaded on first-party pages
A 4268c90 Bug 1192120 - Fix warnings that show up when opening the downloads window in private browsing mode; r=mconley
A 3c89cbf Bug #16311: Fix the responseEnd attribute (navigation timing)
R 85e63e7 Bug 16488:  Remove "Sign in to Sync" from the menu.
R bb566bc Bug #13313: Pref 'font.system.whitelist' restricts set of permitted fonts
U d5e200f Bug #13313 Part 2: Fix windows cross-compile for --enable-bundled-fonts
R 56597fc Bug #15703: Regression tests for isolation of mediasource URI
R 5e849a9 Bug #15502. Isolate blob, mediasource & mediastream URLs to first party
D e4e326d Revert "Bug #15502. Isolate blob URLs to first party; no blobURLs in Web Workers"
U d01aaac Bug 1078657 - Add SpawnTask.js for async tasks in mochitests. r=jmaher
P d74d9a3 Bug 16236: Windows updater: avoid writing to the registry.
R 3b3a6c1 Bug 16528: Prevent indexedDB Modernizr breakage (e10s highrisk).
R 175a3e4 Bug #16005: Restrict WebGL minimal mode a bit.
A 6af09f7 Bug 16316: Fix New Tiles pref bustage.
R 46729ee Bug #16315: Test spoofed media queries in picture elements
R c4d1a61 Bug #13670.1: Isolate favicon requests by first party
R 5ad573e3 Bug 15646: Prevent keyboard layout fingerprinting in KeyboardEvent
R dc834bf Bug #16005: Relax minimal mode.
R f642400 Bug 16300: Isolate Broadcast Channels to first party.
R c3a33a3 Bug 16439: remove screencasting code.
R c886505 Bug 16285: Disabling EME bits
A a8d8642 Bug 1057908 - GeckoMediaPluginService needs to be proxied from Content processes to parent process. Add nsServiceManagerUtils.h include to WMFDecoderModule.cpp.
R ab2ce70 Bug 15910: Disable GMPs for now
A f01ccbd Bug 16351: Upgrade our toolchain (Binutils/GCC)
A 29e3813 Bug 918827 - Remove caching for ftp connections. r=michal
A cc14bed Bug 1151345 - Firefox app menu sometimes contains only "Quit" on OS X. r=spohl
A ec9cb22 bug 1183967 - fixup correct case of mfidl.h
A 5751773 Bug 1133689 - Make D3DVsyncDisplay destructor private. r=jmuizelaar
R 4a27f7e Don't package things we don't build
R 028f802 Bug 13900: Remove 3rd party HTTP auth tokens.
R 39ab4c4 Bug #15502, Part 2: Regression tests for blob URL isolation
D e083bbb Bug #15502. Isolate blob URLs to first party; no blobURLs in Web Workers
R c3f8f15 Bug 13670.2: Isolate OCSP requests by first party domain
R 3eb1aa1 Bug #13749.1: regression tests for first party isolation of localStorage
R 41d0d77 Bug #13749.2: Regression tests for first-party isolation of cache
R df36076 Bug #6564: Isolate DOM storage to first party URI.
W 415f86f Bug #6539: Isolate the Image Cache per url bar domain.
R 6e9c970 Bug 13742: Isolate cache to URL bar domain.
R af1b667 Bug #10819: Add a pref, "privacy.thirdparty.isolate", to allow the activation or deactivation of isolating DOM storage and image caching by first party URI.
R d82c874 Bug #5742: API allows you to get the url bar URI for a channel or nsIDocument.
P bb74041 Bug 13379: Sign our MAR files.
A f26c37f Bug 1158866 - Enable MAR verification on linux via NSS. r=rstrong
A c60696f Bug 973933 - Fix libmar warnings. r=rstrong. a=Callek
A 43e23e2 Bug 973933 - Fix Nightly builds failing on updater-xpcshell. r=rstrong
A 8d026f8 Bug 973933 - Temporarily disable Linux for MAR verification. r=rstrong
A 40a911a Bug 973933 - Fix mochitest chrome updater tests. r=rstrong
A 9547661 Bug 973933 - New updater-xpcshell binary for updater tests. r=rstrong
A 0fef593 Bug 991993: Disable NSS for updater on OSX and enable native APIs. r=smichaud,rstrong
A 4f65cb1 Bug 903126 - Replace DER file with XPCShell cert. r=rstrong
A f783eff Bug 903126 - Don't use an xpcshell cert for verification. r=rstrong
A 8d2ab52 Bug 903135 - Multi platform MAR verification updater support. r=rstrong
A e688472 Bug 903135 - Multi platform MAR verification build config. r=rstrong
A 61e3f10 Bug 903135 - Updates to libmar needed to support B2G MAR signature verification. r=bbondy
P 3358ed4 Bug #4234: Use the Firefox Update Process for Tor Browser.
P 73e122b Bug #11641: change TBB directory structure to be more like Firefox's
P 91fdf8b Bug #9173: Change the default Firefox profile directory to be TBB-relative.
R 145727a Bug #6253: Add canvas image extraction prompt.
R f862331 Bug 12827: Create preference to disable SVG.
R be59619 Bug 13548: Create preference to disable MathML.
W b09bdbf Bug 1517: Reduce precision of time for Javascript.
B 8c876c1 Bug #5926: Allow JS locale to be set to English/C.
U 8b3d00b Bug #6786: Do not expose system colors to CSS or canvas.
U 0cfc2a0 Bug 13025: Lie about screen orientation.
U 3c54815 Bug 13016: Hide CSS -moz-osx-font-smoothing values.
R 9645cb2 Regression tests for #5856: Do not expose physical screen info via window & window.screen.
U d9ea633 Bug #5856: Do not expose physical screen info via window & window.screen.
R c188fd1 Regression tests for #2875: Limit device and system specific CSS Media Queries.
U 9afd24e Bug #2875: Limit device and system specific CSS Media Queries.
R 8b43089 Regression tests for #4755: Return client window coordinates for mouse event screenX/Y (for dragend, 0,0 is returned).
U 648a3de Bug #4755: Return client window coordinates for mouse event screenX/Y (for dragend, 0,0 is returned).
A c6ca7ea Bug 10761: Fix shutdown crashes on Windows
R e8d86e0 TB3: Tor Browser's official .mozconfigs.
R 13c7e53 Bug 14392: Make about:tor behave like other initial pages.
R 33122ba Bug #2176: Rebrand Firefox to TorBrowser
R db648a6 Regression tests for "Omnibox: Add DDG, Startpage, Disconnect, Youtube, Twitter; remove Amazon, eBay, bing"
R c219db4 Omnibox: Add DDG, Startpage, Disconnect, Youtube, Twitter; remove Amazon, eBay, bing
R b9566ae Regression tests for TB4: Tor Browser's Firefox preference overrides.
R 1d8b7a2 TB4: Tor Browser's Firefox preference overrides.
U 19688d0 Bug #2949: Make Intermediate Cert Store memory-only.
R ddc5b7b Regression tests for Bug #2950: Make Permissions Manager memory-only
U 555bf3b Bug #2950: Make Permissions Manager memory-only
R 5bfa856 Regression tests for #2874: Block Components.interfaces from content
R ef7a707 Bug #12620: TorBrowser regression tests folder
R b474453 Bug #2874: Block Components.interfaces from content
R f7e3ce6 Bug 14631: Improve profile access error msgs (strings).
R 511c61b Bug 14631: Improve profile access error messages.
R d704575 Bug 14716: HTTP Basic Authentication prompt only displayed once
U fbdf8d1 Bug #3455.2. Allow RFC1929 authentication (username/password) to SOCKS servers.
R 5df872da Bug #3875: Use Optimistic Data SOCKS variant.
R b654aae Bug #5715: Make nsICacheService.EvictEntries synchronous
U de59ee8 TB2: Provide an observer event to close persistent connections
R f8b56c1 Bug #5282: Randomize HTTP request order and pipeline depth.
R dfa5228 Bug 13028: Prevent potential proxy bypass cases.
R f5d75eb Bug #5741: Prevent WebSocket DNS leak.
R 2e2ebc2 Bug #12974: Disable NTLM and Negotiate HTTP Auth
R dd4c42d Bug 10280: Don't load any plugins into the address space.
R 15a3319 Bug #8312: Remove "This plugin is disabled" barrier.
R bcf625d Bug #3547: Block all plugins except flash.
U 10d37d6 Bug 12430: Disable external jar: via preference
Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:15 Changed 3 years ago by arthuredelstein

Status: newneeds_review

comment:16 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201603R added; TorBrowserTeam201603 removed

comment:17 Changed 3 years ago by arthuredelstein

(I posted an extra fixup to the branch at https://github.com/arthuredelstein/tor-browser/commits/15197+7). The hash is 2fb91f844d36ce5b56a35088d1d460455f0dcdc4)

comment:18 Changed 3 years ago by gk

Here is the first part of the review up to and not including

W 415f86f Bug #6539: Isolate the Image Cache per url bar domain.

I started from 10d37d6 and looked at the corresponding commits on 15197+7 that were not made by Kathy and Mark assuming they doublecheck their patches themselves and you checked the rebased updater patches. Commits I have nothing to say about are omitted.

e839bf9af83f995c36a4d1a7295869dee68c13b0 and
79a6c2a9971a596511b5902b44e074b825c5f465 and
4b81ffadd3ccdc95b208b9594231c5d8b09ff09d => into one .mozconfig related commit?

We should try upstreaming 527211deedc9fef0afdad2b3fdc64c82f6a05cd2 again. We have a patch and I guess Mozilla would take it after we reopen the bug

14d63523794c82c83bfa145262d9ec46b4a18bba

  _pluginEnableButton: null,
  _pluginHeader: null,

landed in gSearchView but should be in gListView I guess

ae367aad222b70d37e5de936dd594e9cdc554ef1 -- could you put the block in AsnycResolveExtended() again after if (mNotifyResolution) {}? Might be helpful for debugging things (observer notification gets still fired even if DNS resolution does not get through)?

246c62e001eefe5864d705610bfafd74dd8e54ce -- okay (a bunch of trailing whitespaces (might be in the original patch) + an additional newline:

-    if (mNumHalfOpenConns < parallelSpeculativeConnectLimit &&
+
+    if (ent->SupportsPipelining()) {

)

094a68930f2d0f17ddc7a50339cc19a4bae44e6c -- okay but it seems to be not needed anymore, see: https://bugzilla.mozilla.org/show_bug.cgi?id=962334, no?

850e2636cd7526cf9631ac629b1f5c45148e98c1 -- okay (why is this still needed after
https://bugzilla.mozilla.org/show_bug.cgi?id=429070 got fixed years ago?)

308d80c19a1ad61f03ca7fecaf1102ee32659b76 -- okay (just superfluous whitespace in the test)

8f1395bc9c13b0ccdf61343a91d413a2bad30f6c -- okay (just superfluous whitespace in the test)

49072ce9ec279ab6c5f7903dcfa4142abae7ba48 -- okay (just superfluous whitespace in the test)

caebb2528e34d7ab6edf0bfd32503bc303755a8f -- we really need #include "nsIURL.h" in this commit?

09e5754cf7748c35305d5e2d72a50de7ae0a1b1f -- okay (missing newline after

    +    extension.Append(nsPrintfCString("%s@", cacheDomain.get()));

)

Could you address Mark's question in #18632?

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

comment:19 Changed 3 years ago by gk

Cc: mcs brade added
Keywords: TorBrowserTeam201604 added; TorBrowserTeam201603R removed
Status: needs_reviewneeds_revision

b98f45490b533834cf19eddf474eef8658ba8921 -- okay (do we want to have the squash! line in the commit message?)

5ff7a84ed956d430d853a29cea44b4d2d6422640 -- fixup ThirdPartyUtil like
c42fc0680c470771a226d9005efe3989354f0b31 ?; whitespace; fix speculative connect
-> Trac ticket?; indentation in nsUDPSocketProvider

8e152d91164ac608acd0c8400bba5cf356d3f7ad -- drop (just conflict marker is left)
2afa5f4a36b8f5af4b7b6ac82ce1d17d20002c40 -- merge with TB3 commit

merge 498fa520a52d034bd403a1afe2db190fce17d421 and 32ac145863c7b75dcf27d93200a600bd1bf0a2fb

224013acae734e76a543cc3b327a00011be26f15 -- we don't need + // return 0; anymore?

f4140e88fa1bb538844c0e020da4e716f73b1f50 -- fixup for ThirdPartyUtil?; LoadImage comment indentation

ac802b13b44f2beebe681aae4ef011bc1351a4f3 -- update commit message FF45 -> FF52

9d65a5039cd4c36149680a13285672897b07235d --

-nsCString isolationKey; in GetFirstPartyHost() is unused
-why do we have static nsCString GetFirstParty(const GlobalObject& aGlobal); in nsURL.h?
-we should line wrap in NS_GetStreamForMediaStreamURI and NS_GetSourceForMediaSourceURI in
nsHostObjectProtocolHandler.h;
-what speaks against

+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return nullptr;
+    }

in FetchRequest()?
-what speaks against

+        nsresult rv = GetFirstPartyHost(GetOwnerDocument(), isolationKey);
+        if (NS_SUCCEEDED(rv)) {

         }

in AfterSetAttr() as well?

810cfbfe2bc3531478b6c266e66eee28aaa79392 --

+                gfxFontEntry* fe =
+                    gfxPlatform::GetPlatform()->LookupLocalFont(currSrc.mLocalName,

just: "fe ="

cd84e61d2a13cb069558b30cca500f8260305f1e -- duckduckgo.xml is not removed; merge with 7f1e0d207b57441b6903b4faed25ef1339ac7b3d

Okay, I think I have looked at all non mcs/brade commits. Nice work, Arthur! If there are things unclear with my remarks let me know. Not sure whether treating the ThirdPartyUtil related things with fixup commits is worth it, but I guess it might be cleaner that way. If we have time it might be good if mcs/brade could look over 9d65a5039cd4c36149680a13285672897b07235d as well. Did we need to adapt the tests for #15502?

comment:20 in reply to:  18 ; Changed 3 years ago by bugzilla

Replying to gk:

79a6c2a9971a596511b5902b44e074b825c5f465 and

tbb-hardened is separated from others, thus that patch wasn't squashed. That's no good, and the whole conception should be revised: separation of hardened and testing/debug versions is required - hardening features such as DEP/ASLR are on all versions, but debug features such as Sanitizer/Wrap (from GCC manual) are on hardened that is not secure (http://www.openwall.com/lists/oss-security/2016/02/17/9). Next, some debug facilities can be used only one after another and not together, so several configs are required, but ASan can be combined with some others, so special config for ASan is not needed.

# Mandatory flags for ASan
export ASANFLAGS="-fsanitize=address -Dxmalloc=myxmalloc -fPIC"

-fPIC can't harm, but if after adding it something (hashes) changes...
Mozilla thinks the same: https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer
-fsanitize-trap-on-error could be used if you have problems with libraries.

+# We need to add -ldl explicitely due to bug 1213698
+export LDFLAGS="-fsanitize=address -ldl"

For this awful crap Mozilla's apologizes should be added:

# Additionally, we need the ASan flag during linking. Normally, our C/CXXFLAGS would
# be used during linking as well but there is at least one place in our build where
# our CFLAGS are not added during linking.
# Note: The use of this flag causes Clang to automatically link the ASan runtime :)

So, here is your "bug 1213698" comes from: compiler options for ld??? (undocumented? or useless), GCC 5+ starts to pass all requirements for linking to ld, thus it passed "no ASan" for parts without CFLAGS...
Bottom change for Ubuntu Lucid can be removed.

4b81ffadd3ccdc95b208b9594231c5d8b09ff09d => into one .mozconfig related commit?

Better to remove than to merge. See #17983.

ae367aad222b70d37e5de936dd594e9cdc554ef1 -- could you put the block in AsnycResolveExtended() again after if (mNotifyResolution) {}? Might be helpful for debugging things (observer notification gets still fired even if DNS resolution does not get through)?

12:12:25.697 [Exception... "Component returned failure code: 0x804b002a (NS_ERROR_UNKNOWN_PROXY_HOST) [nsIDNSService.asyncResolve]"  nsresult: "0x804b002a (NS_ERROR_UNKNOWN_PROXY_HOST)"  location: "JS frame :: chrome://browser/content/browser.js :: gKeywordURIFixup :: line 11572"  data: no]1 browser.js:11577:0

Is this from that patch? Is this correct to end with exc?
STR: Enter 7z into the URL bar and press Enter.

094a68930f2d0f17ddc7a50339cc19a4bae44e6c -- okay but it seems to be not needed anymore, see: https://bugzilla.mozilla.org/show_bug.cgi?id=962334, no?

It doesn't interfere with anything, so it can be removed when that feature will be removed.

850e2636cd7526cf9631ac629b1f5c45148e98c1 -- okay (why is this still needed after
https://bugzilla.mozilla.org/show_bug.cgi?id=429070 got fixed years ago?)

Because it's a "bugfix" for https://bugzilla.mozilla.org/show_bug.cgi?id=790732 :)
As #2874 was filed as "Block Ci", Mozilla's new bug or feature "Ci shim" (for compatibility only or not? They didn't state!) was addressed in that ticket too. Maybe, filing or stating "regression" could be helpful.

Last edited 3 years ago by bugzilla (previous) (diff)

comment:21 in reply to:  19 Changed 3 years ago by bugzilla

Replying to gk:

b98f45490b533834cf19eddf474eef8658ba8921 -- okay (do we want to have the squash! line in the commit message?)

Where is the Development Guide we can find it out from? :)
By the way, have you ensured that your testsuite doesn't use Ci, because:

Joel, one thing about this patch is that it's going to bork the dozen-or-so crashtests that use |Components|. Now, those crashtests are green right now, because currently they'll just throw when they access the undefined property and the test will finish, which is considered PASS for crashtests. But they're not testing what they're supposed to.

Is it good to merge everything about configs into one commit?

2afa5f4a36b8f5af4b7b6ac82ce1d17d20002c40 -- merge with TB3 commit

It has it's own ticket #16285, doesn't interfere with others, so it can be easily applied / reverted / anything else.

9d65a5039cd4c36149680a13285672897b07235d --

It was completely rewritten in #16429: https://github.com/arthuredelstein/tor-browser/commit/f538052047ad61cf74f926381bc7c41b60fa2a3d

810cfbfe2bc3531478b6c266e66eee28aaa79392 --

Seems to be rewritten too, because it's fixed on the latest branch.

Last edited 3 years ago by bugzilla (previous) (diff)

comment:22 in reply to:  18 Changed 3 years ago by gk

Replying to gk:

850e2636cd7526cf9631ac629b1f5c45148e98c1 -- okay (why is this still needed after
https://bugzilla.mozilla.org/show_bug.cgi?id=429070 got fixed years ago?)

IIRC we might have kept this one as defense-in-depth. If so, keeping it again + mentioning the reasoning in the commit message sounds like a way to go to me. Otherwise, I think dropping it but making sure our nightly builds would catch any regressions seems to me a fine plan, too.

Re: commit merge. If we go that way for .mozconfig and pref related things putting the bug numbers in the commit message (as is already done in part at least) sounds like a good way of keeping history easier avaiable.

comment:23 Changed 3 years ago by mcs

Kathy and I are part way through our review of the brade/mcs patches. Here is what we found so far:

For #14716 (f66d0f7d06baba14e97bf63e5b2e0fbd5674c62d), it looks like throw "string" was replaced with throw new Error("string") throughout nsLoginManager.js, so we should probably do the same for the two throw statements that we add.

For #6786 (upstreamed), part of our original patch was dropped. See https://bugzilla.mozilla.org/show_bug.cgi?id=232227#c42 Is there another Bugzilla bug for this? I think we want the missing part in Tor Browser.

comment:24 Changed 3 years ago by mcs

We completed our review of all of the brade/mcs patches. Here are the additional things we found:

The "fixup" patch for #13252 is missing (b9a6bef70d96a5aab0cea088d2dc3da4a17354a0 on tor-browser-38.7.1esr-6.0-1). Arthur: do you want us to rebase that patch or are you planning to do it?

For #6786 (upstreamed), we need to ensure that ui.use_standins_for_native_colors = true.

For #12827 (f1a5dfae111d9ab281e766a67ae00559cc13578a), some additional changes are needed. Kathy and I can provide a fixup patch or a revised patch; which one do you prefer?

For #6253 (1914acdc24d3fdcdc3e94d24b4225f6804004f75), additional work is needed (a crash is occurring inside canvas.getImageData). Kathy and I have not debugged this yet. The patch looked like it was correctly rebased, so something must have changed inside Firefox to cause the crash.

For #16488 (3b20b1e58f08883f6dc2aba44091c49780a11982), additional work is needed. The "Sync in to Sync" text is hidden but the icon is not.

comment:25 in reply to:  24 ; Changed 3 years ago by gk

Replying to mcs:

For #6253 (1914acdc24d3fdcdc3e94d24b4225f6804004f75), additional work is needed (a crash is occurring inside canvas.getImageData). Kathy and I have not debugged this yet. The patch looked like it was correctly rebased, so something must have changed inside Firefox to cause the crash.

How can I reproduce that one? Allowing canvas on e.g. github.com works fine for me on a 64bit Linux box.

comment:26 in reply to:  24 ; Changed 3 years ago by gk

Replying to mcs:

For #16488 (3b20b1e58f08883f6dc2aba44091c49780a11982), additional work is needed. The "Sync in to Sync" text is hidden but the icon is not.

Sounds to me like a good thing for the post-alpha timeframe? I think holding off the switch to ESR45 in our nightly builds to get that one properly fixed (too) might not be worth it. Could you file a follow-up ticket?

comment:27 Changed 3 years ago by gk

arthuredelstein: When you are preparing a new branch, please don't forget the 5 patches in #18290 that need to get applied, too (although I still hope we can find a better fix instead of reverting Jacek's fix). Ideally these should be the first ones on top of the Mozilla branch I think. For the view management fix you need --ignore-whitespace when using git am.

comment:28 Changed 3 years ago by gk

Keywords: tbb-6.0a5 added

comment:29 in reply to:  25 ; Changed 3 years ago by mcs

Replying to gk:

How can I reproduce that one? Allowing canvas on e.g. github.com works fine for me on a 64bit Linux box.

Looking a little more closely, this probably only occurs in debug builds (assertion failure). Here is a partial call stack (Mac OS debug build):

Assertion failure: !mIsMapped (Someone forgot to call Unmap()), at /Users/brade/dev/tor/tor-browser-arthur/gfx/2d/2D.h:383
#01: mozilla::gfx::DataSourceSurface::~DataSourceSurface()[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x1412e1d]
#02: mozilla::gfx::DataSourceSurfaceWrapper::~DataSourceSurfaceWrapper()[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x1412db1]
#03: mozilla::gfx::DataSourceSurfaceWrapper::~DataSourceSurfaceWrapper()[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x1412bf5]
#04: mozilla::gfx::DataSourceSurfaceWrapper::~DataSourceSurfaceWrapper()[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x1412c19]
#05: mozilla::detail::RefCounted<mozilla::gfx::SourceSurface, (mozilla::detail::RefCountAtomicity)1>::Release() const[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x13b223a]
#06: RefPtr<mozilla::gfx::DataSourceSurface>::AddRefTraitsReleaseHelper(mozilla::gfx::DataSourceSurface*)[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x13b211c]
#07: RefPtr<mozilla::gfx::DataSourceSurface>::AddRefTraits<mozilla::gfx::DataSourceSurface>::Release(mozilla::gfx::DataSourceSurface*)[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x13b20f5]
#08: RefPtr<mozilla::gfx::DataSourceSurface>::~RefPtr()[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x13b22fd]
#09: RefPtr<mozilla::gfx::DataSourceSurface>::~RefPtr()[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x13b1e15]
#10: mozilla::dom::CanvasRenderingContext2D::GetImageDataArray(JSContext*, int, int, unsigned int, unsigned int, JSObject**)[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x2c2021a]
#11: mozilla::dom::CanvasRenderingContext2D::GetImageData(JSContext*, double, double, double, double, mozilla::ErrorResult&)[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x2c1f581]
#12: mozilla::dom::CanvasRenderingContext2DBinding::getImageData(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&)[/Users/brade/Desktop/tb-esr45/tb-esr45.app/Contents/MacOS/XUL +0x23f686f]
[snip]

comment:30 in reply to:  26 Changed 3 years ago by mcs

Replying to gk:

Replying to mcs:

For #16488 (3b20b1e58f08883f6dc2aba44091c49780a11982), additional work is needed. The "Sync in to Sync" text is hidden but the icon is not.

Sounds to me like a good thing for the post-alpha timeframe? I think holding off the switch to ESR45 in our nightly builds to get that one properly fixed (too) might not be worth it. Could you file a follow-up ticket?

This is #18743.

comment:31 in reply to:  24 ; Changed 3 years ago by mcs

Replying to mcs:

For #12827 (f1a5dfae111d9ab281e766a67ae00559cc13578a), some additional changes are needed. Kathy and I can provide a fixup patch or a revised patch; which one do you prefer?

We went ahead and created a replacement patch. I will attach it to this ticket.

Last edited 3 years ago by mcs (previous) (diff)

Changed 3 years ago by mcs

revised patch for #12827 (replaces f1a5dfae111d9ab281e766a67ae00559cc13578a)

comment:32 in reply to:  31 ; Changed 3 years ago by bugzilla

Replying to mcs:

Replying to mcs:

For #12827 (f1a5dfae111d9ab281e766a67ae00559cc13578a), some additional changes are needed. Kathy and I can provide a fixup patch or a revised patch; which one do you prefer?

We went ahead and created a replacement patch. I will attach it to this ticket.

.gz? Why not .7z? Trac doesn't eat .gz!

fixup patch
revised patch
replacement patch

What do all that mean? Where is the definition of that? Difference?
Nevertheless, all of them have a lot of holes!
For ex. with STR:
https://en.wikipedia.org/wiki/Scalable_Vector_Graphics
See Page Info / Media:
data:image/svg+xml,..., type Background or Bullet
All were loaded and displayed.

comment:33 in reply to:  32 ; Changed 3 years ago by mcs

Replying to bugzilla:

Replying to mcs:

Replying to mcs:

For #12827 (f1a5dfae111d9ab281e766a67ae00559cc13578a), some additional changes are needed. Kathy and I can provide a fixup patch or a revised patch; which one do you prefer?

We went ahead and created a replacement patch. I will attach it to this ticket.

.gz? Why not .7z? Trac doesn't eat .gz!

fixup patch
revised patch
replacement patch

What do all that mean? Where is the definition of that? Difference?

A fixup patch is an additional patch that needs to be applied (on top of the older one).
A revised or replacement patch supersedes an older patch.

Nevertheless, all of them have a lot of holes!
For ex. with STR:
https://en.wikipedia.org/wiki/Scalable_Vector_Graphics
See Page Info / Media:
data:image/svg+xml,..., type Background or Bullet
All were loaded and displayed.

I forgot to mention that Page Info is still a hole / known issue. I am not sure if there is already an open ticket but we should open one if not.

comment:34 in reply to:  31 ; Changed 3 years ago by gk

Replying to mcs:

Replying to mcs:

For #12827 (f1a5dfae111d9ab281e766a67ae00559cc13578a), some additional changes are needed. Kathy and I can provide a fixup patch or a revised patch; which one do you prefer?

We went ahead and created a replacement patch. I will attach it to this ticket.

Arthur: Getting that one properly reviewed and tested should not prevent a new branch with all the rebase related fixes we can use for our nightlies. I can imagine just reverting the old SVG patch when we land the new one and just cutting the old one and the revert out when rebasing later to a new ESR45.

comment:35 in reply to:  18 Changed 3 years ago by arthuredelstein

Replying to gk:

Here is the first part of the review up to and not including

W 415f86f Bug #6539: Isolate the Image Cache per url bar domain.

I started from 10d37d6 and looked at the corresponding commits on 15197+7 that were not made by Kathy and Mark assuming they doublecheck their patches themselves and you checked the rebased updater patches. Commits I have nothing to say about are omitted.

Thanks for the reviews!

e839bf9af83f995c36a4d1a7295869dee68c13b0 and
79a6c2a9971a596511b5902b44e074b825c5f465 and
4b81ffadd3ccdc95b208b9594231c5d8b09ff09d => into one .mozconfig related commit?

Done

We should try upstreaming 527211deedc9fef0afdad2b3fdc64c82f6a05cd2 again. We have a patch and I guess Mozilla would take it after we reopen the bug

Good idea. I was only able to

14d63523794c82c83bfa145262d9ec46b4a18bba

  _pluginEnableButton: null,
  _pluginHeader: null,

landed in gSearchView but should be in gListView I guess

Done.

ae367aad222b70d37e5de936dd594e9cdc554ef1 -- could you put the block in AsnycResolveExtended() again after if (mNotifyResolution) {}? Might be helpful for debugging things (observer notification gets still fired even if DNS resolution does not get through)?

Fixed.

246c62e001eefe5864d705610bfafd74dd8e54ce -- okay (a bunch of trailing whitespaces (might be in the original patch) + an additional newline:

-    if (mNumHalfOpenConns < parallelSpeculativeConnectLimit &&
+
+    if (ent->SupportsPipelining()) {

)

Fixed.

094a68930f2d0f17ddc7a50339cc19a4bae44e6c -- okay but it seems to be not needed anymore, see: https://bugzilla.mozilla.org/show_bug.cgi?id=962334, no?

You're right -- I will drop this.

850e2636cd7526cf9631ac629b1f5c45148e98c1 -- okay (why is this still needed after
https://bugzilla.mozilla.org/show_bug.cgi?id=429070 got fixed years ago?)

I agree -- I will drop this patch and the corresponding regression test.

308d80c19a1ad61f03ca7fecaf1102ee32659b76 -- okay (just superfluous whitespace in the test)

Fixed.

8f1395bc9c13b0ccdf61343a91d413a2bad30f6c -- okay (just superfluous whitespace in the test)

Fixed.

49072ce9ec279ab6c5f7903dcfa4142abae7ba48 -- okay (just superfluous whitespace in the test)

I merged this patch with our patch for #16315: this consolidates a single test file into one commit, and also deals with the superfluous whitespace.

caebb2528e34d7ab6edf0bfd32503bc303755a8f -- we really need #include "nsIURL.h" in this commit?

Yes, it appears to be necessary given the line

+  nsCOMPtr<nsIURL> url = do_QueryInterface(aFirstPartyURI);

later in the commit. I tried compiling without it the #include, but that failed.

09e5754cf7748c35305d5e2d72a50de7ae0a1b1f -- okay (missing newline after

    +    extension.Append(nsPrintfCString("%s@", cacheDomain.get()));

)

I didn't add an extra newline because the following lines are closely related.

Could you address Mark's question in #18632?

Yes, I will address those questions there.

The branch after these changes is at
https://github.com/arthuredelstein/tor-browser/commits/15197+8

comment:36 in reply to:  33 Changed 3 years ago by mcs

Replying to mcs:

I forgot to mention that Page Info is still a hole / known issue. I am not sure if there is already an open ticket but we should open one if not.

I filed #18770 to cover this issue.

comment:37 in reply to:  29 Changed 3 years ago by mcs

Replying to mcs:

Replying to gk:

How can I reproduce that one? Allowing canvas on e.g. github.com works fine for me on a 64bit Linux box.

Looking a little more closely, this probably only occurs in debug builds (assertion failure). Here is a partial call stack (Mac OS debug build):
[snip]

Kathy and I have a fixup patch that fixes this problem. I will attach it in a moment.

comment:38 in reply to:  33 Changed 3 years ago by bugzilla

Who's that?
There's no response for 2 months (for ex. in #17597). Others seem to use Full Ignore mode...
Is this a community project or only looks like?

Replying to mcs:

Replying to bugzilla:

Trac doesn't eat .gz!

fixup patch
revised patch
replacement patch

What do all that mean? Where is the definition of that? Difference?

A fixup patch is an additional patch that needs to be applied (on top of the older one).
A revised or replacement patch supersedes an older patch.

That wasn't a question. Patch Management/Versioning could help to avoid weird terminology.

I forgot to mention that Page Info is still a hole / known issue. I am not sure if there is already an open ticket but we should open one if not.

Heh, that meant: Bullets were displayed on a webpage...

comment:39 in reply to:  19 Changed 3 years ago by arthuredelstein

Replying to gk:

b98f45490b533834cf19eddf474eef8658ba8921 -- okay (do we want to have the squash! line in the commit message?)

Fixed the commit message.

5ff7a84ed956d430d853a29cea44b4d2d6422640 -- fixup ThirdPartyUtil like
c42fc0680c470771a226d9005efe3989354f0b31 ?

Good idea -- I have now done that for the ThirdPartyUtil changes in OCSP and favicons

whitespace; fix speculative connect -> Trac ticket?;

#18762

indentation in nsUDPSocketProvider

Fixed.

8e152d91164ac608acd0c8400bba5cf356d3f7ad -- drop (just conflict marker is left)

Dropped.

2afa5f4a36b8f5af4b7b6ac82ce1d17d20002c40 -- merge with TB3 commit

Done.

merge 498fa520a52d034bd403a1afe2db190fce17d421 and 32ac145863c7b75dcf27d93200a600bd1bf0a2fb

Done.

224013acae734e76a543cc3b327a00011be26f15 -- we don't need + // return 0; anymore?

Yes, we don't. I'm not sure why this line was changed, but I don't think there is any need to modify the default behavior. We could consider fixing up the 38ESR branch to uncomment the line, but I don't think it's doing any harm either.

f4140e88fa1bb538844c0e020da4e716f73b1f50 -- fixup for ThirdPartyUtil?;

Fixed (as mentioned above).

LoadImage comment indentation

Fixed.

ac802b13b44f2beebe681aae4ef011bc1351a4f3 -- update commit message FF45 -> FF52

Fixed.

9d65a5039cd4c36149680a13285672897b07235d --

-nsCString isolationKey; in GetFirstPartyHost() is unused

Fixed.

-why do we have static nsCString GetFirstParty(const GlobalObject& aGlobal); in nsURL.h?

Oops -- removed.

-we should line wrap in NS_GetStreamForMediaStreamURI and NS_GetSourceForMediaSourceURI in
nsHostObjectProtocolHandler.h;

Done.

-what speaks against

+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return nullptr;
+    }

in FetchRequest()?

Good idea -- added.

-what speaks against

+        nsresult rv = GetFirstPartyHost(GetOwnerDocument(), isolationKey);
+        if (NS_SUCCEEDED(rv)) {

         }

in AfterSetAttr() as well?

Also added.

810cfbfe2bc3531478b6c266e66eee28aaa79392 --

+                gfxFontEntry* fe =
+                    gfxPlatform::GetPlatform()->LookupLocalFont(currSrc.mLocalName,

just: "fe ="

Fixed.

cd84e61d2a13cb069558b30cca500f8260305f1e -- duckduckgo.xml is not removed;

Fixed.

merge with 7f1e0d207b57441b6903b4faed25ef1339ac7b3d

Done.

Okay, I think I have looked at all non mcs/brade commits. Nice work, Arthur! If there are things unclear with my remarks let me know. Not sure whether treating the ThirdPartyUtil related things with fixup commits is worth it, but I guess it might be cleaner that way.

I have now merged all ThirdPartyUtil-related code into a single patch.

If we have time it might be good if mcs/brade could look over 9d65a5039cd4c36149680a13285672897b07235d as well.

Did we need to adapt the tests for #15502?

I will check all tests again after fixing the mcs/brade patches.

Here's the new version: https://github.com/arthuredelstein/tor-browser/commits/15197+9
hash: 92d3f1214681c45f66c446a122066a30a83919b2
This includes the changes made for 15197+8.

comment:40 in reply to:  23 Changed 3 years ago by arthuredelstein

Replying to mcs:

Kathy and I are part way through our review of the brade/mcs patches. Here is what we found so far:

Thank you for looking at these.

For #14716 (f66d0f7d06baba14e97bf63e5b2e0fbd5674c62d), it looks like throw "string" was replaced with throw new Error("string") throughout nsLoginManager.js, so we should probably do the same for the two throw statements that we add.

Fixed.

For #6786 (upstreamed), part of our original patch was dropped. See https://bugzilla.mozilla.org/show_bug.cgi?id=232227#c42 Is there another Bugzilla bug for this? I think we want the missing part in Tor Browser.

I hadn't realized this. I'm opening a ticket to bring this back the dropped piece: #18777.

The new branch will be in the next comment.

comment:41 in reply to:  24 Changed 3 years ago by arthuredelstein

Replying to mcs:

We completed our review of all of the brade/mcs patches. Here are the additional things we found:

The "fixup" patch for #13252 is missing (b9a6bef70d96a5aab0cea088d2dc3da4a17354a0 on tor-browser-38.7.1esr-6.0-1). Arthur: do you want us to rebase that patch or are you planning to do it?

I have now rebased the patch.

For #6786 (upstreamed), we need to ensure that ui.use_standins_for_native_colors = true.

I have added this pref setting to our browser/app/profile/000-tor-browser.js file.

For #12827 (f1a5dfae111d9ab281e766a67ae00559cc13578a), some additional changes are needed. Kathy and I can provide a fixup patch or a revised patch; which one do you prefer?

I will be reviewing the patch you posted and add it to the branch soon.

For #6253 (1914acdc24d3fdcdc3e94d24b4225f6804004f75), additional work is needed (a crash is occurring inside canvas.getImageData). Kathy and I have not debugged this yet. The patch looked like it was correctly rebased, so something must have changed inside Firefox to cause the crash.

I have added your fixup patch from comment:37. Thanks for making it!

For #16488 (3b20b1e58f08883f6dc2aba44091c49780a11982), additional work is needed. The "Sign in to Sync" text is hidden but the icon is not.

(For completeness, the ticket for that issue is #18743.)

Here's the latest branch. (For clarity, I haven't autosquashed the fixups yet.)
https://github.com/arthuredelstein/tor-browser/commits/15197+10

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

comment:42 in reply to:  27 Changed 3 years ago by arthuredelstein

Replying to gk:

arthuredelstein: When you are preparing a new branch, please don't forget the 5 patches in #18290 that need to get applied, too (although I still hope we can find a better fix instead of reverting Jacek's fix). Ideally these should be the first ones on top of the Mozilla branch I think. For the view management fix you need --ignore-whitespace when using git am.

Here's the new branch, autosquashed including the 5 patches from #18290.
https://github.com/arthuredelstein/tor-browser/commits/15197+11

comment:43 Changed 3 years ago by gk

After looking at 15197+11 and going over the last comments I think the following things are still open

1) duckduckgo.xml is still there.
2) Are we good with the tests for #15502?

(+ the child tickets)

comment:44 in reply to:  34 Changed 3 years ago by mcs

Replying to gk:

Replying to mcs:

Replying to mcs:

For #12827 (f1a5dfae111d9ab281e766a67ae00559cc13578a), some additional changes are needed. Kathy and I can provide a fixup patch or a revised patch; which one do you prefer?

We went ahead and created a replacement patch. I will attach it to this ticket.

Arthur: Getting that one properly reviewed and tested should not prevent a new branch with all the rebase related fixes we can use for our nightlies. I can imagine just reverting the old SVG patch when we land the new one and just cutting the old one and the revert out when rebasing later to a new ESR45.

We will want the revised patch soon. Without it, debug builds crash due to a failed assertion in GetProgressTracker() (inside image/Image.h) when asked to load SVG images, assuming you have set svg.in-content.enabled = false. I have not tried a non-debug build, but looking at the code I think a NULL pointer dereference will occur.

comment:45 in reply to:  43 ; Changed 3 years ago by gk

Replying to gk:

After looking at 15197+11 and going over the last comments I think the following things are still open

1) duckduckgo.xml is still there.
2) Are we good with the tests for #15502?

(+ the child tickets)

FWIW in #17506 there is another small patch we want. This one is for the hardened builds.

comment:46 in reply to:  20 ; Changed 3 years ago by bugzilla

Replying to gk:

Replying to bugzilla:

Bottom change for Ubuntu Lucid can be removed.

Hmm... And that's all you've found useful from that comment?..

comment:47 in reply to:  46 ; Changed 3 years ago by gk

Replying to bugzilla:

Replying to gk:

Replying to bugzilla:

Bottom change for Ubuntu Lucid can be removed.

Hmm... And that's all you've found useful from that comment?..

That's not from that comment. As I've said I've been working on #17506.

comment:48 in reply to:  47 ; Changed 3 years ago by bugzilla

Replying to gk:

That's not from that comment. As I've said I've been working on #17506.

Of course, not. It seems the development is going in hidden mode (e.g. nothing in #17506 during 5 months) and comments are not required...

comment:49 in reply to:  48 ; Changed 3 years ago by gk

Replying to bugzilla:

Replying to gk:

That's not from that comment. As I've said I've been working on #17506.

Of course, not. It seems the development is going in hidden mode (e.g. nothing in #17506 during 5 months) and comments are not required...

There was actually no development on this ticket for the last five months which is the sole reason for no comments there. I realized on the weekend that we might want to check our toolchain for the hardened builds as well and therefore I dug it out again. There is no mystery here.

comment:50 in reply to:  49 ; Changed 3 years ago by bugzilla

Replying to gk:

Replying to bugzilla:

Replying to gk:

That's not from that comment. As I've said I've been working on #17506.

Of course, not. It seems the development is going in hidden mode (e.g. nothing in #17506 during 5 months) and comments are not required...

There was actually no development on this ticket for the last five months which is the sole reason for no comments there. I realized on the weekend that we might want to check our toolchain for the hardened builds as well and therefore I dug it out again. There is no mystery here.

Thank you for your confirmation that you haven't read comments written a week ago.

comment:51 in reply to:  50 ; Changed 3 years ago by gk

Replying to bugzilla:

Replying to gk:

Replying to bugzilla:

Replying to gk:

That's not from that comment. As I've said I've been working on #17506.

Of course, not. It seems the development is going in hidden mode (e.g. nothing in #17506 during 5 months) and comments are not required...

There was actually no development on this ticket for the last five months which is the sole reason for no comments there. I realized on the weekend that we might want to check our toolchain for the hardened builds as well and therefore I dug it out again. There is no mystery here.

Thank you for your confirmation that you haven't read comments written a week ago.

This will be my last off-topic remark: you are wrong again. I've read all comments. I just did not have the hardened series on my radar until last weekend.

comment:52 in reply to:  51 Changed 3 years ago by bugzilla

Replying to gk:

Replying to bugzilla:

Replying to gk:

Replying to bugzilla:

Replying to gk:

That's not from that comment. As I've said I've been working on #17506.

Of course, not. It seems the development is going in hidden mode (e.g. nothing in #17506 during 5 months) and comments are not required...

There was actually no development on this ticket for the last five months which is the sole reason for no comments there. I realized on the weekend that we might want to check our toolchain for the hardened builds as well and therefore I dug it out again. There is no mystery here.

Thank you for your confirmation that you haven't read comments written a week ago.

This will be my last off-topic remark: you are wrong again. I've read all comments. I just did not have the hardened series on my radar until last weekend.

You are impelling to reply you. Wrong? Again? This discussion became more general (but still about development, not off-topic), exactly because of no discussion on topic. Also, you could PM to email at any moment, but preffered to continue here. So, you've been reviewing the rebasing of tbb-hardened patches last week without having the hardened series on your radar. This explains the situation...

comment:53 in reply to:  43 Changed 3 years ago by arthuredelstein

Replying to gk:

After looking at 15197+11 and going over the last comments I think the following things are still open

1) duckduckgo.xml is still there.

Sorry -- fixed with a fixup (hash c4455193b26e20f8201b75853533cfa169fb97e0).

2) Are we good with the tests for #15502?

Unfortunately, it turns out there is a regression. I opened a ticket here: #18811

comment:54 in reply to:  45 Changed 3 years ago by arthuredelstein

Replying to gk:

Replying to gk:

After looking at 15197+11 and going over the last comments I think the following things are still open

1) duckduckgo.xml is still there.
2) Are we good with the tests for #15502?

(+ the child tickets)

FWIW in #17506 there is another small patch we want. This one is for the hardened builds.

Added to https://github.com/arthuredelstein/tor-browser/commits/15197+11
Hash 6cdd4137c3f2f38b57b28f68c4ceefef4623d1c1

comment:55 in reply to:  31 ; Changed 3 years ago by arthuredelstein

Replying to mcs:

Replying to mcs:

For #12827 (f1a5dfae111d9ab281e766a67ae00559cc13578a), some additional changes are needed. Kathy and I can provide a fixup patch or a revised patch; which one do you prefer?

We went ahead and created a replacement patch. I will attach it to this ticket.

I reviewed this patch and it looks good to me. If gk thinks it looks OK as well, I will add it to our branch. In case it's useful for reviewing, here's a fixup version of the same patch:
https://github.com/arthuredelstein/tor-browser/commit/9947fad289055f7ed973271d4e78a3bad87d0383

comment:56 in reply to:  55 Changed 3 years ago by arthuredelstein

Replying to arthuredelstein:

Replying to mcs:

Replying to mcs:

For #12827 (f1a5dfae111d9ab281e766a67ae00559cc13578a), some additional changes are needed. Kathy and I can provide a fixup patch or a revised patch; which one do you prefer?

We went ahead and created a replacement patch. I will attach it to this ticket.

I reviewed this patch and it looks good to me. If gk thinks it looks OK as well, I will add it to our branch. In case it's useful for reviewing, here's a fixup version of the same patch:
https://github.com/arthuredelstein/tor-browser/commit/9947fad289055f7ed973271d4e78a3bad87d0383

I see now that Georg already reviewed it on #18602. So I have added the fixup to our 15197+11 branch:
https://github.com/arthuredelstein/tor-browser/commits/15197+11
hash d508fccd831cce863a7c3e9bdb7df1a389cd19d5

comment:57 in reply to:  55 Changed 3 years ago by arthuredelstein

Replying to arthuredelstein:

Replying to mcs:

Replying to mcs:

For #12827 (f1a5dfae111d9ab281e766a67ae00559cc13578a), some additional changes are needed. Kathy and I can provide a fixup patch or a revised patch; which one do you prefer?

We went ahead and created a replacement patch. I will attach it to this ticket.

I reviewed this patch and it looks good to me. If gk thinks it looks OK as well, I will add it to our branch. In case it's useful for reviewing, here's a fixup version of the same patch:
https://github.com/arthuredelstein/tor-browser/commit/9947fad289055f7ed973271d4e78a3bad87d0383

I see now that Georg reviewed it in #18602. So I have added it to our 15197+11 branch:
https://github.com/arthuredelstein/tor-browser/commits/15197+11
hash: f3584b6dc11f7b76ddae72af2ebacf23198d5258

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

comment:58 Changed 3 years ago by gk

Cc: arthuredelstein added

Arthur, could you take care of #18632 and #18777 + a fixup for #18802? These are the current blockers for getting our nightlies switched to ESR45 and I'd like to do that over the weekend.

comment:59 Changed 3 years ago by gk

Cc: arthuredelstein removed

comment:60 in reply to:  58 Changed 3 years ago by arthuredelstein

Replying to gk:

Arthur, could you take care of #18632 and #18777 + a fixup for #18802? These are the current blockers for getting our nightlies switched to ESR45 and I'd like to do that over the weekend.

I have tried to address each of these. The new branch is
https://github.com/arthuredelstein/tor-browser/commits/15197+12
hash ee7e5be3df465a9b32690e767796a3e258ef81b0

comment:61 Changed 3 years ago by gk

Resolution: fixed
Status: needs_revisionclosed

I think we can declare victory here. \o/ The result (rebased against 45.0.2esr), tor-browser-45.0.2esr-6.x-1, is currently pushed.

Note: See TracTickets for help on using tickets.