858686 - Uninstallation code interferes with the Firefox uninstaller and causes update failures due to files still in use

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

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

So this is kinda interesting. While checking for bug 803489 I have seen that we get an update status of '7' for the zh-TW locale (20.0b6 -> whatever current). Not sure what that update status means (Rob can you please tell me?) but with that a modal dialog gets opened which mentions that the update cannot be applied. It seems like that Mozmill in it's current state cannot kill the application and runs into an application disconnect. That's majorly a mozmill bug so I will have to file another bug for it.

In case of our tests that means that the already downloaded update files are still located in 'C:\Users\mozauto\AppData\Local\Mozilla\Firefox\firefox'. With the next start of Firefox it automatically tries to apply this already downloaded update and fails with 'The update could not be installed. Please make sure there are no other copies of Firefox running on your computer, and then restart Firefox to try again'. 

What we are seeing here is the updater application and not Firefox itself. So due its blocking Firefox never gets launched and jsbridge cannot establish the connection to Firefox and fails with the failure as reported in bug 803489.

Similar to that Rob asked me on bug 816472 comment 32 why our update tests are not starting at 0KB. So the above mentioned behavior is totally the cause of that.

Rob it would be great if you can give me some information about the update status 7 error. Also is there a way to force Firefox to cache the update files inside the binary folder? That one we know and delete after each testrun. Finding the %app_data% folder might be a bit hard. Is that only a behavior on Windows or are other platforms involved too?

Comment 1

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

So we might possibly trap into issues with the background update here in conjunction with our fallback updates. I'm not sure if those are valid anymore, at least not as long as the update is getting applied in the background.

1. We download the update
2. The update gets applied
3. We change the status file and add 'status: 6'
4. We restart Firefox
5. Firefox detects that something went wrong with the update and downloads the full mar file

Given that we already have applied the update in step 2, could it be that we mess up with the application binary files when marking the update as broken afterward?

Is it still required to run the fallback updates? Anthony?

If yes, would there be a way to disable applying it in the background right after its download? Rob?

Comment 2

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> Is it still required to run the fallback updates? Anthony?

I believe it is still required, yes. I'd prefer not to disable fallback update tests unless  there is a serious Mozmill tests regression which is impacts our ability to trust the results.

As long as fallback updates are a valid update path we will need to test them. If we don't have trustworthy automation this will need to be checked manually, dramatically increasing the effort to QA a release.

Rob would be the best person to answer whether it remains a valid update path or not.

Comment 3

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

Ehsan worked on the 'apply update in the background' feature on bug 307181. So he might also have some helpful information for us in how we should proceed with the tests.

I wouldn't stop running our tests with that feature given that it is the default behavior of Firefox now.

Comment 4

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

7 is a write error. Other errors for the next time
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/common/errors.h

You can see the different cases where WRITE_ERROR is returned
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp

and the log file for the updater provides more specifics as to the operation that failed.

There is no way to always force the files to be cached in the binary folder.

Other platforms will also use a directory outside of the installation directory in the future. Follow bug 394984 which is extremely likely to be the next one. It will be using UpdRootD to get the directory so only code that isn't using UpdRootD correctly will have a problem with the change.

Comment 5

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

So the reason why we are failing is:

EXECUTE PATCH maintenanceservice_installer.exe
rename_file: failed to rename file - src: maintenanceservice_installer.exe, dst:maintenanceservice_installer.exe.moz-backup, err: 13
### execution failed

This is for the direct update which is performed after the fallback test. The fallback always passes. Same for the direct update if no fallback is run before. And it only occurs if fallback updates have been ran right before the direct updates AND the folder 'C:\Users\mozauto\AppData\Local\Mozilla\Firefox\firefox' exists right from the beginning. If this folder doesn't exist both type of tests are successful.

1. Have a clean system
2. Run update tests (fallback + direct) => success
3. Run update tests (fallback + direct) => failure
4. Remove 'C:\Users\mozauto\AppData\Local\Mozilla\Firefox\firefox'
5. Run update tests (fallback + direct) => success
6. Run update tests (fallback + direct) => failure
7. Run update tests (direct) => success

I can check what specifically in that folder is causing us to fail when we run the fallback tests before the direct tests.

Comment 6

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

It seems to be not dependent on any of the files which are located in that folder. Whatever I remove before it results in a passing testrun.

So what is the correlation between the folder and maintenanceservice_installer.exe?

Comment 7

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

It might still be in use when you are trying to simulate the conditions with mozmill.

Comment 8

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

Hm, between steps 2 and 3 I was waiting about a minute. But even if the direct update test should wait a bit after the fallback has been finished why does it work on a clean system? I will add some sleeps to see if that changes something.

Comment 9

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

Which is why I said "It *might* still be in use". All I know is that we have tons of tests in the tree that don't have these problems and that the mozmill tests simulate the required conditions in ways that are fragile and often don't reflect the conditions for real clients. Getting the mozmill tests to more closely reflect the conditions for real clients would likely make them more robust and solve this bug. The last time I went through the mozmill test code there were several changes I made to lessen this fragility but it also took quite a bit of my time which prevents me from the work I am already tasked with to improve the experience for Firefox users... it is a tough trade off between the two.

Comment 10

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

So the actual problem here are not the tests but the scripts which drive the installation, uninstallation, and execution. For fallback updates we make a backup of the binary and restore it after the first update test. Therefore we are trying to delete the folder which contains the updated binary. Given that the maintenance service application is still running we seem to fail in removing it completely. Then we are copying the backup-ed files into that folder and are doing the direct updates.

A simple wait before removing the binary folder did the trick. Given that we do not know how long it takes until every file is released we should check that in a loop and only continue when we were able to remove all the files. If we still fail we should report a failure. See bug 857956 for that, which is currently based on the same symptom.

A fix here should be easy and I will try to come up with one over the weekend.

Comment 11

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

Ok, so handling the downloaded patches after our tests I would forward to another bug given that it is a bit more complicated. This one is an important fix to get our updates working again across platforms.

The patch which I will upload in a bit will retry to delete the binary folder within 15s and if it still isn't able to after that timeout we raise a failure.

Comment 12

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

Attachment: Patch v1

Something we might also have to check is how mozinstall handles the uninstallation and if it is also affected by this issue.

Comment 13

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

Attachment: Patch v1.1

Comment 14

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

Comment on attachment 734559 [details] [diff] [review]
Patch v1.1

Unrequesting review given that the patch doesn't seem to fix the problem for real. The maintenance service file doesn't seem to be part of the binary we are trying to delete.

Comment 15

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

Comment on attachment 734559 [details] [diff] [review]
Patch v1.1

Moved this patch over to bug 857956 which better fits its purposal. I will have to investigate more for this bug.

Comment 16

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

I'm not sure how the uninstaller works, and in this case the silent one. But our code around it seems to interfere with its actions. We only wait until the log file created by the installer has been removed and then quit our script. With that the uninstaller is not able to remove all installed files and especially a part of /browser will still be around.

I have now modified the code so we are waiting for the whole installation folder to be gone. The timeout therefor is 20s. With that the installer finally removes all the installed files, and most likely stops the maintenanceservice installer or whatever it is doing. That said, with exactly this change I can no longer reproduce the failure as reported by this bug. I did about 20 different runs across different versions and locales.

I will do some more tests and will have a patch uploaded soon.

Comment 17

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

Attachment: Patch v1

Patch but untested so far. I will test all platforms and request review if no regressions can be spotted.

Comment 18

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

Robert, could you please give us a little bit of feedback how the maintenance service installer file is used? Or if you haven't implemented it please let us know whom we have to ask. It would be good to know when we launch that installer and how long it will be kept open. Until we solved this problem I wonder if there is a way to temporarily disable the maintenance service. Thanks.

Comment 19

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

Since the landing of my patch on bug 857956 I have no longer seen this problem. It looks like that this fixed it and that the uninstaller is not affected. I will wait some days and will closely watch the processing of updates on this win8 node.

Comment 20

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

This came back today by the ondemand update testrun QA was performing for releasetest of Firefox 20.0.1. We are not done yet. :(

Two next steps are:
1. Remove already downloaded updates before we start our update tests
2. Also uninstall the maintenance service after an update testrun on windows.

Comment 21

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

So with all my work recently for mozprocess, I can believe that the problem as we are facing here with subprocess.call() all comes together because we return too early from that method. With mozprocess we will have a way better integration, and everything already fixed. So I would suggest that we finally kill our own mozinstall code and replace it finally with the official one. This work is being tracked at https://github.com/mozilla/mozmill-automation/issues/127.

Given that we are running with the attached patch nearly a year now, I don't think that I will revert the change but will make sure that mozinstall handles it correctly.