1155743 - firefox-ui-tests hits Windows UAC prompt because mozinstall uses older options to extract Windows installers

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Armen [:armenzg]]

Proposed solution:
------------------
If we extract instead of installing we can work around this issue.
We could teach mozfile to extract the stub installer and mozinstall to call to that instead of trying to use /C /D with the stub installer.

This bug doesn't really block me as I can work around it, however, it would be ideal not to work around it.

##########################

On Windows, running the firefox-ui-updates is triggers the UAC prompt.

Internally, we're calling: [2]

"C:\Users\Mozilla\repos\build-tools\release\updates\downloads\Firefox
    Setup 38.0b3.exe" /S
    /D="C:\Users\Mozilla\AppData\Local\Temp\tmpvrbzlg\Firefox Setup 38"

which first self-extracts to a temp dir and then it tries to instal into the temp dir we specify.
The self-extraction does not trigger the UAC prompt but the setup.exe file that comes out of it (use 7zip to see contents of Stub Installer).
This is why you would notice a delay on seeing the UAC.

The releng unpacking scripts call this [3]:

    > $ 7z x ../downloads/Firefox\ 38.0b3.exe

I can work around it by calling releng's unpack_build() and then using --binary, however, I would have to determine paths to the binaries on the releng script.

I would like us to switch to use mozfile and teach it how to extract .exe files. [4]

My suspicion is that, even though, we're not extracting to "Program Files" (which requires UAC prompt), it is because the setup.exe file inside of the stub installer is actually called "Firefox Installer" (which contains the sub-string "Install" which triggers the UAC). 

[1] https://github.com/mozilla/firefox-ui-tests/blob/master/firefox_ui_harness/runtests.py#L33
[2] http://hg.mozilla.org/mozilla-central/file/d0a6539a1ca8/testing/mozbase/mozinstall/mozinstall/mozinstall.py#l283
[3] https://github.com/mozilla/build-tools/blob/master/release/common/unpack.sh#L49
[4] http://hg.mozilla.org/mozilla-central/file/d0a6539a1ca8/testing/mozbase/mozfile/mozfile/mozfile.py#l77

Comment 1

[Armen [:armenzg]]

Per Rob Strong:
> There are several HKLM registry keys that are required if Firefox is to be installed with full OS
> integration so it will always try to elevate so it can accomplish this.

Comment 2

[Armen [:armenzg]]

We can simply use this instead:
./Firefox\ Setup\ 38.0b3.exe /extractdir=C:\\Users\\Mozilla\\repos\\build-tools\\release\\updates\\downloads\\temp5

Comment 3

[Armen [:armenzg]]

Attachment: extract.diff

I could not make it work with subprocess.call() or mozprocess.
I would get this error:
InstallError: Failed to install "c:\Users\Mozilla\repos\build-tools\release\updates\downloads\Firefox Setup 38.0b3.exe ([Error 5] Access is denied.)"

Comment 4

[Armen [:armenzg]]

Attachment: extract.diff

The diff was based of m-c which did not contain the first part of the patch (which has landed on m-i).

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8594080 [details] [diff] [review]
extract.diff

Review of attachment 8594080 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nit fixed.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py
@@ +279,5 @@
>      dest = os.path.join(dest, filename.split('.')[0])
>  
>      # possibly gets around UAC in vista (still need to run as administrator)
>      os.environ['__compat_layer'] = 'RunAsInvoker'
> +    result = os.system('"%s /extractdir=%s' % (src, os.path.realpath(dest)))

Please continue to use subprocess. If you need to invoke the shell, you can pass "shell=True" into subprocess.call(), though I don't think you need to:
https://docs.python.org/2/library/subprocess.html#replacing-os-system

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8594080 [details] [diff] [review]
extract.diff

Oops, looks like I had loaded the review before it got obsoleted :)

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8594108 [details] [diff] [review]
extract.diff

Review of attachment 8594108 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nit fixed.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py
@@ +279,5 @@
>      dest = os.path.join(dest, filename.split('.')[0])
>  
>      # possibly gets around UAC in vista (still need to run as administrator)
>      os.environ['__compat_layer'] = 'RunAsInvoker'
> +    result = os.system('"%s /extractdir=%s' % (src, os.path.realpath(dest)))

Please continue to use subprocess. If you need to invoke the shell, you can pass "shell=True" into subprocess.call(), though I don't think you need to:
https://docs.python.org/2/library/subprocess.html#replacing-os-system

Comment 8

[Armen [:armenzg]]

Attachment: extract.diff

You're right!

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8594174 [details] [diff] [review]
extract.diff

Review of attachment 8594174 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py
@@ +279,5 @@
>      dest = os.path.join(dest, filename.split('.')[0])
>  
>      # possibly gets around UAC in vista (still need to run as administrator)
>      os.environ['__compat_layer'] = 'RunAsInvoker'
> +    cmd = '"%s" /extractdir=%s' % (src, os.path.realpath(dest))

It's usually better practice to use a list vs a string, but I don't think it really matters in this case.

Comment 10

[Armen [:armenzg]]

Trying with a list:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46d9f3c49df2

Comment 11

[Armen [:armenzg]]

There was a reason that I was not using a list [1]
If I use shell=True, I get this instead [2].

I'm going to leave the patch as I had it originally.


[1]
 0:00.00 LOG: MainThread INFO Installing application "c:\Users\Mozilla\repos\build-tools\release\updates\downloads\Firefox Setup 38.0b3.exe" to "c:\users\mozilla\appdata\local\temp\tmpvtdc61"
 0:00.01 LOG: MainThread ERROR Failure during execution of the update test.
Traceback (most recent call last):

  File "c:\Users\Mozilla\repos\build-tools\release\updates\venv\lib\site-packages\firefox_ui_harness\runtests.py", line 61, in cli
    runner = startTestRunner(runner_class, options, tests)

  File "c:\Users\Mozilla\repos\build-tools\release\updates\venv\lib\site-packages\firefox_ui_harness\runtests.py", line 33, in startTestRunner
    install_folder = mozinstall.install(installer, dest_folder)

  File "c:\Users\Mozilla\repos\build-tools\release\updates\venv\lib\site-packages\mozinstall\mozinstall.py", line 119, in install
    install_dir = _install_exe(src, dest)

  File "c:\Users\Mozilla\repos\build-tools\release\updates\venv\lib\site-packages\mozinstall\mozinstall.py", line 286, in _install_exe
    result = subprocess.call(cmd)

  File "c:\mozilla-build-test\python\Lib\subprocess.py", line 522, in call
    return Popen(*popenargs, **kwargs).wait()

  File "c:\mozilla-build-test\python\Lib\subprocess.py", line 710, in __init__
    errread, errwrite)

  File "c:\mozilla-build-test\python\Lib\subprocess.py", line 958, in _execute_child
    startupinfo)

InstallError: Failed to install "c:\Users\Mozilla\repos\build-tools\release\updates\downloads\Firefox Setup 38.0b3.exe ([Error 5] Access is denied)"



[2]
Traceback (most recent call last):

  File "c:\Users\Mozilla\repos\build-tools\release\updates\venv\lib\site-packages\firefox_ui_harness\runtests.py", line 61, in cli
    runner = startTestRunner(runner_class, options, tests)

  File "c:\Users\Mozilla\repos\build-tools\release\updates\venv\lib\site-packages\firefox_ui_harness\runtests.py", line 33, in startTestRunner
    install_folder = mozinstall.install(installer, dest_folder)

  File "c:\Users\Mozilla\repos\build-tools\release\updates\venv\lib\site-packages\mozinstall\mozinstall.py", line 119, in install
    install_dir = _install_exe(src, dest)

  File "c:\Users\Mozilla\repos\build-tools\release\updates\venv\lib\site-packages\mozinstall\mozinstall.py", line 288, in _install_exe
    raise Exception('Execution of installer failed.')

InstallError: Failed to install "c:\Users\Mozilla\repos\build-tools\release\updates\downloads\Firefox Setup 38.0b3.exe (Execution of installer failed.)"

Comment 12

[Armen [:armenzg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58b9c5e89670

Comment 13

[Armen [:armenzg]]

It seems I will have to fix the mozinstall test before I can land this:

10:12:16     INFO - TEST-START | test.TestMozInstall.test_get_binary

command timed out: 10800 seconds without output, attempting to kill
13:12:18  WARNING - TEST-UNEXPECTED-ERROR | test.TestMozInstall.test_get_binary | InstallError: Failed to install "c:\builds\moz2_slave\try-w64-0000000000000000000000\build\src\testing\mozbase\mozinstall\tests\Installer-Stubs\firefox.exe (Execution of installer failed.)"
13:12:18     INFO - Traceback (most recent call last):
13:12:18     INFO -   File "c:\builds\moz2_slave\try-w64-0000000000000000000000\build\src\testing\mozbase\mozinstall\tests\test.py", line 43, in test_get_binary
13:12:18     INFO -     os.path.join(self.tempdir, 'exe'))
13:12:18     INFO -   File "c:\builds\moz2_slave\try-w64-0000000000000000000000\build\src\testing\mozbase\mozinstall\mozinstall\mozinstall.py", line 119, in install
13:12:18     INFO -     install_dir = _install_exe(src, dest)
13:12:18     INFO -   File "c:\builds\moz2_slave\try-w64-0000000000000000000000\build\src\testing\mozbase\mozinstall\mozinstall\mozinstall.py", line 288, in _install_exe
13:12:18     INFO -     raise Exception('Execution of installer failed.')
13:12:18     INFO - TEST-INFO took 10801579ms
13:12:18     INFO - TEST-START | test.TestMozInstall.test_get_binary_error

Comment 14

[Armen [:armenzg]]

Attachment: mozinstall.diff

It seems that we have old .exe and .zip files under Installer-Stubs/
I did not include the diff of the binaries on the patch.

In order for this patch to work:
cd mozilla-central/testing/mozbase/mozinstall/tests/Installer-Stubs
wget -Ofirefox.zip ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-40.0a1.en-US.win32.zip
wget -Ofirefox.exe ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-40.0a1.en-US.win32.installer.exe
cd ..
python -m unittest test.TestMozInstall

I will test the patch on try.

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 8595431 [details] [diff] [review]
mozinstall.diff

Review of attachment 8595431 [details] [diff] [review]:
-----------------------------------------------------------------

Lgtm!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a3924a37fa0e

Comment 17

[Armen [:armenzg]]

FTR: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49e5c2961557

Comment 18

[Armen [:armenzg]]

So it seems that the change will backed out and stripped from the hg servers.
#developers were not happy with the 90MB increase on mozilla-central.

Should we redesign the tests for Mozinstall to use whichever binary is generated by the build system?

Comment 19

[Armen [:armenzg]]

Should we look into trying to figure out how firefox.exe and firefox.zip were generated on bug 895940?
Or should we file a bug to re-work the tests and disable it for now? (so I can land my change)

Comment 20

[Armen [:armenzg]]

Attachment: Necessary changes for installing Windows installers properly + disable tests

Let me know if this works for you.

We should probably re-work this on bug 1157352 for the other installers as well.

Comment 21

[Andrew Halberstadt [:ahal]]

Comment on attachment 8596086 [details] [diff] [review]
Necessary changes for installing Windows installers properly + disable tests

Review of attachment 8596086 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but instead of deleting the isWin part, I'd just add a skip-if to the manifest. That way we can still run it locally if we have the binaries.

Comment 22

[Armen [:armenzg]]

Attachment: Necessary changes to extract the Windows installer + skip get_binary test for Windows

Does this look better?
I should have asked for feedback instead of review.
I will test this patch in the next 2 days and push to try before a proper review request.

Do you want me to remove firefox.exe since I know that it won't work?

Comment 23

[Andrew Halberstadt [:ahal]]

Comment on attachment 8596138 [details] [diff] [review]
Necessary changes to extract the Windows installer + skip get_binary test for Windows

Review of attachment 8596138 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, that looks better. And yes, we should remove the existing binaries if they are no longer being used. Thanks!

Comment 24

[Armen [:armenzg]]

Attachment: Necessary changes to extract the Windows installer + skip some tests for Windows

I've tested the patch on my local machine.
I needed to add another 3 skips and modify a test for Mac.
We also remove the firefox.exe.
We can leave the firefox.zip as it should still work.

Being tested:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=601aef0813e2

I'm only testing a Mac and Windows builds because we're only modifying test.py.
The changes for mozinstall.py are the same as before.

Comment 25

[Armen [:armenzg]]

Comment on attachment 8596591 [details] [diff] [review]
Necessary changes to extract the Windows installer + skip some tests for Windows

This is it!

From Windows (2 passed; 4 skipped)
09:31:23     INFO - TEST-SKIP | test.TestMozInstall.test_get_binary | took 0ms
09:31:23     INFO - TEST-PASS | test.TestMozInstall.test_get_binary_error | took 2ms
09:31:23     INFO - TEST-SKIP | test.TestMozInstall.test_install | took 0ms
09:31:23     INFO - TEST-PASS | test.TestMozInstall.test_invalid_source_error | took 1ms
09:31:23     INFO - TEST-SKIP | test.TestMozInstall.test_is_installer | took 0ms
09:31:23     INFO - TEST-SKIP | test.TestMozInstall.test_uninstall | took 0ms

From Mac (6 passed)
09:23:07     INFO - TEST-PASS | test.TestMozInstall.test_get_binary | took 7695ms
09:23:07     INFO - TEST-PASS | test.TestMozInstall.test_get_binary_error | took 1ms
09:23:08     INFO - TEST-PASS | test.TestMozInstall.test_install | took 1313ms
09:23:08     INFO - TEST-PASS | test.TestMozInstall.test_invalid_source_error | took 1ms
09:23:08     INFO - TEST-PASS | test.TestMozInstall.test_is_installer | took 0ms
09:23:09     INFO - TEST-PASS | test.TestMozInstall.test_uninstall | took 1312ms

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/181d06e8c7df

Comment 27

[Armen [:armenzg]]

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=181d06e8c7df

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/181d06e8c7df