830240 - Gaia UI tests failing to launch/kill apps

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

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

Recent changes to Gaia have caused a majority of the Gaia UI tests to fail. The failures appear to be related to the launchWithName method in tests/atoms/gaia_apps.js returning before the application frame is available.

To replicate, you can run the following:

$ cd ~/workspace
$ git clone git://github.com/mozilla/gaia-ui-tests.git
$ cd gaia-ui-tests
$ python setup.py develop
$ adb forward tcp:2828 tcp:2828
$ gaiatest --address=localhost:2828 gaiatest/tests/unit/test_killall.py

You will need a target with the latest Gaia revision and running a build with Marionette enabled.

All tests should pass as these simply check that the launch/kill methods used frequently by the tests are functioning. Instead, the following (cropped) output is returned:

starting httpd
running webserver on http://10.61.32.79:57618/
TEST-START test_killall.py
test_kill_all (test_killall.TestKillAll) ... ERROR
test_kill_all_twice (test_killall.TestKillAll) ... ERROR
test_kill_all_with_no_apps_running (test_killall.TestKillAll) ... ERROR

======================================================================
ERROR: test_kill_all (test_killall.TestKillAll)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/dhunt/workspace/gaia-ui-tests/gaiatest/tests/unit/test_killall.py", line 12, in test_kill_all
    self.apps.launch(app)
  File "/Users/dhunt/workspace/gaia-ui-tests/gaiatest/gaia_test.py", line 66, in launch
    self.switch_to_frame(app.frame_id, url)
  File "/Users/dhunt/workspace/gaia-ui-tests/gaiatest/gaia_test.py", line 93, in switch_to_frame
    self.marionette.switch_to_frame(app_frame)
  File "/Users/dhunt/workspace/mozilla-central/testing/marionette/client/marionette/marionette.py", line 381, in switch_to_frame
    response = self._send_message('switchToFrame', 'ok', value=frame, focus=focus)
  File "/Users/dhunt/workspace/mozilla-central/testing/marionette/client/marionette/marionette.py", line 225, in _send_message
    self._handle_error(response)
  File "/Users/dhunt/workspace/mozilla-central/testing/marionette/client/marionette/marionette.py", line 248, in _handle_error
    raise NoSuchFrameException(message=message, status=status, stacktrace=stacktrace)
TEST-UNEXPECTED-FAIL | test_killall.py TestKillAll.test_kill_all | NoSuchFrameException: Unable to locate frame: appframe4
======================================================================
ERROR: test_kill_all_twice (test_killall.TestKillAll)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/dhunt/workspace/gaia-ui-tests/gaiatest/tests/unit/test_killall.py", line 25, in test_kill_all_twice
    self.apps.launch(app)
  File "/Users/dhunt/workspace/gaia-ui-tests/gaiatest/gaia_test.py", line 66, in launch
    self.switch_to_frame(app.frame_id, url)
  File "/Users/dhunt/workspace/gaia-ui-tests/gaiatest/gaia_test.py", line 93, in switch_to_frame
    self.marionette.switch_to_frame(app_frame)
  File "/Users/dhunt/workspace/mozilla-central/testing/marionette/client/marionette/marionette.py", line 381, in switch_to_frame
    response = self._send_message('switchToFrame', 'ok', value=frame, focus=focus)
  File "/Users/dhunt/workspace/mozilla-central/testing/marionette/client/marionette/marionette.py", line 225, in _send_message
    self._handle_error(response)
  File "/Users/dhunt/workspace/mozilla-central/testing/marionette/client/marionette/marionette.py", line 248, in _handle_error
    raise NoSuchFrameException(message=message, status=status, stacktrace=stacktrace)
TEST-UNEXPECTED-FAIL | test_killall.py TestKillAll.test_kill_all_twice | NoSuchFrameException: Unable to locate frame: appframe5
======================================================================
ERROR: test_kill_all_with_no_apps_running (test_killall.TestKillAll)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/dhunt/workspace/gaia-ui-tests/gaiatest/tests/unit/test_killall.py", line 18, in test_kill_all_with_no_apps_running
    self.check_no_apps_running()
  File "/Users/dhunt/workspace/gaia-ui-tests/gaiatest/tests/unit/test_killall.py", line 36, in check_no_apps_running
    runningApps = self.apps.runningApps()
  File "/Users/dhunt/workspace/gaia-ui-tests/gaiatest/gaia_test.py", line 89, in runningApps
    """)
  File "/Users/dhunt/workspace/mozilla-central/testing/marionette/client/marionette/marionette.py", line 465, in execute_script
    specialPowers=special_powers)
  File "/Users/dhunt/workspace/mozilla-central/testing/marionette/client/marionette/marionette.py", line 225, in _send_message
    self._handle_error(response)
  File "/Users/dhunt/workspace/mozilla-central/testing/marionette/client/marionette/marionette.py", line 260, in _handle_error
    raise JavascriptException(message=message, status=status, stacktrace=stacktrace)
JavascriptException: InternalError: too much recursion
stacktrace:
	EM_wrapValue@chrome://marionette/content/marionette-elements.js:149
	EM_wrapValue@chrome://marionette/content/marionette-elements.js:149

...this line is repeated hundreds (thousands?) of times.

        EM_wrapValue@chrome://marionette/content/marionette-elements.js:149
	EM_wrapValue@chrome://marionette/content/marionette-elements.js:149
TEST-UNEXPECTED-FAIL | test_killall.py TestKillAll.test_kill_all_with_no_apps_running | 	EM_wrapValue@chrome://marionette/content/marionette-elements.js:149
----------------------------------------------------------------------
Ran 3 tests in 39.646s

Comment 1

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

After a bisect I found that has been caused by https://github.com/mozilla-b2g/gaia/commit/5d583cfc84b78b4a1375faf3fb5c5e0644181e9c

Comment 2

[Jason Smith [:jsmith]]

Alive - Any ideas?

Comment 3

[Will Kahn-Greene [:willkg] ET needinfo? me]

I just hit this same problem with the integration tests. Here's what I know from poking around a bit.

After commit 5d583cfc, calling GaiaApps.launchWithName('Email') shows this marionette debug output:

  marionette:command-stream writing  +1ms { type: 'executeAsyncScript',
  value: 'GaiaApps.launchWithName("Email");',
  args: [],
  to: 'conn0.marionette1',
  session: '6-b2g' } to socket
  marionette:command-stream got raw bytes  +279ms {
  "from": "conn0.marionette1",
  "value": {
    "frame": "appframe1",
    "src": null,
    "name": "E-Mail",
    "origin": "app://email.gaiamobile.org"
  }
}


Of note, the "src" is null and the frame is "appframe1" which causes launchWithName to die when it tries to switch to that frame.

The code for that atom is here:

https://github.com/mozilla-b2g/gaia/blob/master/tests/atoms/gaia_apps.js#L146

On line 158 of that file, if you change:

    src: app.frame.src,

to:

    src: app.iframe.src,

that picks up the right src because of this change:

https://github.com/mozilla-b2g/gaia/commit/5d583cfc84b78b4a1375faf3fb5c5e0644181e9c#L1R1380

If you change:

    frame: app.frame.id,

to:

    frame: app.iframe.id,

that doesn't work--you get a null because the code doesn't seem to be populating the iframe id. I'm not sure where to get the name for the iframe.

Comment 4

[Jason Smith [:jsmith]]

Noming to block because this blocks running automation (UI tests and JS integration tests).

Comment 5

[Will Kahn-Greene [:willkg] ET needinfo? me]

Oo--just figured it out. Grabbing this one and I'll create a patch.

Comment 6

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: link to pull 7599

Comment 7

[Will Kahn-Greene [:willkg] ET needinfo? me]

It's test-only, so it doesn't need approval to land. It just needs someone to review it. I don't know offhand who should review this. Any ideas?

Comment 8

[James Lal [:lightsofapollo]]

Comment on attachment 701863 [details]
link to pull 7599

Looks good to me, lets add some error detection code here though to make sure we don't run into similar issues.. We can do this by detecting some attributes on the element we return to verify it really is the app iframe. Then in the future if we change things we will know its this problem.

Comment 9

[Will Kahn-Greene [:willkg] ET needinfo? me]

My patch fixes my JS integration tests. However, the Python integration tests have their own atom code in the gaiatest repository. Pretty sure it needs an equivalent fix here:

https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/atoms/gaia_apps.js#L158

I can probably do that next basing it on the change I made in the gaia repository.

Comment 10

[Zac C (:zac)]

:willkg that would be much appreciated. Thanks for your investigation :)

Comment 11

[James Lal [:lightsofapollo]]

We landed this on the gaia side... Since we still don't share a history you will need to manually add the change to gaia-ui-tests and then we can close this bug.

Comment 12

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

Ths appears to fix the launching issue, but there's still an issue with killing apps. The same tests mentioned in comment 0 are failing, but now all due to the 'too much recursion' issue.

JavascriptException: InternalError: too much recursion
stacktrace:
	EM_wrapValue@chrome://marionette/content/marionette-elements.js:149
	EM_wrapValue@chrome://marionette/content/marionette-elements.js:149

Comment 13

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

Also, tests/unit/test_launch_entrypoint.py is failing because the ID returned is now '' instead of the app frame ID. This means that we're probably switching to the system app context when we're expecting to switch to the app frame.

Comment 14

[Jonathan Griffin (:jgriffin)]

(In reply to Dave Hunt (:davehunt) from comment #12)
> Ths appears to fix the launching issue, but there's still an issue with
> killing apps. The same tests mentioned in comment 0 are failing, but now all
> due to the 'too much recursion' issue.
> 
> JavascriptException: InternalError: too much recursion
> stacktrace:
> 	EM_wrapValue@chrome://marionette/content/marionette-elements.js:149
> 	EM_wrapValue@chrome://marionette/content/marionette-elements.js:149

I'll look at this recursion issue.

Comment 15

[Will Kahn-Greene [:willkg] ET needinfo? me]

I think that recursion issue is from test_kill_all_with_no_apps_running in test_killall.py. I haven't seen it in any of the other unit tests I've run.

The other two tests in test_killall.py error out without a recursion error. I see what Dave is seeing with test_launch_entrypoint.py where the frame id is '' and that's definitely problematic.

I have no idea why this fix worked for the JS integration tests, but fails for the Python integration tests. Dave is correct in that the gaia_apps.js atom file is identical between the two (after making the two-line fix). So, I'm guessing it's a difference between the launch code used in the Python tests vs. the launch code in the JS tests.

Comment 16

[Jonathan Griffin (:jgriffin)]

The JS tests don't use the kill/killAll atoms, so they don't see the recursion problems.

Comment 17

[James Lal [:lightsofapollo]]

JS has a max stack depth, either the file is doing something it really should not be or its doing the right thing and hitting the limit. In the latter case you can use setTimeout to work around the issue by remove some stack depth...

Comment 18

[Jonathan Griffin (:jgriffin)]

Actually the recursion problem comes from this code:

    def runningApps(self):
        apps = self.marionette.execute_script("""
return window.wrappedJSObject.WindowManager.getRunningApps();
            """)
        return apps

runningApps (from Gaia's window manager) is now a more complicated object, one which can't be converted into a JSON-serializable form for transport to the Marionette client.  I'll have a fix soon.

Comment 19

[Jonathan Griffin (:jgriffin)]

Attachment: Pointer to pull request

Comment 20

[Jonathan Griffin (:jgriffin)]

test_launch_entrypoint is failing because application frames do not have to have an id any longer.  Here's the clock iframe after launch:

<div class="appWindow opening" id="appframe16" style="width: 320px; height: 460px; background: -moz-linear-gradient(center top , rgba(0, 0, 0, 0.5) 0%, rgba(0, 0, 0, 0.5) 100%) repeat scroll 0% 0%, url(&quot;blob:b9a7b509-9908-418d-a94d-525c101357f0&quot;) repeat scroll 0% 0%, none repeat scroll 0% 0% rgb(255, 255, 255);" data-bg-object-u-r-l="blob:b9a7b509-9908-418d-a94d-525c101357f0"><iframe mozallowfullscreen="true" data-frame-origin="app://clock.gaiamobile.org" data-frame-u-r-l="app://clock.gaiamobile.org/index.html" data-unpainted="true" mozbrowser="true" remote="true" mozapp="app://clock.gaiamobile.org/manifest.webapp" src="app://clock.gaiamobile.org/index.html" data-frame-type="window"></iframe></div>

Notice that the id now belongs to the <div> element, not the <iframe>.  In Marionette, we need to identify the <iframe> so we can switch to it, so we'll have to use another method.

Comment 21

[Jonathan Griffin (:jgriffin)]

https://github.com/mozilla/gaia-ui-tests/pull/263 updated with a fix for test_launch_entrypoint as well.

Comment 22

[Will Kahn-Greene [:willkg] ET needinfo? me]

Hrm. You're right. I don't know why the patch I had worked at all. I was definitely seeing tests pass, but they're definitely not passing now that I'm double-checking everything. So I'm thoroughly puzzled.

The switchToFrame in gaia/tests/js/vendor/marionette.js only takes ids (as I read it) and can't take an element. So I think that needs to get fixed. Otherwise the changes to the launchWithName atom you did won't work in the js integration tests.

Having said that, I've been kind of wrong all along and it's late, so take this with a grain of salt.

Comment 23

[Jonathan Griffin (:jgriffin)]

(In reply to Will Kahn-Greene [:willkg] from comment #22)
> Hrm. You're right. I don't know why the patch I had worked at all. I was
> definitely seeing tests pass, but they're definitely not passing now that
> I'm double-checking everything. So I'm thoroughly puzzled.
> 
> The switchToFrame in gaia/tests/js/vendor/marionette.js only takes ids (as I
> read it) and can't take an element. So I think that needs to get fixed.
> Otherwise the changes to the launchWithName atom you did won't work in the
> js integration tests.
> 
> Having said that, I've been kind of wrong all along and it's late, so take
> this with a grain of salt.

switchToFrame can actually take an id or an Element object in marionette.js; if an Element object, we pass Element.id to the Marionette server.  The 'id' property of an Element object is actually a server-assigned uuid, and not the 'id' attribute of the corresponding tag.

Looking at the current code in gaia, I think it should work with these changes without modification.

Comment 24

[(inactive after 6/18) Alive Kuo [:alive]]

Sounds like the patch of bug 824677 also influences these tests which are deeply bind to Window Manager.. sad, I didn't notice this.

BTW, I am told by Vivien that the 'iframe' property should be renamed to 'browser' ....afterward.
Who should I contact if I change this? Thanks.

Comment 25

[Jonathan Griffin (:jgriffin)]

(In reply to Alive Kuo [:alive] from comment #24)
> Sounds like the patch of bug 824677 also influences these tests which are
> deeply bind to Window Manager.. sad, I didn't notice this.
> 
> BTW, I am told by Vivien that the 'iframe' property should be renamed to
> 'browser' ....afterward.
> Who should I contact if I change this? Thanks.

Please let me know, :jgriffin on bugzilla or #ateam on irc.  Thanks!

Comment 26

[Jonathan Griffin (:jgriffin)]

Comment on attachment 701973 [details]
Pointer to pull request

Dave merged the PR.  Need to mirror to gaia now.

Comment 27

[Zac C (:zac)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/gaia-ui-tests/pull/263

Pointer to Github pull-request

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Zac C (:zac) from comment #27)
> Created attachment 702122 [details]
> Pointer to Github pull request:
> https://github.com/mozilla/gaia-ui-tests/pull/263
> 
> Pointer to Github pull-request

Yes, Zac's fix is correct, sorry for not communicating across the team about the change.

Comment 29

[Zac C (:zac)]

That was an accident, I accidentally clicked the link in Github! Sorry

Comment 30

[Will Kahn-Greene [:willkg] ET needinfo? me]

Passing this to :jgriffin who's doing the work.

Comment 31

[Jonathan Griffin (:jgriffin)]

AFAIK, this is fixed now.