Opened 4 years ago

Closed 4 years ago

#16906 closed defect (fixed)

Code for 38.3.0 ESR is failing to compile with mingw-w64

Reported by: gk Owned by: tbb-team
Priority: Immediate Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-gitian, TorBrowserTeam201509R
Cc: mikeperry, boklm, brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by gk)

https://hg.mozilla.org/releases/mozilla-esr38/rev/be6396f0eb7c fails to compile with mingw-w64 and is likely going to get shipped in Firefox 38.3.0 ESR. The bug on Mozilla's side is https://bugzilla.mozilla.org/show_bug.cgi?id=1199118.

If Mozilla does not get this fixed on esr38 we either need to backport a patch or come up with one ourselves. glandium and Mike hashed things out in #build:

00:47 < mikeperry> can we link against those libs in an alternate way?
00:47 < glandium> OS_LIBS += ['wintrust', 'crypt32'] in the corresponding 
                  moz.build
00:51 < mikeperry> so under the existing CONFIG['MOZ_WIDGET_TOOLKIT'] == 
                   'windows' check in toolkit/mozapps/update/common/moz.build 
                   perhaps?
00:51 < glandium> mikeperry: sounds right
00:53 < mikeperry> am wondering if should use if CONFIG['OS_ARCH'] == 'WINNT' 
                   instead
00:53 < glandium> mikeperry: it doesn't really matter

Child Tickets

Change History (11)

comment:1 Changed 4 years ago by mcs

Cc: brade mcs added

I see there is a proposed fix. Yesterday, Kathy and I spent some time looking at the changes associated with Mozilla bug 1177861 because we thought that changeset was going to be part of the imminent release. It may be that the code that introduces a dependency on the wintrust and crypt32 DLLs is not needed in Tor Browser. I do not think it will ever be called by our updater, which means it may be better for Tor Browser to not build that code at all. Previously, most of it was part of the maintenance service which we omit. But we will need to examine our code closely to confirm that it is not needed and then decide if it is worthwhile to patch things to remove the dependency.

comment:2 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201509 added; TorBrowserTeam201508 removed

Move remaining August tickets to September.

comment:3 Changed 4 years ago by mikeperry

mcs, brade: Did we verify that this code won't cause any issues with our updater?

comment:4 Changed 4 years ago by gk

Description: modified (diff)
Summary: Code for 38.3.0 ESR is failing to compile with mingw-w64Code for 38.x.0 ESR is failing to compile with mingw-w64

It is not included in ESR 38.3.0 thus we will have some more time to think about.

comment:5 Changed 4 years ago by gk

Description: modified (diff)
Summary: Code for 38.x.0 ESR is failing to compile with mingw-w64Code for 38.3.0 ESR is failing to compile with mingw-w64

Ugh, scratch the previous comment. It is included.

comment:6 in reply to:  3 Changed 4 years ago by mcs

Replying to mikeperry:

mcs, brade: Did we verify that this code won't cause any issues with our updater?

We do not think it will cause any issues with our updater (the changes mainly affect users of the maintenance service, which we do not build as part of Tor Browser). However, it does introduce a dependency on a couple of Windows crypto DLLs that previously we did not have, and it adds code that will not be used in Tor Browser. Kathy and I would rather put in #ifdef's to skip all of the new stuff if we have time to do so, but we do not have a patch in hand yet.

comment:7 Changed 4 years ago by mcs

Keywords: TorBrowserTeam201509R added; TorBrowserTeam201509 removed
Status: newneeds_review

Here is a patch that Kathy and I created:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16906-01&id=733f63e89a86fe16fdfd6008baa376b681e7d505

If it is too late or too risky for TB 5.0.3 and TB 5.5a3, it can wait.
We did test it in a Windows 5.5 alpha build.

comment:8 Changed 4 years ago by gk

This looks good to me. I have two questions, though:

1) Why do we need the second ifdef given that the third ifdef prevents sUsingService from ever getting true?

2) Looking a bit deeper I wonder why we even need the third ifdef given that MOZ_USING_SERVICE gets only set in workmonitor.cpp which in turn only gets compiled in if we don't do --disable-maintenance-service. Hardening against environment manipulation by some rogue third party program?

comment:9 in reply to:  8 Changed 4 years ago by mcs

Replying to gk:

This looks good to me. I have two questions, though:

1) Why do we need the second ifdef given that the third ifdef prevents sUsingService from ever getting true?

You cannot see it without more context in the diff, but that block contains a call to DoesBinaryMatchAllowedCertificates() which we would not be including in our builds.

2) Looking a bit deeper I wonder why we even need the third ifdef given that MOZ_USING_SERVICE gets only set in workmonitor.cpp which in turn only gets compiled in if we don't do --disable-maintenance-service. Hardening against environment manipulation by some rogue third party program?

Yes, exactly: a little bit of paranoia.

comment:10 Changed 4 years ago by gk

Good. Fine with me.

comment:11 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Mike had the idea to make this a separate commit in order to get that upstreamed. I think this is a good idea (at least we could try it). i adapted your commit accordingly. It is now ac15e62dc45841a3a21f091fcfa3dc0e455ed637 on tor-browser-38.3.0esr-5.0-2 and a2ebb7e7bf88433bfe056ab543b697dd431d3343 on tor-browser-38.3.0esr-5.5-2.

Oh, and sorry for my first question. I don't review code with just the snippets my browser shows me but with the whole files in front of me. (This is where the "This looks good to me." came from) But then I took a final glance at the diff and came up with question 2) and then thought looking further at the diff what about 1) :/ Seems I shouldn't try doing things in parallel while reviewing code.

Note: See TracTickets for help on using tickets.