Make launch atom wait for both appload and appopen events.

RESOLVED FIXED

Status

Firefox OS
Gaia::UI Tests
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: zac, Assigned: zac)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
davehunt
: review+
Details | Review | Splinter Review
(Assignee)

Description

4 years ago
Our launch atom waits for apploadtime event (source unknown) and appopened (for v1.3 backwards compatibility)

Alive has explained to me that when an app is launched, appopen and appload occurs. For most Gaia apps this is fine because appopen will occur before appload.

However if the app is very basic and lightweight then appload can occur before the frame has finished animating. Gaia controls the animation of the frame to make it purdy.

In my experience debugging test_geolocation_prompt.py against desktopb2g, the launch atom is returning its event and starting the test before the Gaia animation is complete.

Marionette cannot correctly calculate the position of the elements in the app (probably) while Gaia is animating the frame so it will occasionally tap on the wrong coordinates.
We can't really use the old trick of waiting for the element to be in a particular position because Marionette is untrustworthy in this case. We need to wait for animation complete at the System level, ideally within the atom.

The ideal scenario here would be to wait for both appopen and appload events before starting the test.
I am not totally sure of the correct events to use so I am opening up this bug for some investigation by whoever is game.

This bug is only relevant to Gaia where the AppWindowManager is present, so v1.4 upwards.

It doesn't seem to be a problem on v1.3.
(Assignee)

Updated

4 years ago
Blocks: 949897, 952292
OS: Linux → Gonk (Firefox OS)
Priority: -- → P2
Hardware: x86_64 → Other
I'm not having much luck duplicating the failure of test_geolocation_prompt on b2g desktop. I ran it 275 times without a failure on my Mac Mini. I am trying again on my MBP, and I'll try a loop of 1000 runs on the mini as well.
I have yet to reproduce this on b2g desktop. I ran a loop of 1001 iterations and all of them passed. I will try again with ` test_cards_view_kill_apps_with_two_apps.py`
(Assignee)

Comment 3

4 years ago
I can replicate it running a build from Friday.

I don't know what changed but the test duration is ~2 seconds slower on this week's desktopb2g and I wasn't able to replicate it. 9.1s -> 11.1s test duration.
I just ran some tests launching the Geolocation app with various event listeners (appopened, appopen, apploadtime) and they consistently showed them firing in the following order:

appopened, appopen, apploadtime

As apploadtime fires last, and is in both 1.4 and 1.3, I wonder if we can simply wait for this event. Could you try this out Zac, as you're able to replicate the issue.
(In reply to Dave Hunt (:davehunt) from comment #4)
> I just ran some tests launching the Geolocation app with various event
> listeners (appopened, appopen, apploadtime) and they consistently showed
> them firing in the following order:
> 
> appopened, appopen, apploadtime

open is dupe of opened exactly.

> 
> As apploadtime fires last, and is in both 1.4 and 1.3, I wonder if we can
> simply wait for this event. Could you try this out Zac, as you're able to
> replicate the issue.

The order of loaded(loadtime) and opened depends the content of the app.
(Assignee)

Comment 6

4 years ago
I tried waiting for just 'apploadtime' as per comment #4 but there was no improvement.

Upon Dave's suggestion in IRC I changed the atom to wait only for 'appopen' and I got 101 passes from the test run.

With the old atom (waiting for appopened or apploadtime) I can replicate it within 10-15 tests quite easily.
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> (In reply to Dave Hunt (:davehunt) from comment #4)
> > I just ran some tests launching the Geolocation app with various event
> > listeners (appopened, appopen, apploadtime) and they consistently showed
> > them firing in the following order:
> > 
> > appopened, appopen, apploadtime
> 
> open is dupe of opened exactly.

As they fire at different times, I'm not sure they are dupes.

> > As apploadtime fires last, and is in both 1.4 and 1.3, I wonder if we can
> > simply wait for this event. Could you try this out Zac, as you're able to
> > replicate the issue.
> 
> The order of loaded(loadtime) and opened depends the content of the app.

Can we stick to the event names (apploadtime, appopened) for less ambiguity? I suspect it's more likely to do with the opening transitions, but I'm not sure.

(In reply to Zac C (:zac) from comment #6)
> I tried waiting for just 'apploadtime' as per comment #4 but there was no
> improvement.
> 
> Upon Dave's suggestion in IRC I changed the atom to wait only for 'appopen'
> and I got 101 passes from the test run.
> 
> With the old atom (waiting for appopened or apploadtime) I can replicate it
> within 10-15 tests quite easily.

Can you test this with v1.3? If the appopen event is not fired (as I suspect) then we'll need to conditionally set the event based on the presence of the app window manager. We may be able to drop the apploadtime if we can 100% rely on appopen/appopened.

Requesting more info from Alive in case any of the assumptions made on this bug are inaccurate.
Flags: needinfo?(alive)
(Assignee)

Comment 8

4 years ago
I'll test on v1.3
(Assignee)

Comment 9

4 years ago
Stability aside, functionally it works fine on v1.3.

My method was to copy the whole modified gaia_apps.js atom (that was successful in comment#6) across from v1.4 and regenerate the virtualenv with that.

I then ran test_geolocation_prompt.py against v1.3 desktop and device and it was fine.

I'll expand out to look into stability now.
(Assignee)

Comment 10

4 years ago
using appopen on v1.3 seems both stable and not to cause any regressions using a run of desktopb2g suite.

I've only ran a much smaller subset of tests on device but it seems fine there too.
Sounds like we may have a winner then! :)
(Assignee)

Comment 12

4 years ago
Worth a pull request at least!
(Assignee)

Updated

4 years ago
Assignee: nobody → zcampbell
(Assignee)

Comment 13

4 years ago
Created attachment 8356618 [details] [review]
github pr

A double check testing against v1.3 would be very appreciated.
Attachment #8356618 - Flags: review?(florin.strugariu)
Attachment #8356618 - Flags: review?(dave.hunt)
Attachment #8356618 - Flags: review?(bob.silverberg)
Comment on attachment 8356618 [details] [review]
github pr

Looks good and passes for me on both 1.4 (master) and 1.3 using gcli to launch apps. A couple of comments need to be addressed before merging though. See pull request for details.
Attachment #8356618 - Flags: review?(florin.strugariu)
Attachment #8356618 - Flags: review?(dave.hunt)
Attachment #8356618 - Flags: review?(bob.silverberg)
Attachment #8356618 - Flags: review-
(Assignee)

Comment 15

4 years ago
Comment on attachment 8356618 [details] [review]
github pr

Updated the other atom (pending Travis pass)
Updated commit message to reflect
Removed travis.yml changes
Attachment #8356618 - Flags: review- → review?(dave.hunt)

Updated

4 years ago
Attachment #8356618 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 16

4 years ago
Comment on attachment 8356618 [details] [review]
github pr

Only change event in test/atoms/gaia_apps.js
Attachment #8356618 - Flags: review- → review?(dave.hunt)
Comment on attachment 8356618 [details] [review]
github pr

Looks good, let's wait for a passing Travis-CI run before merging though.
Attachment #8356618 - Flags: review?(dave.hunt) → review+
(In reply to Dave Hunt (:davehunt) from comment #7)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> > (In reply to Dave Hunt (:davehunt) from comment #4)
> > > I just ran some tests launching the Geolocation app with various event
> > > listeners (appopened, appopen, apploadtime) and they consistently showed
> > > them firing in the following order:
> > > 
> > > appopened, appopen, apploadtime
> > 
> > open is dupe of opened exactly.
> 
> As they fire at different times, I'm not sure they are dupes.

They are.

> 
> > > As apploadtime fires last, and is in both 1.4 and 1.3, I wonder if we can
> > > simply wait for this event. Could you try this out Zac, as you're able to
> > > replicate the issue.
> > 
> > The order of loaded(loadtime) and opened depends the content of the app.
> 
> Can we stick to the event names (apploadtime, appopened) for less ambiguity?
> I suspect it's more likely to do with the opening transitions, but I'm not
> sure.
> 

For transition events:
I reserve the old name for backward compatibility. See https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_transition_controller.js#L89

apploadtime !== appopened in cold boot case.
apploadtime is implemented long time ago for performance test only, appopened(old: appopen) is an important event for inner modules in window management. I could not merge these two events. They are meaning the same things when it's warm boot but not the same when it's cold boot. I hope you understand what I mean.
Flags: needinfo?(alive)
So does that mean we shouldn't expect a different behaviour when using appopen instead of appopened? Does this mean the patch is passing by a fluke? If so, I'd rather we found a robust way to wait for the app to be fully open.
(Assignee)

Comment 20

4 years ago
This patch isn't critical - it's only blocking re-enabling one test - so we can afford to take time to get this right. As if we get it wrong we can mess up a lot!
The patch looks correct to me -- if you don't care about build before v1.3
For v1.4+ appopened event means the app is fully opened.

For v1.3- apploadtime is either a mistake - we are lucky because before v1.4 transition are slower than loading.

Lemme know your requirement. If you want to go with both v1.4+ and v1.3-, use appopen event.
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #21)
> The patch looks correct to me -- if you don't care about build before v1.3
> For v1.4+ appopened event means the app is fully opened.

But appopened wasn't working for Zac, only appopen appears to work 100% of the time. Right, Zac?

> For v1.3- apploadtime is either a mistake - we are lucky because before v1.4
> transition are slower than loading.
> 
> Lemme know your requirement. If you want to go with both v1.4+ and v1.3-,
> use appopen event.

The launch method should ideally not return until the app is opened and the opening transition is complete for both 1.4 and 1.3 as these are the two current test targets for consumers of the gaiatest package in PyPI.

From what Zac was saying, appopened currently returns before the opening transition is completed. I'm not sure how Zac is measuring this though, will needinfo him for more details. If it turns out the transition is complete, perhaps the issue is elsewhere.
Flags: needinfo?(zcampbell)
(Assignee)

Comment 23

4 years ago
(In reply to Dave Hunt (:davehunt) from comment #22)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #21)
> > The patch looks correct to me -- if you don't care about build before v1.3
> > For v1.4+ appopened event means the app is fully opened.
> 
> But appopened wasn't working for Zac, only appopen appears to work 100% of
> the time. Right, Zac?
> 
Yes that's correct.

> > For v1.3- apploadtime is either a mistake - we are lucky because before v1.4
> > transition are slower than loading.
> > 
> > Lemme know your requirement. If you want to go with both v1.4+ and v1.3-,
> > use appopen event.
> 
> The launch method should ideally not return until the app is opened and the
> opening transition is complete for both 1.4 and 1.3 as these are the two
> current test targets for consumers of the gaiatest package in PyPI.
> 
> From what Zac was saying, appopened currently returns before the opening
> transition is completed. I'm not sure how Zac is measuring this though, will
> needinfo him for more details. If it turns out the transition is complete,
> perhaps the issue is elsewhere.

My test case was just an app that fires the load event before the animation is finished. It was not a black and white test case but if we could even artificially slow the app animation it would be easier to replicate.

Alive I'm not sure if 1.3- is inclusive of 1.3 or not!  What we want to support is >= v1.3. PS I am in Taipei, I'll try and chat to you this week.
I think the animation/transition event is more important. The app's load event is often devalued by post-load ajax and so forth anyway, which we'll always need to account for with an explicit wait.
Flags: needinfo?(zcampbell)
For what it's worth, I just encountered an issue with the Browser app in b2g desktop where waiting for appopened meant that tapping on the awesome bar was too soon for the keyboard to pop-up. Changing it to appopen makes it work every time.
I'm not sure where we stand with this patch, but I figured I'd run an adhoc to see if CI has any problems with it. It's running now at http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/85/console
(Assignee)

Comment 26

4 years ago
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #21)
> Lemme know your requirement. If you want to go with both v1.4+ and v1.3-,
> use appopen event.

I think based on this comment we should go with the pull request as is here.

The above adhoc looks a bit weird; I'll rebase the PR and try again with the next nightly (current one is known bad).
(Assignee)

Comment 27

4 years ago
Alive did mention to me that appopen may be deprecated but we can cross that when we get there.
(Assignee)

Comment 28

4 years ago
I just can't replicate it on today's build using the STR from comment #3.

I wonder if it was a temporary regression of something that exposed it.
(Assignee)

Comment 29

4 years ago
I can't replicate it locally but here are two Travis runs of test_cards_view_kill_apps_with_two_apps.py run 255 times.

Current atom:
https://travis-ci.org/mozilla-b2g/gaia/jobs/16984753#
(most failures immediately after launches)

956780 proposed atom:
https://travis-ci.org/mozilla-b2g/gaia/jobs/16985375

Pending a good on-device run I still think we should merge this.
I'm not clear yet if this is the best solution. This patch does appear to help, and I'm be happy with merging this so long as it has no ill effect.

I still don't understand the difference between appopened and appopen (and why one works, but the other doesn't), or if either of these are expected to only return after the transition has completed. I wonder if it's ultimately worth also waiting for additional changes, such as CSS classes or data attribute values. It's also concerning if appopen will be deprecated, which is what we're switching to in this patch.
(Assignee)

Comment 31

4 years ago
When I looked into the CSS/data-attribute values in https://bugzilla.mozilla.org/show_bug.cgi?id=936010 I was having good success until I realised it was not backward compatible with v1.3. It will be an option when we move onto >= v1.4.

I didn't get the impression from alive that it will be deprecated soon, at least when we move off v1.3 support we can reopen 936010 perhaps.
See https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_transition_controller.js#L102 for transitioning events.
The order:
In https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_transition_controller.js#L156 the 'transition-state' property of appWindow element is changed to 'opened' and then
in https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_transition_controller.js#L88 we publish appopened at first,
and publish backward-compatible appopen event at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_transition_controller.js#L107
The full opening process:

1. https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_transition_controller.js#L235
   Transition State changes from 'closed' to 'opening'
2. AppTransitionController's _do_opening put opening class on the appWindow element
3. AppTransitionController publish 'opening' event
4. Whenever animationend or timeout occurs, the state changes from 'opening' to 'opened'
5. AppTransitionController publish 'opened' event which triggers:
   AppWindow.broadcast('_opened') -trigger-> AppTransitionController._handle_opened, which does setVisible(means usable) + append active class
   Then AppWindow.publish('opened') would dispatch appopened event on |window| which trigger your handler.
(Assignee)

Comment 35

4 years ago
A perfect green on device with this patch:
http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/97/console
Based on comment 33 and comment 34, there's no difference between appopened and appopen. Therefore we shouldn't rely on one being an improvement over the other. They are fired one after the other, but there's no change of state between them being fired, so it would be risky to assume we can rely on the 'later' event.

Alive: Is the _do_opened method asynchronous? If so, it's possible the events are fired before the transition is completed. If this is the case it may be safer to wait for the window to not have the class of 'transition-opening'.

Zac: You mentioned some success with doing this in 1.4 (comment 31), but had issues with backwards compatibility. Perhaps we can either determine what changed (and what to wait for) in 1.3 and make this conditional, or we consider only fixing this in 1.4 and letting 1.3 continue with the existing solution. We should be able to base any conditional logic on window.wrappedJSObject.AppWindowManager being defined.
(Assignee)

Comment 37

4 years ago
Dave my interpretation of the improvement in this patch is the removal of apploadtime is the key improvement because in some cases it is fired before the transition animation is finished, in turn leaving us exposed to Marionette's flaky location algorithm trying to interact with a moving target. but without apploadtime we have to switch to use appopen (instead of the appopened dupe) to retain v1.3 backwards compatibility.

For now can we choose the current solution (which we've well tested anyway) and aim for a better one when we merge the atoms or when we are off v1.3 and can rely on AppWindowManager 100%?
(In reply to Zac C (:zac) from comment #37)
> Dave my interpretation of the improvement in this patch is the removal of
> apploadtime is the key improvement because in some cases it is fired before
> the transition animation is finished, in turn leaving us exposed to
> Marionette's flaky location algorithm trying to interact with a moving
> target. but without apploadtime we have to switch to use appopen (instead of
> the appopened dupe) to retain v1.3 backwards compatibility.
> 
> For now can we choose the current solution (which we've well tested anyway)
> and aim for a better one when we merge the atoms or when we are off v1.3 and
> can rely on AppWindowManager 100%?

I agree that dropping listening for apploadtime makes sense. I also agree that we should use appopen if appopened is not supported in v1.3. For those reasons I still support this patch, but I do feel that if appopened and appopen behave differently on v1.4 then there's a risk of intermittents.

Updated

4 years ago
Blocks: 959217
Landed in:
https://github.com/mozilla-b2g/gaia/commit/100cc7421f68307460dc5ea0533516e33699ff88

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.