911257 - [mozrunner] Add feature to set Firefox as the default browser on Windows 8 and later

Status: RESOLVED INCOMPLETE

(Testing :: Mozbase, defect)

Description

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

If you want to start Firefox in Metro mode, the browser has to be set as the default browser first. Given that Firefox can be located at different locations on disk, it would have to be updated each time MetroFirefox gets started.

So far I have seen different ways how it could be implemented:
* Use Autoit (http://www.autoitscript.com/site/) to script the OS level steps
* Use something like:  http://blogs.technet.com/b/mrmlcgn/archive/2013/02/26/windows-8-associate-a-file-type-or-protocol-with-a-specific-app-using-a-gpo-e-g-default-mail-client-for-mailto-protocol.aspx
* Or similar to releng did this: https://bugzilla.mozilla.org/show_bug.cgi?id=864940

Clint mentioned to me that Rail might know more from the releng side. So I will put needinfo on him for feedback.

Comment 1

[Rail Aliiev [:rail]]

Hmm, I'm not sure what to suggest here. Q may have some ideas.

Comment 2

[Rail Aliiev [:rail]]

whimboo, can you ping Q in #releng instead.

Comment 3

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

This shouldn't block our final 2.0 release. We can work on it for 2.0.1.

(In reply to Rail Aliiev [:rail] from comment #2)
> whimboo, can you ping Q in #releng instead.

Thanks Rail. Will do so once I start working on it.

Comment 4

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

I start working on it now. Once it is ready it will be part of a possible 2.0.2 release or 2.1.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

I followed the steps from: http://technet.microsoft.com/en-us/library/hh824855.aspx
First I set the Nigthly as default then I exported the settings, using the file format I made one for setting the Nightly as default.
The thing is, after I imported the settings I had to reboot the machine in order for changes to take place.
I also tried to call the uninstall.exe with the '/SetAsDefaultAppUser' argument as I found here: http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/stub.nsi#1818
>cmdArgs = ['%s\uninstall\helper.exe' % dest, '/SetAsDefaultAppUser']
>subprocess.call(cmdArgs)
This returns 2 as an exit code.

Also a thing good to mention is that on windows every time we remove the installed build we call the uninstall.exe which will clear the windows registry so the application won't be the default one anymore. https://github.com/mozilla/mozbase/blob/master/mozinstall/mozinstall/mozinstall.py#L201

Two weeks ago I had ran metro testruns on our local jenkins by unzipping the binary prior of running the testrun. Giving the directory as a build parameter and not a zip or installer, we won't remove the binary, and the preferences manually set before will persist. So until we find a way to set the firefox as default I would suggest we go with this option.

Comment 6

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

You shouldn't simply copy and paste your comment from the github issue for mozmill-ci without accounting the feedback I already gave you for that over there. Please keep in mind that this bug is about mozrunner and has nothing to do in how we want to handle the builds in mozmill-ci. It is plain about setting the default browser.

So it looks like I missed to comment my findings from a couple of weeks ago. The prefered way via group policies and the necessary xml file doesn't work, because you have to at least re-login.  That's something what we cannot support for mozrunner. So we might have to use autoit to automate the ui steps for setting the default browser.

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

I made the autoIt script for setting the FF as default and I compiled it as an exe, I will still have to call it inside the mozinstall after we install a build on windows.

So far I named the file setDefault.exe. If you have a better suggestion let me know. I think we can add it inside the environment, and call it in inside mozinstall/_install_exe if we can find it.
So far I call it from command line as "setDefault.exe $path_to_build".

Comment 8

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

Cosmin, that is great! I'm looking forward to it. So please attach the file if possible to this bug, so we can get this tested. Also some notes:

* Setting as default browser should not be done in mozinstall! As this bug shows this should become a mozrunner feature
* Also we might have to save off the currently selected default brower, so we can restore it after mozrunner has been finished running the application

Comment 9

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

And we would also have to get the autoit script checked-in. So please do not only attach the exe file.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: setDefault.au3

Hi Henrik, here is the AutoIt script, is just a scrap as I had rush to have it working I will return with a cleaner and more safe code.
I called the executable both in mozinstall and in mozmill-automation I don't know where to  call it in mozrunner.

An issue that I have encountered is that when the application is "metrofirefox" we fail to get the binary (mozinstall.get_binry) in https://github.com/mozilla/mozmill-automation/blob/master/mozmill_automation/testrun.py#L220
So there we have to use: APPLICATION_BINARY_NAMES[self.options.application]

By doing all of the above it sets the application as default in a testrun but when running metro we will get:
>14:38:02 *** Creating profile: c:\jenkins\workspace\mozilla-central_functional\data\profile
>14:38:03 INFO | metrotestharness.exe | Launching browser...
>14:38:03 INFO | metrotestharness.exe | App model id='47D05E158567E544'
>14:38:03 INFO | metrotestharness.exe | Harness process id: 220
>14:38:03 INFO | metrotestharness.exe | Using bin path: 'c:\jenkins\workspace\mozilla-central_functional\data\binary\2014-02-04-03-02-01-mozilla-central-firefox-30\firefox.exe'
>14:38:03 INFO | metrotestharness.exe | Writing out tests.ini to: 'c:\jenkins\workspace\mozilla-central_functional\data\binary\2014-02-04-03-02-01-mozilla-central-firefox-30\tests.ini'
>14:38:03 INFO | metrotestharness.exe | Browser command line args: 'c:\jenkins\workspace\mozilla-central_functional\data\binary\2014-02-04-03-02-01-mozilla-central-firefox-30\firefox.exe -profile c:\jenkins\workspace\mozilla-central_functional\data\profile'
>14:38:03 TEST-UNEXPECTED-FAIL | metrotestharness.exe | ActivateApplication result 80270254
>14:38:03 INFO | metrotestharness.exe | Deleting c:\jenkins\workspace\mozilla-central_functional\data\binary\2014-02-04-03-02-01-mozilla-central-firefox-30\tests.ini
>14:39:03 RESULTS | Passed: 0
>14:39:03 RESULTS | Failed: 0
>14:39:03 RESULTS | Skipped: 0
We saw that when running metro tests manually a couple a weeks ago I remember we tried to use zipped builds.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

Here is the python code for calling the executable

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: setDefault.exe

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

The hr code above is because it failed to set the FF as default, I removed the self.options.application is 'metrofirefox'.
Then the hr code was 0x80270250 (TEST-UNEXPECTED-FAIL | metrotestharness.exe | ActivateApplication result 80270250), this is because the screen size of the windows doesn't allow the metro mode so I had just to full screen the windows resolution.

As  next step I will try to move this login in mozrunner and to make it look like metrotestharness.exe
https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/local.py#L193

Comment 14

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

Comment on attachment 8373303 [details] [diff] [review]
patch_v1.0

This patch is not mozrunner related. Please do not depend on patches for different tools in mozrunner. Marking attachment as obsolete.

Comment 15

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: [mozrunner] patch 1.0

Here is the patch for mozrunner.

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: firefoxdefault.au3

Here is the autoIt script, I still have to do a safety check, in case FF is already default. For instance if we ran with the same build and it failed to uninstall. 
Also I will have to pass in as a second parameter the Firefox branch name(Nightly, Beta, Aurora).

(In reply to Henrik Skupin (:whimboo) from comment #8)
> * Also we might have to save off the currently selected default browser, so
> we can restore it after mozrunner has been finished running the application
This will be overhead, because for the moment I don't know how to check which is default, also all the installed builds will be uninstalled and the OS will set the previous application as default.

Comment 17

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

Comment on attachment 8373374 [details] [diff] [review]
[mozrunner] patch 1.0

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

I think it is a good start but we have to do a lot of changes still to make it working properly. Beside the mentioned issues below, please also make sure to add any additional resources to setup.py. Otherwise those will be missing in the final release packages.

::: mozrunner/mozrunner/local.py
@@ +201,5 @@
>  
>      def __init__(self, profile, binary=None, **kwargs):
>  
> +        if mozinfo.isWin:
> +            # Set the application as default

Setting the application as default or not should be passed in as parameter to the run() method. I don't see why this has to be done in __init__() while it could be reverted by other tools until run() gets called. Also you want to reset the default browser setting when the application gets closed, so we do not mess up with the users system.

@@ +205,5 @@
> +            # Set the application as default
> +            default_cmd = [self.defaultSetterPath, binary]
> +            result = subprocess.call(default_cmd)
> +            if not result is 0:
> +                raise Exception('Failed to set Firefox as default')

We should really use mozprocess here to run this application.

@@ +207,5 @@
> +            result = subprocess.call(default_cmd)
> +            if not result is 0:
> +                raise Exception('Failed to set Firefox as default')
> +
> +        # take the binary from BROWSER_PATH environment variable

Please don't modify this comment.

Comment 18

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

And also please include the autoit script so that we do not only have the executable present. The script doesn't have to be part of the release package.

Comment 19

[Cosmin Malutan, [:cosmin-malutan]]

Sorry for late answer here, I've got really catched by the enhancing the script to set back the user default.

The AutoIt script was already attached:
https://bug911257.bugzilla.mozilla.org/attachment.cgi?id=8373381

I will return soon with an update on mozrunner patch and possibly on AutoIt script too.

Comment 20

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: [mozrunner] patch v2.0

Now it checks which was the previous default application, saves the name in a file and the second time we call the code, without a parameter, it will set the users application back as default.

Because windows notification cannot be handled, or at least I couldn't find someone who did that, I iterate through options until the default application has the name we gave.

I'm still not sure where it will be the best to call the code inside of mozrunner. So I extended the start and cleanup methods for MetroFirefoxRunner, so prior of running tests it will set the FF as default and will remove it after. I personally think here is the best place as we don't have to add additional checks for determining if the application is metrofirefox.

At this time it sets only the Nightly, but it has the ability to set any application by name, so as a next step we will have to retrieve the name of the application (Nightly/Aurora/Firefox) and call the method according to it.

Comment 21

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: firefoxdefault.au3

Here is the Autoit script

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.1

I included the autoit script in patch.
Thanks

Comment 23

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

Comment on attachment 8374819 [details] [diff] [review]
patch_v2.1

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

::: mozrunner/mozrunner/local.py
@@ +196,5 @@
>                                              'resources',
>                                              'metrotestharness.exe'])
> +    defaultSetterPath = os.path.sep.join([here,
> +                                          'resources',
> +                                          'firefoxdefault.exe'])

Should be fine for now to be located only here. But it might be that we would also need it for Firefox in the future. So we should also add it to the FirefoxRunner. But as said we can do this later.

@@ +212,5 @@
>              raise Exception('Firefox Metro mode is only supported on Windows 8 and onwards')
>  
> +    def start(self, debug_args=None, interactive=False, timeout=None, outputTimeout=None):
> +        if mozinfo.isWin:
> +            print '*** Setting Firefox as a default browser'

No print statements please.

@@ +213,5 @@
>  
> +    def start(self, debug_args=None, interactive=False, timeout=None, outputTimeout=None):
> +        if mozinfo.isWin:
> +            print '*** Setting Firefox as a default browser'
> +            default_cmd = [self.defaultSetterPath, 'Nightly']

Try to check the new mozversion package, which might give us the name we can use here.

@@ +217,5 @@
> +            default_cmd = [self.defaultSetterPath, 'Nightly']
> +            process = ProcessHandler(default_cmd)
> +            process.run(timeout=60)
> +            if (process.wait(timeout=60) != 0):
> +                raise Exception('Failed to set Firefox as default browser')

There is no need to set timeout for both run() and wait(). I would propose you do it like https://github.com/mozilla/mozbase/blob/master/mozinstall/tests/build_stub/build.py#L42

@@ +230,5 @@
> +            default_cmd = [self.defaultSetterPath]
> +            process = ProcessHandler(default_cmd)
> +            process.run(timeout=60)
> +            if (process.wait(timeout=60) != 0):
> +                raise Exception('Failed to remove Firefox as default')

Please run that code before calling cleanup for the LocalRunner.

::: mozrunner/mozrunner/resources/firefoxdefault.au3
@@ +1,1 @@
> +Global $setDefault = False, $default, $found = False, $l = 0, $httpsIndex

I would propose that you are adding more comments to that file so it will be easier to understand by others. Also we need a readme file which explains how to build the executable.

@@ +12,5 @@
> +
> +; Wait for listbox to be displayed then 2.5 extra seconds to be sure all elements ar randered
> +Local $counter
> +Do
> +	Sleep(1000)

nit: fix the indentation please for all those instances to 4 blanks.

@@ +15,5 @@
> +Do
> +	Sleep(1000)
> +    ControlGetHandle($hControlPanel, "", "SysListView321")
> +    $counter += 1
> +    If $counter = 10 Then

Can you please explain what we are looping over?

@@ +26,5 @@
> +$httpsIndex = ControlListView($hControlPanel, "", "SysListView321", "FindItem", "HTTPS")
> +
> +If $setDefault Then
> +	$default = ControlListView($hControlPanel, "", "SysListView321", "GetText", $httpsIndex, 2)
> +	$file = FileOpen("previousdefault.txt", 2)

Where is that file stored? I think it should be under /tmp and get a random name. I would suggest we give it as parameter to the compiled script.

@@ +36,5 @@
> +EndIf
> +
> +
> +While Not (ControlListView($hControlPanel, "", "SysListView321", "GetText", ControlListView($hControlPanel, "", "SysListView321", "FindItem", "HTTPS"), 2) = $default)
> +	$httpOption = ControlListView($hControlPanel, "", "SysListView321", "Select", $httpsIndex)

There is an inconsistance here. You named it httpOption but use HTTPS. What about HTTP? Don't we have to set this?

@@ +39,5 @@
> +While Not (ControlListView($hControlPanel, "", "SysListView321", "GetText", ControlListView($hControlPanel, "", "SysListView321", "FindItem", "HTTPS"), 2) = $default)
> +	$httpOption = ControlListView($hControlPanel, "", "SysListView321", "Select", $httpsIndex)
> +	ControlFocus($hControlPanel, "", $httpOption)
> +	ControlClick($hControlPanel, "", "[CLASS:Button; INSTANCE:1]")
> +	Sleep(1000)

So there is no way to listen for events, and we have to use sleep?

@@ +44,5 @@
> +	For $i = 0 To $l
> +		Send("{DOWN}")
> +		Sleep(500)
> +	Next
> +	; Here we increment the lenght so we take the next option next time

nit: length

@@ +63,5 @@
> +		EndIf
> +	Until @error <> 1
> +	Sleep(2500)
> +WEnd
> +; Close the Control Panel and the Firefox

Why is Firefox open?

Comment 24

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.0

Here is the updated patch.
(In reply to Henrik Skupin (:whimboo) from comment #23)
> Try to check the new mozversion package, which might give us the name we can
> use here.
The mozversion package doesn't give the name of application but only the version number, so I use application.ini to get the branch and use a dictionary.

> > +    If $counter = 10 Then
> 
> Can you please explain what we are looping over?
> ...
> So there is no way to listen for events, and we have to use sleep?
We can't listen for events so as away for waiting is to search for elements and until they exists and catch the error if they don't  
> There is an inconsistance here. You named it httpOption but use HTTPS. What
> about HTTP? Don't we have to set this?
Setting the https will set the http and ftp also by default.

Comment 25

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

Comment on attachment 8376164 [details] [diff] [review]
patch_v3.0

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

(In reply to Cosmin Malutan from comment #24)
> > Try to check the new mozversion package, which might give us the name we can
> > use here.
> The mozversion package doesn't give the name of application but only the
> version number, so I use application.ini to get the branch and use a
> dictionary.

If it doesn't give us what we need, it has to be extended. Please don't start in implementing workarounds.

> > > +    If $counter = 10 Then
> > 
> > Can you please explain what we are looping over?
> > ...
> > So there is no way to listen for events, and we have to use sleep?
> We can't listen for events so as away for waiting is to search for elements
> and until they exists and catch the error if they don't  

Please add that as comments to the script file. Also I can see a lot of cases where we still need more comments in that file. Lots of code blocks are still not easy to understand.

::: mozrunner/mozrunner/local.py
@@ +16,5 @@
>      from plistlib import readPlist
>  
>  from mozprofile import Profile, FirefoxProfile, MetroFirefoxProfile, ThunderbirdProfile, MozProfileCLI
> +from mozprocess.processhandler import ProcessHandler
> +from mozmill_automation import application

We are not going to depend on any outside package. Also this would add a circular dependency. So kill that.

@@ +206,5 @@
>                                              'metrotestharness.exe'])
> +    defaultSetterPath = os.path.sep.join([here,
> +                                          'resources',
> +                                          'firefoxdefault.exe'])
> +    defaultSetterFile = mkstemp('.defaultSetterFile')[1]

This is not what you really wanna do. The call to mkstemp will not close the file handle. You will not be able to ever remove or re-use this file.

::: mozrunner/mozrunner/resources/firefoxdefault.au3
@@ +1,4 @@
> +; Author(s):        Cosmin Malutan <[e-mail]>
> +; AutoIt Version:   3.3.10.2
> +
> +Global $setDefault = False ; This variable stands for setting the default application if no argument is given when we call the script it means we should fallback on restoring the previous default

I would love to see no hanging comments after the actual step but in its separate line. We should also try to obey the 80 char limit as what we do for any other code as well.

::: mozrunner/mozrunner/resources/readme.txt
@@ +1,2 @@
> +For building the script please download the kit from:
> +http://www.autoitscript.com/cgi-bin/getfile.pl?../autoit3/scite/download/SciTE4AutoIt3.exe

You should not point directly to the download link but better to the download page.

Comment 26

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.0

Here is the updated patch.

Comment 27

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

Comment on attachment 8376343 [details] [diff] [review]
patch_v4.0

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

Cosmin, while reviewing this code I have seen that a couple of things have been changed without any description (at least in the review request). I have mentioned that already in the past, so please get started to explain what else beside the review comments was necessary to be updated.

::: mozrunner/mozrunner/local.py
@@ +200,5 @@
>                                              'metrotestharness.exe'])
> +    defaultSetterPath = os.path.sep.join([here,
> +                                          'resources',
> +                                          'firefoxdefault.exe'])
> +    defaultSetterFile = tempfile.NamedTemporaryFile(delete=False).name

You should use mozfile's NamedTemporaryFile instead:
https://github.com/mozilla/mozbase/blob/master/mozfile/mozfile/mozfile.py#L294

Also I think you want to have this line in start() and update a class property.

@@ +206,1 @@
>      def __init__(self, profile, binary=None, **kwargs):

What about blank lines? I assume you haven't run pep8?

@@ -200,5 @@
>  
>          # if no binary given take it from the BROWSER_PATH environment variable
>          binary = binary or os.environ.get('BROWSER_PATH')
>          LocalRunner.__init__(self, profile, binary, **kwargs)
> -

Can you please explain, why you did this removal?

@@ +214,5 @@
>          if not mozinfo.isWin:
>              raise Exception('Firefox Metro mode is only supported on Windows 8 and onwards')
>  
> +    def start(self, debug_args=None, interactive=False, timeout=None, outputTimeout=None):
> +        if mozinfo.isWin:

I would avoid those checks and already bail out in __init__ if someone tries to instantiate the MetroFirefoxRunner class on a system other than Windows and earlier than 8.1.

@@ +231,5 @@
> +            process = ProcessHandler(default_cmd)
> +            process.run(timeout=60)
> +            if process.wait() != 0:
> +                raise Exception('Failed to remove Firefox as default')
> +            os.remove(self.defaultSetterFile)

mozfile.remove() please.

@@ +232,5 @@
> +            process.run(timeout=60)
> +            if process.wait() != 0:
> +                raise Exception('Failed to remove Firefox as default')
> +            os.remove(self.defaultSetterFile)
> +        LocalRunner.cleanup(self)

nit: an empty line above please.

::: mozrunner/mozrunner/resources/firefoxdefault.au3
@@ +1,2 @@
> +; Author(s):        Cosmin Malutan <[e-mail]>
> +; AutoIt Version:   3.3.10.2

This file should get the MPL 2 license.

::: mozrunner/mozrunner/resources/readme.txt
@@ +1,1 @@
> +For building the script please download the kit from:

Please keep in mind that multiple resources are located in this folder. So mentioning 'the script' only, is kinda blurry.

::: mozversion/mozversion/mozversion.py
@@ +59,5 @@
>                          section, key) and config.get(section, key) or None
> +                if self._info.get('application_name') == 'Firefox':
> +                    repository = self._info.get('application_repository')
> +                    branch = repository.rsplit('/', 1)[1]
> +                    self._info['application_title'] = APPLICATION_TITLES[branch]

Why have you made changes to mozversion in your patch when I instructed you to file a new bug for getting this implemented?

Comment 28

[Cosmin Malutan, [:cosmin-malutan]]

Regarding the situation where we already have a Firefox version installed, as discussed this morning on irc, I tested this and the testrun will install the application and it will ran it as a default.

After removing the instance used by the testrun the users instance will be default. I honestly think that  we are safe here now.

For setting the default browser I opened it initially. Firefox would then prompt me.
Now that we depend only on the name of application we don't need to open the firefox anymore. 

Another thing that I would like to mention is that we cannot handle the windows notification popup with AutoIt properly (I've searched for a solution for about 2 days and haven't found anything).
So for setting the default browser, I prompt the notification window, send the "DOWN" key, then "ENTER", then check in control panel that the default is "Firefox". If its not, I will repeat doing so until it is.

This is harmless unless we hit in to "Go to store option".
This might happen if we somehow miss "Firefox" while changing the default application.
We can remove this chance completely by disabling the Store for our production machines:
http://www.howtogeek.com/111933/how-to-disable-the-windows-store-in-windows-8/

Comment 29

[Andrei Eftimie]

Attachment: WIP v5

This has been rebased on mozilla-central.

This is still WIP, since I haven't tested it thoroughly and still needs a few items:
- commit message
- take the new application name from application.ini (and fallback to the current used one if that does not exist)

I'll have these implemented tomorrow.

Comment 30

[cmtalbert]

Cosmin, that sounds like a good approach. If we decide to turn off the store on our production machines (which seems like a fine idea to me) we will probably want to file a dependent bug linked to this one to track making that change to the VM templates.  Would you mind filing it now and we can take action on it if we find ourselves having that problem?

Comment 31

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

(In reply to Cosmin Malutan from comment #28)
> We can remove this chance completely by disabling the Store for our
> production machines:
> http://www.howtogeek.com/111933/how-to-disable-the-windows-store-in-windows-
> 8/

Sounds good to me. Same as what Clint mentioned, lets get filed an infra bug so we can update our machines and templates to disable the store. 

Also is this the only situation when we can miss to set Firefox as default browser?

Comment 32

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) [away 02/24 - 02/28] from comment #31)
> Also is this the only situation when we can miss to set Firefox as default
> browser?
From what I met so far yes.

Comment 33

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

Comment on attachment 8378988 [details] [diff] [review]
WIP v5

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

::: testing/mozbase/mozrunner/mozrunner/local.py
@@ +206,4 @@
>  
>      def __init__(self, profile, binary=None, **kwargs):
> +        if not mozinfo.isWin:
> +            raise Exception('Firefox Metro mode is only supported on Windows 8 and onwards')

You are not doing a version check here. So XP would also pass.

@@ +215,5 @@
>          if not os.path.exists(self.immersiveHelperPath):
>              raise OSError('Can not find Metro launcher: %s' % self.immersiveHelperPath)
>  
> +    def start(self, debug_args=None, interactive=False, timeout=None, outputTimeout=None):
> +        title = mozversion.get_version(self.binary).get('application_title')

For an updated patch keep in mind that the code name does not always exist.

@@ +216,5 @@
>              raise OSError('Can not find Metro launcher: %s' % self.immersiveHelperPath)
>  
> +    def start(self, debug_args=None, interactive=False, timeout=None, outputTimeout=None):
> +        title = mozversion.get_version(self.binary).get('application_title')
> +        default_cmd = [self.defaultSetterPath, title, self.defaultSetterFile]

remove default_cmd and put the array directly into the constructor of ProcessHandler.

::: testing/mozbase/mozrunner/setup.py
@@ +41,5 @@
>        license='MPL 2.0',
>        packages=['mozrunner'],
>        package_data={'mozrunner': [
> +            'resources/metrotestharness.exe',
> +            'resources/firefoxdefault.exe'

Don't forget to adjust the version to the mozversion package.

Comment 34

[Andrei Eftimie]

Quick update. The AutoIt script works fine on 32b machines, but fails on any 64b Windows machine.
We might need to compile it on a 64b architecture for it to work... if this is the case we'll end up with 2 win executable files.

Comment 35

[Cosmin Malutan, [:cosmin-malutan]]

That's correct, some commands may fail when using a 32-bit AutoIt process to read from a 64-bit process.
Also after struggling to compile it in 64bits I come to find out that compiling it on a 64bit machine is not enough, we have to compile it from terminal with a /x64 parameter.
The exact command that I used is:
>Aut2exe.exe /in firefoxdefault.au3 /out firefoxdefault.exe /x64
After that I tested the code and works fine for 64bits machines. As Andrei said we would have to include both executable in patch, and this should go in to readme file.

Comment 36

[Andrei Eftimie]

Attachment: 0006-Bug-911257-Support-Firefox-as-a-default-Metro-browse.patch

We fixed the problem for 32b vs 64b architectures by having 2 separate executable files. We invoke them depending on the cpu architecture.

This is still a WIP since the logic to select the correct browser as Default is not completely sane yet. If we have multiple browsers in that list this might not work as expected.

Comment 37

[Andrei Eftimie]

Attachment: WIP_0007-Bug-911257-Support-Firefox-as-a-default-Metro-browse.patch

Another update. Setting Firefox as default works better now.
This is still far from perfect, but with the limited access we have to Windows' Metro settings it works good enough.

We don't know how many applications are "registered" as browsers (read are able to work with http/https content) in Metro. If this number is 3 or lower we won't fail selecting the one we need (Firefox, Aurora or Nightly).

For 4 or more it gets tricky, but this will not be the case for our CI-machines.

===
Now for the bad part. This works, but it seems something regressed in mozmill. Trying to run a testrun fails with a jsbridge error. We correctly set the application as default, start it but we fail to connect to the application.

We're doing a regression range now to see where this regressed. Alternatively taking a 2.0.3 environment and bumping dependencies to see if we can find it like this.

Comment 38

[Cosmin Malutan, [:cosmin-malutan]]

I tested the patches wit 2.0.3 and I had the same problem, so I made some dumps inside wait_and_create_network method from jsbridge, where it failed.

The exception thrown is:
>socket.error.errno  10061
>socket.error.strerror  No connection could be made because the target machine actively refused it

Comment 39

[Andrei Eftimie]

Cosmin(In reply to Cosmin Malutan from comment #38)
> I tested the patches wit 2.0.3 and I had the same problem, so I made some
> dumps inside wait_and_create_network method from jsbridge, where it failed.
> 
> The exception thrown is:
> >socket.error.errno  10061
> >socket.error.strerror  No connection could be made because the target machine actively refused it

I've had success in running Metro testruns with 2.0.3:
http://mozmill-crowd.blargon7.com/#/functional/report/e248c845ffb052c61b3635551b2253d6

Steps:
1) get a fresh 2.0.3 windows env from http://mozqa.com/mozmill-env/
2) bump only the required dependencies for running Metro testruns, specifically:
- mozprofile == 0.21
- mozprocess == 0.18
- mozrunner == 5.35
- mozversion == 0.2
- mozlog == 1.5

This works.

Cosmin please check what other dependencies have been update if this fails for you. Might help speed up our search for the regressor. I'll work backwards in comparing these deps versions with what I have in the broken 2.0.5 env.

====

In the meantime we can use the 2.0.3 env to start running tests on the local CI!!!

Comment 40

[Andrei Eftimie]

Well I need backtrack a bit.
What I said works by using a firefox binary directly. It still fails with using the installer.
I'm still debugging this.

We correctly install firefox... I've checked the paths, the timings. I'm still at a loss as to why this fails.

Comment 41

[Andrei Eftimie]

Attachment: 0008-Bug-911257-Support-Firefox-as-a-default-Metro-browse.patch

The exe's were broken in the last patch. Here is a working version.
===

Now to reiterate on the problems we're still facing.
1) Testruns work with a binary
2) Testruns fail with an installer.

With the installer:
a) Firefox gets installed properly
b) We manage to set it as default
c) Firefox starts in Metro mode
d) a jsbridge connection never gets established

Comment 42

[Cosmin Malutan, [:cosmin-malutan]]

I copied an patched env on staging node mm-win-81-32, and I ran a testrun in metromode and it worked by the first try. So it might be that we broke something on our station. 
We will connect an clear node to our CI for testing, if everything will work fine we could start to patch the staging too.

Also I would like to mention that while we were debugging I checked if the server on the JsBridge side get's started, and it did.
https://github.com/mozilla/mozmill/blob/master/jsbridge/jsbridge/extension/components/jsbridge.js#L66
So I don't know what causes error from comment 38, because the JsBridge was listening.

Comment 43

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Cosmin also mentioned on IRC that the JSBridge extension was not installed on the Firefox instance that was started, and Andrei determined that the profile path was empty.

Comment 44

[Cosmin Malutan, [:cosmin-malutan]]

I have set our CI, and it works fine on a clean machine, I took two config logs from master and ran the pulse with a --push-message parameter. Everything worked fine so I started the pulse again and leave it listening. The metro tests should be ran against all new builds on our CI from now.

Here are the reporst sent to staging:
http://mozmill-staging.blargon7.com/#/functional/reports?app=MetroFirefox&branch=All&platform=All&from=2014-02-23&to=

(In reply to Dave Hunt (:davehunt) from comment #43)
> Cosmin also mentioned on IRC that the JSBridge extension was not installed
> on the Firefox instance that was started, and Andrei determined that the
> profile path was empty.
Here I checked the addons manually, by navigating to about:addons. Because I didn't saw the there I thought that they were not installed so I made dumps in jsbridge.js. The server was never initialized while in metromode. It was though when I ran in desktop mode even when it was hanging.
If we will ran in to this again we should debug in jsbridge.js.

Comment 45

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

(In reply to Cosmin Malutan from comment #35)
> That's correct, some commands may fail when using a 32-bit AutoIt process to
> read from a 64-bit process.

Can you please explain further or reference some information about that behavior? I would like to read through. Where did the 32bit version fail exactly? I haven't seen any comment yet which is about the real underlying issue.

Comment 46

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #45)
> (In reply to Cosmin Malutan from comment #35)
> > That's correct, some commands may fail when using a 32-bit AutoIt process to
> > read from a 64-bit process.
> 
> Can you please explain further or reference some information about that
> behavior? I would like to read through. Where did the 32bit version fail
> exactly? I haven't seen any comment yet which is about the real underlying
> issue.

We only had to compile the script with the following flag: /x64
That's why we have 2 exe's now. The compile process is briefly explained in the readme.txt file

Comment 47

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

Not sure why you reset the needinfo flag from Cosmin. Also your answer is absolutely not related to my question. So please read it carefully again or let Cosmin answer, if he knows the details only. Thanks.

Comment 48

[Cosmin Malutan, [:cosmin-malutan]]

Additional to my comment 35:
The script runs until it finds the "SysListView321" controller, where it crashes.
The problem it might be that the SysListView32 it will be a 64 bit process on 64 bit stations, and the autoit script if compiled in 32-bit will crash when it tries to eread from 64-bit process.
http://www.autoitscript.com/autoit3/docs/functions/ControlListView.htm "Remarks"
If we run a script compiled in 64 bits it works.

Comment 49

[Andrei Eftimie]

Attachment: 0009-Bug-911257-Support-Firefox-as-a-default-Metro-browse.patch

Another update here.
We were not correctly cleaning up the Control Panel / Defaults windows if something would fail. This would lead to multiple windows being opened, and in a subsequent run the previous window would get called (without being refreshed) and we would again failing in setting the default.

This is more robust now, as we're always going to close the window.

Comment 50

[Andrei Eftimie]

Comment on attachment 8386096 [details] [diff] [review]
0009-Bug-911257-Support-Firefox-as-a-default-Metro-browse.patch

Henrik would you please have a look over the script?

I'm not sure how much polish this would need.

Comment 51

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

Comment on attachment 8386096 [details] [diff] [review]
0009-Bug-911257-Support-Firefox-as-a-default-Metro-browse.patch

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

::: testing/mozbase/mozrunner/mozrunner/local.py
@@ +6,5 @@
>  
>  import ConfigParser
>  import mozinfo
> +import mozversion
> +import mozfile

nit: alphabetical order please

@@ +200,5 @@
>                                              'resources',
>                                              'metrotestharness.exe'])
> +    defaultSetterPath = os.path.sep.join([here,
> +                                          'resources',
> +                                          'firefoxdefault_' + str(mozinfo.bits) + '.exe'])

please change this to `'default_browser_%d.exe' % mozinfo.bits`

@@ +206,4 @@
>  
>      def __init__(self, profile, binary=None, **kwargs):
> +        if not mozinfo.isWin and float(mozinfo.version.rsplit('.', 1)[0]) >= 6.1:
> +            raise Exception('Firefox Metro mode is only supported on Windows 8 and upwards')

You are not testing Windows 8 here, but even 7 and upwards. I would suggest we call `(maj, min, rev) = mozinfo.version.split('.')` and work directly with maj and min for comparison, so no type change is involved.

@@ +216,5 @@
>              raise OSError('Can not find Metro launcher: %s' % self.immersiveHelperPath)
>  
> +    def start(self, debug_args=None, interactive=False, timeout=None, outputTimeout=None):
> +        code_name = mozversion.get_version(self.binary).get('application_code_name') or \
> +                    mozversion.get_version(self.binary).get('application_name')

I think we should update mozversion to return application_name if the code name doesn't exist. In those cases we would have the same values. Also I would call the binary app_name.

@@ +230,5 @@
> +            process = ProcessHandler([self.defaultSetterPath, self.defaultSetterFile])
> +            process.run(timeout=60)
> +            if process.wait() != 0:
> +                raise Exception('Failed to remove Firefox as default')
> +            mozfile.remove(self.defaultSetterFile)

We will not remove the file if an exception gets raised before. Put this into a final block?

::: testing/mozbase/mozrunner/mozrunner/resources/firefoxdefault.au3
@@ +2,5 @@
> +; License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +; You can obtain one at http://mozilla.org/MPL/2.0/.
> +;
> +; Author(s):        Cosmin Malutan <[e-mail]>
> +;                   Andrei Eftimie <[e-mail]>

As you know MPL2 doesn't list any authors. So please remove those two lines.

@@ +9,5 @@
> +; This variable stands for setting the default application
> +Global $setDefault = False
> +; Number times we click "down" until we successfully
> +; set our default, it will increment with each failure
> +Global $l = 0

When we give that variable a better name and group variables, the lengthy comment would most likely be not necessary.

@@ +13,5 @@
> +Global $l = 0
> +Global $httpsIndex
> +Global $httpsElement
> +; Is name of application which we will set as default.
> +Global $default

Better variable name please so we can get rid of the comment.

@@ +18,5 @@
> +Global $error = False
> +
> +; If there are two parameter given then one is the target
> +; default application and one the file where we save the
> +; name of the current default if $setDefault is False we

nit: Missing point?

@@ +34,5 @@
> +; Open the "Set Associations" under the control panel
> +Run("control /name Microsoft.DefaultPrograms /page pageFileAssoc")
> +
> +; Wait for Control Panel and then focus it
> +Local $hControlPanel = WinWait("[TITLE:Set Associations; CLASS:CabinetWClass]")

Any reason why this has to be a local variable?

@@ +39,5 @@
> +WinActivate($hControlPanel)
> +
> +; If we don't have $setDefault argument then we should fallback
> +; on restoring the previous default
> +If $setDefault Then

The comment doesn't capture both conditions below

@@ +44,5 @@
> +  $default = GetTextValue("HTTPS")
> +  $file = FileOpen($fileLocation, 2)
> +  FileWrite($file, $default)
> +  FileClose($file)
> +  $default = $setDefault

Why do we overwrite the app name with a boolean?

@@ +54,5 @@
> +  EndIf
> +EndIf
> +
> +; Keep trying to set the default until the $httpElemet will have
> +; the value of the $default

nit: Element

@@ +55,5 @@
> +EndIf
> +
> +; Keep trying to set the default until the $httpElemet will have
> +; the value of the $default
> +While Not (GetTextValue("HTTPS") = $default)

See above. In case of `$setDefault = True`, $default will be a boolean.

@@ +57,5 @@
> +; Keep trying to set the default until the $httpElemet will have
> +; the value of the $default
> +While Not (GetTextValue("HTTPS") = $default)
> +  ; He re we increment the length so we take the next option next time
> +  If $l >= 4 Then ; After four tries we exit the loop

You have mentioned a problem if we do this more than four times. This should be added as comment. So everyone knows why we can't step further until we reach the end of the list.

@@ +73,5 @@
> +  While $i < $l
> +    Send("{DOWN}")
> +    Sleep(500)
> +    $i = $i + 1
> +  WEnd

I'm not sure I understand this block. Lets say the browser is listed as 3rd item and we try to reach it inside the first outer while iteration. We only click 'down' once? I don't see how we would reach the target element.

@@ +82,5 @@
> +
> +CleanUp()
> +
> +
> +Func WaitForElement($aElement)

Please move all helper methods up to the top before any other code starts.

@@ +94,5 @@
> +      ExitLoop
> +    EndIf
> +
> +    If @error = 1 Then
> +      Sleep(5000)

Do we really need such a long sleep? Can't we make it 500ms and just retry more often?

@@ +99,5 @@
> +    EndIf
> +  Until @error <> 1
> +  $counter = 0
> +  Do
> +    Sleep(2500)

Same here. If the handle of the windows is available, why cannot we directly call FindItem? The sleep can be done in case of `@error = 1` and then a bit shorter?

::: testing/mozbase/mozrunner/mozrunner/resources/readme.txt
@@ +1,2 @@
> +For building the firefoxdefault.au3 script please download the kit from:
> +http://www.autoitscript.com/site/autoit-script-editor/downloads/

You are not building the au3 script, but the executable. Please update the wording and fix spelling issues.

@@ +1,4 @@
> +For building the firefoxdefault.au3 script please download the kit from:
> +http://www.autoitscript.com/site/autoit-script-editor/downloads/
> +
> +We need 2 executables for both 32b and 64b architecture. For this we need to

nit: 32bit and 64bit

@@ +3,5 @@
> +
> +We need 2 executables for both 32b and 64b architecture. For this we need to
> +set a flag when compiling the au3 script:
> +> Aut2exe.exe /in firefoxdefault.au3 /out firefoxdefault_32.exe /x86
> +> Aut2exe.exe /in firefoxdefault.au3 /out firefoxdefault_64.exe /x64

Mind putting a .cmd file into this folder?

Comment 52

[(not currently active) Ted Mielczarek]

This seems unnecessary in light of bug 981166.

Comment 53

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

This is not limited to Metro but will work with a plain Firefox too. Also Metro is not totally dead. So we still want that community members can run our tests with MetroFirefox. We will finish this bug off and get support out.

Comment 54

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #51)
> > +    def start(self, debug_args=None, interactive=False, timeout=None, outputTimeout=None):
> > +        code_name = mozversion.get_version(self.binary).get('application_code_name') or \
> > +                    mozversion.get_version(self.binary).get('application_name')
> 
> I think we should update mozversion to return application_name if the code
> name doesn't exist. In those cases we would have the same values. Also I
> would call the binary app_name.
Henrik, for this I should file a new bug in mozversion and wait for that to get fixed?

Comment 55

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

(In reply to Cosmin Malutan from comment #54)
> > I think we should update mozversion to return application_name if the code
> > name doesn't exist. In those cases we would have the same values. Also I
> > would call the binary app_name.
> Henrik, for this I should file a new bug in mozversion and wait for that to
> get fixed?

File and fix it. It's a very tiny change, which will help a lot. Thanks

Comment 56

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v7.0

I updated the patch and tested it, with the changes from mozversion too.

(In reply to Henrik Skupin (:whimboo) from comment #51)
> > +> Aut2exe.exe /in firefoxdefault.au3 /out firefoxdefault_32.exe /x86
> > +> Aut2exe.exe /in firefoxdefault.au3 /out firefoxdefault_64.exe /x64
> 
> Mind putting a .cmd file into this folder?
I made a cmd  file on my station but the path to "Aut2exe.exe" depends on the location where the user installed the Autoit, and that would be hard to find from cmd.

Comment 57

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

(In reply to Cosmin Malutan from comment #56)
> I made a cmd  file on my station but the path to "Aut2exe.exe" depends on
> the location where the user installed the Autoit, and that would be hard to
> find from cmd.

We could pass in the path via a parameter but I think that's overhead. Lets leave it as it is.

Comment 58

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

Comment on attachment 8411007 [details] [diff] [review]
patch_v7.0

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

::: testing/mozbase/mozrunner/mozrunner/local.py
@@ +203,5 @@
> +                                          'resources',
> +                                          'default_browser_%d.exe' % mozinfo.bits])
> +    defaultSetterFile = mozfile.NamedTemporaryFile(delete=False).name
> +
> +    def isWin8(self):

I don't see why this is necessary as a separate method. It's necessary only once. Also please check the latest mozinfo in regards of bug 945869. You should be able to get the version details directly now.

@@ +240,5 @@
> +            try:
> +                process = ProcessHandler([self.defaultSetterPath, self.defaultSetterFile])
> +                process.run(timeout=60)
> +                if process.wait() != 0:
> +                    raise Exception('Failed to remove Firefox as default')

You don't want to hard-code the name, but use app_name here. Also use 'default browser'.

::: testing/mozbase/mozrunner/mozrunner/resources/firefoxdefault.au3
@@ +1,1 @@
> +; This Source Code Form is subject to the terms of the Mozilla Public

We renamed the .exe to 'default_browser...'. Why this file has not been renamed?

@@ +10,5 @@
> +Global $length = 0
> +Global $setDefault = False
> +Global $targetDefaultApp
> +
> +Func WaitForElement($aElement)

As you know we want to have an alphabetical sorting for methods. Why this hasn't been applied here?

@@ +79,5 @@
> +Global $hControlPanel = WinWait("[TITLE:Set Associations; CLASS:CabinetWClass]")
> +WinActivate($hControlPanel)
> +
> +; If we have $setDefault argument we write it in file for persistance
> +; if we don't then we should fallback on restoring the previous default

nit: Check the wording of those _two_ sentences.

::: testing/mozbase/mozrunner/setup.py
@@ +43,5 @@
>        packages=['mozrunner'],
>        package_data={'mozrunner': [
> +            'resources/metrotestharness.exe',
> +            'resources/firefoxdefault_32.exe',
> +            'resources/firefoxdefault_64.exe'

You have not tested your patch, Cosmin! This would totally fail because we changed the name of the exe file.

I wonder if we can get a unit test for this feature.

Comment 59

[Cosmin Malutan, [:cosmin-malutan]]

Mozrunner has been completely changed, so we will need a great re-factoring if we want this to work, do we want to invest any more time in this or we can size the work here and close it with wontfix?
https://github.com/mozilla/gecko-dev/commit/e60605718c40eb097bf52bc1c9db97bbedae6269

We can always re-open it if Metro comes back on table. Henrik what do you think?

Comment 60

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

That's what I said before the major refactoring was merged. So as the state is right now, I don't think that we should invest any time here to get this landed. As you say, when we have people starting to run Metro tests as based on community work, we can reopen this bug and get it finished.

Lets close as incomplete.