711792 - Speed up and simplify maintenance service update command processing

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

We currently start the maintenance service, watch a directory for changes from the service, and write out a work item file to be processed by the service.

This is a bit slower than normal software updates without the service.
The original code was done this way because the service was always running.

Since the service is now always on demand, we can simplify the code significantly, speed up the update process through the service, and reduce failure conditions by passing the arguments to StartService.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

The patch looks big but the changes to the logic is very small. It has all the benefits as mentioned in the original comment and speeds up getting updater.exe started faster to avoid a user clicking the firefox icon too soon before it is locked.  It is noticeably faster than the old service updates, sheds about 2 seconds.  

I'm going to run it through elm and hope for the best, tests pass on my local machine as they did before this patch.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Fixed build problem if cert check is not disabled.  Had it ifdef'ed so didn't see it.

Comment 3

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

I did a quick once over of the patch and it looks good. I'm going to be away from the computer tomorrow so I'll do a more thorough review on Monday.

Comment 4

[Brian R. Bondy [:bbondy]]

OK thanks, sounds great. It didn't fix the xpcshell test but it removed a lot of possibilities of what could be wrong. We should keep this patch anyway though.

I'll have another small patch to fix the xpcshell test for you to review Monday, but I expect that fix to be in the xpcshell JS code.

Comment 5

[Brian R. Bondy [:bbondy]]

Just a quick note that I had this on elm but then realized that we will need to update the build servers to use the newer maintenanceservice because we can't use the old pre-existing installed service to do the first udpate (since no more work items are written).  

We should still include this patch but I've backed this out of elm until RelEng can update the build servers hopefully on Monday.
CC'ing bhearsum.  I think all I need to give him is a new maintenanceservice.exe and maintenanceservice_installer.exe.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3.

Some extra cleanup.

Comment 7

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

Comment on attachment 582724 [details] [diff] [review]
Patch v3.

I think this patch is my favorite so far

>diff --git a/toolkit/components/maintenanceservice/workmonitor.cpp b/toolkit/components/maintenanceservice/workmonitor.cpp
>--- a/toolkit/components/maintenanceservice/workmonitor.cpp
>+++ b/toolkit/components/maintenanceservice/workmonitor.cpp
>...
>+  // The tests work by making sure the log has changed, so we put a 
>+  // unique ID in the log.
>+  RPC_WSTR guidString = RPC_WSTR(L"");
>+  GUID guid;
>+  HRESULT hr = CoCreateGuid(&guid);
>+  if (SUCCEEDED(hr)) {
>+    UuidToString(&guid, &guidString);
>+  }
>+  LOG(("Executing service command %ls, ID: %ls\n", 
>+       argv[2], reinterpret_cast<LPCWSTR>(guidString)));
>+  RpcStringFree(&guidString);
>+
>+  BOOL result = FALSE;
>+  if (!lstrcmpi(argv[2], L"software-update")) {
>+    result = ProessSoftwareUpdateCommand(argc - 3, argv + 3);
>+    LOG(("Service command %ls complete.\n", argv[2]));
>+  } else {
>+    LOG(("Service command not recognized: %ls.\n", argv[2]));
>+    //result is already set to FALSE
nit: space between // and result

Thanks!

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4.

Fixed nit. Carrying forward r+.

Comment 9

[Brian R. Bondy [:bbondy]]

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