711054 - Stop using readstrings errors for updater

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

From rs: 
The updater currently overloads readstrings errors. This is an artifact of the time we needed readstrings available for the WinCE / WinMo installer. 

>--- a/toolkit/mozapps/readstrings/errors.h
>+++ b/toolkit/mozapps/readstrings/errors.h
>@@ -44,10 +44,13 @@
> // #define IO_ERROR 2  // Use READ_ERROR or WRITE_ERROR instead
> #define USAGE_ERROR 3
> #define CRC_ERROR 4
> #define PARSE_ERROR 5
> #define READ_ERROR 6
> #define WRITE_ERROR 7
> #define UNEXPECTED_ERROR 8
> #define ELEVATION_CANCELED 9
> #define CERT_LOAD_ERROR 10
> #define CERT_HANDLING_ERROR 11
> #define CERT_VERIFY_ERROR 12

Comment 1

[Brian R. Bondy [:bbondy]]

I think it would be best to move the files inside readstrings directly into mozapps/update/common since both the maintenance service and updater use it.

Maintenance service has its own error codes it keeps separate in workmonitor.cpp that could go inside there as well.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

- Moved readstrings and common error codes into update/common
- Updated error codes to be sequential
- Removed some duplicated code that's been driving me crazy for a while inside the test code.

Comment 3

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

Comment on attachment 601860 [details] [diff] [review]
Patch v1.

Did you run the test suite and it passed?
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#145

If so, some additional tests need to be added.

Comment 4

[Brian R. Bondy [:bbondy]]

Oops no haven't ran it yet, will fix that, make sure they pass then re-request review.

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Updated js error codes, now passing try tests.

Comment 6

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

Did you verify that the tests fail without these changes? I won't request that fixes for this be included in this bug if they don't but I'd like to know whether there needs to be a followup. Thanks

Comment 7

[Brian R. Bondy [:bbondy]]

I had not run the old patch through try. I can now though.

Comment 8

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

Comment on attachment 602876 [details] [diff] [review]
Patch v2.

>diff --git a/toolkit/mozapps/update/common/Makefile.in b/toolkit/mozapps/update/common/Makefile.in
>--- a/toolkit/mozapps/update/common/Makefile.in
>+++ b/toolkit/mozapps/update/common/Makefile.in
>@@ -47,20 +47,22 @@ LIBRARY_NAME = updatecommon
> FORCE_STATIC_LIB =1
> LIBXUL_LIBRARY = 1
> ifeq ($(OS_ARCH),WINNT)
> USE_STATIC_LIBS = 1
> endif
> 
> CPPSRCS = \
>   updatelogging.cpp \
>+	readstrings.cpp \
spaces please

>   $(NULL)
> 
> EXPORTS = updatelogging.h \
>   updatedefines.h \
>+	readstrings.h \
spaces please

If the tests pass without the fix to nsUpdateService.js please file a followup bug to add tests for the error codes. Thanks

Comment 9

[Brian R. Bondy [:bbondy]]

Pushed the old patch to try here:
https://tbpl.mozilla.org/?tree=Try&rev=029ce61ed88a

I'll update the results and post a new bug if it doesn't fail.

Comment 10

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7a690d2c2e23

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/7a690d2c2e23

Comment 12

[Brian R. Bondy [:bbondy]]

re comment 9:
> I'll update the results and post a new bug if it doesn't fail.

I pushed this a couple times to try and I think it fails. It's hard to tell though because there was a signing problem on releng side around the same time.

https://tbpl.mozilla.org/?tree=Try&rev=1188a5ce1cf3
https://tbpl.mozilla.org/?tree=Try&rev=029ce61ed88a