bgupdates breaks uninstalls if you uninstall after an update

RESOLVED FIXED in mozilla15

Status

()

Toolkit
Application Update
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bbondy, Assigned: Ehsan)

Tracking

12 Branch
mozilla15
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 307181
No longer depends on: 307181
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'
(Reporter)

Comment 2

6 years ago
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.
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.
(Reporter)

Comment 4

6 years ago
> 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?
(Reporter)

Comment 5

6 years ago
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
(Reporter)

Comment 6

6 years ago
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
(Assignee)

Comment 7

6 years ago
I'll look into this more deeply, should be easy to fix...
Assignee: nobody → ehsan
(Reporter)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
(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.
(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.
(Assignee)

Comment 11

6 years ago
(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

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
(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.
Agreed... that would be the best stage to run it.
(Assignee)

Comment 15

6 years ago
Created attachment 628501 [details] [diff] [review]
Patch (v1)
Attachment #628501 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 16

6 years ago
Created attachment 628504 [details] [diff] [review]
Patch (v1)

Fixed a typo, sorry.
Attachment #628501 - Attachment is obsolete: true
Attachment #628501 - Flags: review?(robert.bugzilla)
Attachment #628504 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

6 years ago
Depends on: 758998
(Assignee)

Comment 17

6 years ago
Created attachment 628541 [details] [diff] [review]
Patch (v2)

We should also be passing the installation directory to LaunchWinPostProcess, not the callback file name.
Attachment #628504 - Attachment is obsolete: true
Attachment #628504 - Flags: review?(robert.bugzilla)
Attachment #628541 - Flags: review?(robert.bugzilla)
(Reporter)

Updated

6 years ago
Duplicate of this bug: 760155
(Assignee)

Comment 19

6 years ago
FWIW, I verified this fix on the Oak branch.
Attachment #628541 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 20

6 years ago
https://hg.mozilla.org/mozilla-central/rev/30babf8e5573
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.