1072722 - With older clients the new maintenance service checks the updated directory's updater.exe when verifying the updater.exe for replace requests

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

This prevents the service from completing the replace request. It does properly fallback to a UAC prompt but this needs to be fixed before landing the mac v2 signing changes. Glad I caught this!

Comment 1

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

Attachment: patch rev1

The maintenance service wasn't correctly checking the command line for old versions of Firefox for replace requests and hence was failing.

Comment 2

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

Pushed to oak
https://hg.mozilla.org/projects/oak/rev/2050a94fd07e

Comment 3

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

I verified this does the right thing with Firefox Beta and Thunderbird Release as well as still does the right thing with oak.

Comment 4

[Stephen A Pohl]

Comment on attachment 8495045 [details] [diff] [review]
patch rev1

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

Good catch! Just one nit below.

::: toolkit/components/maintenanceservice/workmonitor.cpp
@@ +124,5 @@
>  static bool
>  IsOldCommandline(int argc, LPWSTR *argv)
>  {
>    return argc == 4 && !wcscmp(argv[3], L"-1") ||
> +         argc >= 4 && (wcsstr(argv[3], L"/replace") || IsDigits(argv[3]));

I would prefer if the |IsDigits| condition stayed on its own separate line to better conform with our coding style[1], namely: "Break long conditions after && and || logical connectives.". I think it makes the three conditions more obvious.

I'd also accept putting parenthesis around each condition to make the precedence between && and || obvious to new contributors, but it's your call and separate lines may already achieve this.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

Comment 5

[Stephen A Pohl]

Comment on attachment 8495045 [details] [diff] [review]
patch rev1

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

::: toolkit/components/maintenanceservice/workmonitor.cpp
@@ +124,5 @@
>  static bool
>  IsOldCommandline(int argc, LPWSTR *argv)
>  {
>    return argc == 4 && !wcscmp(argv[3], L"-1") ||
> +         argc >= 4 && (wcsstr(argv[3], L"/replace") || IsDigits(argv[3]));

I just realized that you made the |argc >= 4| apply to both |wcsstr| and |IsDigits| via the extra parenthesis around |wcsstr| and |IsDigits|, so please ignore my comment.

Comment 6

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

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/be53b555dda6

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/be53b555dda6

Comment 8

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

Landed on aurora in the Mac V2 signing combined patch in bug 1047584