Kevin Grandon :kgrandon Assignee
	Description • 10 years ago

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)

	Julien Wajsberg [:julienw]
	Comment 1 • 10 years ago

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

	Kevin Grandon :kgrandon Assignee
	Comment 2 • 10 years ago

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.

	Kevin Grandon :kgrandon Assignee
	Comment 3 • 10 years ago

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.

	Kevin Grandon :kgrandon Assignee
	Comment 4 • 10 years ago

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

	Julien Wajsberg [:julienw]
	Comment 5 • 10 years ago

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

	Kevin Grandon :kgrandon Assignee
	Comment 6 • 10 years ago

No, unfortunately I'm not entirely sure =/

	Kevin Grandon :kgrandon Assignee
	Comment 7 • 10 years ago

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.

	Gregor Wagner [:gwagner]
	Comment 8 • 10 years ago

Blocking a QC blocker.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 9 • 10 years ago

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

See github

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 10 • 10 years ago

(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.

	Kevin Grandon :kgrandon Assignee
	Comment 11 • 10 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 12 • 10 years ago

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

Works for me.

	Kevin Grandon :kgrandon Assignee
	Comment 13 • 10 years ago

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

	Kevin Grandon :kgrandon Assignee
	Comment 14 • 10 years ago

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

	Kevin Grandon :kgrandon Assignee
	Comment 15 • 10 years ago

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!

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 17 • 10 years ago

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.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 18 • 10 years ago

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.

	Kevin Grandon :kgrandon Assignee
	Comment 19 • 10 years ago

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.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 20 • 10 years ago

(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.

	Kevin Grandon :kgrandon Assignee
	Comment 21 • 10 years ago

(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.

	Kevin Grandon :kgrandon Assignee
	Comment 22 • 10 years ago

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!

	Kevin Grandon :kgrandon Assignee
	Comment 23 • 10 years ago

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.

	Kevin Grandon :kgrandon Assignee
	Comment 24 • 10 years ago

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

	Kevin Grandon :kgrandon Assignee
	Comment 25 • 10 years ago

May end up going with this.

	Kevin Grandon :kgrandon Assignee
	Comment 26 • 10 years ago

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

Hey Vivien - any thoughts on this patch? Thanks!

	Kevin Grandon :kgrandon Assignee
	Comment 27 • 10 years ago

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!

	[:fabrice] Fabrice Desré
	Comment 28 • 10 years ago

Kevin, please post a patch here instead.

	Kevin Grandon :kgrandon Assignee
	Comment 29 • 10 years ago

(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

	Kevin Grandon :kgrandon Assignee
	Comment 30 • 10 years ago

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...

	Kevin Grandon :kgrandon Assignee
	Comment 31 • 10 years ago

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

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 32 • 10 years ago

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 :/

	Kevin Grandon :kgrandon Assignee
	Comment 33 • 10 years ago

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.

	Kevin Grandon :kgrandon Assignee
	Comment 34 • 10 years ago

Updated pull request with nits addressed.

Could you guys take a look at this? Thanks!

	Kevin Grandon :kgrandon Assignee
	Comment 35 • 10 years ago

New patch to update commit message, and carrying review.

	Gregor Wagner [:gwagner]
	Comment 36 • 10 years ago

Thanks Kevin!

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

	Kevin Grandon :kgrandon Assignee
	Comment 37 • 10 years ago

Carrying review.

	Kevin Grandon :kgrandon Assignee
	Comment 38 • 10 years ago

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!

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 40 • 10 years ago

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