946672 - Mozmill fails to open Nightly in metro mode after bug 924995

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andreea Matei [:AndreeaMatei]]

Henrik raised this issue on Monday when I could still open in metro mode, but the tests I prepared for the UI library were failing.

With the nightly build from December 3rd, I could open in metro mode by running: mozmill --manual -b nightly_path --app=metrofirefox

After updating the build, metrotestharness.exe failed with Disconnect Error and Cannot connect to jsbridge extension, but visually, it's opening Metro mode for 1 second (at least the logo part) and then returns to Start.
I'm using dumps through mozmill and mozrunner code to see how far it goes.

$ mozmill --manual -b ../builds/firefox/firefox.exe --app=metrofirefox
INFO | metrotestharness.exe | Launching browser...
INFO | metrotestharness.exe | App model id='B3E9650A250B89E'
INFO | metrotestharness.exe | Harness process id: 3344
INFO | metrotestharness.exe | Using bin path: 'c:\Users\user\Desktop\env2.0\mozmill-env\builds\firefox\firefox.exe'
INFO | metrotestharness.exe | Writing out tests.ini to: 'c:\Users\user\Desktop\env2.0\mozmill-env\builds\firefox\tests.ini'
INFO | metrotestharness.exe | Browser command line args: 'c:\Users\user\Desktop\env2.0\mozmill-env\builds\firefox\firefox.exe -profile c:\users\user\appdata\local\temp\tmpuoqwqy.mozrunner'
TEST-UNEXPECTED-FAIL | metrotestharness.exe | ActivateApplication result 80070005
INFO | metrotestharness.exe | Deleting c:\Users\user\Desktop\env2.0\mozmill-env\builds\firefox\tests.ini
Traceback (most recent call last):
  File "c:\Users\user\Desktop\env2.0\mozmill-env\python\Scripts\mozmill-script.py", line 8, in <module>
    load_entry_point('mozmill==2.0', 'console_scripts', 'mozmill')()
  File "c:\Users\user\Desktop\env2.0\mozmill-env\python\lib\site-packages\mozmill\__init__.py", line 793, in cli
    CLI(args).run()
  File "c:\Users\user\Desktop\env2.0\mozmill-env\python\lib\site-packages\mozmill\__init__.py", line 765, in run
    mozmill.start_runner()
  File "c:\Users\user\Desktop\env2.0\mozmill-env\python\lib\site-packages\mozmill\__init__.py", line 324, in start_runner
    self.create_network()
  File "c:\Users\user\Desktop\env2.0\mozmill-env\python\lib\site-packages\mozmill\__init__.py", line 285, in create_network
    self.jsbridge_port)
  File "c:\Users\user\Desktop\env2.0\mozmill-env\python\lib\site-packages\jsbridge\__init__.py", line 44, in wait_and_create_network
    raise Exception("Cannot connect to jsbridge extension, port %s" % port)
Exception: Cannot connect to jsbridge extension, port 53359

Comment 1

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

You do not necessarily have to use mozmill here to get started in investigation. Use the mozrunner script directly. Is that one able to start Firefox in Metro mode? Do have have anything more as the above stack from your investigation of the last 3.5 days?

Comment 2

[Andrei Eftimie]

Attachment: 0001-Bug-946672-Check-for-Metro-using-the-API-instead-of-.patch

I switched from relying on the App name to use an API to check if we are in metro `immersive` mode.

Services.metro.*
http://dxr.mozilla.org/mozilla-central/source/widget/nsIWinMetroUtils.idl

From what I've tested this works reliably.
I've successfully ran a metro test (usign Nightly), a regular test (using both Nightly and a Beta build).

I'll now trigger some testruns, but they should all pass as well.

Comment 3

[Andreea Matei [:AndreeaMatei]]

Not sure how it was fixed in the meantime, it's still failing for me even with mozrunner only. I'll clean up everything on my VM, but great that it was working for Andrei and we can fix it :)

Comment 4

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

Comment on attachment 8343699 [details] [diff] [review]
0001-Bug-946672-Check-for-Metro-using-the-API-instead-of-.patch

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

It nice that it works that way. Where did you get this information from? Also what about the application ID? Has this been changed too or is that still the old one?

::: mozmill/mozmill/extension/resource/stdlib/utils.js
@@ +48,2 @@
>    }
> +  return aWindow.gBrowser;

We need a method which returns the application from a list of known ones, so we can make use of it on various places. Also do not remove the usage of appinfo and the related switch statement! Both are necessary!

Comment 5

[Andrei Eftimie]

> It nice that it works that way. Where did you get this information from?
> Also what about the application ID? Has this been changed too or is that
> still the old one?

The application ID has not changed.
Would you like to use that as an identificator.

See below what Services.appinfo returns:

New Version
===========
Metro
-----
appBuildID: 20131205030207
ID: {99bceaaa-e3c6-48c1-b981-ef9b46b67d60}
name: Firefox
platformBuildID: 20131205030207
platformVersion: 28.0a1
vendor: Mozilla
version: 28.0a1

Desktop Mode
------------
appBuildID: 20131205030207
ID: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
name: Firefox
platformBuildID: 20131205030207
platformVersion: 28.0a1
vendor: Mozilla
version: 28.0a1


Old Version
===========
Metro
-----
appBuildID: 20131128030201
ID: {99bceaaa-e3c6-48c1-b981-ef9b46b67d60}
name: MetroFirefox
platformBuildID: 20131128030201
platformVersion: 28.0a1
vendor: Mozilla
version: 28.0a1

Desktop Mode
------------
appBuildID: 20131128030201
ID: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
name: Firefox
platformBuildID: 20131128030201
platformVersion: 28.0a1
vendor: Mozilla
version: 28.0a1

> ::: mozmill/mozmill/extension/resource/stdlib/utils.js
> @@ +48,2 @@
> >    }
> > +  return aWindow.gBrowser;
> 
> We need a method which returns the application from a list of known ones, so
> we can make use of it on various places.
We could make a map based on the application ID.
That might work if it will not change often.

> Also do not remove the usage of
> appinfo and the related switch statement! Both are necessary!

Why? This method returns the browser object.
In the previous version it would return something different only for Metro use, which we preserve.

The only way I can see the use of appinfo here is for the ID, all other information is identical.

Comment 6

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

(In reply to Andrei Eftimie from comment #5)
> > We need a method which returns the application from a list of known ones, so
> > we can make use of it on various places.
> We could make a map based on the application ID.
> That might work if it will not change often.

That's what I have in mind, yes. The ID should basically never change once set. So we are pretty safe when using it for the identification.

> > Also do not remove the usage of
> > appinfo and the related switch statement! Both are necessary!
> 
> Why? This method returns the browser object.
> In the previous version it would return something different only for Metro
> use, which we preserve.
> 
> The only way I can see the use of appinfo here is for the ID, all other
> information is identical.

For support of other applications the browser object could live somewhere else or could even not exist. if/else cases would make it very hard to read. That's why we used a switch statement here.

Comment 7

[Andrei Eftimie]

Attachment: 0001-Bug-946672-Check-for-Metro-using-the-API-instead-of-.patch

I've mapped the Application ID's to the names we used until now.

Comment 8

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

Comment on attachment 8344524 [details] [diff] [review]
0001-Bug-946672-Check-for-Metro-using-the-API-instead-of-.patch

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

::: mozmill/mozmill/extension/resource/stdlib/utils.js
@@ +15,5 @@
>  
> +const appID = {
> +  '{ec8030f7-c20a-464f-9b0e-13a3a9e97384}': 'Firefox',
> +  '{99bceaaa-e3c6-48c1-b981-ef9b46b67d60}': 'MetroFirefox'
> +}

At the same time I would also export the current application name, so other modules can make use of it. In that step you will also have to update mozmill.js for the `Application` variable.

Comment 9

[Andrei Eftimie]

Attachment: 0001-Bug-946672-Get-the-App-Name-based-on-the-App-ID-firs.patch

Exported the name as `utils.getAppName()`
Updated all references of Services.appinfo.name to use the new method.
And the new method falls back onto using Services.appinfo.name if no suitable ID is found.

Comment 10

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

Comment on attachment 8344554 [details] [diff] [review]
0001-Bug-946672-Get-the-App-Name-based-on-the-App-ID-firs.patch

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

::: mozmill/mozmill/extension/resource/driver/mozmill.js
@@ +119,5 @@
>    // Put all our necessary information into JSON and return it:
>    // appinfo, startupinfo, and addons
>    var details = {
>      application_id: appInfo.ID,
> +    application_name: utils.getAppName(),

No idea why to get the value again. You have Application already set.

::: mozmill/mozmill/extension/resource/stdlib/utils.js
@@ +45,5 @@
> + * @returns {String} The name of the app
> + */
> +function getAppName() {
> +  return appID[Services.appinfo.ID] || Services.appinfo.name;
> +}

I do not see a reason why this has to be a method. The value will never change, so this is overhead.

Comment 11

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

Andrei, can we please get the update for this patch by today? I would like to get all the pieces landed for a release of 2.0.2 on Wednesday. Thanks.

Comment 12

[Andrei Eftimie]

Attachment: 0001-Bug-946672-Get-the-App-Name-based-on-the-App-ID-firs.patch

Successfully ran both an individual metro test and a functional metro testrun -- just 1 test, but still a testrun ;)

Sorry for the delay, I had this finished yesterday, but couldn't reliably test this with the Metro mode due to mozmill-env / easy_install missing issues.

Comment 13

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

Comment on attachment 8345218 [details] [diff] [review]
0001-Bug-946672-Get-the-App-Name-based-on-the-App-ID-firs.patch

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

This looks okay to me, but I'm going to defer to Henrik for the review.

Comment 14

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

Comment on attachment 8345218 [details] [diff] [review]
0001-Bug-946672-Get-the-App-Name-based-on-the-App-ID-firs.patch

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

::: mozmill/mozmill/extension/resource/stdlib/utils.js
@@ +26,5 @@
>  var broker = {}; Cu.import('resource://mozmill/driver/msgbroker.js', broker);
>  var errors = {}; Cu.import('resource://mozmill/modules/errors.js', errors);
>  
>  var assert = new assertions.Assert();
> +var appName = appID[Services.appinfo.ID] || Services.appinfo.name;

It should also be a const given that it never changes. See my last review comment. Therefor put it right next to the other const. I would also prefer a better name for both, as best not shortened.

Comment 15

[Andrei Eftimie]

Attachment: 0001-Bug-946672-Get-the-App-Name-based-on-the-App-ID-firs.patch

Gave the new variables long (unabbreviated) names and changed the applicationName to be a constant.

Comment 16

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

Comment on attachment 8345733 [details] [diff] [review]
0001-Bug-946672-Get-the-App-Name-based-on-the-App-ID-firs.patch

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

Looks ok now. I will land it in a bit.

Comment 17

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

Landed as:
https://github.com/mozilla/mozmill/commit/88e8bd691bd70bec4bc752af9ebb61dfaac2f50a (master)
https://github.com/mozilla/mozmill/commit/ad2e63fd5a9fb8b6b298febbb40158b229d1af77 (hotfix-2.0)