1161489 - Gecko should let gaia know mozApp is ready by state check or some way else instead of event

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

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

Current applications.js in gaia system is waiting for a mozChromeEvent to tell us "now we could query all mozApps information".

If it is started(listening event) too late, it will miss the event.

We should not rely on the event only to query app info; there should be state check in mozApps to tell us it's ready to query or not.

Comment 1

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

Looks like I can remove the event by wiring getAll() calls to _saveToClone promise here.

https://dxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#357

So the actual content of getAll does not run until saveToClone is resolved.

Fabrice, is this the trivial fix to prevent us from waiting for "webapps-registry-ready" mozChromeEvent in System app? 

BTW we should remove webapps-registry-start mozChromeEvent right? It's not used everywhere but it's in shell.js. I blamed the file and found this https://hg.mozilla.org/mozilla-central/rev/357778ffa801, which has nothing to do with Gaia.

Comment 2

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

Attachment: Patch part 1

I did not remove the promise nor the observer notification because someone else is using them. This is the first patch to land.

Comment 3

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

Attachment: Part 2, remove the mozChromeEvents

This is the second part. Should be landed after Gaia have the listener removed. With this patch new Gecko will no longer work with old Gaia since the old Gaia will wait for the event on booting. The two patch should separate for at least one week.

I will also send out and e-mail to dev-b2g just to ask people to keep their Gaia/Gecko in sync.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8636385 [details] [diff] [review]
Patch part 1

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

There is no guarantee that getAll() will be the first api method called by consumers, so that's not enough. We could try doing that in receiveMessage() where we route all the child -> parent messages.

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 8636389 [details] [diff] [review]
Part 2, remove the mozChromeEvents

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

I guess that will be ok after the gaia changes, but please move this patch to its own bug carrying r+. Thanks!

Comment 6

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

(In reply to [:fabrice] NOT READING BUGMAIL UNTIL JULY 27 from comment #4)
> Comment on attachment 8636385 [details] [diff] [review]
> Patch part 1
> 
> Review of attachment 8636385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is no guarantee that getAll() will be the first api method called by
> consumers, so that's not enough. We could try doing that in receiveMessage()
> where we route all the child -> parent messages.

You are right... the fact that both WebApps.jsm and AppsServiceChild.jsm expose the same |DOMApplicationRegistry| symbol is utterly confusing.

Since WebApps.js always imports AppsServiceChild.jsm (as opposed to AppsServices.js), it looks like a getAll() call in Gaia does not goes to WebApps.jsm at all, and it simply returns content from |this.webapps| which AppsServiceChild.jsm have already pulled with "Webapps:GetList" message in |init()|.

"Webapps:GetList" is handled in WebApps.jsm by doGetList(), and it already protected by the |safeToClone| promise. We should therefore safe to assume getAll() is always protected.

So it looks like we could simply land the 2nd patch without touching anything?

Comment 7

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

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #6)
> So it looks like we could simply land the 2nd patch without touching
> anything?

Gaia needs to be patched first of course. I mean, Apps API should be able to safely be called by Gaia w/o us modifying anything.

Comment 8

[[:fabrice] Fabrice Desré]

Yes, getAll() got optimized to not do extra IPC, and in a *child* process getAll() will always return what you expect. But not in the parent, since in this case we end up in Webapps.jsm. So if the system app needs getAll(), you need to make sure that you're not calling it too early.

Comment 9

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

(In reply to [:fabrice] NOT READING BUGMAIL UNTIL JULY 27 from comment #8)
> Yes, getAll() got optimized to not do extra IPC, and in a *child* process
> getAll() will always return what you expect. But not in the parent, since in
> this case we end up in Webapps.jsm. So if the system app needs getAll(), you
> need to make sure that you're not calling it too early.

I don't understand what you mean. WebApps.js always imports AppsServiceChild.jsm [1]

Cu.import("resource://gre/modules/AppsServiceChild.jsm");

Only AppsService.js do fancy like switching to WebApps.jsm in parent process.

function AppsService()
{
  debug("AppsService Constructor");
  this.inParent = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
                    .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
  debug("inParent: " + this.inParent);
  Cu.import(this.inParent ? "resource://gre/modules/Webapps.jsm" :
                            "resource://gre/modules/AppsServiceChild.jsm");
}

Per previous comment, since WebApps.js always imports AppsServiceChild.jsm, shouldn't we already getting |this.webapps| up and ready when init() returns? Unless you kind of make it async again with threadManager (if I read the code correctly).

[1] https://dxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.js#15
[2] https://dxr.mozilla.org/mozilla-central/source/dom/apps/AppsService.js#26-34

Comment 10

[[:fabrice] Fabrice Desré]

Sorry, you are right about getAll(). I just filed Bug 1191579 to remove the now useless implementation from Webapps.jsm

getAll() is always async since it's returning a DOMRequest, but you are guaranteed that the registry is available when the mozApps object is accessed: this triggers loading Webapps.js, itself loading AppsServiceChild.jsm which does a synchronous ipc call to get the list of apps.

So for getAll(), we actually can't race with the registry indeed. Good news! But if we want to use other mozApps methods, this will not necessarily be the case, so I think it's still useful to dispatch the `registry-ready` event to gaia, even if it's not using it to guard getAll(). We have other consumers of the b2g runtime that may need that.

Comment 11

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

(In reply to [:fabrice] Fabrice Desré from comment #10)
> Sorry, you are right about getAll(). I just filed Bug 1191579 to remove the
> now useless implementation from Webapps.jsm
> 
> getAll() is always async since it's returning a DOMRequest, but you are
> guaranteed that the registry is available when the mozApps object is
> accessed: this triggers loading Webapps.js, itself loading
> AppsServiceChild.jsm which does a synchronous ipc call to get the list of
> apps.

I want to be on the safe side here. What does synchronous ipc call actually do if both the receiver and the caller lives in the chrome process main thread (the System app situation), especially when there is a threadManager call [1] looks really suspicious of calling next tasks while the *synchronous* request is blocked?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#1559-1563

Does that means the Gaia code do, say, |var apps = navigator.mozApp| will be blocked too, so we should be safe when Gaia can finally call |apps.getAll()|? Isn't that breaks run to completion for System app if other System app code is executed while the function is blocked, say, by user interaction?

I am sorry I don't understand XPCOM well enough to tell when what I described happens.

> So for getAll(), we actually can't race with the registry indeed. Good news!
> But if we want to use other mozApps methods, this will not necessarily be
> the case, so I think it's still useful to dispatch the `registry-ready`
> event to gaia, even if it's not using it to guard getAll(). We have other
> consumers of the b2g runtime that may need that.

What are the "other mozApp methods"? Do we have synchronous methods on mozApp that will never behave correctly if accessed too early? If so we should fix them here, not just getAll().

The purpose of this bug, more generally, should be described as "ensure mozApp API behaves correctly at the moment they are available." I am fine if we ended up need to keep the mozChromeEvent for compatibility reason; I could just patch Gaia and let it not to wait for it.

Thanks for the reply.

Comment 12

[[:fabrice] Fabrice Desré]

Hi Tim,

sorry for the late reply...

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #11)

> I want to be on the safe side here. What does synchronous ipc call actually
> do if both the receiver and the caller lives in the chrome process main
> thread (the System app situation), especially when there is a threadManager
> call [1] looks really suspicious of calling next tasks while the
> *synchronous* request is blocked?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#1559-1563
> 
> Does that means the Gaia code do, say, |var apps = navigator.mozApp| will be
> blocked too, so we should be safe when Gaia can finally call
> |apps.getAll()|? Isn't that breaks run to completion for System app if other
> System app code is executed while the function is blocked, say, by user
> interaction?
> 
> I am sorry I don't understand XPCOM well enough to tell when what I
> described happens.

This kind of nested event loop is what we also do when alert() is called, and it's not breaking run to completion, so I think we're fine.

> > So for getAll(), we actually can't race with the registry indeed. Good news!
> > But if we want to use other mozApps methods, this will not necessarily be
> > the case, so I think it's still useful to dispatch the `registry-ready`
> > event to gaia, even if it's not using it to guard getAll(). We have other
> > consumers of the b2g runtime that may need that.
> 
> What are the "other mozApp methods"? Do we have synchronous methods on
> mozApp that will never behave correctly if accessed too early? If so we
> should fix them here, not just getAll().

I looked at the code again, and indeed even in the parent process we block on receiving all the data we need when getting the mozApps object. So we're good.

> The purpose of this bug, more generally, should be described as "ensure
> mozApp API behaves correctly at the moment they are available." I am fine if
> we ended up need to keep the mozChromeEvent for compatibility reason; I
> could just patch Gaia and let it not to wait for it.

What I think we do wrong in shell.js startup is that we start the system app once we receive the value for `language.current` setting, but we don't wait for the registry to be ready. It seems like we are just always lucky because the settings part is so slow...

I have some refactoring coming up for all this startup where I'll fix that. That will mean that gaia will have 100% guarantee that the mozapps api is ready when we launch any app.

Comment 13

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

Unassign myself due to project changes.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Closing as we are not working on Firefox OS anymore.