973933 - Tracking: Multi platform MAR signing and verification

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Comment 1

[Brian R. Bondy [:bbondy]]

I rebased all of the patches for multi platform mar verification and verified it builds on OSX.
I pushed to Oak as well.  If I recall correctly the MAR files need to be re-signed with the dep cert.
So some tests on Oak will probably fail until I do that.
https://tbpl.mozilla.org/?tree=Oak&rev=952187a5492f

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1: Bug 903135 - Updates to libmar needed to support B2G MAR signature verification. r=bbondy

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2: Bug 903135 - Multi platform MAR verification build config. r=rstrong

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3: Bug 903135 - Multi platform MAR verification updater support. r=rstrong

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch 4: Bug 903126 - Don't use an xpcshell cert for verification. r=rstrong

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Patch 5: Bug 903126 - Replace DER file with XPCShell cert. r=rstrong

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Patch 6: Bug 991993: Disable NSS for updater on OSX and enable native APIs. r=smichaud,rstrong

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work

To make things easier as we get closer to landing this, I rebased all of the patches from the various bugs and included them in one place here.  

Patch 7 is a rollup of recent work. Changes include:
- Creating an xpcshell only updater binary. This binary has an embedded xpcshell only cert for verifying test only mars.  It is only used by tests and is not signed w/ authenticode certs.
- Modifying tests to use that new binary
- Adding a check-cert option to the maintenance service
- Using that new cert-check option in new tests to test the authenticode path
- No longer doing an authenticode check during service updater tests on the xpcshell binary.
- Enables more tests for other platforms
 

Outside of this patch, I'll separately make a "patch 8" which disables linux for initial landing. There's also mochitest other failures currently, I think I recall you saying you wanted to disable them but please clarify on that as well.  See that here: https://tbpl.mozilla.org/?tree=Oak&rev=01d4e47f6948

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 8560830 [details] [diff] [review]
Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work

diff --git a/toolkit/mozapps/update/updater/Makefile.in b/toolkit/mozapps/update/updater/Makefile.in
--- a/toolkit/mozapps/update/updater/Makefile.in
+++ b/toolkit/mozapps/update/updater/Makefile.in
@@ -15,27 +15,30 @@ include $(topsrcdir)/config/rules.mk
 
 ifneq (,$(filter beta release esr,$(MOZ_UPDATE_CHANNEL)))
 	PRIMARY_CERT = release_primary.der
 	SECONDARY_CERT = release_secondary.der
 else ifneq (,$(filter nightly aurora nightly-elm nightly-profiling nightly-oak nightly-ux,$(MOZ_UPDATE_CHANNEL)))
 	PRIMARY_CERT = nightly_aurora_level3_primary.der
 	SECONDARY_CERT = nightly_aurora_level3_secondary.der
 else
-	PRIMARY_CERT = xpcshellCertificate.der
-	SECONDARY_CERT = xpcshellCertificate.der
+	PRIMARY_CERT = dep1.der
+	SECONDARY_CERT = dep2.der
 endif
 
-CERT_HEADERS := primaryCert.h secondaryCert.h
+CERT_HEADERS := primaryCert.h secondaryCert.h xpcshellCert.h
 
 export:: $(CERT_HEADERS)
 
 primaryCert.h: $(PRIMARY_CERT)
 secondaryCert.h: $(SECONDARY_CERT)
 
+# ./certutil certutil -L -d modules/libmar/tests/unit/data -n mycert -r > xpcshellCertificate.der
Please provide more info as to what this is for or remove it. Also, certutil doesn't have a certutil argument e.g. "./certutil certutil"

+xpcshellCert.h: xpcshellCertificate.der
+
 $(CERT_HEADERS): gen_cert_header.py
 	$(PYTHON) $< $(@:.h=Data) $(filter-out $<,$^) > $@
 
 ifdef MOZ_WIDGET_GTK
 libs:: updater.png
 	$(NSINSTALL) -D $(DIST)/bin/icons
 	$(INSTALL) $(IFLAGS1) $^ $(DIST)/bin/icons
 endif

diff --git a/toolkit/mozapps/update/tests/unit_service_updater/checkUpdaterSigSvc.js b/toolkit/mozapps/update/tests/unit_service_updater/checkUpdaterSigSvc.js
new file mode 100644
--- /dev/null
+++ b/toolkit/mozapps/update/tests/unit_service_updater/checkUpdaterSigSvc.js
@@ -0,0 +1,40 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+
+/**
+ * We skip authenticode cert checks from the service udpates
+ * so that we can use updater-xpcshell with the wrong certs for testing.
+ * This tests that code path.  */
+
+function run_test() {
+  if (!shouldRunServiceTest(true)) {
+    return;
+  }
This doesn't work as intended. shouldRunServiceTest returns true if DISABLE_UPDATER_AUTHENTICODE_CHECK is defined so this test fails when the binaries aren't signed.

Please use a common .build file that is included by the moz.build file similar to the following:
diff --git a/toolkit/mozapps/update/updater/moz.build b/toolkit/mozapps/update/updater/moz.build
--- a/toolkit/mozapps/update/updater/moz.build
+++ b/toolkit/mozapps/update/updater/moz.build
@@ -1,130 +1,14 @@
 # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
 # vim: set filetype=python:
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 Program('updater')
 
-SOURCES += [
-    'archivereader.cpp',
-    'bspatch.cpp',
-    'updater.cpp',
-]
+DEFINES['UPDATER_REL_PATH'] = ''
 
-have_progressui = 0
-
-if CONFIG['MOZ_VERIFY_MAR_SIGNATURE']:
-    USE_LIBS += [
-        'verifymar',
-    ]
-
-if CONFIG['OS_ARCH'] == 'WINNT':
-    have_progressui = 1
-    SOURCES += [
-        'loaddlls.cpp',
-        'progressui_win.cpp',
-        'win_dirent.cpp',
-    ]
-    RCINCLUDE = 'updater.rc'
-    DEFINES['UNICODE'] = True
-    DEFINES['_UNICODE'] = True
-    DEFINES['NOMINMAX'] = True
-    USE_STATIC_LIBS = True
-
-    # Pick up nsWindowsRestart.cpp
-    LOCAL_INCLUDES += [
-        '/toolkit/xre',
-    ]
-    USE_LIBS += [
-        'updatecommon-standalone',
-    ]
-    OS_LIBS += [
-        'comctl32',
-        'ws2_32',
-        'shell32',
-        'shlwapi',
-        'crypt32',
-        'advapi32',
-    ]
-elif CONFIG['OS_ARCH'] == 'Linux' and CONFIG['MOZ_VERIFY_MAR_SIGNATURE']:
-    USE_LIBS += [
-        '/modules/libmar/sign/signmar',
-        'nss',
-        'updatecommon',
-    ]
-    OS_LIBS += CONFIG['NSPR_LIBS']
-else:
-    USE_LIBS += [
-        'updatecommon',
-    ]
-
-USE_LIBS += [
-    'mar',
-]
-
-if CONFIG['MOZ_NATIVE_BZ2']:
-    OS_LIBS += CONFIG['MOZ_BZ2_LIBS']
-else:
-    USE_LIBS += [
-        'bz2',
-    ]
-
-if CONFIG['MOZ_ENABLE_GTK']:
-    have_progressui = 1
-    SOURCES += [
-        'progressui_gtk.cpp',
-    ]
-
-if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa':
-    have_progressui = 1
-    SOURCES += [
-        'launchchild_osx.mm',
-        'progressui_osx.mm',
-    ]
-    OS_LIBS += ['-framework Cocoa -framework Security']
-elif CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
-    have_progressui = 1
-    SOURCES += [
-        'automounter_gonk.cpp',
-        'progressui_gonk.cpp',
-    ]
-    DISABLE_STL_WRAPPING = True
-    OS_LIBS += [
-        'cutils',
-        'sysutils',
-    ]
-
-if have_progressui == 0:
-    SOURCES += [
-        'progressui_null.cpp',
-    ]
-
-DEFINES['NS_NO_XPCOM'] = True
-DISABLE_STL_WRAPPING = True
-for var in ('MAR_CHANNEL_ID', 'MOZ_APP_VERSION'):
-    DEFINES[var] = '"%s"' % CONFIG[var]
-
-LOCAL_INCLUDES += [
-    '../common',
-    '/xpcom/glue',
-]
-
-DELAYLOAD_DLLS += [
-    'crypt32.dll',
-    'comctl32.dll',
-    'userenv.dll',
-    'wsock32.dll',
-]
-
-if CONFIG['_MSC_VER']:
-    WIN32_EXE_LDFLAGS += ['-ENTRY:wmainCRTStartup']
-elif CONFIG['OS_ARCH'] == 'WINNT':
-    WIN32_EXE_LDFLAGS += ['-municode']
-
-if CONFIG['MOZ_WIDGET_GTK']:
-    CXXFLAGS += CONFIG['TK_CFLAGS']
-    OS_LIBS += CONFIG['TK_LIBS']
+include('updater-common.build')
 
 DIRS += ['updater-xpcshell']
 FAIL_ON_WARNINGS = True
diff --git a/toolkit/mozapps/update/updater/updater-common.build b/toolkit/mozapps/update/updater/updater-common.build
new file mode 100644
--- /dev/null
+++ b/toolkit/mozapps/update/updater/updater-common.build
@@ -0,0 +1,125 @@
+# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
+# vim: set filetype=python:
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+SOURCES += [
+    '%sarchivereader.cpp' % DEFINES['UPDATER_REL_PATH'],
+    '%sbspatch.cpp' % DEFINES['UPDATER_REL_PATH'],
+    '%supdater.cpp' % DEFINES['UPDATER_REL_PATH'],
+]
+
+have_progressui = 0
+
+if CONFIG['MOZ_VERIFY_MAR_SIGNATURE']:
+    USE_LIBS += [
+        'verifymar',
+    ]
+
+if CONFIG['OS_ARCH'] == 'WINNT':
+    have_progressui = 1
+    SOURCES += [
+        '%sloaddlls.cpp' % DEFINES['UPDATER_REL_PATH'],
+        '%sprogressui_win.cpp' % DEFINES['UPDATER_REL_PATH'],
+        '%swin_dirent.cpp' % DEFINES['UPDATER_REL_PATH'],
+    ]
+    RCINCLUDE = '%supdater.rc' % DEFINES['UPDATER_REL_PATH']
+    DEFINES['UNICODE'] = True
+    DEFINES['_UNICODE'] = True
+    DEFINES['NOMINMAX'] = True
+    USE_STATIC_LIBS = True
+
+    # Pick up nsWindowsRestart.cpp
+    LOCAL_INCLUDES += [
+        '/toolkit/xre',
+    ]
+    USE_LIBS += [
+        'updatecommon-standalone',
+    ]
+    OS_LIBS += [
+        'comctl32',
+        'ws2_32',
+        'shell32',
+        'shlwapi',
+        'crypt32',
+        'advapi32',
+    ]
+elif CONFIG['OS_ARCH'] == 'Linux' and CONFIG['MOZ_VERIFY_MAR_SIGNATURE']:
+    USE_LIBS += [
+        '/modules/libmar/sign/signmar',
+        'nss',
+        'updatecommon',
+    ]
+    OS_LIBS += CONFIG['NSPR_LIBS']
+else:
+    USE_LIBS += [
+        'updatecommon',
+    ]
+
+USE_LIBS += [
+    'mar',
+]
+
+if CONFIG['MOZ_NATIVE_BZ2']:
+    OS_LIBS += CONFIG['MOZ_BZ2_LIBS']
+else:
+    USE_LIBS += [
+        'bz2',
+    ]
+
+if CONFIG['MOZ_ENABLE_GTK']:
+    have_progressui = 1
+    SOURCES += [
+        '%sprogressui_gtk.cpp' % DEFINES['UPDATER_REL_PATH'],
+    ]
+
+if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa':
+    have_progressui = 1
+    SOURCES += [
+        '%slaunchchild_osx.mm' % DEFINES['UPDATER_REL_PATH'],
+        '%sprogressui_osx.mm' % DEFINES['UPDATER_REL_PATH'],
+    ]
+    OS_LIBS += ['-framework Cocoa -framework Security']
+elif CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
+    have_progressui = 1
+    SOURCES += [
+        '%sautomounter_gonk.cpp' % DEFINES['UPDATER_REL_PATH'],
+        '%sprogressui_gonk.cpp' % DEFINES['UPDATER_REL_PATH'],
+    ]
+    DISABLE_STL_WRAPPING = True
+    OS_LIBS += [
+        'cutils',
+        'sysutils',
+    ]
+
+if have_progressui == 0:
+    SOURCES += [
+        '%sprogressui_null.cpp' % DEFINES['UPDATER_REL_PATH'],
+    ]
+
+DEFINES['NS_NO_XPCOM'] = True
+DISABLE_STL_WRAPPING = True
+for var in ('MAR_CHANNEL_ID', 'MOZ_APP_VERSION'):
+    DEFINES[var] = '"%s"' % CONFIG[var]
+
+LOCAL_INCLUDES += [
+    '/toolkit/mozapps/update/common',
+    '/xpcom/glue',
+]
+
+DELAYLOAD_DLLS += [
+    'crypt32.dll',
+    'comctl32.dll',
+    'userenv.dll',
+    'wsock32.dll',
+]
+
+if CONFIG['_MSC_VER']:
+    WIN32_EXE_LDFLAGS += ['-ENTRY:wmainCRTStartup']
+elif CONFIG['OS_ARCH'] == 'WINNT':
+    WIN32_EXE_LDFLAGS += ['-municode']
+
+if CONFIG['MOZ_WIDGET_GTK']:
+    CXXFLAGS += CONFIG['TK_CFLAGS']
+    OS_LIBS += CONFIG['TK_LIBS']
diff --git a/toolkit/mozapps/update/updater/updater-xpcshell/moz.build b/toolkit/mozapps/update/updater/updater-xpcshell/moz.build
--- a/toolkit/mozapps/update/updater/updater-xpcshell/moz.build
+++ b/toolkit/mozapps/update/updater/updater-xpcshell/moz.build
@@ -1,127 +1,12 @@
 # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
 # vim: set filetype=python:
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 Program('updater-xpcshell')
 
-SOURCES += [
-    '../archivereader.cpp',
-    '../bspatch.cpp',
-    '../updater.cpp',
-]
+DEFINES['UPDATER_REL_PATH'] = '../'
+DEFINES['UPDATER_XPCSHELL_CERT'] = True
 
-have_progressui = 0
-
-if CONFIG['MOZ_VERIFY_MAR_SIGNATURE']:
-    USE_LIBS += [
-        'verifymar',
-    ]
-    DEFINES['UPDATER_XPCSHELL_CERT'] = True
-
-if CONFIG['OS_ARCH'] == 'WINNT':
-    have_progressui = 1
-    SOURCES += [
-        '../loaddlls.cpp',
-        '../progressui_win.cpp',
-        '../win_dirent.cpp',
-    ]
-    RCINCLUDE = 'updater.rc'
-    DEFINES['UNICODE'] = True
-    DEFINES['_UNICODE'] = True
-    DEFINES['NOMINMAX'] = True
-    USE_STATIC_LIBS = True
-
-    # Pick up nsWindowsRestart.cpp
-    LOCAL_INCLUDES += [
-        '/toolkit/xre',
-    ]
-    USE_LIBS += [
-        'updatecommon-standalone',
-    ]
-    OS_LIBS += [
-        'comctl32',
-        'ws2_32',
-        'shell32',
-        'shlwapi',
-        'crypt32',
-        'advapi32',
-    ]
-elif CONFIG['OS_ARCH'] == 'Linux' and CONFIG['MOZ_VERIFY_MAR_SIGNATURE']:
-    USE_LIBS += [
-        '/modules/libmar/sign/signmar',
-        'nss',
-        'updatecommon',
-    ]
-    OS_LIBS += CONFIG['NSPR_LIBS']
-else:
-    USE_LIBS += [
-        'updatecommon',
-    ]
-
-USE_LIBS += [
-    'mar',
-]
-
-if CONFIG['MOZ_NATIVE_BZ2']:
-    OS_LIBS += CONFIG['MOZ_BZ2_LIBS']
-else:
-    USE_LIBS += [
-        'bz2',
-    ]
-
-if CONFIG['MOZ_ENABLE_GTK']:
-    have_progressui = 1
-    SOURCES += [
-        '../progressui_gtk.cpp',
-    ]
-
-if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa':
-    have_progressui = 1
-    SOURCES += [
-        '../launchchild_osx.mm',
-        '../progressui_osx.mm',
-    ]
-    OS_LIBS += ['-framework Cocoa -framework Security']
-elif CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
-    have_progressui = 1
-    SOURCES += [
-        '../automounter_gonk.cpp',
-        '../progressui_gonk.cpp',
-    ]
-    DISABLE_STL_WRAPPING = True
-    OS_LIBS += [
-        'cutils',
-        'sysutils',
-    ]
-
-if have_progressui == 0:
-    SOURCES += [
-        '../progressui_null.cpp',
-    ]
-
-DEFINES['NS_NO_XPCOM'] = True
-DISABLE_STL_WRAPPING = True
-for var in ('MAR_CHANNEL_ID', 'MOZ_APP_VERSION'):
-    DEFINES[var] = '"%s"' % CONFIG[var]
-
-LOCAL_INCLUDES += [
-    '../../common',
-    '/xpcom/glue',
-]
-
-DELAYLOAD_DLLS += [
-    'crypt32.dll',
-    'userenv.dll',
-    'wsock32.dll',
-]
-
-if CONFIG['_MSC_VER']:
-    WIN32_EXE_LDFLAGS += ['-ENTRY:wmainCRTStartup']
-elif CONFIG['OS_ARCH'] == 'WINNT':
-    WIN32_EXE_LDFLAGS += ['-municode']
-
-if CONFIG['MOZ_WIDGET_GTK']:
-    CXXFLAGS += CONFIG['TK_CFLAGS']
-    OS_LIBS += CONFIG['TK_LIBS']
+include('../updater-common.build')
diff --git a/toolkit/mozapps/update/updater/updater-xpcshell/updater.rc b/toolkit/mozapps/update/updater/updater-xpcshell/updater.rc
deleted file mode 100644
--- a/toolkit/mozapps/update/updater/updater-xpcshell/updater.rc
+++ /dev/null
@@ -1,126 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-// Microsoft Visual C++ generated resource script.
-//
-#include "../resource.h"
-
-#define APSTUDIO_READONLY_SYMBOLS
-/////////////////////////////////////////////////////////////////////////////
-//
-// Generated from the TEXTINCLUDE 2 resource.
-//
-#include "winresrc.h"
-
-/////////////////////////////////////////////////////////////////////////////
-#undef APSTUDIO_READONLY_SYMBOLS
-
-/////////////////////////////////////////////////////////////////////////////
-// English (U.S.) resources
-
-#if !defined(AFX_RESOURCE_DLL) || defined(AFX_TARG_ENU)
-#ifdef _WIN32
-LANGUAGE LANG_ENGLISH, SUBLANG_ENGLISH_US
-#pragma code_page(1252)
-#endif //_WIN32
-
-/////////////////////////////////////////////////////////////////////////////
-//
-// RT_MANIFEST
-//
-
-1                       RT_MANIFEST             "../updater.exe.manifest"
-
-/////////////////////////////////////////////////////////////////////////////
-//
-// Icon
-//
-
-IDI_DIALOG ICON "../updater.ico"
-
-
-/////////////////////////////////////////////////////////////////////////////
-//
-// Embedded an identifier to uniquely identiy this as a Mozilla updater.
-//
-
-STRINGTABLE
-{
-  IDS_UPDATER_IDENTITY, "moz-updater.exe-4cdccec4-5ee0-4a06-9817-4cd899a9db49"
-} 
-
-
-/////////////////////////////////////////////////////////////////////////////
-//
-// Dialog
-//
-
-IDD_DIALOG DIALOGEX 0, 0, 253, 41
-STYLE DS_SETFONT | DS_MODALFRAME | DS_FIXEDSYS | WS_POPUP | WS_CAPTION
-FONT 8, "MS Shell Dlg", 400, 0, 0x1
-BEGIN
-    CONTROL         "",IDC_PROGRESS,"msctls_progress32",WS_BORDER,7,24,239,10
-    LTEXT           "",IDC_INFO,7,8,239,13,SS_NOPREFIX
-END
-
-
-/////////////////////////////////////////////////////////////////////////////
-//
-// DESIGNINFO
-//
-
-#ifdef APSTUDIO_INVOKED
-GUIDELINES DESIGNINFO 
-BEGIN
-    IDD_DIALOG, DIALOG
-    BEGIN
-        LEFTMARGIN, 7
-        RIGHTMARGIN, 246
-        TOPMARGIN, 7
-        BOTTOMMARGIN, 39
-    END
-END
-#endif    // APSTUDIO_INVOKED
-
-
-#ifdef APSTUDIO_INVOKED
-/////////////////////////////////////////////////////////////////////////////
-//
-// TEXTINCLUDE
-//
-
-1 TEXTINCLUDE 
-BEGIN
-    "../resource.h\0"
-END
-
-2 TEXTINCLUDE 
-BEGIN
-    "#include ""winresrc.h""\r\n"
-    "\0"
-END
-
-3 TEXTINCLUDE 
-BEGIN
-    "\r\n"
-    "\0"
-END
-
-#endif    // APSTUDIO_INVOKED
-
-#endif    // English (U.S.) resources
-/////////////////////////////////////////////////////////////////////////////
-
-
-
-#ifndef APSTUDIO_INVOKED
-/////////////////////////////////////////////////////////////////////////////
-//
-// Generated from the TEXTINCLUDE 3 resource.
-//
-
-
-/////////////////////////////////////////////////////////////////////////////
-#endif    // not APSTUDIO_INVOKED
-

I'd like to look at this one more time.

Comment 11

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #10)
> Comment on attachment 8560830 [details] [diff] [review]
> Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work
>...
> Please use a common .build file that is included by the moz.build file
> similar to the following:
> diff --git a/toolkit/mozapps/update/updater/moz.build
> b/toolkit/mozapps/update/updater/moz.build
> --- a/toolkit/mozapps/update/updater/moz.build
> +++ b/toolkit/mozapps/update/updater/moz.build
> @@ -1,130 +1,14 @@
>  # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40
> -*-
>  # vim: set filetype=python:
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  Program('updater')
>  
> -SOURCES += [
> -    'archivereader.cpp',
> -    'bspatch.cpp',
> -    'updater.cpp',
> -]
> +DEFINES['UPDATER_REL_PATH'] = ''
Even cleaner...
updater_rel_path = ''

and
updater_rel_path = '../'

then just
% updater_rel_path

instead of
% DEFINES['UPDATER_REL_PATH']

Comment 12

[Robert Strong (they/them - no direct email)]

For the mochitest-chrome tests we already backup the update-settings.ini and put it back after each test. You should be able to do the same for the updater in the staging tests so they still pass.

Comment 13

[Robert Strong (they/them - no direct email)]

Also, please get the build changes reviewed by a build peer.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev2

Review comments implemented. I had to do some changes for the rc file as well because it wasn't working without that with the rel path stuff.

2 more patches on their way which I'll attach as new patches:
i) mochitest chrome test failing fix (this is in progress)
ii) Disable on linux

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 8564770 [details] [diff] [review]
Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev2

Note: it looks like the patch isn't against tip since it doesn't contain
IDR_COMCTL32_MANIFEST   RT_MANIFEST             "updater.exe.comctl32.manifest"

Comment 16

[Brian R. Bondy [:bbondy]]

For the mochitest-chrome are you ok with using the updater binary from the xpcshell dir, or should I be copying it into the chrome test data section as well?

Comment 17

[Brian R. Bondy [:bbondy]]

It's about a week old, rebasing to tip now.

Comment 18

[Robert Strong (they/them - no direct email)]

I think it would be less likely to break sometime in the future to put it in the mochitest-chrome data dir.

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev3

Rebased to tip, in particular w/ the .rc conflicts resolved stuff.

Comment 20

[Robert Strong (they/them - no direct email)]

Comment on attachment 8564789 [details] [diff] [review]
Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev3

>diff --git a/toolkit/mozapps/update/tests/unit_service_updater/xpcshell.ini b/toolkit/mozapps/update/tests/unit_service_updater/xpcshell.ini
>--- a/toolkit/mozapps/update/tests/unit_service_updater/xpcshell.ini
>+++ b/toolkit/mozapps/update/tests/unit_service_updater/xpcshell.ini
>@@ -76,8 +76,10 @@ run-sequentially = Uses the Mozilla Main
> [marAppApplyDirLockedStageFailureSvc_win.js]
> run-sequentially = Uses the Mozilla Maintenance Service.
> [marAppApplyUpdateAppBinInUseStageSuccessSvc_win.js]
> run-sequentially = Uses the Mozilla Maintenance Service.
> [marAppApplyUpdateSuccessSvc.js]
> run-sequentially = Uses the Mozilla Maintenance Service.
> [marAppApplyUpdateStageSuccessSvc.js]
> run-sequentially = Uses the Mozilla Maintenance Service.
>+[checkUpdaterSigSvc.js]
>+run-sequentially = Uses the Mozilla Maintenance Service.
Does this really have to run sequentially?

Comment 21

[Robert Strong (they/them - no direct email)]

Comment on attachment 8564789 [details] [diff] [review]
Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev3

>diff --git a/toolkit/mozapps/update/updater/moz.build b/toolkit/mozapps/update/updater/moz.build
>--- a/toolkit/mozapps/update/updater/moz.build
>+++ b/toolkit/mozapps/update/updater/moz.build
>@@ -1,130 +1,14 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> Program('updater')
> 
>-SOURCES += [
>-    'archivereader.cpp',
>-    'bspatch.cpp',
>-    'updater.cpp',
>-]
>+# The define is for includes from updater.rc in particular
nit: s/ in particular//


>diff --git a/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in b/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in
>@@ -0,0 +1,48 @@
>+# vim:set ts=8 sw=8 sts=8 noet:
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+XPCSHELLTESTROOT = $(abspath $(DEPTH))/_tests/xpcshell/toolkit/mozapps/update/tests
>+
>+ifndef MOZ_PROFILE_GENERATE
>+ifdef COMPILE_ENVIRONMENT
>+INSTALL_TARGETS              += xpcshell-updater
nit: indentation

>+xpcshell-updater_TARGET  := libs
>+xpcshell-updater_DEST    := $(XPCSHELLTESTROOT)/data
>+xpcshell-updater_FILES   := $(DIST)/bin/updater-xpcshell$(BIN_SUFFIX)
>+endif
>+endif # Not MOZ_PROFILE_GENERATE
>+
>+include $(topsrcdir)/config/rules.mk
>+
>+ifndef MOZ_WINCONSOLE
>+ifdef MOZ_DEBUG
>+MOZ_WINCONSOLE = 1
>+else
>+MOZ_WINCONSOLE = 0
>+endif
>+endif
>+
>+ifdef MOZ_WIDGET_GTK
>+libs:: ../updater.png
>+	$(NSINSTALL) -D $(DIST)/bin/icons
>+	$(INSTALL) $(IFLAGS1) $^ $(DIST)/bin/icons
>+endif
>+
>+libs::
>+ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
>+	$(NSINSTALL) -D $(XPCSHELLTESTROOT)/data/updater-xpcshell.app
>+	rsync -a -C --exclude '*.in' $(srcdir)/../macbuild/Contents $(XPCSHELLTESTROOT)/data/updater-xpcshell.app
>+	sed -e 's/%APP_NAME%/$(MOZ_APP_DISPLAYNAME)/' $(srcdir)/../macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in | \
>+	  iconv -f UTF-8 -t UTF-16 > $(XPCSHELLTESTROOT)/data/updater-xpcshell.app/Contents/Resources/English.lproj/InfoPlist.strings
>+	$(NSINSTALL) -D $(XPCSHELLTESTROOT)/data/updater-xpcshell.app/Contents/MacOS/updater-xpcshell
>+	$(NSINSTALL) $(XPCSHELLTESTROOT)/data/updater-xpcshell $(XPCSHELLTESTROOT)/data/updater-xpcshell.app/Contents/MacOS
>+	rm -f $(XPCSHELLTESTROOT)/data/updater-xpcshell
>+	mv $(XPCSHELLTESTROOT)/data/updater-xpcshell.app $(XPCSHELLTESTROOT)/data/updater.app
>+	mv $(XPCSHELLTESTROOT)/data/updater.app/Contents/MacOS/updater-xpcshell $(XPCSHELLTESTROOT)/data/updater.app/Contents/MacOS/updater
>+else
>+	mv $(XPCSHELLTESTROOT)/data/updater-xpcshell$(BIN_SUFFIX) $(XPCSHELLTESTROOT)/data/updater$(BIN_SUFFIX)
>+endif
>+
>+CXXFLAGS += $(MOZ_BZ2_CFLAGS)
Ideally the common code would be included by the Makefile.in's though I doubt it would work out well. Please ask the build peer that reviews the build changes if there is a way to consolidate the common code and if there isn't add a comment about keeping the two Makefile.in's in sync with each other.

>diff --git a/toolkit/mozapps/update/updater/updater-xpcshell/moz.build b/toolkit/mozapps/update/updater/updater-xpcshell/moz.build
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/update/updater/updater-xpcshell/moz.build
>@@ -0,0 +1,12 @@
>+# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
>+# vim: set filetype=python:
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+Program('updater-xpcshell')
>+
>+# The define is for includes from updater.rc in particular
nit: s/ in particular//

>diff --git a/toolkit/mozapps/update/updater/updater.rc b/toolkit/mozapps/update/updater/updater.rc
>--- a/toolkit/mozapps/update/updater/updater.rc
>+++ b/toolkit/mozapps/update/updater/updater.rc
>@@ -1,15 +1,25 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> // Microsoft Visual C++ generated resource script.
> //
>+#ifdef UPDATER_XPCSHELL_CERT
>+#include "../resource.h"
>+#define MANIFEST_PATH "../updater.exe.manifest"
>+#define CTL32_MANIFEST_PATH "../updater.exe.comctl32.manifest"
>+#define ICON_PATH "../updater.ico"
>+#else
> #include "resource.h"
>+#define MANIFEST_PATH "updater.exe.manifest"
>+#define CTL32_MANIFEST_PATH "updater.exe.comctl32.manifest"
>+#define ICON_PATH "updater.ico"
>+#endif
> 
> #define APSTUDIO_READONLY_SYMBOLS
> /////////////////////////////////////////////////////////////////////////////
> //
> // Generated from the TEXTINCLUDE 2 resource.
> //
> #include "winresrc.h"
> 
>@@ -25,25 +35,25 @@ LANGUAGE LANG_ENGLISH, SUBLANG_ENGLISH_U
> #pragma code_page(1252)
> #endif //_WIN32
> 
> /////////////////////////////////////////////////////////////////////////////
> //
> // RT_MANIFEST
> //
> 
>-1                       RT_MANIFEST             "updater.exe.manifest"
>-IDR_COMCTL32_MANIFEST   RT_MANIFEST             "updater.exe.comctl32.manifest"
>+1                       RT_MANIFEST             MANIFEST_PATH
>+IDR_COMCTL32_MANIFEST   RT_MANIFEST             CTL32_MANIFEST_PATH
nit: just use COMCTL32_MANIFEST_PATH here and elsewhere

Comment 22

[Robert Strong (they/them - no direct email)]

>diff --git a/toolkit/components/maintenanceservice/registrycertificates.cpp b/toolkit/components/maintenanceservice/registrycertificates.cpp
>--- a/toolkit/components/maintenanceservice/registrycertificates.cpp
>+++ b/toolkit/components/maintenanceservice/registrycertificates.cpp
>@@ -12,20 +12,26 @@
> #include "servicebase.h"
> #include "updatehelper.h"
> #define MAX_KEY_LENGTH 255
> 
> /**
>  * Verifies if the file path matches any certificate stored in the registry.
>  *
>  * @param  filePath The file path of the application to check if allowed.
>+ * @param  allowFallbackKeySkip TRUE if the Fallback key can be used to
>+ *   skip the cert check.  Since it is stored in a secured location, and cannot
>+ *   be created / tampered with by a low integrity process, this is the default.
>+ *   The maintenance service binary can be used to do checks directly or can be
>+ *   used by tests.
How about?
>+ * @param  allowFallbackKeySkip when this is TRUE the fallback registry key will
>+ *   be used to skip the certificate check.  This is the default since the
>+ *   fallback registry key is located under HKEY_LOCAL_MACHINE which can't be
>+ *   written to by a low integrity process.
>+ *   Note: the maintenance service binary can be used to perform this check for
>+ *   testing or troubleshooting.


>  * @return TRUE if the binary matches any of the allowed certificates.
>  */
> BOOL
>-DoesBinaryMatchAllowedCertificates(LPCWSTR basePathForUpdate, LPCWSTR filePath)
>+DoesBinaryMatchAllowedCertificates(LPCWSTR basePathForUpdate, LPCWSTR filePath,
>+                                   BOOL allowFallbackKeySkip)
> { 
>   WCHAR maintenanceServiceKey[MAX_PATH + 1];
>   if (!CalculateRegistryPathFromFilePath(basePathForUpdate, 
>                                          maintenanceServiceKey)) {
>     return FALSE;
>   }
> 
>   // We use KEY_WOW64_64KEY to always force 64-bit view.
>@@ -44,16 +50,20 @@ DoesBinaryMatchAllowedCertificates(LPCWS
>     // We use this registry key on our test slaves to store the 
>     // allowed name/issuers.
>     retCode = RegOpenKeyExW(HKEY_LOCAL_MACHINE, 
>                             TEST_ONLY_FALLBACK_KEY_PATH, 0,
>                             KEY_READ | KEY_WOW64_64KEY, &baseKeyRaw);
>     if (retCode != ERROR_SUCCESS) {
>       LOG_WARN(("Could not open fallback key.  (%d)", retCode));
>       return FALSE;
>+    } else if (allowFallbackKeySkip) {
>+      LOG_WARN(("Fallback key present, skipping authenticode "
>+                "check and cert check."));
nit: what is the difference between an authenticode check and a cert check? Also, s/cert/certificate/ as elsewhere (mostly).

Comment 23

[Robert Strong (they/them - no direct email)]

>diff --git a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>--- a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>+++ b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js

>@@ -1243,19 +1249,19 @@ function getTestDirPath() {
>  *
>  * @param   aRelPath (optional)
>  *          The relative path to the file or directory to get from the root of
>  *          the test's data directory. If not specified the test's data
>  *          directory will be returned.
>  * @return  The nsIFile for the file in the test data directory.
>  * @throws  If the file or directory does not exist.
>  */
>-function getTestDirFile(aRelPath) {
>+function getTestDirFile(aRelPath, allowNonExists) {
nit: s/allowNonExists/aAllowNonExists/

Add @param to coment for aAllowNonExists

>   let relpath = getTestDirPath() + (aRelPath ? aRelPath : "");
>-  return do_get_file(relpath, false);
>+  return do_get_file(relpath, !!allowNonExists);
> }
> 
> #ifdef XP_WIN
> function getSpecialFolderDir(aCSIDL) {
>   AUS_Cu.import("resource://gre/modules/ctypes.jsm");
>   let lib = ctypes.open("shell32");
>   let SHGetSpecialFolderPath = lib.declare("SHGetSpecialFolderPathW",
>                                            ctypes.winapi_abi,

>@@ -1771,19 +1775,34 @@ function setupAppFiles() {
>     hasMore = istream.readLine(line);
>     appFiles.push( { relPath  : line.value,
>                      inGreDir : false } );
>   } while(hasMore);
> 
>   istream.close();
> 
>   appFiles.forEach(function CMAF_FLN_FE(aAppFile) {
>-    copyFileToTestAppDir(aAppFile.relPath, aAppFile.inGreDir);
>+    // We use the updater binary after this loop from the data dir instead
>+    // because it has the xpcshell certs.
>+    if (aAppFile.relPath !== FILE_UPDATER_BIN) {
>+        copyFileToTestAppDir(aAppFile.relPath, aAppFile.inGreDir);
nit: indentation

>+    }
>   });
> 
>+  // Copy the xpcshell updater binary
>+  let updater = getTestDirFile("updater.app", true);
>+  if (!updater.exists()) {
>+    updater = getTestDirFile(FILE_UPDATER_BIN);
>+    if (!updater.exists()) {
>+      do_throw("Unable to find updater binary!");
>+    }
>+  }
>+  let outputBinDir = getGREBinDir()
How about just binDir?

Since you are always copying the updater remove it from the list and remove the |if (aAppFile.relPath !== FILE_UPDATER_BIN) {| check above when copying the appFiles.

>+  updater.copyToFollowingLinks(outputBinDir, updater.leafName);

>@@ -2093,20 +2112,24 @@ function runUpdateUsingService(aInitialS
> 
>   if (gSwitchApp) {
>     // We want to set the env vars again
>     gShouldResetEnv = undefined;
>   }
> 
>   setEnvironment();
> 
>-  // There is a security check done by the service to make sure the updater
>-  // we are executing is the same as the one in the apply-to dir.
>-  // To make sure they match from tests we copy updater.exe to the apply-to dir.
>-  copyFileToTestAppDir(FILE_UPDATER_BIN, false);
>+
>+  let updater = getTestDirFile(FILE_UPDATER_BIN);
>+  if (!updater.exists()) {
>+    do_throw("Unable to find updater binary!");
>+  }
>+  let outputBinDir = getGREBinDir()
How about just binDir?

If not, there are other cases where binDir is used and perhaps changing this to testBinDir as well as the other cases.

>+  updater.copyToFollowingLinks(outputBinDir, updater.leafName);
>+  updater.copyToFollowingLinks(updatesDir, updater.leafName);

Comment 24

[Robert Strong (they/them - no direct email)]

Comment on attachment 8564789 [details] [diff] [review]
Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev3

>diff --git a/toolkit/mozapps/update/tests/unit_base_updater/xpcshell.ini b/toolkit/mozapps/update/tests/unit_base_updater/xpcshell.ini
>--- a/toolkit/mozapps/update/tests/unit_base_updater/xpcshell.ini
>+++ b/toolkit/mozapps/update/tests/unit_base_updater/xpcshell.ini
>@@ -15,19 +15,19 @@ generated-files = head_update.js
> [marSuccessComplete.js]
> [marSuccessPartial.js]
> [marFailurePartial.js]
> [marStageSuccessComplete.js]
> skip-if = toolkit == 'gonk'
> reason = bug 820380
> [marStageSuccessPartial.js]
> [marVersionDowngrade.js]
>-run-if = os == 'win'
>+run-if = os == 'win' || os == 'mac' || os == 'linux'
> [marWrongChannel.js]
>-run-if = os == 'win'
>+run-if = os == 'win' || os == 'mac' || os == 'linux'
Will these tests be disabled when you disable Linux?

> [marStageFailurePartial.js]
> [marCallbackAppSuccessComplete_win.js]
> run-if = os == 'win'
> [marCallbackAppSuccessPartial_win.js]
> run-if = os == 'win'
> [marCallbackAppStageSuccessComplete_win.js]
> run-if = os == 'win'
> [marCallbackAppStageSuccessPartial_win.js]
>diff --git a/toolkit/mozapps/update/tests/unit_service_updater/checkUpdaterSigSvc.js b/toolkit/mozapps/update/tests/unit_service_updater/checkUpdaterSigSvc.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/update/tests/unit_service_updater/checkUpdaterSigSvc.js
>@@ -0,0 +1,40 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+/**
>+ * We skip authenticode cert checks from the service udpates
>+ * so that we can use updater-xpcshell with the wrong certs for testing.
>+ * This tests that code path.  */
>+
>+function run_test() {
>+  if (UPDATER_AUTHENTICODE_CHECK_DISABLED) {
>+    return;
>+  }
>+
>+  let binDir = getGREBinDir();
>+  let maintenanceServiceBin = binDir.clone();
>+  maintenanceServiceBin.append(FILE_MAINTENANCE_SERVICE_BIN);
>+
>+  let updaterBin = binDir.clone();
>+  updaterBin.append(FILE_UPDATER_BIN);
>+
>+  logTestInfo("Launching maintenance service bin: " +
>+              maintenanceServiceBin.path + " for updater: " +
>+              updaterBin.path + " to check signature...");
nit: 
"Launching maintenance service bin: " +
 maintenanceServiceBin.path + " to check updater: " +
 updaterBin.path + " signature.");

r=me with comments addressed.

Comment 25

[Robert Strong (they/them - no direct email)]

Be sure to get a build peer to review the build changes as well

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8: Fix mochitest chrome updater tests. rev1

This backs up the old updater binary, saves the test only one in from the data dir, and then restores it after each test.

Comment 27

[Brian R. Bondy [:bbondy]]

Thanks for all the review comments!

> Does this really have to run sequentially?
I think that's best since the other files in the dir do and other tests replace the maintenance service bin that this uses.


> nit: what is the difference between an authenticode check and a cert check? Also, s/cert/certificate/ 
> as elsewhere (mostly).

Authenticode is verifying the windows signature, cert check is checking against attributes in the registry. I'll improve on this comment.

> Re: Get a build peer

Yep I'll get a rollup of all build changes and pass it through a build peer after everything gets r+ed.  Will wait on this until the end so I can just do one ping. 

> Will these tests be disabled when you disable Linux?

Yes I'll just have another patch called patch 9 that does this and other changes like that.

Comment 28

[Brian R. Bondy [:bbondy]]

Attachment: Patch 9: Temporarily disable Linux

I was originally thinking there would be more changes for this to disable on Linux, but I think this is it because of previous refactoring around MOZ_VERIFY_MAR_SIGNATURE.

Comment 29

[Robert Strong (they/them - no direct email)]

The other tests run sequentially due to using the service which only supports running one update at a time and this uses the binary directly and it isn't the binary in the maintenance service directory. Go ahead and try running it in parallel.

Comment 30

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev4

Implemented review comments on patch 7.

Comment 31

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8: Fix mochitest chrome updater tests. rev2

Rebased patch 8 for review comments in patch 7.

Comment 32

[Brian R. Bondy [:bbondy]]

Attachment: Patch 9: Temporarily disable Linux. rev2

Included extra build problem fix in archivereader.cpp for when MOZ_VERIFY_MAR_SIGNATURE is not defined.

Comment 33

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8: Fix mochitest chrome updater tests. rev3

Teak to fix the cleanup test

Comment 34

[Brian R. Bondy [:bbondy]]

Attachment: Patch 9: Temporarily disable Linux. rev2

Just re-attaching same patch 9 for proper ordering.

Comment 35

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8: Fix mochitest chrome updater tests. rev4

win binary copy fix.

Comment 36

[Brian R. Bondy [:bbondy]]

Attachment: Patch 9: Temporarily disable Linux. rev2

Comment 37

[Brian R. Bondy [:bbondy]]

Attachment: Patch 9: Temporarily disable Linux. rev3

Ugh ifdefs are hard sometimes ;)

Comment 38

[Robert Strong (they/them - no direct email)]

Comment on attachment 8565006 [details] [diff] [review]
Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev4

>diff --git a/toolkit/components/maintenanceservice/registrycertificates.cpp b/toolkit/components/maintenanceservice/registrycertificates.cpp
>--- a/toolkit/components/maintenanceservice/registrycertificates.cpp
>+++ b/toolkit/components/maintenanceservice/registrycertificates.cpp
>@@ -12,20 +12,27 @@
> #include "servicebase.h"
> #include "updatehelper.h"
> #define MAX_KEY_LENGTH 255
> 
> /**
>  * Verifies if the file path matches any certificate stored in the registry.
>  *
>  * @param  filePath The file path of the application to check if allowed.
>+ * @param  allowFallbackKeySkip when this is TRUE the fallback registry key will
>+ *   be used to skip the certificate check.  This is the default since the
>+ *   fallback registry key is located under HKEY_LOCAL_MACHINE which can't be
>+ *   written to by a low integrity process.
>+ *   Note: the maintenance service binary can be used to perform this check for
>+ *   testing or troubleshooting.
>  * @return TRUE if the binary matches any of the allowed certificates.
>  */
> BOOL
>-DoesBinaryMatchAllowedCertificates(LPCWSTR basePathForUpdate, LPCWSTR filePath)
>+DoesBinaryMatchAllowedCertificates(LPCWSTR basePathForUpdate, LPCWSTR filePath,
>+                                   BOOL allowFallbackKeySkip)
> { 
>   WCHAR maintenanceServiceKey[MAX_PATH + 1];
>   if (!CalculateRegistryPathFromFilePath(basePathForUpdate, 
>                                          maintenanceServiceKey)) {
>     return FALSE;
>   }
> 
>   // We use KEY_WOW64_64KEY to always force 64-bit view.
>@@ -44,16 +51,20 @@ DoesBinaryMatchAllowedCertificates(LPCWS
>     // We use this registry key on our test slaves to store the 
>     // allowed name/issuers.
>     retCode = RegOpenKeyExW(HKEY_LOCAL_MACHINE, 
>                             TEST_ONLY_FALLBACK_KEY_PATH, 0,
>                             KEY_READ | KEY_WOW64_64KEY, &baseKeyRaw);
>     if (retCode != ERROR_SUCCESS) {
>       LOG_WARN(("Could not open fallback key.  (%d)", retCode));
>       return FALSE;
>+    } else if (allowFallbackKeySkip) {
>+      LOG_WARN(("Fallback key present, skipping VerifyCertificateTrustForFile "
>+                "check and the cert attribute registry matching check."));
Per review comment please use certificate instead of cert.

>diff --git a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>--- a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>+++ b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>@@ -1240,22 +1246,25 @@ function getTestDirPath() {
> /**
>  * Helper function for getting the nsIFile for a file in the test data
>  * directory.
>  *
>  * @param   aRelPath (optional)
>  *          The relative path to the file or directory to get from the root of
>  *          the test's data directory. If not specified the test's data
>  *          directory will be returned.
>+ * @param   aAllowNonExists (optional)
>+ *          Whether or not to throw an error if the path exists.
>+ *          If not specified, the false is used.
s/the/then/

>@@ -1771,19 +1778,34 @@ function setupAppFiles() {
>     hasMore = istream.readLine(line);
>     appFiles.push( { relPath  : line.value,
>                      inGreDir : false } );
>   } while(hasMore);
> 
>   istream.close();
> 
>   appFiles.forEach(function CMAF_FLN_FE(aAppFile) {
>-    copyFileToTestAppDir(aAppFile.relPath, aAppFile.inGreDir);
>+    // We use the updater binary after this loop from the data dir instead
>+    // because it has the xpcshell certs.
>+    if (aAppFile.relPath !== FILE_UPDATER_BIN) {
>+      copyFileToTestAppDir(aAppFile.relPath, aAppFile.inGreDir);
>+    }
>   });
Per review comment
Since you are always copying the updater remove it from the list and remove the |if (aAppFile.relPath !== FILE_UPDATER_BIN) {| check above when copying the appFiles.

Please implement the review previous comments and I'll review this again

Comment 39

[Robert Strong (they/them - no direct email)]

Comment on attachment 8565025 [details] [diff] [review]
Patch 8: Fix mochitest chrome updater tests. rev4

>diff --git a/toolkit/mozapps/update/tests/chrome/utils.js b/toolkit/mozapps/update/tests/chrome/utils.js
>--- a/toolkit/mozapps/update/tests/chrome/utils.js
>+++ b/toolkit/mozapps/update/tests/chrome/utils.js
>@@ -190,16 +190,19 @@ var gDocElem;
> var gPrefToCheck;
> var gDisableNoUpdateAddon = false;
> 
> // Set to true to log additional information for debugging. To log additional
> // information for an individual test set DEBUG_AUS_TEST to true in the test's
> // onload function.
> var DEBUG_AUS_TEST = true;
> 
>+const isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes);
>+const BIN_SUFFIX = (isWindows ? ".exe" : "");
>+
> #include ../shared.js
> 
> /**
>  * The current test in TESTS array.
>  */
> this.__defineGetter__("gTest", function() {
>   return TESTS[gTestCounter];
> });
>@@ -859,16 +862,42 @@ function setupFiles() {
>   let updateSettingsIni = baseAppDir.clone();
>   updateSettingsIni.append(FILE_UPDATE_SETTINGS_INI);
>   if (updateSettingsIni.exists()) {
>     updateSettingsIni.moveTo(baseAppDir, FILE_UPDATE_SETTINGS_INI_BAK);
>   }
>   updateSettingsIni = baseAppDir.clone();
>   updateSettingsIni.append(FILE_UPDATE_SETTINGS_INI);
>   writeFile(updateSettingsIni, UPDATE_SETTINGS_CONTENTS);
This should protect against a previous test failing to copy back the original updater by checking if the updater .bak file exists. It should do something along the lines of removing the updater and copying over the test only updater.

>+
>+  // Move away the real updater binary
nit: just "Move away the real updater" since the updater.app isn't a binary

>+  let updaterBin = baseAppDir.clone();
nit: just updater here and elsewhere

>+  updaterBin.appendRelativePath("updater.app");
>+  if (updaterBin.exists()) {
>+    updaterBin.moveTo(baseAppDir, "updater.app.bak");
>+  } else {
>+    let updaterBin = baseAppDir.clone();
>+    updaterBin.appendRelativePath("updater" + BIN_SUFFIX);
>+    updaterBin.moveTo(baseAppDir, "updater" + BIN_SUFFIX + ".bak");
>+  }
>+
>+  // Move in the test only updater binary
nit: copy the test only updater

>+  let testUpdaterBinDir = AUS_Cc["@mozilla.org/file/directory_service;1"].
>+    getService(AUS_Ci.nsIProperties).
>+    get("CurWorkD", AUS_Ci.nsILocalFile);
>+  testUpdaterBinDir.appendRelativePath(REL_PATH_DATA);
>+  let testUpdaterBin = testUpdaterBinDir.clone();
>+  testUpdaterBin.appendRelativePath("updater.app");
>+  if (testUpdaterBin.exists()) {
>+      testUpdaterBin.copyToFollowingLinks(baseAppDir, "updater.app");
nit: indentation

>+  } else {
>+      let testUpdaterBin = testUpdaterBinDir.clone();
>+      testUpdaterBin.appendRelativePath("updater" + BIN_SUFFIX);
>+      testUpdaterBin.copyToFollowingLinks(baseAppDir, "updater" + BIN_SUFFIX);
nit: indentation

>+  }
> }
> 
> /**
>  * Sets the most common preferences used by tests to values used by the majority
>  * of the tests and when necessary saves the preference's original values if
>  * present so they can be set back to the original values when the test has
>  * finished.
>  */
>@@ -942,16 +971,40 @@ function resetFiles() {
>       removeDirRecursive(updatedDir);
>     }
>     catch (e) {
>       dump("Unable to remove directory\n" +
>            "path: " + updatedDir.path + "\n" +
>            "Exception: " + e + "\n");
>     }
>   }
>+
>+  // Remove the temp updater.app
nit: Remove the temp updater

>+  let updaterBin = baseAppDir.clone();
>+  updaterBin.appendRelativePath("updater.app");
>+  if (updaterBin.exists()) {
>+    updaterBin.remove(true);
>+  } else {
>+    let updaterBin = baseAppDir.clone();
>+    updaterBin.appendRelativePath("updater" + BIN_SUFFIX);
>+    updaterBin.remove(true);
>+  }
Why not the following here and elsewhere?
let updaterBin = baseAppDir.clone();
updaterBin.appendRelativePath("updater.app");
if (!updaterBin.exists()) {
  updaterBin = baseAppDir.clone();
  updaterBin.appendRelativePath("updater" + BIN_SUFFIX);
}
updaterBin.remove(true);

>+
>+  // Move away updater.app
nit: Move back the original updater

>+  updaterBin = baseAppDir.clone();
>+  updaterBin.appendRelativePath("updater.app.bak");
>+  if (updaterBin.exists()) {
>+    updaterBin.moveTo(baseAppDir, "updater.app");
>+  } else {
>+    updaterBin = baseAppDir.clone();
>+    updaterBin.appendRelativePath("updater" + BIN_SUFFIX + ".bak");
>+    if (updaterBin.exists()) {
>+      updaterBin.moveTo(baseAppDir, "updater" + BIN_SUFFIX);
>+    }
>+  }
> }

>diff --git a/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in b/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in
>--- a/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in
>+++ b/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in
>@@ -1,16 +1,17 @@
> # vim:set ts=8 sw=8 sts=8 noet:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> # For changes here, also consider ../updater/Makefile.in
> 
> XPCSHELLTESTROOT = $(abspath $(DEPTH))/_tests/xpcshell/toolkit/mozapps/update/tests
>+MOCHITESTROOT = $(abspath $(DEPTH))/_tests/testing/mochitest/chrome/toolkit/mozapps/update/tests
> 
> ifndef MOZ_PROFILE_GENERATE
> ifdef COMPILE_ENVIRONMENT
> INSTALL_TARGETS += xpcshell-updater
> xpcshell-updater_TARGET  := libs
> xpcshell-updater_DEST    := $(XPCSHELLTESTROOT)/data
> xpcshell-updater_FILES   := $(DIST)/bin/updater-xpcshell$(BIN_SUFFIX)
> endif
>@@ -29,22 +30,28 @@ endif
> ifdef MOZ_WIDGET_GTK
> libs:: ../updater.png
> 	$(NSINSTALL) -D $(DIST)/bin/icons
> 	$(INSTALL) $(IFLAGS1) $^ $(DIST)/bin/icons
> endif
> 
> libs::
> ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
>+	# Copy for xpcshell tests
> 	$(NSINSTALL) -D $(XPCSHELLTESTROOT)/data/updater-xpcshell.app
> 	rsync -a -C --exclude '*.in' $(srcdir)/../macbuild/Contents $(XPCSHELLTESTROOT)/data/updater-xpcshell.app
> 	sed -e 's/%APP_NAME%/$(MOZ_APP_DISPLAYNAME)/' $(srcdir)/../macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in | \
> 	  iconv -f UTF-8 -t UTF-16 > $(XPCSHELLTESTROOT)/data/updater-xpcshell.app/Contents/Resources/English.lproj/InfoPlist.strings
> 	$(NSINSTALL) -D $(XPCSHELLTESTROOT)/data/updater-xpcshell.app/Contents/MacOS/updater-xpcshell
> 	$(NSINSTALL) $(XPCSHELLTESTROOT)/data/updater-xpcshell $(XPCSHELLTESTROOT)/data/updater-xpcshell.app/Contents/MacOS
> 	rm -f $(XPCSHELLTESTROOT)/data/updater-xpcshell
>+	rm -Rf $(XPCSHELLTESTROOT)/data/updater.app
> 	mv $(XPCSHELLTESTROOT)/data/updater-xpcshell.app $(XPCSHELLTESTROOT)/data/updater.app
> 	mv $(XPCSHELLTESTROOT)/data/updater.app/Contents/MacOS/updater-xpcshell $(XPCSHELLTESTROOT)/data/updater.app/Contents/MacOS/updater
>+
>+	# Copy for mochitest chrome tests
>+	rsync -a -C $(XPCSHELLTESTROOT)/data/updater.app $(MOCHITESTROOT)/data/updater.app
> else
> 	mv $(XPCSHELLTESTROOT)/data/updater-xpcshell$(BIN_SUFFIX) $(XPCSHELLTESTROOT)/data/updater$(BIN_SUFFIX)
>+	cp $(XPCSHELLTESTROOT)/data/updater$(BIN_SUFFIX) $(MOCHITESTROOT)/data/updater$(BIN_SUFFIX)
> endif
> 
> CXXFLAGS += $(MOZ_BZ2_CFLAGS)

I have seen issues with running mach build for the updater.app a second time a while back. Could you verify that this does the right thing if you run it more than once?

I'd like to look at this one more time.

Comment 40

[Robert Strong (they/them - no direct email)]

Note: you could also used shared.js to store the value for the updater (e.g. updater, updater.exe, and updater.app).

Comment 41

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8: Fix mochitest chrome updater tests. rev5

- Implemented review comments
- Fixed test failure on Windows with updater data rel path

Comment 42

[Robert Strong (they/them - no direct email)]

Comment on attachment 8567777 [details] [diff] [review]
Patch 8: Fix mochitest chrome updater tests. rev5

I've noticed that moving or removing a file and then moving another file to the same location sometimes fails on debug builds and I've had to use timeouts to workaround it. Could you test out whether this is also affected?

Comment 43

[Robert Strong (they/them - no direct email)]

Comment on attachment 8567777 [details] [diff] [review]
Patch 8: Fix mochitest chrome updater tests. rev5

>diff --git a/toolkit/mozapps/update/tests/chrome/utils.js b/toolkit/mozapps/update/tests/chrome/utils.js
>--- a/toolkit/mozapps/update/tests/chrome/utils.js
>+++ b/toolkit/mozapps/update/tests/chrome/utils.js
>@@ -190,16 +190,19 @@ var gDocElem;
> var gPrefToCheck;
> var gDisableNoUpdateAddon = false;
> 
> // Set to true to log additional information for debugging. To log additional
> // information for an individual test set DEBUG_AUS_TEST to true in the test's
> // onload function.
> var DEBUG_AUS_TEST = true;
> 
>+const isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes);
>+const BIN_SUFFIX = (isWindows ? ".exe" : "");
nit: please place this above with the other constants

>@@ -845,30 +848,90 @@ function verifyTestsRan() {
>     let msg = "Checking if TESTS[" + i + "] test was performed... " +
>               "callback function name = " + gCallback.name + ", " +
>               "pageid = " + test.pageid;
>     ok(test.ranTest, msg);
>   }
> }
> 
> /**
>+ * Restore the updater that was backed up.
>+ * This is called both in setupFiles and resetFiles.
>+ * It is called in setupFiles before the backup is done in case the previous
>+ * test failed.
>+ * It is called in resetFiles to put things back to its original state.
nit: just make this a paragraph

>+ */
>+function resetUpdaterBackup() {
>+  // Move back the original updater
>+  let baseAppDir = getAppBaseDir();
>+  let updater = baseAppDir.clone();
>+  updater.appendRelativePath("updater.app.bak");
>+  if (updater.exists()) {
>+    updater.moveTo(baseAppDir, "updater.app");
>+  } else {
>+    updater = baseAppDir.clone();
>+    updater.appendRelativePath("updater" + BIN_SUFFIX + ".bak");
>+    if (updater.exists()) {
>+      updater.moveTo(baseAppDir, "updater" + BIN_SUFFIX);
>+    }
>+  }
>+}
>+
>+/**
>  * Creates a backup of files the tests need to modify so they can be restored to
>  * the original file when the test has finished and then modifies the files.
>  */
> function setupFiles() {
>   // Backup the updater-settings.ini file if it exists by moving it.
>   let baseAppDir = getAppBaseDir();
>   let updateSettingsIni = baseAppDir.clone();
>   updateSettingsIni.append(FILE_UPDATE_SETTINGS_INI);
>   if (updateSettingsIni.exists()) {
>     updateSettingsIni.moveTo(baseAppDir, FILE_UPDATE_SETTINGS_INI_BAK);
>   }
>   updateSettingsIni = baseAppDir.clone();
>   updateSettingsIni.append(FILE_UPDATE_SETTINGS_INI);
>   writeFile(updateSettingsIni, UPDATE_SETTINGS_CONTENTS);
>+
>+  // Just in case the last test failed, try to reset.
>+  resetUpdaterBackup();
>+
>+  // Move away the real updater
>+  let updater = baseAppDir.clone();
>+  updater.appendRelativePath("updater.app");
>+  if (updater.exists()) {
>+    updater.moveTo(baseAppDir, "updater.app.bak");
>+  } else {
>+    updater = baseAppDir.clone();
>+    updater.appendRelativePath("updater" + BIN_SUFFIX);
>+    updater.moveTo(baseAppDir, "updater" + BIN_SUFFIX + ".bak");
>+  }
>+
>+  // Move in the test only updater
>+  let testUpdaterDir = AUS_Cc["@mozilla.org/file/directory_service;1"].
>+    getService(AUS_Ci.nsIProperties).
>+    get("CurWorkD", AUS_Ci.nsILocalFile);
>+
>+  // Fixup the relPath for Windows, nsLocalFileWin's append code will fail with
>+  // front slashes.
>+  var relPath = REL_PATH_DATA;
let

>+  if (isWindows) {
>+    relPath = relPath.replace(/\//g,"\\");
>+  }
>+
>+  testUpdaterDir.appendRelativePath(relPath);
How about just doing
let pathParts = relPath.split("/");
for (let i = 0; i < pathParts.length; ++i) {
  testUpdaterDir.append(pathParts[i]);
}


>+  let testUpdater = testUpdaterDir.clone();
>+  testUpdater.appendRelativePath("updater.app");
>+  if (testUpdater.exists()) {
>+    testUpdater.copyToFollowingLinks(baseAppDir, "updater.app");
>+  } else {
>+    testUpdater = testUpdaterDir.clone();
>+    testUpdater.appendRelativePath("updater" + BIN_SUFFIX);
>+    testUpdater.copyToFollowingLinks(baseAppDir, "updater" + BIN_SUFFIX);
>+  }
> }
> 
> /**
>  * Sets the most common preferences used by tests to values used by the majority
>  * of the tests and when necessary saves the preference's original values if
>  * present so they can be set back to the original values when the test has
>  * finished.
>  */
>@@ -942,16 +1005,29 @@ function resetFiles() {
>       removeDirRecursive(updatedDir);
>     }
>     catch (e) {
>       dump("Unable to remove directory\n" +
>            "path: " + updatedDir.path + "\n" +
>            "Exception: " + e + "\n");
>     }
>   }
>+
>+  // Remove the temp updater
>+  let updater = baseAppDir.clone();
>+  updater.appendRelativePath("updater.app");
>+  if (!updater.exists()) {
>+    updater = baseAppDir.clone();
>+    updater.appendRelativePath("updater" + BIN_SUFFIX);
>+  }
>+  if (updater.exists()) {
>+    updater.remove(true);
>+  }
I believe the above should be in resetUpdaterBackup inside when a updater bin .bak file is found. It is definitely needed for the first call to resetUpdaterBackup on Mac since moveTo will not overwite a non-empty directory.

>+
>+  resetUpdaterBackup();
> }

I'd like to take another look at this and patch 7

Comment 44

[Brian R. Bondy [:bbondy]]

Attachment: Bug 1125699 + misc work on oak - Rollup of recent work - rev5

Implemented review comments.

Comment 45

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8: Fix mochitest chrome updater tests. rev6

Implemented review comments.

Comment 46

[Robert Strong (they/them - no direct email)]

Comment on attachment 8570314 [details] [diff] [review]
Bug 1125699 + misc work on oak - Rollup of recent work - rev5

>diff --git a/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in b/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in
>@@ -0,0 +1,50 @@
>+# vim:set ts=8 sw=8 sts=8 noet:
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+# For changes here, also consider ../updater/Makefile.in
../updater/Makefile.in should be ../Makefile.in

>+
>+XPCSHELLTESTROOT = $(abspath $(DEPTH))/_tests/xpcshell/toolkit/mozapps/update/tests

Comment 47

[Robert Strong (they/them - no direct email)]

Comment on attachment 8570315 [details] [diff] [review]
Patch 8: Fix mochitest chrome updater tests. rev6

>diff --git a/toolkit/mozapps/update/tests/chrome/utils.js b/toolkit/mozapps/update/tests/chrome/utils.js
>--- a/toolkit/mozapps/update/tests/chrome/utils.js
>+++ b/toolkit/mozapps/update/tests/chrome/utils.js
>@@ -157,16 +157,19 @@ const PREF_EM_HOTFIX_ID = "extensions.ho
> const PREF_EM_SILENT = "app.update.silent";
> const TEST_ADDONS = [ "appdisabled_1", "appdisabled_2",
>                       "compatible_1", "compatible_2",
>                       "noupdate_1", "noupdate_2",
>                       "updatecompatibility_1", "updatecompatibility_2",
>                       "updateversion_1", "updateversion_2",
>                       "userdisabled_1", "userdisabled_2", "hotfix" ];
> 
>+const IS_WINDOWS = ("@mozilla.org/windows-registry-key;1" in Components.classes);
Please use IS_WIN for consistency with the xpcshell tests

>+const BIN_SUFFIX = (IS_WINDOWS ? ".exe" : "");

Please use something like the following with the IS_MACOSX directly after IS_WIN
const IS_MACOSX = ("nsILocalFileMac" in AUS_Ci);
const FILE_UPDATER_BIN = "updater" + IS_MACOSX ? ".app" : BIN_SUFFIX;

and update the code accordingly

>+
> var gURLData = URL_HOST + "/" + REL_PATH_DATA + "/";
> 
> var gTestTimeout = 240000; // 4 minutes
> var gTimeoutTimer;
> 
> // The number of SimpleTest.executeSoon calls to perform when waiting on an
> // update window to close before giving up.
> const CLOSE_WINDOW_TIMEOUT_MAXCOUNT = 10;
>@@ -845,30 +848,97 @@ function verifyTestsRan() {
>     let msg = "Checking if TESTS[" + i + "] test was performed... " +
>               "callback function name = " + gCallback.name + ", " +
>               "pageid = " + test.pageid;
>     ok(test.ranTest, msg);
>   }
> }
> 
> /**
>+ * Restore the updater that was backed up.  This is called both in setupFiles
>+ * and resetFiles.  It is called in setupFiles before the backup is done in
>+ * case the previous test failed.  It is called in resetFiles to put things
>+ * back to its original state.
>+ */
>+function resetUpdaterBackup() {
>+  // Move back the original updater
This comment is not correct.

>+  let baseAppDir = getAppBaseDir();
>+  let updaterBackup = baseAppDir.clone();
>+  let moveToRelPath = "updater.app";
>+  updaterBackup.appendRelativePath("updater.app.bak");
use append when appending a single descendent.

>+  if (!updaterBackup.exists()) {
>+    updater = baseAppDir.clone();
>+    updaterBackup.appendRelativePath("updater" + BIN_SUFFIX + ".bak");
use append when appending a single descendent.

>+    moveToRelPath = "updater" + BIN_SUFFIX;
>+  }
>+  if (updaterBackup.exists()) {
>+     // Remove the temp updater
>+    let updater = baseAppDir.clone();
>+    updater.appendRelativePath("updater.app");
use append when appending a single descendent.

>+    if (!updater.exists()) {
>+      updater = baseAppDir.clone();
>+      updater.appendRelativePath("updater" + BIN_SUFFIX);
use append when appending a single descendent.

>+    }
>+    if (updater.exists()) {
>+      updater.remove(true);
>+    }
>+    updaterBackup.moveTo(baseAppDir, moveToRelPath);
>+  }
>+}
>+
>+/**
>  * Creates a backup of files the tests need to modify so they can be restored to
>  * the original file when the test has finished and then modifies the files.
>  */
> function setupFiles() {
>   // Backup the updater-settings.ini file if it exists by moving it.
>   let baseAppDir = getAppBaseDir();
>   let updateSettingsIni = baseAppDir.clone();
>   updateSettingsIni.append(FILE_UPDATE_SETTINGS_INI);
>   if (updateSettingsIni.exists()) {
>     updateSettingsIni.moveTo(baseAppDir, FILE_UPDATE_SETTINGS_INI_BAK);
>   }
>   updateSettingsIni = baseAppDir.clone();
>   updateSettingsIni.append(FILE_UPDATE_SETTINGS_INI);
>   writeFile(updateSettingsIni, UPDATE_SETTINGS_CONTENTS);
>+
>+  // Just in case the last test failed, try to reset.
>+  resetUpdaterBackup();
>+
>+  // Move away the real updater
>+  let updater = baseAppDir.clone();
>+  updater.appendRelativePath("updater.app");
use append when appending a single descendent.

>+  if (updater.exists()) {
>+    updater.moveTo(baseAppDir, "updater.app.bak");
>+  } else {
>+    updater = baseAppDir.clone();
>+    updater.appendRelativePath("updater" + BIN_SUFFIX);
use append when appending a single descendent.

>+    updater.moveTo(baseAppDir, "updater" + BIN_SUFFIX + ".bak");
>+  }
>+
>+  // Move in the test only updater
>+  let testUpdaterDir = AUS_Cc["@mozilla.org/file/directory_service;1"].
>+    getService(AUS_Ci.nsIProperties).
>+    get("CurWorkD", AUS_Ci.nsILocalFile);
>+
>+  let relPath = REL_PATH_DATA;
>+  let pathParts = relPath.split("/");
>+  for (let i = 0; i < pathParts.length; ++i) {
>+    testUpdaterDir.append(pathParts[i]);
>+  }
>+
>+  let testUpdater = testUpdaterDir.clone();
>+  testUpdater.appendRelativePath("updater.app");
use append when appending a single descendent.

>+  if (testUpdater.exists()) {
>+    testUpdater.copyToFollowingLinks(baseAppDir, "updater.app");
>+  } else {
>+    testUpdater = testUpdaterDir.clone();
>+    testUpdater.appendRelativePath("updater" + BIN_SUFFIX);
use append when appending a single descendent.

>+    testUpdater.copyToFollowingLinks(baseAppDir, "updater" + BIN_SUFFIX);

I'd like to see this once more

Comment 48

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev6

Added nit on comment. Carrying forward r+.

Comment 49

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8: Fix mochitest chrome updater tests. rev7

Cleanup based on review comments.
The 2 code paths before could be simplified to 1, I think before I was thinking of these differently because one is folder and one is a file but the code is the same.  Thanks for the suggestion!

Comment 50

[Robert Strong (they/them - no direct email)]

Comment on attachment 8570840 [details] [diff] [review]
Patch 8: Fix mochitest chrome updater tests. rev7

Looks good!

>diff --git a/toolkit/mozapps/update/tests/chrome/utils.js b/toolkit/mozapps/update/tests/chrome/utils.js
>--- a/toolkit/mozapps/update/tests/chrome/utils.js
>+++ b/toolkit/mozapps/update/tests/chrome/utils.js
>@@ -157,16 +157,22 @@ const PREF_EM_HOTFIX_ID = "extensions.ho
> const PREF_EM_SILENT = "app.update.silent";
> const TEST_ADDONS = [ "appdisabled_1", "appdisabled_2",
>                       "compatible_1", "compatible_2",
>                       "noupdate_1", "noupdate_2",
>                       "updatecompatibility_1", "updatecompatibility_2",
>                       "updateversion_1", "updateversion_2",
>                       "userdisabled_1", "userdisabled_2", "hotfix" ];
> 
>+const IS_WIN = ("@mozilla.org/windows-registry-key;1" in Components.classes);
>+const IS_MACOSX = ("nsILocalFileMac" in Components.interfaces);
>+const BIN_SUFFIX = (IS_WIN ? ".exe" : "");
>+const FILE_UPDATER_BIN = "updater" + IS_MACOSX ? ".app" : BIN_SUFFIX;
>+const FILE_UPDATER_BIN_BAK = FILE_UPDATER_BIN + ".bak";
>+

This will likely need to be reworked after bug 1136358 merges. I'll leave it to you whether you want me to look at it again.

> var gURLData = URL_HOST + "/" + REL_PATH_DATA + "/";
> 
> var gTestTimeout = 240000; // 4 minutes
> var gTimeoutTimer;
> 
> // The number of SimpleTest.executeSoon calls to perform when waiting on an
> // update window to close before giving up.
> const CLOSE_WINDOW_TIMEOUT_MAXCOUNT = 10;
>@@ -845,30 +851,75 @@ function verifyTestsRan() {
>...
> function setupFiles() {
>   // Backup the updater-settings.ini file if it exists by moving it.
>   let baseAppDir = getAppBaseDir();
>   let updateSettingsIni = baseAppDir.clone();
>   updateSettingsIni.append(FILE_UPDATE_SETTINGS_INI);
>   if (updateSettingsIni.exists()) {
>     updateSettingsIni.moveTo(baseAppDir, FILE_UPDATE_SETTINGS_INI_BAK);
>   }
>   updateSettingsIni = baseAppDir.clone();
>   updateSettingsIni.append(FILE_UPDATE_SETTINGS_INI);
>   writeFile(updateSettingsIni, UPDATE_SETTINGS_CONTENTS);
>+
>+  // Just in case the last test failed, try to reset.
>+  resetUpdaterBackup();
>+
>+  // Move away the real updater
>+  let updater = baseAppDir.clone();
>+  updater.append(FILE_UPDATER_BIN);
>+  updater.moveTo(baseAppDir, FILE_UPDATER_BIN_BAK);
>+
>+  // Move in the test only updater
>+  let testUpdaterDir = AUS_Cc["@mozilla.org/file/directory_service;1"].
>+    getService(AUS_Ci.nsIProperties).
>+    get("CurWorkD", AUS_Ci.nsILocalFile);
>+
>+  let relPath = REL_PATH_DATA;
>+  let pathParts = relPath.split("/");
>+  for (let i = 0; i < pathParts.length; ++i) {
>+    testUpdaterDir.append(pathParts[i]);
>+  }
>+
>+  let testUpdater = testUpdaterDir.clone();
>+  testUpdater.append(FILE_UPDATER_BIN);
>+  if (testUpdater.exists()) {
>+    testUpdater.copyToFollowingLinks(baseAppDir, FILE_UPDATER_BIN);
>+  }
I think this should throw early if it doesn't exist.

Comment 51

[Robert Strong (they/them - no direct email)]

Comment on attachment 8570823 [details] [diff] [review]
Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev6


>diff --git a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>--- a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>+++ b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js
>@@ -50,16 +50,22 @@ const IS_ANDROID = false;
> #endif
> 
> #ifdef MOZ_WIDGET_GONK
> const IS_TOOLKIT_GONK = true;
> #else
> const IS_TOOLKIT_GONK = false;
> #endif
> 
>+#ifdef DISABLE_UPDATER_AUTHENTICODE_CHECK
>+const UPDATER_AUTHENTICODE_CHECK_DISABLED = true;
>+#else
>+const UPDATER_AUTHENTICODE_CHECK_DISABLED = false;
>+#endif
This will need to go in data/xpcshellConstantsPP.js after bug 1136358 merges

Comment 52

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8: Fix mochitest chrome updater tests. rev8

Minor change that was causing a failure, carrying forward r+.
-const FILE_UPDATER_BIN = "updater" + IS_MACOSX ? ".app" : BIN_SUFFIX;
+const FILE_UPDATER_BIN = "updater" + (IS_MACOSX ? ".app" : BIN_SUFFIX);

Other updates:
- Rebasing in progress
- Tried the disable tests check for conditionally building updater-xpcshell.  Still failing on tbpl for nightly builds but local windows build with ac_add_options --disable-tests is working.  Still investigating.

Comment 53

[Robert Strong (they/them - no direct email)]

There have been times when I've gone through the build logs and manually initiated each command to troubleshoot issues like the nightly build failures. That might point you in the right direction.

Comment 54

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev6

Rebased because of Bug 1136358, verified tests are passing with rebased version. Carrying forward r+. I don't think a re-review is necessary.

Comment 55

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8: Fix mochitest chrome updater tests. rev9

Rebased because of Bug 1136358, verified tests are passing with rebased version. Carrying forward r+. I don't think a re-review is necessary.

Comment 56

[Brian R. Bondy [:bbondy]]

Attachment: 8565134: Patch 9: Temporarily disable Linux. rev4

Comment 57

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev8

Carrying forward r+.  Fix win failure on the new svc sig test from a constant rename from bug 1136358
-  if (UPDATER_AUTHENTICODE_CHECK_DISABLED) {
+  if (!IS_AUTHENTICODE_CHECK_ENABLED) {

Comment 58

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev9

Renames AUS_Cc and related constants to Cc format in the new test file. This was done on March 9th on Oak I'm just syncing it here now.  The push showed to fix that failing test.  Carrying forward r+.

Comment 59

[Robert Strong (they/them - no direct email)]

Regarding the nightly build failures I think the basic problem is similar to what I have seen on OS X previously when recompiling just the updater (e.g. not the xpcshell one). I also think this was fixed when building with mach a while ago. I never fully investigated it so bear with me.

The following line removed the updater during libs
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/Makefile.in#52

So, running that code in the Makefile a second time would fail if there were no changes to the updater code the updater wouldn't be recompiled and hence wouldn't exist in dist/bin/. Touching the updater.cpp solved this for me.

One solution *might* be to not rename updater-xpcshell to updater in the data directory and have the tests perform the rename. You could then remove

mv $(XPCSHELLTESTROOT)/data/updater-xpcshell$(BIN_SUFFIX) $(XPCSHELLTESTROOT)/data/updater$(BIN_SUFFIX)

from the end of

diff --git a/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in b/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in
new file mode 100644
--- /dev/null
+++ b/toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in
@@ -0,0 +1,50 @@
<snip>
+libs::
+ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
+	$(NSINSTALL) -D $(XPCSHELLTESTROOT)/data/updater-xpcshell.app
+	rsync -a -C --exclude '*.in' $(srcdir)/../macbuild/Contents $(XPCSHELLTESTROOT)/data/updater-xpcshell.app
+	sed -e 's/%APP_NAME%/$(MOZ_APP_DISPLAYNAME)/' $(srcdir)/../macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in | \
+	  iconv -f UTF-8 -t UTF-16 > $(XPCSHELLTESTROOT)/data/updater-xpcshell.app/Contents/Resources/English.lproj/InfoPlist.strings
+	$(NSINSTALL) -D $(XPCSHELLTESTROOT)/data/updater-xpcshell.app/Contents/MacOS/updater-xpcshell
+	$(NSINSTALL) $(XPCSHELLTESTROOT)/data/updater-xpcshell $(XPCSHELLTESTROOT)/data/updater-xpcshell.app/Contents/MacOS
+	rm -f $(XPCSHELLTESTROOT)/data/updater-xpcshell
+	mv $(XPCSHELLTESTROOT)/data/updater-xpcshell.app $(XPCSHELLTESTROOT)/data/updater.app
+	mv $(XPCSHELLTESTROOT)/data/updater.app/Contents/MacOS/updater-xpcshell $(XPCSHELLTESTROOT)/data/updater.app/Contents/MacOS/updater
+else
+	mv $(XPCSHELLTESTROOT)/data/updater-xpcshell$(BIN_SUFFIX) $(XPCSHELLTESTROOT)/data/updater$(BIN_SUFFIX)
+endif

Comment 60

[Robert Strong (they/them - no direct email)]

Comment on attachment 8574502 [details] [diff] [review]
Patch 7: Bug 1125699 + misc work on oak - Rollup of recent work - rev6

There is a newer Patch 7 so obsoleting.

Comment 61

[Robert Strong (they/them - no direct email)]

Any idea why the service tests fail on Win 8 with SERVICE_UPDATER_SIGN_ERROR?

If not, I suggest changing var DEBUG_AUS_TEST = false; to var DEBUG_AUS_TEST = true; in update/tests/data/xpcshellUtilsAUS.js on Oak. It might point to what is going on.

Note: on Win 8 it isn't possible to copy the maintenance service in the tests (there is an open bug to change that). Even with the old maintenance service the majority of the tests should succeed without copying the maintenance service. Perhaps one of the changes in these patches made that no longer possible? Why this is the case will need to be tracked down in case it could cause a problem with updating different versions of Firefox with the maintenance service.

Comment 62

[Robert Strong (they/them - no direct email)]

Looks like the nightly builds are working again... yay!

Comment 63

[Robert Strong (they/them - no direct email)]

Comment on attachment 8560824 [details] [diff] [review]
Patch 1: Bug 903135 - Updates to libmar needed to support B2G MAR signature verification. r=bbondy

With the removal of
-      if (!ReadFile(certFile, certBuffers[k], fileSizes[k], &read, NULL) ||
-          fileSizes[k] != read) {
-        CloseHandle(certFile);
-        for (i = 0; i <= k; i++) {
-          free(certBuffers[i]);
-        }
-        return -1;
-      }


  uint32_t i, k;

needs to be changed to

  uint32_t k;

Comment 64

[Robert Strong (they/them - no direct email)]

I'm also seeing

modules/libmar/tool/mar.c(134) : warning C4090: 'function' : different 'const' qualifiers

Comment 65

[Brian R. Bondy [:bbondy]]

Attachment: Patch 10: Fix busted nightly builds

This fixes the busted Nightly builds on Oak.  The conditions here were originally copied from similar test code but I don't think we want them in this case. 

This also fixes when tests are disabled by not adding updater-xpcshell to DIRS at all.

Note that I'll be posting an aggregated patch of only build changes for review by a build peer as well, but just want a first pass done by you first.

Comment 66

[Robert Strong (they/them - no direct email)]

Comment on attachment 8577686 [details] [diff] [review]
Patch 10: Fix busted nightly builds

I'm not all that familiar with how we do PGO so I'll let a build peer decide what's best to do here. Thanks!

Comment 67

[Brian R. Bondy [:bbondy]]

> Note: on Win 8 it isn't possible to copy the maintenance service in the tests 
>(there is an open bug to change that). Even with the old maintenance service the majority of 
> the tests should succeed without copying the maintenance service. Perhaps one of the changes 
> in these patches made that no longer possible? Why this is the case will need to be tracked down 
> in case it could cause a problem with updating different versions of Firefox with the maintenance service.
Oh I just saw this now and just sent you an email that I think that we're always testing with the old maintenance service on win8.  To verify that I had just pushed out a always fail on service tests to try.

It seems like what you're saying confirms that theory.  This also means that any maintenance service bugs introduced on win8 would silently pass tests.

Could you point me to that bug? It doesn't make sense to me and sounds like maybe a misconfiguration.  The bootstrap test should still replace and those win8 machines should be configured the same as the other ones with the custom service.  The copy that happens before tests isn't required, but just done for safe measures because the bootstrap should work.

>Why this is the case will need to be tracked down 
> in case it could cause a problem with updating different 
> versions of Firefox with the maintenance service.

So this is only relating to us no longer checking signatures in the service which is for tests only.  It won't be a problem for cases outside of tests.  We decided to do that and have a new test which does the sig checks through the maintenance service.   And the maintenance service has a new command to be able to do that, which the tests need.

---

I've also done a lot of manual testing to make sure Nightly builds from Oak update as they should.  Let me know if you also want to test or if I should reach out to Kamil for extra testing before landing.  This is a ton of code spanning a long period of time so I'm particularly nervous.  Before landing I'd also like to give RelEng and the build sherrifs a heads up.

Things tested:
- Updating from old build from Oct. to new signed build, updates should succeed.
- Updating from new build to new build + 1 day’s update.
- Repeat above with x64 and x86 both
- Repeat above with OS X builds.
- Setup a staged mockaus local server that serves the same updates.xml as remote but with the signature stripped.  Updated MD5 + file size to new values. Served update and verified that it fails the update with status code 19: last-update.log contained failed: 19 in /Caches/Mozilla/updates/Applications/Nightly/updates/last-update.log

Comment 68

[Brian R. Bondy [:bbondy]]

In addition to the last message, let me know if you think the win8 stuff should block.  I'm not sure if RelEng can work on this anytime soon but I'm particularly worried about putting a skip service tests on win8 check if we ever obsolete other platforms in favour of win8.  If you pass along the other bug number I might be able to make sense of what the problem is there.

Comment 69

[Brian R. Bondy [:bbondy]]

I'll do a small patch 11 to fix those libmar warnings.

Comment 70

[Brian R. Bondy [:bbondy]]

Attachment: Patch 11: Fix libmar warnings

Compiling on win and osx gave different results (clang vs VC). This works on both.

Comment 71

[Robert Strong (they/them - no direct email)]

According to Bug 1067756 the Win 8 issue should be fixed for the most part. With luck bug 1081450 will pass try and that will show that we can now copy the test's maintenance service on Win 8.

The bootstrap won't work in all cases since the systems don't always reboot between runs so branches with versions less than the one installed won't install the service since it has a lower version than the one already installed.

Crossing fingers that this unblocks you.

Comment 72

[Robert Strong (they/them - no direct email)]

From Bug 1067756

"I see machines that picked up the change of of this morning."

I also looked at a few beta builds and they appear to be copying the maintenance service as well. The systems need to have rebooted so not all of them probably have it at this time... not too worried about that.

Comment 73

[Robert Strong (they/them - no direct email)]

Pushed patch from bug 1081450 to verify win8 tests are working now.
https://hg.mozilla.org/projects/oak/rev/b78d779d15cb

Comment 74

[Brian R. Bondy [:bbondy]]

Attachment: Build config for multi platform MAR verification work

This is the build config for the multi platform MAR verification work.

It was already r+ed as part of a series of other patches in this bug, but hasn't been r+ed by a build peer yet.  I'm pulling it out to make it easier, but please feel free to see how each change relates in a patch by patch basis if that is easier.

I won't be landing this patch directly, but instead will be landing the other patches in this bug that contains this work.

At first we're only enabling this work on OSX. It is enabled already on Windows today.

Comment 75

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7: New updater-xpcshell binary for updater tests. r=rstrong rev9'

Updated patch headers. Carrying forward r+.

Comment 76

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8: Bug 973933 - Fix mochitest chrome updater tests. r=rstrong. rev9'

Updated patch headers. Carrying forward r+.

Comment 77

[Brian R. Bondy [:bbondy]]

Attachment: Patch 9: Bug 973933 - Temporarily disable Linux for MAR verification. r=rstrong rev4

Updated patch headers. Carrying forward r+.

Comment 78

[Brian R. Bondy [:bbondy]]

Attachment: Patch 10: Bug 973933 - Fix Nightly builds failing on updater-xpcshell. r=rstrong

Comment 79

[Brian R. Bondy [:bbondy]]

Attachment: Patch 11: Fix libmar warnings. r=rstrong

Comment 80

[Mike Hommey [:glandium]]

Comment on attachment 8579782 [details] [diff] [review]
Build config for multi platform MAR verification work

Review of attachment 8579782 [details] [diff] [review]:
-----------------------------------------------------------------

You'll want to clean up those * * * and try syntax in the commit message.

::: browser/confvars.sh
@@ +26,4 @@
>  fi
>  
> +# Enable building ./signmar and running libmar signature tests
> +MOZ_ENABLE_SIGNMAR=1

Why not set that in mozconfigs instead? IOW, why enable for both local and automation builds instead of only the latter? (Please excuse me if it's answered in the bug, I didn't go through the past comments)

::: configure.in
@@ +6459,5 @@
>      MOZ_VERIFY_MAR_SIGNATURE=1,
>      MOZ_VERIFY_MAR_SIGNATURE= )
>  
>  if test -n "$MOZ_VERIFY_MAR_SIGNATURE"; then
> +  AC_DEFINE(MOZ_VERIFY_MAR_SIGNATURE)

Does --enable-verify-mar actually make sense on non-windows non-osx?

::: toolkit/mozapps/update/updater/updater-common.build
@@ +6,5 @@
> +
> +SOURCES += [
> +    '%sarchivereader.cpp' % updater_rel_path,
> +    '%sbspatch.cpp' % updater_rel_path,
> +    '%supdater.cpp' % updater_rel_path,

A less ugly way to do this is to do something like:
srcs += [
  'archivereader.cpp',
  ...
]
if ...:
   srcs += [...]

SOURCES += ['%s%s' % (updater_rel_path, f) for f in srcs]

Also please hg copy the moz.build to this file, so that mercurial knows where this all comes from. (this will also make the patch easier to review)

::: toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in
@@ +42,5 @@
> +	mv $(XPCSHELLTESTROOT)/data/updater-xpcshell.app $(XPCSHELLTESTROOT)/data/updater.app
> +	mv $(XPCSHELLTESTROOT)/data/updater.app/Contents/MacOS/updater-xpcshell $(XPCSHELLTESTROOT)/data/updater.app/Contents/MacOS/updater
> +
> +	# Copy for mochitest chrome tests
> +	rsync -a -C $(XPCSHELLTESTROOT)/data/updater.app $(MOCHITESTROOT)/data/updater.app

Ouch. Why all is this rsync/rm dance needed? Is it cargo-culting, or there's an actual need? That is, do you really need an app bundle, as opposed to a simple binary? Relatedly, why can't you just use the actual updater?

Comment 81

[Brian R. Bondy [:bbondy]]

> You'll want to clean up those * * * and try syntax in the commit message.
This patch is not actually landing, it is just the aggregation of the other patches.  It will be landed as parts of the other bugs which already have the cleaned up patch headers.

> Why not set that in mozconfigs instead? IOW, why enable for both local and automation builds instead of
> only the latter? (Please excuse me if it's answered in the bug, I didn't go through the past comments)

Having it ensures that the signmar binary is built which can be used to sign mars, verify mars, and a variety of other things.  Also will ensure updater tests have better coverage. I'd rather enable it in both places if that's ok.


> Does --enable-verify-mar actually make sense on non-windows non-osx?

Yes, and we'll enable it on Linux as a follow up.

> Ouch. Why all is this rsync/rm dance needed? Is it cargo-culting, or there's an actual need? 
> That is, do you really need an app bundle, as opposed to a simple binary? 

Yes we need the app bundle for tests.

> Relatedly, why can't you 
> just use the actual updater?

The updater embeds a certificate w/ a public key which verifies the signature on the MAR file for tests.  Each updater-xpcshell embeds the certificate that verifies the MARs we use for tests no matter which branch it was built on.  Note we tried to avoid this in various ways, but we ended up here.

> ::: toolkit/mozapps/update/updater/updater-common.build
> @@ +6,5 @@
> > +
> > +SOURCES += [
> > +    '%sarchivereader.cpp' % updater_rel_path,
> > +    '%sbspatch.cpp' % updater_rel_path,
> > +    '%supdater.cpp' % updater_rel_path,
> 
> A less ugly way to do this is to do something like:
> srcs += [
>   'archivereader.cpp',
>   ...
> ]
> if ...:
>    srcs += [...]
> 
> SOURCES += ['%s%s' % (updater_rel_path, f) for f in srcs]
> 
> Also please hg copy the moz.build to this file, so that mercurial knows
> where this all comes from. (this will also make the patch easier to review)


Thanks, will do.

Comment 82

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7: New updater-xpcshell binary for updater tests. r=rstrong rev10

- Made src and list comprehension changes to build file suggested by glandium
- Rebased patch to hg copy instead of hg add

Carrying forward the previous r+ on Patch 7.
I'll attach a new aggregated build patch though for glandium's re-review.

Comment 83

[Brian R. Bondy [:bbondy]]

Attachment: Build config for multi platform MAR verification work. rev2

Rev 2 with review comments.

Recall:
This patch is the aggregated build config changes.
This patch will not land, but instead the other patches in this bug will land which contain all of these changes.

See https://bugzilla.mozilla.org/show_bug.cgi?id=973933#c81 for previous comments to review feedback.

Comment 84

[Brian R. Bondy [:bbondy]]

Attachment: Test plan

Comment 85

[Robert Strong (they/them - no direct email)]

Comment on attachment 8582195 [details]
Test plan

It would be a good thing to also test a signed update with an incorrect signature. The xpcshell test mar could be used with an oak nightly build.

Testing the staging vs. non staging on Mac would also be a good thing.

Comment 86

[Mike Hommey [:glandium]]

Comment on attachment 8582120 [details] [diff] [review]
Build config for multi platform MAR verification work. rev2

Review of attachment 8582120 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/update/updater/updater-common.build
@@ +46,5 @@
>          'advapi32',
>      ]
> +elif CONFIG['OS_ARCH'] == 'Linux' and CONFIG['MOZ_VERIFY_MAR_SIGNATURE']:
> +    USE_LIBS += [
> +        '/modules/libmar/sign/signmar',

'signmar' should work. If it doesn't, please file a bug.

@@ +50,5 @@
> +        '/modules/libmar/sign/signmar',
> +        'nss',
> +        'updatecommon',
> +    ]
> +    OS_LIBS += CONFIG['NSPR_LIBS']

USE_LIBS += ['nspr'] does the same and is future-proof.

That said, why does Linux need nspr+nss and not OSX/Windows?

@@ +80,4 @@
>          'launchchild_osx.mm',
>          'progressui_osx.mm',
>      ]
> +    OS_LIBS += ['-framework Cocoa -framework Security']

OS_LIBS += [
    '-framework Cocoa',
    '-framework Security',
]

I guess that answers the question about nspr/nss, but that also makes me wonder why we're not using nss everywhere?

@@ +98,5 @@
>          'progressui_null.cpp',
>      ]
>  
> +srcs.sort()
> +SOURCES += ['%s%s' % (updater_rel_path, f) for f in srcs]

for f in sorted(srcs) instead of doing srcs.sort()

::: toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in
@@ +9,5 @@
> +MOCHITESTROOT = $(abspath $(DEPTH))/_tests/testing/mochitest/chrome/toolkit/mozapps/update/tests
> +INSTALL_TARGETS += xpcshell-updater
> +xpcshell-updater_TARGET  := libs
> +xpcshell-updater_DEST    := $(XPCSHELLTESTROOT)/data
> +xpcshell-updater_FILES   := $(DIST)/bin/updater-xpcshell$(BIN_SUFFIX)

It's not necessarily a good thing to pick files out of $(DIST)/bin to copy them, especially when they probably shouldn't be in $(DIST)/bin in the first place (I'd add NO_DIST_INSTALL = True in the corresponding moz.build). So you should just use $(PROGRAM).

That being said, you move/remove that file you're copying here in the libs:: rule below, so that makes the use of INSTALL_TARGETS moot (one of their goal being to avoid copies during incremental builds, which removing the destination file prevents). So I guess you can remove this block... I was going to suggest making the program use the 'updater' name, in which case you could have the INSTALL_TARGETS rule install it directly in the right directory (even on mac), but mozbuild will reject that (but those rules could be relaxed, but that's not your problem).

@@ +25,5 @@
> +ifdef MOZ_WIDGET_GTK
> +libs:: ../updater.png
> +	$(NSINSTALL) -D $(DIST)/bin/icons
> +	$(INSTALL) $(IFLAGS1) $^ $(DIST)/bin/icons
> +endif

You're essentially copying updater.png on top of the one that was already copied from toolkit/mozapps/update/updater/Makefile.in. Just remove this block.

@@ +45,5 @@
> +	# Copy for mochitest chrome tests
> +	rsync -a -C $(XPCSHELLTESTROOT)/data/updater.app $(MOCHITESTROOT)/data/updater.app
> +else
> +	mv $(XPCSHELLTESTROOT)/data/updater-xpcshell$(BIN_SUFFIX) $(XPCSHELLTESTROOT)/data/updater$(BIN_SUFFIX)
> +	cp $(XPCSHELLTESTROOT)/data/updater$(BIN_SUFFIX) $(MOCHITESTROOT)/data/updater$(BIN_SUFFIX)

It would be better to avoid copying this in two different places. testing/mochitest/Makefile.in has rules to copy files that are needed for different harnesses, and copies them in a "bin" directory in the test package. The "bin" directory is available across harnesses. Feel free to push that to a followup, though.

Comment 87

[Brian R. Bondy [:bbondy]]

Thanks for the review comments, will do.
For the last thing I'll file a followup. 

>That said, why does Linux need nspr+nss and not OSX/Windows?

OSX and Windows use native crypto APIs and not nspr, so it is on purpose. On OSX there are some problems with executing the updater and it being reliant on dylib files which aren't copied and could have security problems by relying on them.  Compiling nspr a second time as a static library wasn't recommended at the time either.  The original reason for this though on Windows was because there was strong objections with us using nspr from the security team in case nspr ever had a bug they didn't want it to block updates I think was the main reason.  In any case if we want to move to nspr later on everywhere, it would be very minor tweaks (other than the dylib or static compiling stuff) that are necessary.

Comment 88

[Brian R. Bondy [:bbondy]]

ugh, s/nspr/nss above.

Comment 89

[Brian R. Bondy [:bbondy]]

Attachment: Build config for multi platform MAR verification work. rev3

Thanks for the suggestions, this is much cleaner.

Updated with 2nd round of review comments.  Created a spinoff for the task you mentioned that was ok for, see bug 1148272.

Comment 90

[Brian R. Bondy [:bbondy]]

Attachment: Patch 10: Bug 973933 - Fix Nightly builds failing on updater-xpcshell. r=rstrong. rev2

Syncing with latest build-review patch config changes.  Carrying forward existing r+. Will re-sync with future build comments if any as well.

Comment 91

[Brian R. Bondy [:bbondy]]

Attachment: Test plan rev2

Updated test plan with new suggestions. For the wrongly signed manual test the best route is to strip and then re-sign with xpcshell nss store for a nightly Oak.

Carrying forward previous feedback+.

Comment 92

[Mike Hommey [:glandium]]

Comment on attachment 8584266 [details] [diff] [review]
Build config for multi platform MAR verification work. rev3

Review of attachment 8584266 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/update/updater/updater-common.build
@@ +46,5 @@
>          'advapi32',
>      ]
> +elif CONFIG['OS_ARCH'] == 'Linux' and CONFIG['MOZ_VERIFY_MAR_SIGNATURE']:
> +    USE_LIBS += [
> +        'signmar',

USE_LIBS expects things to be ordered when adding them, so this can't be working with signmar before nss :)

@@ +80,4 @@
>          'launchchild_osx.mm',
>          'progressui_osx.mm',
>      ]
> +    OS_LIBS += ['-framework Cocoa -framework Security']

cf. previous review:
OS_LIBS += [
    '-framework Cocoa',
    '-framework Security',
]

Comment 93

[Mike Hommey [:glandium]]

Comment on attachment 8584266 [details] [diff] [review]
Build config for multi platform MAR verification work. rev3

Review of attachment 8584266 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, let's make that a r+, as long as you fix the two things I noted and the linux part actually works.

Comment 94

[Brian R. Bondy [:bbondy]]

> USE_LIBS expects things to be ordered when adding them, so this can't be working with signmar before nss :)

Maybe because this isn't enabled on Linux so try/oak pushes were passing. I re-ordered and updated the source patches with that.


> cf. previous review:
> OS_LIBS += [
>    '-framework Cocoa',
>    '-framework Security',
> ]

Oops fixed.  Will update the source patches with that.

Will obsolete the old build patch which was r+ed since it's contained in the other patches.  Thanks again for the reviews.

Comment 95

[Brian R. Bondy [:bbondy]]

Attachment: Patch 10: Bug 973933 - Fix Nightly builds failing on updater-xpcshell. r=rstrong. rev3

Implemented latest build comments which was r+ed.  Carrying forward patch 10's r+.

Comment 96

[Brian R. Bondy [:bbondy]]

Attachment: Patch 8: Bug 973933 - Fix mochitest chrome updater tests. r=rstrong. rev9''

Rebased patch 8 to tip.

Comment 97

[Brian R. Bondy [:bbondy]]

Attachment: Patch 10: Bug 973933 - Fix Nightly builds failing on updater-xpcshell. r=rstrong. rev4

Carrying forward r+.
I introduced build problem on Win/Linux when doing a review comment developed on OSX.

 -	mv $(XPCSHELLTESTROOT)/data/updater-xpcshell$(BIN_SUFFIX) $(XPCSHELLTESTROOT)/data/updater$(BIN_SUFFIX)
+	mv $(PROGRAM) $(XPCSHELLTESTROOT)/data/updater$(BIN_SUFFIX)

Verified that the change works on Oak treeherder.

Comment 98

[Brian R. Bondy [:bbondy]]

Attachment: Patch 7: New updater-xpcshell binary for updater tests. r=rstrong rev10'

Rebased, carrying forward r+.
Compared re-updated/merged Oak to tip w/ patches applied again and everything is matching up.

Comment 99

[Brian R. Bondy [:bbondy]]

I'm ready to go ahead with landing this. 

I was wondering if you could freeze to today's nightly while I test landing this and manually trigger 2 nightly builds for windows and osx desktop.  I'll re-ping when you can unfreeze.

Please confirm when it's ok to land, I plan on landing directly to m-c btw. I let people in m-c know about this already.

Comment 100

[Brian R. Bondy [:bbondy]]

**I let people in #developers know about this already

Comment 101

[Brian R. Bondy [:bbondy]]

Landed directly to m-c.
https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=883e17fc475f

We're freezing nightlies and triggering 2 test nightly builds on m-c for testing with nightlytest. 
We'll unfreeze when done. Callek took care of the freezing.
Testing still in progress.

I'll re-open if any problems occur.