Closed Bug 1419594 Opened 2 years ago Closed 2 years ago

Tests can fail when nsUpdateService.js is writing the xml files


(Toolkit :: Application Update, defect, P2)




Tracking Status
firefox59 --- fixed


(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)



(1 file, 3 obsolete files)

Saw this while doing some other work and I suspect this may be one of the reasons for the increase in app update test intermittent failures.
Attached patch patch rev1 (obsolete) — Splinter Review
Attached patch patch rev1 (obsolete) — Splinter Review
Attachment #8930750 - Attachment is obsolete: true
Comment on attachment 8930752 [details] [diff] [review]
patch rev1

I found around 40 parallel runs would fail out of 300 and with these changes around 1 parallel run out of 300 fail.

I've also added back code to make it easier to locally figure out why a parallel run intermittently fails.
Attachment #8930752 - Flags: review?(mhowell)
Comment on attachment 8930752 [details] [diff] [review]
patch rev1

Review of attachment 8930752 [details] [diff] [review]:

Can you link an example of a failure that this fixes? Not that I really need that to review this patch, it's straightforward enough and you've already verified it works, I'm just curious.

::: toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js
@@ +794,5 @@
> +      gRealDump = dump;
> +      dump = dumpOverride;
> +    }
> +  } else if (gDeleteLogFile) {
> +    if (logFile.exists()) {

Haven't initialized logFile on this branch.

@@ +795,5 @@
> +      dump = dumpOverride;
> +    }
> +  } else if (gDeleteLogFile) {
> +    if (logFile.exists()) {
> +      logFile.remove(false);

I don't quite get what gDeleteLogFile is supposed to be for; the way it's implemented here (delete the existing log at the start of the test, but only if we're not about to do this logging for this run), I can't really see what the use case would be.

@@ +1238,5 @@
>    }
>  }
>  /**
> + * Waits until the update files exists or not based on the parameters specified

"exists" should be "exist"
Attachment #8930752 - Flags: review?(mhowell) → review-
Here is one example:
"message":"NS_ERROR_FILE_IS_LOCKED: Component returned failure code: 0x8052000e (NS_ERROR_FILE_IS_LOCKED) [nsIFileOutputStream.init]","stack":"writeFile@../data/xpcshellUtilsAUS.js -> file:///c:/moz/_1_mozilla-central/ff-x64-opt/_tests/xpcshell/toolkit/mozapps/update/tests/data/shared.js:278:3
writeUpdatesToXMLFile@../data/xpcshellUtilsAUS.js -> file:///c:/moz/_1_mozilla-central/ff-x64-opt/_tests/xpcshell/toolkit/mozapps/update/tests/data/shared.js:201:3
Attached patch patch rev2 (obsolete) — Splinter Review
I removed gDeleteLogFile and the associated code. It was mainly used in the old logging code but doesn't provide much value now.
Attachment #8930752 - Attachment is obsolete: true
Attachment #8931040 - Flags: review?(mhowell)
Out of 300 runs last night only 1 parallel run failed but it failed outside of the test after the test run had finished.
Comment on attachment 8931040 [details] [diff] [review]
patch rev2

Review of attachment 8931040 [details] [diff] [review]:

All right, sounds good. Thanks!
Attachment #8931040 - Flags: review?(mhowell) → review+
Attached patch patch rev3Splinter Review
Forgot to change gDebugTestLog in the last patch... carrying forward r+
Attachment #8931040 - Attachment is obsolete: true
Attachment #8931048 - Flags: review+
Pushed by
test only - make update tests less likely to fail during async writes of the update xml files. r=mhowell
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.