999574 - System app receiving application launch events in invalid order

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Kevin Grandon :kgrandon]

Intermittent issue where --runapp will fail to display an application or homescreen. I don't believe this is the only thing that's causing bug 907103, but solving it will enable us debug that one, because this is what we're currently stuck on.

See gregors comment: https://bugzilla.mozilla.org/show_bug.cgi?id=980549#c74

(In reply to Gregor Wagner [:gwagner] from comment #74)
> We have some race condition when we start the test-agent and getting the
> ftu-skip event to display the home screen.
> 
> the breaking order is:
> 
> 1) call launch for the test-agent and we call DISPLAY:
> http://test-agent.gaiamobile.org:8080
> 2) receive the ftu-skip event and call display.
> 
> It works if we get 2) before 1)

Comment 1

[Julien Wajsberg [:julienw]]

Yep, I see this with "bin/gaia-test -d" too.

Comment 2

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Ugh, getting pretty confused here. Something like this should work, but not having too much luck on b2g desktop. B2g desktop seems to be pretty buggy too which is making this difficult to debug. Will continue to look into this though.

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8410602 [details] [review]
Github pull request

Actually it appears I was testing with b2g-bin on desktop and it was totally exploding. This seems to work totally fine with just the b2g executable, and I have verified that this works with Ben's 100% failure patch.

Comment 4

[Kevin Grandon :kgrandon]

Attachment: bug_999574_100_percent_failure.patch

Attaching the patch which causes this to fail 100% of the time.

Comment 5

[Julien Wajsberg [:julienw]]

I don't understand the difference between b2g and b2g-bin, do you know Kevin?

Comment 6

[Kevin Grandon :kgrandon]

No, unfortunately I'm not entirely sure =/

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8410602 [details] [review]
Github pull request

Hey Alive - 

Do you have any preference on how to solve this? This essentially just buffers any non-homescreen AppWindowManager.display calls until after FTU is done which should then be a safe time to display the app.

I'm more than happy to investigate alternatives, but this seemed to fix the issue for me.

Comment 8

[Gregor Wagner [:gwagner]]

Blocking a QC blocker.

Comment 9

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

Comment on attachment 8410602 [details] [review]
Github pull request

See github

Comment 10

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

(In reply to Kevin Grandon :kgrandon from comment #7)
> Comment on attachment 8410602 [details] [review]
> Github pull request
> 
> Hey Alive - 
> 
> Do you have any preference on how to solve this? This essentially just
> buffers any non-homescreen AppWindowManager.display calls until after FTU is
> done which should then be a safe time to display the app.
> 
> I'm more than happy to investigate alternatives, but this seemed to fix the
> issue for me.

Proposal: let AppWindowFactory to queue the launchapp request until ftu is done/opened or skipped.
Note: check the manifestURL !== FtuLauncher._ftuManifestURL.

Comment 11

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Hey Alive - alternative version here, with the code moved into app_window_factory. You're right - it feels more clean in there. Let me know what you think.

Comment 12

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

Comment on attachment 8411554 [details] [review]
Github pull request

Works for me.

Comment 13

[Kevin Grandon :kgrandon]

Thanks. Tests *are* broken, so if there are significant changes I will flag you or someone else for review.

Comment 14

[Kevin Grandon :kgrandon]

Updating to say that I am currently exploring another fix to avoid having to rewrite two testing frameworks. A WIP is here:

https://github.com/mozilla-b2g/gaia/pull/18722
https://github.com/KevinGrandon/gecko-dev/pull/1

Comment 15

[Kevin Grandon :kgrandon]

Attachment: Gaia patch - pull request

This is a much simplified pull request which does not require us to make large testing framework changes. As this goes with a gecko patch I'll move the review over to Vivien.

The main idea of the patch is that we signal to gecko to let it know when we are ready to launch apps. Vivien, let me know what you think. Thanks!

Comment 16

[Kevin Grandon :kgrandon]

Attachment: Gecko patch - pull request

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8413519 [details] [review]
Gecko patch - pull request

I think what you want to do is to solve the issue from the Gecko side only. By listening for the creation of mozbrowser iframes you can get a reference to the homescreen iframe beeing created and send a custom event to replace browser-ui-startup-complete.

See http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/devtools.js#151 to retrieve the mozbrowser iframe.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8413517 [details] [review]
Gaia patch - pull request

Let's not create any more glue between Gaia and Gecko and let's solve the issue directly from Gecko.

Comment 19

[Kevin Grandon :kgrandon]

Vivien - would you say that the homescreen should be a requirement for launching apps? Part of me feels like it should not, and that we're much more flexible if we control gecko from the system app like this. E.g., we can optimize and fix the system app later, then move this event to a different point.

Also if we don't want to use this, it seems like we should also try to get rid of the system-message-listener-ready event? I was basing this patch off of that usage - it's a similar need, basically let gecko know when we are ready to go.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #19)
> Vivien - would you say that the homescreen should be a requirement for
> launching apps? 

Not in the general case. But looking at runapp.js it seems like that's what it expect ? So instead of binding a new event from the homescreen app to tell runapp.js to start an app, why not just have runapp.js wait for the homescreen app to do what it wants ?

> Part of me feels like it should not, and that we're much
> more flexible if we control gecko from the system app like this. E.g., we
> can optimize and fix the system app later, then move this event to a
> different point.
> 

Controlling Gecko from the system app with events sounds bad in general. All the mozChrome/mozContent event machinery is a hack :/

> Also if we don't want to use this, it seems like we should also try to get
> rid of the system-message-listener-ready event? I was basing this patch off
> of that usage - it's a similar need, basically let gecko know when we are
> ready to go.


system-message-listener-ready sucks. So yes if we can get rid of it that would be nice.

Comment 21

[Kevin Grandon :kgrandon]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #20)
> (In reply to Kevin Grandon :kgrandon from comment #19)
> > Vivien - would you say that the homescreen should be a requirement for
> > launching apps? 
> 
> Not in the general case. But looking at runapp.js it seems like that's what
> it expect ? So instead of binding a new event from the homescreen app to
> tell runapp.js to start an app, why not just have runapp.js wait for the
> homescreen app to do what it wants ?

It's currently only expecting that because that's what will work for our testsuite =/ I don't think it's a very good fix to wait for the homescreen, but I think it's the only safe thing we can do for 1.4.

Comment 22

[Kevin Grandon :kgrandon]

Hey Vivien,

I tried with the remote-browser-shown event, but this turns out to be too early I think. With the current system app, we need to wait for the equivalent of mozbrowserloadend. Can you point me to some code for how to wait for this event in chrome land? Thanks!

Comment 23

[Kevin Grandon :kgrandon]

Nevermind, I found out how to wait for mozbrowserloadend, but I don't think this will work. I think the only way to do this is to wait on some arbitrary system event because we need to wait for a specific system render event.

Comment 24

[Kevin Grandon :kgrandon]

Changing this bug to only address the ordering situation of the launch events. May track the --runapp issue in another bug.

Comment 25

[Kevin Grandon :kgrandon]

Attachment: Pull request v2

May end up going with this.

Comment 26

[Kevin Grandon :kgrandon]

Comment on attachment 8414139 [details] [review]
Pull request v2

Hey Vivien - any thoughts on this patch? Thanks!

Comment 27

[Kevin Grandon :kgrandon]

Attachment: Gecko Patch - Ensure runapp app ordering

Hey Fabrice - the old pull request is still active, but I suppose this should be a patch instead. I've made the updates and was wondering if you could review this. Thanks!

Comment 28

[[:fabrice] Fabrice Desré]

Kevin, please post a patch here instead.

Comment 29

[Kevin Grandon :kgrandon]

(In reply to Fabrice Desré [:fabrice] from comment #28)
> Kevin, please post a patch here instead.

I uploaded a patch here: https://bug999574.bugzilla.mozilla.org/attachment.cgi?id=8414297

Comment 30

[Kevin Grandon :kgrandon]

Comment on attachment 8414297 [details] [diff] [review]
Gecko Patch - Ensure runapp app ordering

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

Hmm, why the review clear? Re-adding Fabrice, Vivien for review for review in case I'm missing something...

Comment 31

[Kevin Grandon :kgrandon]

This is on try here: https://tbpl.mozilla.org/?tree=Try&rev=c9eaaaf7c01b

Comment 32

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8414297 [details] [diff] [review]
Gecko Patch - Ensure runapp app ordering

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

Wants to see a new patch before r+'ing.

::: b2g/chrome/content/runapp.js
@@ +53,5 @@
> +    frameLoader.QueryInterface(Ci.nsIFrameLoader);
> +    // Ignore notifications that aren't from a BrowserOrApp
> +    if (!frameLoader.ownerIsBrowserOrAppFrame) {
> +      return;
> +    }

nit: add a line break

@@ +66,1 @@
>      }

Maybe you want to check the topic first and do an early return if that's not what you were expecting ? All the work above is unnecessary in this case.

@@ +70,3 @@
>      // - Get the list of apps since the parameter was specified
> +    if (this._apps.length) {
> +      this.getAllSuccess(this._apps, currentFrame)

I will not have been against an early return :)

@@ +120,5 @@
>  
> +    let appsService = Cc["@mozilla.org/AppsService;1"].getService(Ci.nsIAppsService);
> +    let currentApp = appsService.getAppByManifestURL(currentFrame.appManifestURL);
> +
> +    //let currentApp = findAppWithManifestURL(currentFrame.appManifestURL);

leftover ?

@@ +133,4 @@
>        return;
>      }
>  
> +    currentFrame.addEventListener('mozbrowserloadend', launchApp);

You may want to remove the listener once it has fired.

@@ +138,5 @@
> +    function launchApp() {
> +      let setReq =
> +        navigator.mozSettings.createLock().set({'lockscreen.enabled': false});
> +      setReq.onsuccess = function() {
> +        // give the event loop 100ms to disable the lock screen

Not your bad, but it makes me sad :/

Comment 33

[Kevin Grandon :kgrandon]

Comment on attachment 8414297 [details] [diff] [review]
Gecko Patch - Ensure runapp app ordering

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

Uploading a new version shortly...

::: b2g/chrome/content/runapp.js
@@ +66,1 @@
>      }

I'd prefer to leave it for now unless you're certain that we would always not have a BrowserOrApp frame, and it would always have a manifestURL for these events. I didn't write the original code so I thought it would be best to leave it alone for now?

@@ +120,5 @@
>  
> +    let appsService = Cc["@mozilla.org/AppsService;1"].getService(Ci.nsIAppsService);
> +    let currentApp = appsService.getAppByManifestURL(currentFrame.appManifestURL);
> +
> +    //let currentApp = findAppWithManifestURL(currentFrame.appManifestURL);

Yup, removed.

@@ +133,4 @@
>        return;
>      }
>  
> +    currentFrame.addEventListener('mozbrowserloadend', launchApp);

Done, thanks.

Comment 34

[Kevin Grandon :kgrandon]

Attachment: Gecko patch

Updated pull request with nits addressed.

Could you guys take a look at this? Thanks!

Comment 35

[Kevin Grandon :kgrandon]

Attachment: Patch - update runapp.js to wait for homescreen

New patch to update commit message, and carrying review.

Comment 36

[Gregor Wagner [:gwagner]]

Thanks Kevin!

https://hg.mozilla.org/integration/b2g-inbound/rev/f965ef325903

Comment 37

[Kevin Grandon :kgrandon]

Attachment: Patch - update commit message for proper reviewiers, dontbuild

Carrying review.

Comment 38

[Kevin Grandon :kgrandon]

Apologies, I had a wrong commit message in the above commit. I think I've fixed it here by backing out the original patch and applying a new one.

Adding checkin-needed on the patch here: https://bug999574.bugzilla.mozilla.org/attachment.cgi?id=8418178

If there are any issues importing this patch please let me know and I will try it again. Thanks!

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9fec1eb9dfeb

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/81cd7fac8df9