Closed Bug 999574 Opened 6 years ago Closed 6 years ago

System app receiving application launch events in invalid order

Categories

(Firefox OS Graveyard :: Gaia, defect)

x86
macOS
defect
Not set

Tracking

(blocking-b2g:1.4+, firefox30 wontfix, firefox31 wontfix, firefox32 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: kgrandon, Assigned: kgrandon)

References

Details

(Whiteboard: [p=4],[systemsfe])

Attachments

(2 files, 8 obsolete files)

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)
Blocks: 980549
No longer blocks: 907103
Yep, I see this with "bin/gaia-test -d" too.
Attached file Github pull request (obsolete) —
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 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.
Attachment #8410602 - Attachment description: not yet working patch → Github pull request
Attaching the patch which causes this to fail 100% of the time.
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
I don't understand the difference between b2g and b2g-bin, do you know Kevin?
No, unfortunately I'm not entirely sure =/
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.
Attachment #8410602 - Flags: review?(alive)
Blocking a QC blocker.
blocking-b2g: --- → 1.4+
See Also: → 985037
Comment on attachment 8410602 [details] [review]
Github pull request

See github
Attachment #8410602 - Flags: review?(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.
Attached file Github pull request (obsolete) —
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.
Attachment #8410602 - Attachment is obsolete: true
Attachment #8411554 - Flags: review?(alive)
Comment on attachment 8411554 [details] [review]
Github pull request

Works for me.
Attachment #8411554 - Flags: review?(alive) → review+
Thanks. Tests *are* broken, so if there are significant changes I will flag you or someone else for review.
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
Attached file Gaia patch - pull request (obsolete) —
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!
Attachment #8411554 - Attachment is obsolete: true
Attachment #8413517 - Flags: review?(21)
Attached file Gecko patch - pull request (obsolete) —
Attachment #8413519 - Flags: review?(21)
Attachment #8413517 - Attachment description: Github pull request → Gaia patch - pull request
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.
Attachment #8413519 - Flags: review?(21) → review-
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.
Attachment #8413517 - Flags: review?(21)
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.
Flags: needinfo?(21)
(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.
Flags: needinfo?(21)
(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.
Whiteboard: [p=4],[systemsfe]
Target Milestone: --- → 2.0 S1 (9may)
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!
Flags: needinfo?(21)
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.
Flags: needinfo?(21)
Changing this bug to only address the ordering situation of the launch events. May track the --runapp issue in another bug.
Summary: Runapp CLI argument fails to display application → System app receiving application launch events in invalid order
Attached file Pull request v2 (obsolete) —
May end up going with this.
Comment on attachment 8414139 [details] [review]
Pull request v2

Hey Vivien - any thoughts on this patch? Thanks!
Attachment #8414139 - Flags: review?(21)
Attachment #8414139 - Flags: review?(21) → review-
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!
Attachment #8414139 - Attachment is obsolete: true
Attachment #8414297 - Flags: review?(fabrice)
Kevin, please post a patch here instead.
(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
Attachment #8414297 - Flags: review?(fabrice)
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...
Attachment #8414297 - Flags: review?(fabrice)
Attachment #8414297 - Flags: review?(21)
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 :/
Attachment #8414297 - Flags: review?(21) → feedback+
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.
Attachment #8414297 - Flags: review?(fabrice)
Attached patch Gecko patch (obsolete) — Splinter Review
Updated pull request with nits addressed.

Could you guys take a look at this? Thanks!
Attachment #8414297 - Attachment is obsolete: true
Attachment #8417515 - Flags: review?(fabrice)
Attachment #8417515 - Flags: review?(21)
Attachment #8413519 - Attachment is obsolete: true
Attachment #8413517 - Attachment is obsolete: true
Attachment #8410605 - Attachment is obsolete: true
Attachment #8417515 - Flags: review?(fabrice)
New patch to update commit message, and carrying review.
Attachment #8417515 - Attachment is obsolete: true
Attachment #8418120 - Flags: review+
Keywords: checkin-needed
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!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9fec1eb9dfeb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.