759615 - bgupdates breaks uninstalls if you uninstall after an update

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

1. Install the second newest Nightly build of m-c.
2. Do an update
3. Try to uninstall.

Actual result:
You get an error "An error occurred while tying to uninstall Nightly 15.0a1 (x86 en-US). It may have already been uninstalled.

Expected result:
The uninstall works.

I think what's happening is the PostUpdate sets the install location to installdir/updated.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Brian, does it for you also apply to the silent mode of the uninstaller? At least I can't see this issue with our Mozmill update tests which do exactly the same but with the silent mode.

> [mozilla-central_update] $ cmd.exe /C '"mozmill-env\run mozmill-automation\download.py --type=%BUILD_TYPE% --branch=mozilla-central --platform=%PLATFORM% --locale=%LOCALE% --build-id=%BUILD_ID% --directory=builds && exit %%ERRORLEVEL%%"'
> Retrieving list of builds from https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/05/
> Downloading build: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/05/2012-05-29-03-05-18-mozilla-central/firefox-15.0a1.en-US.win32.installer.exe
> [mozilla-central_update] $ cmd.exe /C '"mozmill-env\run mozmill-automation\testrun_update.py --target-buildid=%TARGET_BUILD_ID% --no-fallback --repository=mozmill-tests --junit=report.xml --report=%REPORT_URL% builds && exit %%ERRORLEVEL%%"'
> *** Cloning repository to 'c:\docume~1\mozilla\locals~1\temp\tmpzbcpmb.mozmill-tests'
> requesting all changes
> adding changesets
> adding manifests
> adding file changes
> added 2250 changesets with 5740 changes to 680 files (+5 heads)
> updating to branch default
> 346 files updated, 0 files merged, 0 files removed, 0 files unresolved
> *** Installing 2012-05-29-03-05-18-mozilla-central-firefox-15.0a1.en-US.win32.installer.exe => c:\docume~1\mozilla\locals~1\temp\tmplidfaa.binary\
> *** Updating to branch 'default'
> pulling from mozmill-tests
> searching for changes
> no changes found
> 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
[...]
> INFO Passed: 7
> INFO Failed: 0
> INFO Skipped: 0
> Report document created at 'http://mozmill-ci.blargon7.com/db/fdec829b93b19c73985be1d38820744b'
> *** Uninstalling c:\docume~1\mozilla\locals~1\temp\tmplidfaa.binary\
> *** Removing repository 'c:\docume~1\mozilla\locals~1\temp\tmpzbcpmb.mozmill-tests'

Comment 2

[Brian R. Bondy [:bbondy]]

I suspect the mozmill script is not using background updates at all or else is only doing the apply operation and not the replace operation. I think it would apply to both silent and not although I haven't tested it with silent.

I did notice if you create a folder called updated and copy everything into it, then uninstall with the uninstall option in control panel, it will find the uninstall/helper.exe in that case and launch the uninstall wizard.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Our scripts are using the old update ui which downloads and applies the update immediately before the restart of the browser. So I thought it could be the same code path. But yeah, could be a bit different.

Comment 4

[Brian R. Bondy [:bbondy]]

> Our scripts are using the old update ui 

Is there a reason why we can't always use the current update UI code?  I don't know much of how mozmill works so sorry for the potential ignorance embedded in that question.

> ...which downloads and applies the 
> update immediately before the restart of the browser.  So I thought it could be the same code path.

That sounds like it's using background updates, so should be the same code path.

> *** Uninstalling c:\docume~1\mozilla\locals~1\temp\tmplidfaa.binary\
> *** Removing repository 'c:\docume~1\mozilla\locals~1\temp\tmpzbcpmb.mozmill-tests'


The silent unisntall option probably just fails silently.  Do you actually check if the directory is empty after uninstall?  By the way I often see bugs that are found towards the end of a release cycle about a folder or file not being removed from the uninstaller.  Could you explicitly check if the directory is empty or doesn't exist after an uninstall?

Comment 5

[Brian R. Bondy [:bbondy]]

More info:

The /PostUpdate process can be started from one of these locations in updater.cpp:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#2435
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#2754

Or here from workmontior.cpp:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/maintenanceservice/workmonitor.cpp#251


What executes can be found here:
http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/shared.nsh#5

It looks as though this line gets the wrong install dir inside SetUninstallKeys:
http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/shared.nsh#480

> ${WriteRegStr2} $1 "$0" "UninstallString" "$8\uninstall\helper.exe" 0

Comment 6

[Brian R. Bondy [:bbondy]]

Before an upgrade with bgupdates:
HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\Microsoft\Windows\CurrentVersion\Uninstall\Nightly 15.0a1 (x86 en-US)
value: C:\Program Files (x86)\Nightly\uninstall\helper.exe

After an upgrade with bgupdates:
HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\Microsoft\Windows\CurrentVersion\Uninstall\Nightly 15.0a1 (x86 en-US)
C:\Program Files (x86)\Nightly\updated\uninstall\helper.exe

Comment 7

[(no longer active)]

I'll look into this more deeply, should be easy to fix...

Comment 8

[Brian R. Bondy [:bbondy]]

I think the best way to fix this bug is if you're using background updates, then don't run the post upgrade process until after the replace stage.  Currently it runs after the apply stage but before the replace stage.

Comment 9

[(no longer active)]

(In reply to Brian R. Bondy [:bbondy] from comment #8)
> I think the best way to fix this bug is if you're using background updates,
> then don't run the post upgrade process until after the replace stage. 
> Currently it runs after the apply stage but before the replace stage.

Agreed.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Brian R. Bondy [:bbondy] from comment #4)
> Is there a reason why we can't always use the current update UI code?  I
> don't know much of how mozmill works so sorry for the potential ignorance
> embedded in that question.

We haven't been had a chance yet to fully move to the new ui. Mainly because the old UI is still in place when incompatible add-ons will be disabled on a major upgrade. For more details see bug 599290.

> The silent unisntall option probably just fails silently.  Do you actually
> check if the directory is empty after uninstall?  By the way I often see

No, we don't check that. The uninstaller which will soon be part of mozinstall will simply remove the remaining installation folder. So we haven't seen that behavior. 

> file not being removed from the uninstaller.  Could you explicitly check if
> the directory is empty or doesn't exist after an uninstall?

It's not something we could easily integrate into a Mozmill test. Therefore we would have to create tests on the Python side. So far any test is run inside the browser and reports also only include those results.

Clint and Jeff, I think it would be a good idea to extend the Python side of Mozmill to easily allow additional tests and include those into the log/report.

Comment 11

[(no longer active)]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Clint and Jeff, I think it would be a good idea to extend the Python side of
> Mozmill to easily allow additional tests and include those into the
> log/report.

Can we please have that discussion somewhere else?  As it's not related to the bug at hand.

Comment 12

[cmtalbert]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> It's not something we could easily integrate into a Mozmill test. Therefore
> we would have to create tests on the Python side. So far any test is run
> inside the browser and reports also only include those results.
> 
> Clint and Jeff, I think it would be a good idea to extend the Python side of
> Mozmill to easily allow additional tests and include those into the
> log/report.
Mozmill has no notion of install/uninstall. This has to happen outside of the tool, so therefore this is better suited to living in mozinstall and up leveling the failure status into the wrapping automation scripts. This is something that AutomatedTester's tool will be needing to do for Apps anyway, so work is already underway. So, yes, this is something we can do, it's not part of mozmill proper, it's part of the code that calls mozmill in your automation. We can take this up in a follow on bug (feel free to file) so we don't hijack Ehsan's bug here.

Comment 13

[(no longer active)]

(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> (In reply to Brian R. Bondy [:bbondy] from comment #8)
> > I think the best way to fix this bug is if you're using background updates,
> > then don't run the post upgrade process until after the replace stage. 
> > Currently it runs after the apply stage but before the replace stage.
> 
> Agreed.

Robert, do you also agree with this?  I'd like to know which kind of fix you think is appropriate here.

Comment 14

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

Agreed... that would be the best stage to run it.

Comment 15

[(no longer active)]

Attachment: Patch (v1)

Comment 16

[(no longer active)]

Attachment: Patch (v1)

Fixed a typo, sorry.

Comment 17

[(no longer active)]

Attachment: Patch (v2)

We should also be passing the installation directory to LaunchWinPostProcess, not the callback file name.

Comment 19

[(no longer active)]

FWIW, I verified this fix on the Oak branch.

Comment 20

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/30babf8e5573