Closed Bug 711834 Opened 8 years ago Closed 8 years ago

Maintenance service: Improve xpcshell error logging and don't allow fallback to UAC prompt for updates

Categories

(Toolkit :: Application Update, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

Attachments

(2 files, 5 obsolete files)

When the service command cannot be launched from the unelevated updater.exe, we should not fallback to displaying a UAC prompt for XPCShell tests.

Also we should write out the reason why there was a failure so it will be logged properly in the XPCShell test log.

To avoid the need for manual testing to remove the fallback key, we should also set a user environment variable from XPCShell tests for whether we should even consider the fallback key or not.
Attached patch Patch v1. (obsolete) — Splinter Review
Attachment #582668 - Flags: review?(robert.bugzilla)
Note: There should still be an intermittent problem after this patch, but this patch will allow us to see what the error is and should no longer fail the test due to timeout
Was getting a timeout kill error again but realized that it's because we need new maintenance services installed on the build server for bug 711792.  I've reverted bug 711792 from elm and re-included this bug on elm.  I'm retrying a build now which will hopefully reveal the intermittent error.
Attached patch Patch v2. (obsolete) — Splinter Review
Some extra cleanup.
Attachment #582668 - Attachment is obsolete: true
Attachment #582668 - Flags: review?(robert.bugzilla)
Attachment #582725 - Flags: review?(robert.bugzilla)
Attached patch Patch v3. (obsolete) — Splinter Review
It felt wrong for me to have an env var saying it was an XPCShell test and then have code specific to XPCShell tests in updater.cpp.

Instead I renamed this to MOZ_NO_SERVICE_FALLBACK and removed the XPCShell dependent wording.

Yes we use this only from XPCShell tests currently, but it is better to name it by what it does and not by what it is used for.
Attachment #582725 - Attachment is obsolete: true
Attachment #582725 - Flags: review?(robert.bugzilla)
Attachment #582737 - Flags: review?(robert.bugzilla)
Comment on attachment 582737 [details] [diff] [review]
Patch v3.

Looks good
Attachment #582737 - Flags: review?(robert.bugzilla) → review+
Attachment #583339 - Flags: review?(robert.bugzilla)
Forgot to clear out the important env var that I told you about. Ignore the previous purple errors on tbpl by the way.
Attachment #583382 - Flags: review?(robert.bugzilla)
Attachment #583339 - Attachment is obsolete: true
Attachment #583339 - Flags: review?(robert.bugzilla)
Comment on attachment 583382 [details] [diff] [review]
Clear out env variable after getting value from unelevated updater.exe. v2.

Do you think it would be a good thing to make runningAsTest a bool like you did for usingService?
Attachment #583382 - Flags: review?(robert.bugzilla) → review+
Carrying forward r+
- Changed from LPCWSTR to bool
- renamed var name runningAsTest to noServiceFallback 
- added && !noServiceFallback to the fallback UAC check which may fix the purples on win opt.
Attachment #583382 - Attachment is obsolete: true
Attachment #583528 - Flags: review+
Carrying forward r+.
Synced to elm / rebased.
Attachment #582737 - Attachment is obsolete: true
Attachment #584001 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.