Use a mock update directory for update tests

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

unspecified
mozilla29
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 18 obsolete attachments)

15.12 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
12.85 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
2.76 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
882 bytes, patch
bbondy
: review+
Details | Diff | Splinter Review
1009 bytes, patch
bbondy
: review+
Details | Diff | Splinter Review
3.38 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
484.06 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
7.49 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
1.45 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
72.27 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
103.10 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
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
Switches the term background with stage or staged to differentiate it from background updating where there is no user prompt.
Attachment #8349417 - Flags: review?(netzen)
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.
Attachment #8349418 - Flags: review?(netzen)
Saw this while debugging and decided to include it.
Attachment #8349420 - Flags: review?(netzen)
Posted patch 4. build config changes (obsolete) — Splinter Review
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.
Attachment #8349421 - Flags: review?(netzen)
Attachment #8349417 - Attachment is patch: true
Attachment #8349417 - Flags: review?(netzen) → review+
Attachment #8349418 - Flags: review?(netzen) → review+
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
Attachment #8349420 - Flags: review?(netzen) → review+
Comment on attachment 8349421 [details] [diff] [review]
4. build config changes

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

Please request from build peer
Attachment #8349421 - Flags: review?(netzen) → feedback+
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?
Attachment #8349421 - Flags: review?(gps)
(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
Posted patch 4. build config changes rev2 (obsolete) — Splinter Review
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?
Attachment #8349421 - Attachment is obsolete: true
Attachment #8350878 - Flags: review?(gps)
Found while trying to make the service and non-service tests more consistent.
Attachment #8351589 - Flags: review?(netzen)
Specifically, LogFinish was called to soon for the non sharing violation case.
Attachment #8351589 - Flags: review?(netzen) → review+
Posted patch 6 tests changes in progress (obsolete) — Splinter Review
Attachment #8349427 - Attachment is obsolete: true
Turns out that MOZ_APP_PROFILE isn't needed.
Attachment #8350878 - Attachment is obsolete: true
Attachment #8350878 - Flags: review?(gps)
Attachment #8351665 - Flags: review?(gps)
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.
Attachment #8351593 - Attachment is obsolete: true
Attachment #8351667 - Flags: review?(netzen)
(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.
I have a fix in the works for the intermittent marAppApplyUpdateSuccess.js | true == false failure in the try pushes.
Posted patch 7. test changes in progress (obsolete) — Splinter Review
Attachment #8351668 - Attachment is obsolete: true
Posted patch 7. test changes in progress (obsolete) — Splinter Review
Attachment #8355103 - Attachment is obsolete: true
Attachment #8355104 - Attachment is obsolete: true
Attachment #8355108 - Flags: review?(netzen)
Posted patch 8. test changes (obsolete) — Splinter Review
Attachment #8355110 - Flags: review?(netzen)
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
There are a couple of oranges I will deal with in a separate patch
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) &&
Attachment #8351667 - Flags: review?(netzen) → review+
Attachment #8355108 - Flags: review?(netzen) → review+
(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 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.
Attachment #8355110 - Flags: review?(netzen) → review+
(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!)
Attachment #8355110 - Attachment is obsolete: true
Attachment #8355285 - Flags: review+
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?
Attachment #8351665 - Flags: review?(netzen)
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 on attachment 8351665 [details] [diff] [review]
3. build config changes rev3

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

looks good to me
Attachment #8351665 - Flags: review?(netzen) → review+
Since try Win x64 is having problems I ran the tests several times on a local Win x64 build and all is well.
OS: Windows 7 → All
I am fairly certain I know what is going on with the marAppApplyUpdateStageSuccess.js failure and will work on a fix for it.
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.
Attachment #8355387 - Flags: review?(netzen)
Comment on attachment 8355387 [details] [diff] [review]
9. fix file removal race condition

bah... didn't fix it
Attachment #8355387 - Attachment is obsolete: true
Attachment #8355387 - Flags: review?(netzen)
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.
Attachment #8355393 - Flags: review?(netzen)
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.
Attachment #8355393 - Flags: review?(netzen) → review+
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
Attachment #8355616 - Flags: review?(netzen)
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
Attachment #8355616 - Flags: review?(netzen) → review+
updated the patch so it logs the test output from the failed parallel run at the start of the non-parallel run.
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.
Attachment #8355678 - Attachment is obsolete: true
Attachment #8356225 - Attachment is obsolete: true
Attachment #8357388 - Flags: review?(netzen)
Comment on attachment 8349420 [details] [diff] [review]
3. update service fix undeclared var

There is a patch in bug 956468 for this so obsoleting
Attachment #8349420 - Attachment is obsolete: true
Attachment #8349420 - Flags: review+
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
Attachment #8358320 - Flags: review?(netzen)
Attachment #8357388 - Flags: review?(netzen) → review+
Attachment #8358320 - Flags: review?(netzen) → review+
Added FILE_UPDATE_SETTINGS_INI_BAK back since it is used by chrome tests. Carrying forward r+
Attachment #8357388 - Attachment is obsolete: true
Attachment #8358581 - Flags: review+
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
Try run for all platforms after adding back FILE_UPDATE_SETTINGS_INI_BAK
https://tbpl.mozilla.org/?tree=Try&rev=b071697a5512
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
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
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.
Attachment #8358320 - Attachment is obsolete: true
Attachment #8358893 - Flags: review+
Attachment #8358895 - Attachment is obsolete: true
Attachment #8351665 - Attachment description: 4. build config changes rev3 → 3. build config changes rev3
Attachment #8351589 - Attachment description: 5. fix updater logging and make file in use text consistent → 4. fix updater logging and make file in use text consistent
Attachment #8355284 - Attachment description: 6. fixes for nsXREDirProvider::GetUpdateRootDir rev 2 → 5. fixes for nsXREDirProvider::GetUpdateRootDir rev 2
Attachment #8355108 - Attachment description: 7. fix xpcshell path for B2G → 6. fix xpcshell path for B2G
Attachment #8355285 - Attachment description: 8. test changes rev 2 → 7. test changes rev 2
Attachment #8355393 - Attachment description: 9. fix file removal race condition in progress → 8. fix file removal race condition in progress
Attachment #8355616 - Attachment description: 10. handle munged update log → 9. handle munged update log
Attachment #8358894 - Attachment description: 11. declare gRegisteredServiceCleanup and improve debugging rev2 unbitrotted → 10. declare gRegisteredServiceCleanup and improve debugging rev2 unbitrotted
Attachment #8358893 - Attachment description: 12. fix dir lock in cleanUpUpdatesDir, make helper sleep timeout consistant, etc. rev2 → 11. fix dir lock in cleanUpUpdatesDir, make helper sleep timeout consistant, etc. rev2
Attachment #8349439 - Attachment is obsolete: true
Duplicate of this bug: 894728
Duplicate of this bug: 894730
Duplicate of this bug: 932006
Duplicate of this bug: 932023
Duplicate of this bug: 821034
You need to log in before you can comment on or make changes to this bug.