951662 - Use a mock update directory for update tests

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

This is so bug 921148 can be fixed. I'm using a separate bug since there is a lot of app update application code and test cleanup that I am doing at the same time and not all of it is necessary to fix bug 921148

Comment 1

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

Attachment: 1. updater changes

Switches the term background with stage or staged to differentiate it from background updating where there is no user prompt.

Comment 2

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

Attachment: 2. startup changes

Switches the term background with stage or staged to differentiate it from background updating where there is no user prompt and removes MOZ_UPDATE_NO_HASH_DIR, MOZ_UPDATE_ROOT_OVERRIDE, and MOZ_UPDATE_APPDIR_OVERRIDE since they will no longer be needed with the test changes.

Comment 3

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

Attachment: 3. update service fix undeclared var

Saw this while debugging and decided to include it.

Comment 4

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

Attachment: 4. build config changes

I have a bit more to do before I submit the test changes and I hope to have it done today. I'll submit a patch specific to xpcshell in bug 921148.

Comment 5

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

Attachment: test changes in progress

Comment 6

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

Attachment: patch for bug 921148

Comment 7

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

try push with current patches
https://tbpl.mozilla.org/?tree=Try&rev=601cfb6693a3

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 8349420 [details] [diff] [review]
3. update service fix undeclared var

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

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1324,1 @@
>    for each (res in ['app', 'gre']) {

nit: since we only need `res` within the body of the `for each` let's scope `res` inside the for each

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 8349421 [details] [diff] [review]
4. build config changes

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

Please request from build peer

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8349421 [details] [diff] [review]
4. build config changes

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

::: toolkit/mozapps/update/tests/moz.build
@@ +31,5 @@
> +    else:
> +        if CONFIG['MOZ_APP_VENDOR']:
> +            DEFINES['MOZ_APP_VENDOR'] = '"%s"' % CONFIG['MOZ_APP_VENDOR']
> +        if CONFIG['MOZ_APP_BASENAME']:
> +            DEFINES['MOZ_APP_BASENAME'] = '"%s"' % CONFIG['MOZ_APP_BASENAME']

Why is this needed? I see no record in the bug comments. We need a source comment before this gets r+.

Why can't you perform the quoting in the source?

Comment 11

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

(In reply to Gregory Szorc [:gps] from comment #10)
> Comment on attachment 8349421 [details] [diff] [review]
> 4. build config changes
> 
> Review of attachment 8349421 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/tests/moz.build
> @@ +31,5 @@
> > +    else:
> > +        if CONFIG['MOZ_APP_VENDOR']:
> > +            DEFINES['MOZ_APP_VENDOR'] = '"%s"' % CONFIG['MOZ_APP_VENDOR']
> > +        if CONFIG['MOZ_APP_BASENAME']:
> > +            DEFINES['MOZ_APP_BASENAME'] = '"%s"' % CONFIG['MOZ_APP_BASENAME']
> 
> Why is this needed? I see no record in the bug comments. We need a source
> comment before this gets r+.
Will do

> Why can't you perform the quoting in the source?
I have checked and this is a copy / paste from the following moz.build which was moved to moz.build in bug 874266
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/moz.build#78

Comment 12

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

Attachment: 4. build config changes rev2

Followed the same usage as seen elsewhere
http://mxr.mozilla.org/mozilla-central/source/build/moz.build#28

Would adding comments to where it is preprocessed in the js be sufficient for you?

Comment 13

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

Attachment: 4. fix updater logging and make file in use text consistent

Found while trying to make the service and non-service tests more consistent.

Comment 14

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

Specifically, LogFinish was called to soon for the non sharing violation case.

Comment 15

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

Attachment: 6 tests changes in progress

Comment 16

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

Attachment: 3. build config changes rev3

Turns out that MOZ_APP_PROFILE isn't needed.

Comment 17

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

Attachment: 6. fixes for nsXREDirProvider::GetUpdateRootDir

MOZ_APP_BASENAME is the value that should be looked for under SOFTWARE\Mozilla in the registry instead of MOZ_APP_NAME. It worked before since the query is not case sensitive.

|if (gAppData->vendor)| evaluates to true so I added a check for the value being a zero length string.

I added a fallback to look under SOFTWARE\Mozilla when gAppData->vendor is not defined. Thunderbird already sets its TaskBarIDs under SOFTWARE\Mozilla\Thunderbird\TaskBarIDs so this makes it so its updates directory is under %LOCALAPPDATA%\Thunderbird\updates\TaskBarID\etc. This also makes the update tests pass when run in parallel on Thunderbird.

Comment 18

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

Attachment: 7. test changes in progress

Comment 19

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #17)
> Created attachment 8351667 [details] [diff] [review]
> 6. fixes for nsXREDirProvider::GetUpdateRootDir
>...
> |if (gAppData->vendor)| evaluates to true so I added a check for the value
> being a zero length string.
when MOZ_APP_VENDOR is not defined.

Comment 20

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

Try push
https://tbpl.mozilla.org/?tree=Try&rev=191e23f71672

Try push without MOZ_APP_VENDOR defined
https://tbpl.mozilla.org/?tree=Try&rev=26547ccb9293

Comment 21

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

I have a fix in the works for the intermittent marAppApplyUpdateSuccess.js | true == false failure in the try pushes.

Comment 22

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

Attachment: 7. test changes in progress

Comment 23

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

Attachment: 7. test changes in progress

Comment 24

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

Attachment: 6. fix xpcshell path for B2G

Comment 25

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

Attachment: 8. test changes

Comment 26

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

I re-triggered the test runs a ton of times on previous try pushes to catch and fix intermittent failures. I'll re-trigger the test runs on the last try push below as well.
 
Mac Try push
https://tbpl.mozilla.org/?tree=Try&rev=6d9aa1403232

Win Try push (not sure x64 will pass but the rest should)
https://tbpl.mozilla.org/?tree=Try&rev=453135462bed&showIgnore=1

Linux Try push
https://tbpl.mozilla.org/?tree=Try&rev=3e0bc08f5ce2

B2G Try push
https://tbpl.mozilla.org/?tree=Try&rev=8ec4c1b13538

All relevant platforms try push
https://tbpl.mozilla.org/?tree=Try&rev=09d7e25affe6

Comment 27

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

There are a couple of oranges I will deal with in a separate patch

Comment 28

[Brian R. Bondy [:bbondy]]

Comment on attachment 8351667 [details] [diff] [review]
6. fixes for nsXREDirProvider::GetUpdateRootDir

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

::: toolkit/xre/nsXREDirProvider.cpp
@@ +1012,3 @@
>      wchar_t regPath[1024] = { L'\0' };
>      swprintf_s(regPath, mozilla::ArrayLength(regPath), L"SOFTWARE\\%S\\%S\\TaskBarIDs",
> +               (hasVendor ? gAppData->vendor : "Mozilla"), MOZ_APP_BASENAME);

Is gAppData->vendor ever empty when MOZ_APP_VENDOR is defined. Is MOZ_APP_VENDOR always defined?
If MOZ_APP_VENDOR is not always defined, this is fine, else maybe use MOZ_APP_VENDOR here instead of "Mozilla".

@@ +1029,5 @@
>    // old handling.  We don't use the product name on purpose because we want a
>    // shared update directory for different apps run from the same path (like
>    // Metro & Desktop).
>    nsCOMPtr<nsIFile> localDir;
> +  if (pathHashResult && gAppData->name &&

Was this change intentional? Maybe instead do:
 if (pathHashResult && (hasVendor || gAppData->name) &&

Comment 29

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

(In reply to Brian R. Bondy [:bbondy] from comment #28)
> Comment on attachment 8351667 [details] [diff] [review]
> 6. fixes for nsXREDirProvider::GetUpdateRootDir
> 
> Review of attachment 8351667 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/xre/nsXREDirProvider.cpp
> @@ +1012,3 @@
> >      wchar_t regPath[1024] = { L'\0' };
> >      swprintf_s(regPath, mozilla::ArrayLength(regPath), L"SOFTWARE\\%S\\%S\\TaskBarIDs",
> > +               (hasVendor ? gAppData->vendor : "Mozilla"), MOZ_APP_BASENAME);
> 
> Is gAppData->vendor ever empty when MOZ_APP_VENDOR is defined. Is
> MOZ_APP_VENDOR always defined?
> If MOZ_APP_VENDOR is not always defined, this is fine, else maybe use
> MOZ_APP_VENDOR here instead of "Mozilla".
It is not always defined.

> @@ +1029,5 @@
> >    // old handling.  We don't use the product name on purpose because we want a
> >    // shared update directory for different apps run from the same path (like
> >    // Metro & Desktop).
> >    nsCOMPtr<nsIFile> localDir;
> > +  if (pathHashResult && gAppData->name &&
> 
> Was this change intentional? Maybe instead do:
>  if (pathHashResult && (hasVendor || gAppData->name) &&
Will do and thanks!

Comment 30

[Brian R. Bondy [:bbondy]]

Comment on attachment 8355110 [details] [diff] [review]
8. test changes

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

::: toolkit/mozapps/update/tests/unit_aus_update/head_update.js
@@ +419,5 @@
> +    catch (e) {
> +      dump("Unable to remove updates directory\n" +
> +           "Path: " + updatesDir.path + "\n" +
> +           "Exception: " + e + "\n");
> +      throw(e);

nit: extra parentheses aren't necessary

@@ +1028,5 @@
> +    }
> +    else {
> +      checkUpdateApplied();
> +    }
> +  }

Do you want checkUpdateApplplied to be skipped if callback is exactly null? If so this looks good.

@@ +1243,5 @@
> +  let fileRelPath = aFileRelPath;
> +  let srcFile = gGREDirOrig.clone();
> +  let pathParts = fileRelPath.split("/");
> +  let i;
> +  for (i = 0; i < pathParts.length; i++) {

nit: please put let inside the for( let i = 0
so it only exists in that scope, makes the reader look to see if it's used anywhere else otherwise

@@ +1254,5 @@
> +    logTestInfo("unable to copy file since it doesn't exist! Checking if " +
> +                 fileRelPath + DIR_APP_SUFFIX + " exists. Path: " +
> +                 srcFile.path);
> +    srcFile = gGREDirOrig.clone();
> +    for (i = 0; i < pathParts.length; i++) {

let i = 0

@@ +2202,5 @@
>   * and onload nsIDomEventListener handleEvent.
>   */
> +function makeHandler(aVal) {
> +  if (typeof aVal == "function")
> +    return ({ handleEvent: aVal });

nit: remove unseeded parentheses

::: toolkit/mozapps/update/tests/unit_aus_update/updateRootDirMigration_win.js
@@ +136,5 @@
>        relPath.forEach(relPathPart => {
>          oldFile.append(relPathPart);
>        });
> +    }
> +    else {

I though the convention for these was on the same line, but I don't mind either way personally.

Comment 31

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

(In reply to Brian R. Bondy [:bbondy] from comment #30)
> Comment on attachment 8355110 [details] [diff] [review]
> 8. test changes
> 
> Review of attachment 8355110 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/tests/unit_aus_update/head_update.js
> @@ +419,5 @@
> > +    catch (e) {
> > +      dump("Unable to remove updates directory\n" +
> > +           "Path: " + updatesDir.path + "\n" +
> > +           "Exception: " + e + "\n");
> > +      throw(e);
> 
> nit: extra parentheses aren't necessary
I changed this so it doesn't throw since it shouldn't exist anyways.

> 
> @@ +1028,5 @@
> > +    }
> > +    else {
> > +      checkUpdateApplied();
> > +    }
> > +  }
> 
> Do you want checkUpdateApplplied to be skipped if callback is exactly null?
> If so this looks good.
Yes for now.

> @@ +1243,5 @@
> > +  let fileRelPath = aFileRelPath;
> > +  let srcFile = gGREDirOrig.clone();
> > +  let pathParts = fileRelPath.split("/");
> > +  let i;
> > +  for (i = 0; i < pathParts.length; i++) {
> 
> nit: please put let inside the for( let i = 0
> so it only exists in that scope, makes the reader look to see if it's used
> anywhere else otherwise
Fixed

> @@ +1254,5 @@
> > +    logTestInfo("unable to copy file since it doesn't exist! Checking if " +
> > +                 fileRelPath + DIR_APP_SUFFIX + " exists. Path: " +
> > +                 srcFile.path);
> > +    srcFile = gGREDirOrig.clone();
> > +    for (i = 0; i < pathParts.length; i++) {
> 
> let i = 0
Fixed

> @@ +2202,5 @@
> >   * and onload nsIDomEventListener handleEvent.
> >   */
> > +function makeHandler(aVal) {
> > +  if (typeof aVal == "function")
> > +    return ({ handleEvent: aVal });
> 
> nit: remove unseeded parentheses
Fixed

> :::
> toolkit/mozapps/update/tests/unit_aus_update/updateRootDirMigration_win.js
> @@ +136,5 @@
> >        relPath.forEach(relPathPart => {
> >          oldFile.append(relPathPart);
> >        });
> > +    }
> > +    else {
> 
> I though the convention for these was on the same line, but I don't mind
> either way personally.
Fixed (I thought this was the convention... checked the style guide and you are correct. Thanks!)

Comment 32

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

Attachment: 5. fixes for nsXREDirProvider::GetUpdateRootDir rev 2

Comment 33

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

Attachment: 7. test changes rev 2

Comment 34

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

Comment on attachment 8351665 [details] [diff] [review]
3. build config changes rev3

Brian, are you comfortable with reviewing this since it is now much simpler and is the same as elsewhere in the tree?

Comment 35

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

Try push with updated patches
https://tbpl.mozilla.org/?tree=Try&rev=7057be2f304e

Try push with updated patches and MOZ_APP_VENDOR not defined
https://tbpl.mozilla.org/?tree=Try&rev=8fb917dbf7e5

Comment 36

[Brian R. Bondy [:bbondy]]

Comment on attachment 8351665 [details] [diff] [review]
3. build config changes rev3

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

looks good to me

Comment 37

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

Since try Win x64 is having problems I ran the tests several times on a local Win x64 build and all is well.

Comment 38

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

I am fairly certain I know what is going on with the marAppApplyUpdateStageSuccess.js failure and will work on a fix for it.

Comment 39

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

Attachment: 9. fix file removal race condition

I think I can come up with a better fix but I think this should suffice for now. I'll push it to try and trigger a bunch of Win7 and Win8 debug tests as well as run it a bunch of times locally.

Comment 40

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

Comment on attachment 8355387 [details] [diff] [review]
9. fix file removal race condition

bah... didn't fix it

Comment 41

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

Attachment: 8. fix file removal race condition in progress

Windows try run
https://tbpl.mozilla.org/?tree=Try&rev=cb43732373d9

Mac and Linux try run
https://tbpl.mozilla.org/?tree=Try&rev=15e1c2fd77c9

Comment 42

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

Comment on attachment 8355393 [details] [diff] [review]
8. fix file removal race condition in progress

I was able to get tests to fail with a debug build without this patch and I haven't been able to get tests to fail with a debug build with this patch so requesting review. I'll trigger a bunch on Windows debug test runs later tonight as well.

Comment 43

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

Several of the tests periodically fail when run in parallel with a debug build. Since the logging is to say the least minimal when running in parallel I'll add a file logger for the test to see if I can figure out what is going on.

Comment 44

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

Attachment: 9. handle munged update log

I believe I have seen this intermittently in the update log for my Firefox install and think it is pre-existing. It appears that the update log has data from running the updater twice quickly. for now I'd like to just account for this when comparing the update log and I'll file a bug to investigate further.

example of failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=32507182&tree=Try&full=1

Comment 45

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

Attachment: 11. declare gRegisteredServiceCleanup and add ability to log parallel tests to file

This adds a simple file logger so I can check locally what is going on in parallel test runs.

try push of all patches
https://tbpl.mozilla.org/?tree=Try&rev=581a81ee2afa

Comment 46

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

Attachment: 11. declare gRegisteredServiceCleanup and add ability to log parallel tests to file

updated the patch so it logs the test output from the failed parallel run at the start of the non-parallel run.

Comment 47

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

Attachment: 11. declare gRegisteredServiceCleanup and improve debugging

Primary changes:
1. ability to dump a parallel test failure to the sync run for the same test so it is easier to debug those failure.
2. declares gRegisteredServiceCleanup.
3. adds update status to the call to runUpdate so it simplifies dumping the update log, etc. when a test fails.

Comment 48

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

Comment on attachment 8349420 [details] [diff] [review]
3. update service fix undeclared var

There is a patch in bug 956468 for this so obsoleting

Comment 49

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

Attachment: 12. fix dir lock in cleanUpUpdatesDir, make helper sleep timeout consistant, etc.

This should be the final patch. It covers

1. fixes the updates patch dir from being in use caused by moving the update.log when enumerating directory entries.

2. fixes the pipe to null for Windows... it was still printing to the log file.

3. makes the helper sleep timeout consistent across tests.

4. makes the non-service staging tests use STATE_APPLIED instead of STATE_APPLIED_SVC.

Two Windows try pushes with lots of test runs without parallel debugging (I meant to push one with parallel debugging but screwed up)
https://tbpl.mozilla.org/?tree=Try&rev=5c1618e2cdfc
https://tbpl.mozilla.org/?tree=Try&rev=d5362e177cce

Try push for all platforms
https://tbpl.mozilla.org/?tree=Try&rev=d3eb4482ecb1

Comment 50

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

Attachment: 11. declare gRegisteredServiceCleanup and improve debugging rev2

Added FILE_UPDATE_SETTINGS_INI_BAK back since it is used by chrome tests. Carrying forward r+

Comment 51

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

Out of 840 test runs there was 1 failure in marRMRFDirFileInUseStageSuccessCompleteSvc_win.js.
https://tbpl.mozilla.org/php/getParsedLog.php?id=32810775&tree=Try

Since the log shows the update succeeded when it is supposed to fail it appears that the helper isn't locking the file. I'll investigate further but with this seldom happening I might land the patches as is.

I also checked parallel failures for XP. These parallel failures fon't seem out of hand and happen much less often than many other tests. I'm going to investigate this further as well but I won't hold up landing on figuring it out.

Latest XP Dbg 140 runs, 6 runs with parallel failures, 2 with 1 failure, 1 with 3 failures, and 3 with more than 5 failures
marFileInUseFallbackStageFailureComplete_win.js

marFileInUseStageSuccessPartial_win.js

marRMRFDirFileInUseSuccessPartial_win.js
marFileLockedFallbackStageFailurePartial_win.js
marFileInUseStageSuccessComplete_win.js
marFileInUseStageSuccessPartial_win.js
marRMRFDirFileInUseStageSuccessPartial_win.js
marFileInUseFallbackStageFailureComplete_win.js
marFileInUseFallbackStageFailurePartial_win.js
marRMRFDirFileInUseFallbackStageFailureComplete_win.js
marRMRFDirFileInUseFallbackStageFailurePartial_win.js
marRMRFDirFileInUseStageSuccessComplete_win.js

marRMRFDirFileInUseSuccessPartial_win.js
marFileLockedFallbackStageFailurePartial_win.js
marFileInUseSuccessComplete_win.js
marRMRFDirFileInUseFallbackStageFailureComplete_win.js
marFileInUseStageSuccessComplete_win.js
marFileInUseStageSuccessPartial_win.js
marRMRFDirFileInUseStageSuccessComplete_win.js
marRMRFDirFileInUseStageSuccessPartial_win.js
marFileInUseFallbackStageFailureComplete_win.js
marFileInUseFallbackStageFailurePartial_win.js
marRMRFDirFileInUseFallbackStageFailurePartial_win.js

marFileInUseStageSuccessComplete_win.js
marFileInUseStageSuccessPartial_win.js
marRMRFDirFileInUseStageSuccessComplete_win.js

marRMRFDirFileInUseSuccessPartial_win.js
marRMRFDirFileInUseSuccessComplete_win.js
marFileInUseStageSuccessComplete_win.js
marRMRFDirFileInUseStageSuccessPartial_win.js
marFileInUseStageSuccessPartial_win.js
marRMRFDirFileInUseStageSuccessComplete_win.js
marFileInUseFallbackStageFailureComplete_win.js
marFileInUseFallbackStageFailurePartial_win.js
marRMRFDirFileInUseFallbackStageFailureComplete_win.js
marRMRFDirFileInUseFallbackStageFailurePartial_win.js


Latest XP Opt 140 runs, 5 runs with parallel failures, 3 with 1 failure, 2 with more than 5 failures
marAppApplyUpdateStageSuccess.js

marFileInUseStageSuccessComplete_win.js

marRMRFDirFileInUseStageSuccessComplete_win.js

marFileInUseStageSuccessComplete_win.js
marFileInUseStageSuccessPartial_win.js
marRMRFDirFileInUseStageSuccessComplete_win.js
marRMRFDirFileInUseStageSuccessPartial_win.js
marRMRFDirFileInUseFallbackStageFailureComplete_win.js
marFileInUseFallbackStageFailureComplete_win.js
marFileInUseFallbackStageFailurePartial_win.js

marRMRFDirFileInUseStageSuccessComplete_win.js
marFileInUseStageSuccessComplete_win.js
marFileInUseStageSuccessPartial_win.js
marRMRFDirFileInUseStageSuccessPartial_win.js
marFileInUseFallbackStageFailureComplete_win.js
marRMRFDirFileInUseFallbackStageFailureComplete_win.js
marRMRFDirFileInUseFallbackStageFailurePartial_win.js
marFileInUseFallbackStageFailurePartial_win.js

Comment 52

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

Try run for all platforms after adding back FILE_UPDATE_SETTINGS_INI_BAK
https://tbpl.mozilla.org/?tree=Try&rev=b071697a5512

Comment 53

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

Results after doubling the sleep timeout for the helper from 40 seconds to 80 seconds with the following try run
https://tbpl.mozilla.org/?tree=Try&rev=67f3b8ea3865

1 test failure in marFileInUseStageSuccessCompleteSvc_win.js where the return code of the updater was 0 when it should have been 1 due to a file being locked. It is the orange that isn't starred on the page linked above.

Note: I only checked the XP logs due to the time it takes to go through them.

Latest XP Dbg 150 runs - 2 runs with parallel failures both of which had 1 failure

marAppApplyUpdateStageSuccess.js

marFileInUseStageSuccessCompleteSvc_win.js

Latest XP Opt 150 runs - 8 runs with parallel failures all of which had 1 failure

marAppInUseStageFailureComplete_win.js

marAppApplyUpdateAppBinInUseStageSuccess_win.js

marFileLockedStageFailurePartial_win.js

marFileLockedFallbackStageFailureComplete_win.js

marAppApplyUpdateAppBinInUseStageSuccess_win.js

marAppApplyUpdateAppBinInUseStageSuccess_win.js

marAppApplyUpdateAppBinInUseStageSuccess_win.js

marAppApplyUpdateSuccess.js (failed in parallel with a ton of other parallel failures as well as test_ev_certs.js failing)

So, it does appear that when running in parallel the helper sleep timeout wasn't nearly long enough. Also, I doubled the timeout because one of the failures with parallel debugging turned on showed over 40 seconds to complete the update and it had failed in the same place as the marFileInUseStageSuccessCompleteSvc_win.js failure above.

Here is another try run with the helper sleep timeout set to 180 which is what I will likely land with depending on the outcome of this try run.
https://tbpl.mozilla.org/?tree=Try&rev=e980d8061912

Comment 54

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

Results after modifying the sleep timeout for the helper 180 seconds with the following try run
https://tbpl.mozilla.org/?tree=Try&rev=e980d8061912

Dbg marFileInUseStageSuccessCompleteSvc_win.js where the update status was "" and "applied" was expected.

Latest XP 155 Dbg runs - 1 runs with parallel failures which had only 1 failure

marAppApplyUpdateAppBinInUseStageSuccess_win.js

Latest XP Opt 155 runs - 3 runs with parallel failures all of which had only 1 failure

marFileInUseStageSuccessComplete_win.js (failed in parallel with a ton of other parallel failures)

marFileInUseFallbackStageFailureComplete_win.js

marRMRFDirFileInUseFallbackStageFailurePartial_win.js

marAppApplyUpdateAppBinInUseStageSuccess_win.js

I'm going to go with 180 in the patch helper's sleep timeout

Comment 55

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

Attachment: 11. fix dir lock in cleanUpUpdatesDir, make helper sleep timeout consistant, etc. rev2

The only change from the previous patch is changing the sleep timeout for the helper from 40 to 180 which makes the tests pass much more often when they run in parallel... carrying forward r+ for this minor change.

Comment 56

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

Attachment: 10. declare gRegisteredServiceCleanup and improve debugging rev2 unbitrotted

Unbitrotting this patch

Comment 57

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

Attachment: combined patch for pushing

Comment 58

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

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/2377adfa7f5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1631d3a6042
https://hg.mozilla.org/integration/mozilla-inbound/rev/66d27b7d4fb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fde84bd35ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a5dceca698
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6a9cff371c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc647e2ac092
https://hg.mozilla.org/integration/mozilla-inbound/rev/b04e74ae3cb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6391c82f357d
https://hg.mozilla.org/integration/mozilla-inbound/rev/399d1a584acf
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4ed21fed28

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2377adfa7f5c
https://hg.mozilla.org/mozilla-central/rev/b1631d3a6042
https://hg.mozilla.org/mozilla-central/rev/66d27b7d4fb9
https://hg.mozilla.org/mozilla-central/rev/3fde84bd35ef
https://hg.mozilla.org/mozilla-central/rev/a3a5dceca698
https://hg.mozilla.org/mozilla-central/rev/5e6a9cff371c
https://hg.mozilla.org/mozilla-central/rev/cc647e2ac092
https://hg.mozilla.org/mozilla-central/rev/b04e74ae3cb7
https://hg.mozilla.org/mozilla-central/rev/6391c82f357d
https://hg.mozilla.org/mozilla-central/rev/399d1a584acf
https://hg.mozilla.org/mozilla-central/rev/7c4ed21fed28