903291 - App objects on the child processes sometimes are created with a wrong state

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Carmen Jimenez Cabezas]

STR:
1. Connect to any page that installs apps (for example, Mozilla marketplace).
2. Click Install
3. Kill the installer app (marketplace, browser, whatever) before the confirm dialog appears. Note that while I'm killing it intentionally, it could be killed by OOM
4. Click Install on the dialog

Expected results (and what happened before bug 873567 landed):

The app is installed correctly

Actual results:

The app icon appears (or not, half of the times I don't even get the icon, but still the app *is* added to webapps) but the app never finishes installing (in fact it never ever starts downloading). The only thing the user can do (if he got lucky and actually got the icon) is to 'deinstall' the app and try again.

If he didn't get the icon, then any further attempts to install the app will fail silently (since it'll be considered a reinstall). After a reboot of the phone, the icons appear and then he can deinstall the app and try again.

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

The solution for 893800 depends on the solution taken here. FWIW, I don't like much the solution taken on bug 873567. If the intention was to ensure that the onsuccess handler had time to be defined no matter the download speed I think a better approach would have been to queue the event on the child process in the case it was received before the handler was set, and then fire the events immediately if the event handler is set after the event has happened. That way the actual download flow isn't impacted by the child process.

Comment 2

[Antonio Manuel Amaya Calvo (:amac)]

Besides breaking this, the original patch fixed it so the tests run but didn't fix the underlying problem (it's still possible to miss on critical events, leaving the installed apps in funny states till the phone is rebooted).

I think we should revert the change in [1], and then we can fix the original problems by one of those two ways(that I can see):

1. Change Webapps.js so the one-shot event semantics is as follows:
"This event handler will fire when the event happens. In case the event has already happened, it will fire immediately" 

instead of the current one which is: 

"This event handler will fire when the event happens. If the event has already
happened, it will not fire at all"


or 

2. 
  a. Change Gaia so whenever a installPackage/install is called, or wherever a apps.mgmt.onwhatever is defined, the code checks if the event has already happened and calls the event manually. That is, do something like:

var handler = function () {
  // We might need to check if this is the first time we're called or not
}

app.ondownloadapplied = handler();

if (app.installState === 'installed') { 
  // Maybe we set the handler too late... let's call it just in case
  handler();
  // And we unset it.. this should avoid it being called
  app.ondownloadapplied = null;
}

for all the handlers (also the onsuccess, onwhatever).

b. Change the unit tests so they take the current DOMRequest semantics into account.


I've been talking about this with Fabrice on IRC, pending on his decision on which way to go. 


[1] https://hg.mozilla.org/mozilla-central/rev/ce95e81ebee8

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1: Rollback the Install:Return:Ack related changes

This just rolls back the changes introduced on Bug 873567 and Bug 839810 that queued the downloads until the child process/launcher returned an ACK.

Note that this patch by itself would fix the problem described in c0, but it would generate again the problems that were fixed on the rolled back bugs.

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Make app event handlers fire immediately when set if already should have fired

This makes all the apps.onwhatever fire immediately when they're set if the related event has happened already. This by itself fixes part of the problems with the tests that Bug 873567 and Bug 839810 fixed originally. The rest of the problems are fixed by fixing the tests themselves.

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: WIP Part 3. Fixed unit tests for apps

And this fixes the texts for the DOMRequests that I didn't touch in part 2 (DOMRequest events only fire if the handler is set before the event happens).

This is marked as a WIP because I haven't ran the tests yet, so it's up just for feedback.

Comment 6

[Jason Smith [:jsmith]]

Comment on attachment 798609 [details] [diff] [review]
WIP Part 3. Fixed unit tests for apps

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

::: dom/apps/tests/test_app_update.html
@@ +78,5 @@
>  
>      var request = navigator.mozApps.install(gHostedManifestURL);
> +    request.onerror = mozAppsError.bind(undefined, request);
> +    request.onsuccess = continueTest.bind(undefined, request);
> +    setTimeout(function(request) {

Why is setTimeout needed here and other locations in this file?

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jason Smith [:jsmith] from comment #6)
> Comment on attachment 798609 [details] [diff] [review]
> WIP Part 3. Fixed unit tests for apps
> 
> Review of attachment 798609 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/tests/test_app_update.html
> @@ +78,5 @@
> >  
> >      var request = navigator.mozApps.install(gHostedManifestURL);
> > +    request.onerror = mozAppsError.bind(undefined, request);
> > +    request.onsuccess = continueTest.bind(undefined, request);
> > +    setTimeout(function(request) {
> 
> Why is setTimeout needed here and other locations in this file?
Because the original tests used yield/iterator.next as a way to control de flow and wait for event handlers to fire. But the problem is that with the DOMRequests the handlers are only called if they're set before the event happens. So it's possible that on some cases the events won't get fired and the original tests would hang. And you cannot call iterator.next on the same flow that has the yield (cause it gives an exception). setTimeout is just a nice trick to get around that and to keep the flow going even when the handlers don't fire.

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 798607 [details] [diff] [review]
Part 1: Rollback the Install:Return:Ack related changes

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

::: dom/apps/src/Webapps.jsm
@@ +2001,5 @@
>        aData.app[aProp] = appObject[aProp];
>       });
>  
>      if (manifest.appcache_path) {
> +      this.startOfflineCacheDownload(manifest, app, aPprofileDir);

This is aProfileDir... seems I missed a qref, sorry about that.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 798608 [details] [diff] [review]
Part 2. Make app event handlers fire immediately when set if already should have fired

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

Looks good to me.

Just remember to also change _onprogress at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#464

::: dom/apps/src/Webapps.js
@@ +366,5 @@
>    },
>  
> +  _setEventHandler: function(aType, aHandler) {
> +    this._eventHandlers[aType].handler = aHandler;
> +    if (this._eventHandlers[aType].queued) {

if (aHandler && this._eventHandlers[aType].queued)

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 798608 [details] [diff] [review]
Part 2. Make app event handlers fire immediately when set if already should have fired

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

::: dom/apps/src/Webapps.js
@@ +364,5 @@
>                             "Webapps:PackageEvent",
>                             "Webapps:CheckForUpdate:Return:OK"]);
>    },
>  
> +  _setEventHandler: function(aType, aHandler) {

I know this is not needed for new files anymore, but not sure about already existing ones, so name the function, please. Also _getEventHandler.

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 798609 [details] [diff] [review]
WIP Part 3. Fixed unit tests for apps

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

I only did a quick pass. Quite ugly, but it looks like it could work.

I still wonder why DOMRequest has not a queue for events that happen before the handler is set...

::: dom/apps/tests/test_app_update.html
@@ +38,5 @@
>    }
>  
> +  function mozAppsError(request) {
> +    if (request) {
> +      this.onerror = null;

request.onerror

::: dom/apps/tests/test_packaged_app_install.html
@@ +106,5 @@
>        finish();
>      }
>    };
> +  if (req.result && req.onsuccess) {
> +    req.onsucess();

typo: onsuccess

@@ +136,5 @@
>                                 aName) {
>    var req = navigator.mozApps.installPackage(aMiniManifestURL);
>    req.onsuccess = function() {
>      ok(true, "App installed");
> +    req.onsucess = undefined;

typo: onsuccess

Comment 12

[[:fabrice] Fabrice Desré]

Attachment: alternate solution

Here's my take on the problem: we track dying processes and force an Ack when we are in confirmInstall() from a dead process. This is a wip, missing cleanup in "normal" case and in denyInstall().

With this patch I was able to install and run an app from the marketplace following the steps in comment #0.

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #12)
> Created attachment 799299 [details] [diff] [review]
> alternate solution
> 
> Here's my take on the problem: we track dying processes and force an Ack
> when we are in confirmInstall() from a dead process. This is a wip, missing
> cleanup in "normal" case and in denyInstall().
> 
> With this patch I was able to install and run an app from the marketplace
> following the steps in comment #0.

Going back to the classics:

Procure siempre acertalla
el honrado y principal;
pero si la acierta mal,
defendella y no enmendalla.

Which, on the other hand, can apply to both of us :P.

With your proposed change, we're still linking the download to what the child process do, which isn't really necessary, so as to no acknowledge that we have another underlying problem. Even with detecting child process deaths, what happens if the child process hangs instead of dieing? For example, what happens if I do a 

while (true); 

after doing mozApps.installPackage()? Yes, that's a problem on the caller's part, but the point is the caller should not be able to affect the callee in this way. 

Going by parts: 

* I didn't touch the DOMRequest on my proposed patch, because I understand that sooner than later they'll be replaced with futures, which have a much saner semantics (meaning the .then() fires immediately if it's called after the promise has already been fulfilled).  

* The original problem, as detected on the tests, can be fixed by touching the tests and ensuring the handler is called (by checking request.result) instead of linking the child and parent process in any way. This is in fact what part 3 does because as I said previously, I didn't touch the DOMRequest.

* What I did touch was mozIDOMApplication implementation of the event handlers. mozIDOMApplication isn't even a EventTarget... and most people I've talked to assume that the on* handlers there already behave as they would do after my part 2 patch. 

In fact, part 1 + part 3 would have fixed the problem described in c0 without reintroducing the problems that the rolled back patches originally aimed to fix. But it would have leave open the problem that's happening actually. On Homescreen we have something like (much simplified)

var appMgr = navigator.mozApps.mgmt;

appMgr.oninstall = function oninstall(event) {
  app = event.application;
  if (!app.isBookmark) {
    app.ondownloadapplied = function ondownloadapplied(event) {
      // do something
      app.ondownloadapplied = null;
      app.ondownloaderror = null;
    };
    app.ondownloaderror = function ondownloaderror(event) {
      // Do something else
    };
  }
};

which sounds like a sensate use of the ondownloadapplied event, only with the current semantics it's wrong (and in fact it IS failing: sometimes when the downloaded app is very small, the "downloadapplied" event gets fired before the handler is set, and so the icon stays on the rocket until the phone is rebooted or the homescreen dies). 

That was what part2 fixed.

I still think the send ack/wait for ack is evil. If you don't like changing the semantics of the .on* handlers then I can implement comment 2 solution 2, which still removes the ACK and fixes the problem by changing the client code. That would still mean rolling back the ack change, and then changing the tests and Gaiam as described on comment 2.

Comment 14

[[:fabrice] Fabrice Desré]

> * What I did touch was mozIDOMApplication implementation of the event
> handlers. mozIDOMApplication isn't even a EventTarget... and most people
> I've talked to assume that the on* handlers there already behave as they
> would do after my part 2 patch. 

So, these people believe that when you attach a resize event handler on a window you get the previous resize?
Also, mozIDOMApplication is not a real event target because we could not implement it in JS until recently. As soon as we move the implementation to webidl (see bug 899322) it will be an EventTarget, so there is no way we will land changes to these semantics.

> I still think the send ack/wait for ack is evil. If you don't like changing
> the semantics of the .on* handlers then I can implement comment 2 solution
> 2, which still removes the ACK and fixes the problem by changing the client
> code. That would still mean rolling back the ack change, and then changing
> the tests and Gaiam as described on comment 2.

Yes, let's fix all the call sites first, and then rollback the gecko parts.

Comment 15

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Update child apps when they're outdated at construction time

This is mostly done (and tested). Needs some cleaning up, but before that I wanted to check that this is what we talked about yesterday.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 804104 [details] [diff] [review]
Part 2. Update child apps when they're outdated at construction time

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

::: dom/apps/src/Webapps.jsm
@@ +828,5 @@
> +
> +        // If it wasn't registered before, let's update its state
> +        if ((aMsgName === 'Webapps:PackageEvent') ||
> +            (aMsgName === 'Webapps:OfflineCache')) {
> +          if (aApp && aApp.manifestURL) {

nit: if (man)

@@ +941,5 @@
>        case "Webapps:InstallPackage":
>          this.doInstallPackage(msg, mm);
>          break;
>        case "Webapps:RegisterForMessages":
> +        this.addMessageListener(msg.messages, mm, msg.app);

nit: keep the mm as the last argument : (msg.messages, msg.app, mm)

Comment 17

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Update child apps when they're outdated at construction time

Comment 18

[[:fabrice] Fabrice Desré]

Comment on attachment 804350 [details] [diff] [review]
Part 2. Update child apps when they're outdated at construction time

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

r=me Let's wait for the try build...

::: dom/apps/src/Webapps.jsm
@@ +994,4 @@
>        return;
>      }
>      this.children[aMsgName].forEach(function(mmRef) {
> +      debug("BroadcastMessage: Sending " + aMsgName + ":" + aContent.type + " to " + mmRef.mm);

Please remove these debug messages before landing.

Comment 19

[Antonio Manuel Amaya Calvo (:amac)]

Try run for part 2 is at: https://tbpl.mozilla.org/?tree=Try&rev=bafcaa6ca87e

Only part 2 (https://bugzilla.mozilla.org/attachment.cgi?id=804350) must be checked in now. I want to run a few more tests on part 1 (which is the one that actually fixes the problem described in comment 0) before asking to checkin for it. Reviewing the r+ from it now to avoid confusions :).

Comment 20

[Antonio Manuel Amaya Calvo (:amac)]

I've just remembered I forgot to upload a new version of the patch with the nits fixed. I will upload it in a few and rerequest checkin then.

Comment 21

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Update child app objects when they're outdated at creation time

r=fabrice

Test run is at https://tbpl.mozilla.org/?tree=Try&rev=0e1e9c92b0a3 and at 
https://tbpl.mozilla.org/?tree=Try&rev=bafcaa6ca87e (the second one has already finished and the only difference between the two is the removal of a couple of debug() calls.

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

Requesting checkin since https://tbpl.mozilla.org/?tree=Try&rev=bafcaa6ca87e finished correctly (and this one only removes a couple of debug calls). 

Only attachment 804889 [details] [diff] [review] should be checked in now, I still want to run some more tests on the other one, but this one by itself should help with random test failures for tests relateds with the mozApp API.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c7d7ef276127

Comment 25

[Antonio Manuel Amaya Calvo (:amac)]

Sorry for the long time without news here, had a bad week. I've been checking out the possibility of removing the ACK exchanging (because its the root cause of this bug, and while it fixes the test cases it doesn't really fix the real usage) and I've ran into a wall: installation/download errors.

What I did on the landed patch is to update the child object with the parent state if they were outdated at creation time. And that works well for most cases. But it doesn't work for this one (and the ACK doesn't really help either if there are two child process), when this happens:

T0: Child process 1 registers mozApps.mgmt.oninstall.
T1: Child process 2: calls mozApps.oninstall.
T2: Parent receives the msg to install, starts processing it, broadcasts the PackageEvent, sends the Ack
T3: Child process 2: executes the onsuccess handler, sends the ACK
T4: Child process 2 starts executing the app constructor.
T5: Parent process, receives the ACK, installs the package, gets an error, broadcasts the error.
T6: Child process 2 finishes executing the app constructor. Sets the onerror handler on the app object. T7: Since the The object is outdated, it's updated. But we've lost the error event!

The problem here is that error events or error states are not kept on the parent process at all. They're not part of the object. But that means that there's no real way of making this:

navigator.mozApps.mgmt.oninstall = function(evt) {
  var aApp = evt.application;
  aApp.ondownloaderror = downloaderrorHandler;
  // 2 
}

Because if the error happens before the app object is built at the child process (different child than the one calling install!) then the newly created object will have downloadError = undefined/null, and downloaderror won't be called ever. And doing:

if (aApp.ondownloaderror) {
  // Call error handler
}

at // 2 won't work either since aApp.ondownloaderror will be null/undefined.

If we don't want to cache events, the only solution I can think to this is to keep the last download operation state at the parent also (downloadResult == null means nothing has been done to the object since creation at the parent, otherwise we kept either DOWNLOAD_SUCCESS or the latest error reported and sending that as part of the PackageEvent so the child process can also update downloadError).

Comment 26

[[:fabrice] Fabrice Desré]

I'm not saying that we don't have issues there, but what is important is that at any time T, all DOM objects have the correct properties. If we ensure that, we are good.

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #25)
 
> The problem here is that error events or error states are not kept on the
> parent process at all. They're not part of the object. But that means that
> there's no real way of making this:
> 
> navigator.mozApps.mgmt.oninstall = function(evt) {
>   var aApp = evt.application;
>   aApp.ondownloaderror = downloaderrorHandler;
>   // 2 
> }

What is the value of aApp.downloadError before you set ondownloaderror ?

Comment 27

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #26)
> I'm not saying that we don't have issues there, but what is important is
> that at any time T, all DOM objects have the correct properties. If we
> ensure that, we are good.

Yeah, that's not happening. From T5 onwards, the child process 2 has an incorrect state.
> 
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #25)
>  
> > The problem here is that error events or error states are not kept on the
> > parent process at all. They're not part of the object. But that means that
> > there's no real way of making this:
> > 
> > navigator.mozApps.mgmt.oninstall = function(evt) {
> >   var aApp = evt.application;
> >   aApp.ondownloaderror = downloaderrorHandler;
> >   // 2 
> > }
> 
> What is the value of aApp.downloadError before you set ondownloaderror ?

Null. The problem is that the error happens between the moment the child process is told 'hey, you have to create an app object' and the time the app object is actually created. That means the child hasn't registered its event handlers, and when it does so its state is updated from the parent... but the parent state doesn't currently include the possible download errors.

Comment 28

[[:fabrice] Fabrice Desré]

Which event handlers are you talking about? The web facing ones, or the IPC message listeners?

Comment 29

[Antonio Manuel Amaya Calvo (:amac)]

The message listeners, sorry for the misnaming. The listeners are added at the end of the object constructor, and if by that time there has been an error, that process won't get the error ever. 

I have an idea for a possible patch. Was going to implement it today but got derailed. My idea is basically storing the download errors for apps and on the object creation (when it registers it's listeners) if the child object is outdated then I'll send the last error along with the updated object. That way the child process will have the correct data and error info.

Comment 30

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: WIP, doesn't work reliably yet

This is what I was talking about doing. It does work... more or less (which is another way of saying it doesn't work, yet :P ). Each child process/message manager is notified for outdated objects only once (per manifest URL). And they're notified only if the object being created is obsolete at the time of creation.

Do you feel I should finish this, or is this something that won't be r+'ed ever? I can't think on any other solution, TBH. And what we currently have (the ACKs) make the test work, but still doesn't work reliably on the field.

Comment 31

[Antonio Manuel Amaya Calvo (:amac)]

BTW I think I discovered the problem with the uploaded version during the drive home. If this approach is ok I'll check it and finish it tomorrow.

Comment 32

[[:fabrice] Fabrice Desré]

So I'd like to try a different approach. Currently each dom app object is listening for ipc message even if eg two of them represent the same application. That's a waste of ipc. Instead, what about:
- each process is getting the list of apps, and updates to these. Let's call them cached apps.
- when we create a dom object, it gets its state by proxying the cached app (no copy necessary).
- events are also dispatched to the cached app that relays them to the N dom objects. If the dom object has a content event listener registered, we dispatch the event.

This means that the dom app objects are very lightweight, and mostly just manage event listeners. That will come basically for free with the webidl bindings.

I think that would solve our issues correctly, at the expense of a larger refactoring. Thoughts?

Comment 33

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #32)
> So I'd like to try a different approach. Currently each dom app object is
> listening for ipc message even if eg two of them represent the same
> application. 
That's why the patch I uploaded yesterday fails some times also (because if there were two objects created at the child process and one of them registered his listeners in time, some times the second object would get two updates because it would see the update for the first and it's own).

> That's a waste of ipc. Instead, what about:
> - each process is getting the list of apps, and updates to these. Let's call
> them cached apps.
Do you mean copying all of the registry to each child process? Or just the apps it has created?

> - when we create a dom object, it gets its state by proxying the cached app
> (no copy necessary).
> - events are also dispatched to the cached app that relays them to the N dom
> objects. If the dom object has a content event listener registered, we
> dispatch the event.
> 
> This means that the dom app objects are very lightweight, and mostly just
> manage event listeners. That will come basically for free with the webidl
> bindings.
> 
> I think that would solve our issues correctly, at the expense of a larger
> refactoring. Thoughts?

Hmm... a not too refactore-y approach could be (dunno if that's the same you were proposing :)):

* Register all the listeners at the constructor of mozApps (that is, WebappsRegistry around [1])
* Remove the registration of each individual object listener ([2])
* When a mozApps create a new new mozIDOMApplication is created, it adds it to the cached app object lists and returns a new object that has a reference to the cached object (and its own handlers). 
* When an app specific event is received, it'll be received by WebappsRegistry and it will:
  * If the app isn't on the cache, discard it
  * Otherwise, call the handlers for all the objects that depend on that cache element

I think that would solve all of the problems we're having... since the listeners would be set *before* any object is created, we won't run into any situation where an object is outdated by its creation time.

If that's the general idea, I can write a patch for that.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#233
[2] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#352

Comment 34

[[:fabrice] Fabrice Desré]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #33)
 
> Hmm... a not too refactore-y approach could be (dunno if that's the same you
> were proposing :)):
> 
> * Register all the listeners at the constructor of mozApps (that is,
> WebappsRegistry around [1])

Not exactly. We already keep an updated list of apps per process for the appsService. It doesn't have all the information we need for the DOM api but we must augment this one and not create a new cache.

> * Remove the registration of each individual object listener ([2])

yes

> * When a mozApps create a new new mozIDOMApplication is created, it adds it
> to the cached app object lists and returns a new object that has a reference
> to the cached object (and its own handlers).

I'm not sure to follow the ownership model here. I think the dom object will have a strong ref on the cached app, and the cached app weak refs to the matching dom objects to dispatch events.


> * When an app specific event is received, it'll be received by
> WebappsRegistry and it will:
>   * If the app isn't on the cache, discard it
>   * Otherwise, call the handlers for all the objects that depend on that
> cache element

I would rather not have the registry receive these messages, since you can have several instances of webapps.js per process, but we want to manage only one cached app list per process.

Comment 35

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #34)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #33)
>  
> > Hmm... a not too refactore-y approach could be (dunno if that's the same you
> > were proposing :)):
> > 
> > * Register all the listeners at the constructor of mozApps (that is,
> > WebappsRegistry around [1])
> 
> Not exactly. We already keep an updated list of apps per process for the
> appsService. It doesn't have all the information we need for the DOM api but
> we must augment this one and not create a new cache.

On AppsServiceChild.jsm, right? I totally forgot about that one.
 
> > * Remove the registration of each individual object listener ([2])
> 
> yes
> 
> > * When a mozApps create a new new mozIDOMApplication is created, it adds it
> > to the cached app object lists and returns a new object that has a reference
> > to the cached object (and its own handlers).
> 
> I'm not sure to follow the ownership model here. I think the dom object will
> have a strong ref on the cached app, and the cached app weak refs to the
> matching dom objects to dispatch events.

Yep, that was my idea. The DOM object refers the cached app (and the cached app cannot get GCed while there's a DOM object pointing to it). The DOM object, though, can and should be GCed even if there are references from the registry/cached app to it.

> > * When an app specific event is received, it'll be received by
> > WebappsRegistry and it will:
> >   * If the app isn't on the cache, discard it
> >   * Otherwise, call the handlers for all the objects that depend on that
> > cache element
> 
> I would rather not have the registry receive these messages, since you can
> have several instances of webapps.js per process, but we want to manage only
> one cached app list per process.

Yeah, as I said, I forgot about the AppsService. I think... s/WebappsRegistry/DOMApplicationRegistry on the previous description.

Is this something you want to do on this bug? Because I almost would prefer finishing what I started (propagating the errors, that is fixing my WIP patch) even if that's something we'll throw away soon so we can get a working version and then do the rest on a followup bug. Mostly because this is something that's failing on 1.1 and 1.2 and it would be easier to ask for approval that way, because it'll be less risky that way, I think.

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fabrice Desré [:fabrice] from comment #32)
> So I'd like to try a different approach. Currently each dom app object is
> listening for ipc message even if eg two of them represent the same
> application. That's a waste of ipc. Instead, what about:
> - each process is getting the list of apps, and updates to these. Let's call
> them cached apps.
> - when we create a dom object, it gets its state by proxying the cached app
> (no copy necessary).
> - events are also dispatched to the cached app that relays them to the N dom
> objects. If the dom object has a content event listener registered, we
> dispatch the event.
> 
> This means that the dom app objects are very lightweight, and mostly just
> manage event listeners. That will come basically for free with the webidl
> bindings.
> 
> I think that would solve our issues correctly, at the expense of a larger
> refactoring. Thoughts?

Yes, please :)

This would be a great follow up for bug 910466.

Comment 37

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 810696 [details] [diff] [review]
WIP, doesn't work reliably yet

I'd like to try the approach explained in comment 32

Comment 38

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #37)
> Comment on attachment 810696 [details] [diff] [review]
> WIP, doesn't work reliably yet
> 
> I'd like to try the approach explained in comment 32

I would like to do a transient solution based of my latest patch since it would be much less risky to land it on 1.2, and this is something that's failing currently (on 1.0.1, on 1.1 and on 1.2). But if you prefer to implement the refactor directly, feel free to steal the bug :).

Comment 39

[[:fabrice] Fabrice Desré]

Nothing will be backported on 1.2 here. It's too late.

Comment 40

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #38)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #37)
> > Comment on attachment 810696 [details] [diff] [review]
> > WIP, doesn't work reliably yet
> > 
> > I'd like to try the approach explained in comment 32
> 
> I would like to do a transient solution based of my latest patch since it
> would be much less risky to land it on 1.2, and this is something that's
> failing currently (on 1.0.1, on 1.1 and on 1.2). But if you prefer to
> implement the refactor directly, feel free to steal the bug :).

Stealing it :)

Comment 41

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Very early WIP

Comment 42

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 43

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Now with the proxy object

Comment 44

[Fernando Jiménez Moreno [:ferjm]]

Rafael, could you explain which are your reasons for nominating this bug for koi, please?

I would strongly suggest not to land this kind of refactors for koi if possible.

Comment 45

[[:fabrice] Fabrice Desré]

That is just not gonna happen.

Comment 46

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Now with update app state.

Comment 47

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Fabrice, this is a very basic WIP but it contains the basic idea. It still needs a lot of work, but I would appreciate if you could give me some feedback in the mean time. Thanks!

Comment 48

[[:fabrice] Fabrice Desré]

Comment on attachment 8347354 [details] [diff] [review]
WIP

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

I like it! That looks good overall. Once case I think is not managed by the code is:
- start with a set of apps.
- the content holds a DOM object on app1.
- app1 is deleted, that removes it from DOMApps
- the content can still access properties on app1 and we will fail in interesting ways.

::: dom/apps/src/AppsServiceChild.jsm
@@ +115,5 @@
> +  addDOMApp: function(aApp, aManifestURL, aId) {
> +    let weakRef = Cu.getWeakReference(aApp);
> +    if (!this.DOMApps) {
> +      this.DOMApps = {};
> +    }

That could be done by default in init()

@@ +144,5 @@
> +    };
> +  },
> +
> +  _fireEvent: function(aMessage) {
> +    let msg = aMessage.json;

s/aMessage.json/aMessage.data

::: dom/apps/src/Webapps.js
@@ +548,5 @@
>        }
>      }
>    },
>  
> +  _fireRequestResult: function(aMessage) {

this function is unused as of now

@@ +560,5 @@
> +      req = this.takeRequest(msg.requestID);
> +    }
> +
> +    if (req) {
> +      Services.DOMRequest.fireSuccess(req, msg.manifestURL);

that will not work for the promises, right?

Comment 49

[Fernando Jiménez Moreno [:ferjm]]

Thanks for the feedback Fabrice!

(In reply to Fabrice Desré [:fabrice] from comment #48)
> Comment on attachment 8347354 [details] [diff] [review]
> WIP
> 
> Review of attachment 8347354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like it! That looks good overall. Once case I think is not managed by the
> code is:
> - start with a set of apps.
> - the content holds a DOM object on app1.
> - app1 is deleted, that removes it from DOMApps
> - the content can still access properties on app1 and we will fail in
> interesting ways.
>

This is now controlled in the DOMApp proxy. Thanks.
 
>
> > +  _fireRequestResult: function(aMessage) {
> 
> this function is unused as of now
>

It is used from AppsServiceChild :)
 
> @@ +560,5 @@
> > +      req = this.takeRequest(msg.requestID);
> > +    }
> > +
> > +    if (req) {
> > +      Services.DOMRequest.fireSuccess(req, msg.manifestURL);
> 
> that will not work for the promises, right?

That's true, but there is no case where we need to use Promises yet.

Comment 50

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 51

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Rebased and including a lot of fixes. Only a few more red tests to fix and we should be good for a first try.

Comment 52

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Try: https://tbpl.mozilla.org/?tree=Try&rev=5a0fea749b87

Comment 53

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

This should fix the orange.

New try build: https://tbpl.mozilla.org/?tree=Try&rev=935d6f44fc3e

Comment 54

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 55

[[:fabrice] Fabrice Desré]

Comment on attachment 8369909 [details] [diff] [review]
v1

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

Looks great! The only part that I'd really like to see fixed is the manifest one.

::: dom/apps/src/AppsServiceChild.jsm
@@ +63,5 @@
> +  },
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    // Clear the cache on memory pressure.
> +    this._cache = { };

We should also trigger a forced GC here, because we don't really control whether we are before or after the GC in the memory-pressure observer list, but we really want everything to be cleared asap.

@@ +122,2 @@
>        this.cpmm.removeMessageListener(aMsgName, this);
>      }).bind(this));

nit: use => here

::: dom/apps/src/Webapps.js
@@ +290,2 @@
>      this._manifest = aApp.manifest;
>      this._updateManifest = aApp.updateManifest;

I'd prefer if we didn't special cased the manifest here. Can you try to get them to be sent like the other properties?

::: dom/apps/src/Webapps.jsm
@@ +61,1 @@
>  #endif

That makes debug() undefined on non DEBUG builds no?

Comment 56

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Fabrice, this patch should address your feedback. Thanks!

Comment 57

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8377574 [details] [diff] [review]
v2

Actually, let me test again before asking for r?

Comment 58

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Try: https://tbpl.mozilla.org/?tree=Try&rev=7548ebe8e388

Comment 59

[Fernando Jiménez Moreno [:ferjm]]

Thanks Fabrice!

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

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6058a0d53426

Comment 61

[Fernando Jiménez Moreno [:ferjm]]

I think this can be closed now :)

Comment 62

[Antonio Manuel Amaya Calvo (:amac)]

This should fix the intermittent error from bug 928262. Can you ask for it to be re-enabled, Fernando?

Comment 63

[Fernando Jiménez Moreno [:ferjm]]

Yes, trying at bug 972927

Comment 64

[Jason Smith [:jsmith]]

This is confirmed to cause bug 977215, which is a smoketest regression.

Adding checkin-needed for a backout.

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Jason Smith [:jsmith] from comment #64)
> This is confirmed to cause bug 977215, which is a smoketest regression.
> 
> Adding checkin-needed for a backout.

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

Comment 66

[Julien Wajsberg [:julienw]]

This also likely breaks SMS notifications with this logcat error:

  GeckoConsole: [JavaScript Error: "TypeError: app.manifest is undefined" {file: "app://sms.gaiamobile.org/shared/js/notification_helper.js" line: 16}]

Comment 67

[Julien Wajsberg [:julienw]]

Confirmed, backing this out fixed the issue.

Comment 68

[Ed Morley [:emorley]]

(In reply to Wes Kocher (:KWierso) from comment #65)
> (In reply to Jason Smith [:jsmith] from comment #64)
> > This is confirmed to cause bug 977215, which is a smoketest regression.
> > 
> > Adding checkin-needed for a backout.
> 
> https://hg.mozilla.org/integration/b2g-inbound/rev/061c67d92d57

Backout is now on mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/061c67d92d57

Comment 69

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

This should fix the regression reported in bug 978089. Couldn't reproduce bug 977215 or comment 66.

Comment 70

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

Rebased on top of m-i.

I am afraid that with my new assignments I am not able to spend the time that this bug requires so Carmen will be helping here.

Carmen, as we spoke, we need to:

1- Fix the new failing tests introduced in bug 880043. I triggered a try run yesterday that shows the new oranges https://tbpl.mozilla.org/?tree=Try&rev=4606c8b198c8

2- Try to reproduce and fix the regressions reported in bug 977215 and comment 66.

3- Ask for a full QA verification as this bug is quite sensitive.

Thanks so much for your help!

Comment 71

[Carmen Jimenez Cabezas]

Ok!, Taking this, thanks Fernando

Comment 72

[Carmen Jimenez Cabezas]

Attachment: Proposed patch - v1. Incremental over Ferjm patch

This patch continues Fernando's work and fixes the latest problem which was that applications that included an origin on the manifest couldn't be installed correctly.
Try run is at: 
https://tbpl.mozilla.org/?tree=Try&rev=db6736f4cf96

Comment 73

[Carmen Jimenez Cabezas]

Attachment: v3 patch rebased with lastest Mozilla Central changes

Comment 74

[Carmen Jimenez Cabezas]

With the small change I made on the second patch (Proposed patch - v1. Incremental over Ferjm patch) the mochitests run correctly and after two days of testing I'm unable to reproduce the problems described by Fernando in comment 70. Requesting review

Comment 75

[[:fabrice] Fabrice Desré]

Comment on attachment 8426246 [details] [diff] [review]
Proposed patch - v1. Incremental over Ferjm patch

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

Thanks for making an inter-diff!

Comment 76

[[:fabrice] Fabrice Desré]

Comment on attachment 8427148 [details] [diff] [review]
v3 patch rebased with lastest Mozilla Central changes

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

I also tested locally and this looks fine. I'll check with QA before landing to make sure they are aware of things to look at.

Comment 77

[[:fabrice] Fabrice Desré]

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

Marcia, this is the bug I talked about.

Comment 78

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/718b0c6fa873 for Gu bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=40305845&tree=B2g-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=40306431&tree=B2g-Inbound

Comment 79

[Carmen Jimenez Cabezas]

Attachment: Proposed patch - v2. Incremental over Ferjm patch

Comment 80

[Carmen Jimenez Cabezas]

Comment on attachment 8428300 [details] [diff] [review]
Proposed patch - v2. Incremental over Ferjm patch

I think this fixes it, it was just a silly typo on a message name

Comment 81

[Carmen Jimenez Cabezas]

Try run is at: https://tbpl.mozilla.org/?tree=Try&rev=1bed0b1e75bc

Comment 82

[[:fabrice] Fabrice Desré]

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

Comment 83

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09ebd0af27a4

Comment 84

[Ryan VanderMeulen [:RyanVM]]

Backed out for being the likely cause of the big spike of B2G debug mochitest crashes (as tracked in bug 950653).

https://hg.mozilla.org/mozilla-central/rev/ca042038cdee

Comment 85

[Myk Melez [:myk] [@mykmelez]]

The other issue with these patches, which I just noticed this morning, is that installing an app and then retrieving its record via mozApps.mgmt.getAll results in the mozIDOMApplication.manifest property being undefined, even though DOMApplicationRegistry._writeManifestFile correctly writes the data to disk, and DOMApplicationRegistry._readManifests correctly reads the manifest data from disk, before getAll returns the app record (so there isn't a file read/write race like in bug 991397).

I rebuilt after the patches were reverted, and the problem went away.

Comment 86

[[:fabrice] Fabrice Desré]

Carmen, do you still have time to work on that patch?

Comment 87

[Carmen Jimenez Cabezas]

At this moment I'm pretty tied up with closing 2.0 blockers and some other things that while not blockers we would like to go into 2.0. So in the near future I don't think I'll have time for this. Releasing it and I'll retake it once the dust has settled if nobody takes it first.

Comment 88

[James Lal [:lightsofapollo]]

I am not sure I am handling all the cases here but I am attempting fixes for cancellation states (which I filed separate bugs for) I will try to sync up with fabrice tomorrow.

Comment 89

[[:fabrice] Fabrice Desré]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #85)
> The other issue with these patches, which I just noticed this morning, is
> that installing an app and then retrieving its record via
> mozApps.mgmt.getAll results in the mozIDOMApplication.manifest property
> being undefined, even though DOMApplicationRegistry._writeManifestFile
> correctly writes the data to disk, and DOMApplicationRegistry._readManifests
> correctly reads the manifest data from disk, before getAll returns the app
> record (so there isn't a file read/write race like in bug 991397).
> 
> I rebuilt after the patches were reverted, and the problem went away.

Myk, I can't reproduce with this code:

document.addEventListener("DOMContentLoaded", function() {
  console.log("Document loaded");
  document.getElementById("install-button").addEventListener("click",
  function() {
    console.log("Installing app");
    var req = navigator.mozApps.install("https://m.facebook.com/openwebapp/manifest.webapp");
    req.onsuccess = function() {
      var all = navigator.mozApps.mgmt.getAll();
      all.onsuccess = function() {
        var apps = all.result;
        console.log("Got " + apps.length + " apps");
        var i;
        for (i = 0; i < apps.length; i++) {
          if (!apps[i].manifest) {
            console.error("No manifest for " + apps[i].manifestURL);
          }
        }
      }
    }
  });

});

Is that similar to your test?

Comment 90

[[:fabrice] Fabrice Desré]

Attachment: app-ipc.patch

Rebased on current inbound.

Comment 91

[Myk Melez [:myk] [@mykmelez]]

(In reply to Fabrice Desré [:fabrice] from comment #89)
> Myk, I can't reproduce with this code:
…
> Is that similar to your test?

Hmm, I'm having trouble remembering exactly what I was testing, but I bet I was testing sideloading apps on Android devices, which calls DOMApplicationRegistry.doInstall/doInstallPackage directly and then calls mozApps.getAll.  And the latest patch appears to have the same problem (or a similar one), as an app I sideloaded and then ran for the first time never finished starting up.  But I'll dig into this further tomorrow to confirm the problem.  Leaving the needinfo request to remind me to do that.

Comment 92

[[:fabrice] Fabrice Desré]

Hold on Myk - I found a race with how we set the manifest properties with the current patch.

Comment 93

[Myk Melez [:myk] [@mykmelez]]

(In reply to Fabrice Desré [:fabrice] from comment #92)
> Hold on Myk - I found a race with how we set the manifest properties with
> the current patch.

Ok, canceling the needinfo request in the meantime!

Comment 94

[[:fabrice] Fabrice Desré]

Attachment: Part 1. app-ipc.patch updated

I fixed a few issues and now got a nice green try build (https://tbpl.mozilla.org/?tree=Try&rev=1a5fc0532276) and all my local tests working too.

Fixed:
- a race in GetList where we would not return some manifests.
- a bug in the WrappedManifestCache where we stored undefined values for a key, and afterward failed to update it.
- a typo introduced when rebasing.

Comment 95

[[:fabrice] Fabrice Desré]

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

Comment 96

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea4acc8b76dd

Comment 97

[Fernando Jiménez Moreno [:ferjm]]

Thanks Fabrice!

Comment 98

[Kevin Grandon :kgrandon]

Since this is causing bug 1036143 can we either get a fix quickly or a backout?

Comment 99

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing frequent B2G crashtest failures like the log below shows. Retriggers from before and after this landed on b2g-inbound confirm it to be the culprit.
https://hg.mozilla.org/mozilla-central/rev/cd6457486d15

https://tbpl.mozilla.org/php/getParsedLog.php?id=43443107&tree=Mozilla-Central

08:23:42     INFO -  REFTEST TEST-START | http://10.0.2.2:8888/tests/layout/base/crashtests/365909-2.xhtml
08:23:42     INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8888/tests/layout/base/crashtests/365909-2.xhtml | 14 / 883 (1%)
08:23:42     INFO -  System JS : ERROR chrome://reftest/content/reftest-content.js:884 - NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsISyncMessageSender.sendSyncMessage]
08:23:42     INFO -  *** UTM:SVC TimerManager:notify - notified @mozilla.org/b2g/webapps-update-timer;1
08:23:42     INFO -  ###################################### forms.js loaded
08:23:42     INFO -  ############################### browserElementPanning.js loaded
08:23:42     INFO -  ######################## BrowserElementChildPreload.js loaded
08:23:42     INFO -  REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/base/crashtests/365909-2.xhtml | load failed: timed out waiting for test to complete (waiting for onload scripts to complete)

It's very possible that bug 984274 will wallpaper over this, for better or for worse.

Comment 100

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Kevin Grandon :kgrandon from comment #98)
> Since this is causing bug 1036143 can we either get a fix quickly or a
> backout?

Actually bug 1036143 is bug 1013279, which in turn is bug 997717... which has been unfixed for two months now. I think this last patch just exposed the problem (and if we're not seeing it without it is because we're not really cancelling the download?).

Comment 101

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2. Reject the promise when cancelling a downlaod

This is incremental over the other patches. I saw that two of the changes I had proposed on bug 1013279 are already on the patch, but there's one case where the promise isn't rejected (and so it's trying to fulfill it incorrectly).

Comment 102

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Delay the Webapps updater until the DOMApplicationRegistry is ready

I believe (and the code word there is believe cause I haven't seen it fail) that the crashtests failures are related with the Webapps updater (because on the log Ryan linked there was an updater trace and because otherwise there's no Webapps code that run there).

There are two possible solutions here... one is to just don't run the updater code on the tests, and the other (which is what this patch does) is at least to try and wait until the application registry is ready (although navigator.mozApps should already take care of that, so it's still probably better to take the other solution and just don't launch the updater on the tests).

Comment 103

[Antonio Manuel Amaya Calvo (:amac)]

There's a try run for the previous patch at https://tbpl.mozilla.org/?tree=Try&rev=0cc456cb5be5

Comment 104

[[:fabrice] Fabrice Desré]

Comment on attachment 8453888 [details] [diff] [review]
Delay the Webapps updater until the DOMApplicationRegistry is ready

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

That seems to break xpcshell tests, and I'm not a fan of having consumers of the api do that.

Comment 105

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Delay processing mozApps.mgmt.getAll petition until the registry is ready

And second approach. This one makes more sense, actually, and is just to check on Webapps.jsm if the registry is ready before trying to process the petition. 
There's a fresh try run for this on https://tbpl.mozilla.org/?tree=Try&rev=5f5e5820bd64

Comment 106

[[:fabrice] Fabrice Desré]

Comment on attachment 8454341 [details] [diff] [review]
Delay processing mozApps.mgmt.getAll petition until the registry is ready

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

I was thinking about doing something similar yesterday... I think we need to do 2 things:
- what you do here because we will call directly getAll() in Bug 972076
- also wait for this.registryReady().then(...) in receiveMessage(), because other methods of the dom api can also be racy.

That may let us get rid of the webapps-registry-ready observer notification, but I need to check all the consumers.

Comment 107

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Delay processing DOM API petitions until the application registry is ready

Changed the last version to delay all the petitions until the registry is ready. Emulator try run is at: https://tbpl.mozilla.org/?tree=Try&rev=96d0df0e4571 (it that doesn't explode I'll do a full run later).

Comment 108

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: With 100% less deadlock

So... the last one had a very nice deadlock. Turns out that one of the way the registryReady promise is resolved is via a message that I had made dependant on the promise being resolved. Go figure.

New try is at: https://tbpl.mozilla.org/?tree=Try&rev=a5bf45fcb803

And sorry for all the tries but I can't run the emulator locally. Although TBH I could have catched this also if I had tried it on an actual device instead of just running the tests on desktop. The device not booting would have been a dead giveaway.

Comment 109

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: P4, WIP! Change the way AppsServiceChild.jsm is initialized and work

Comment 110

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Still WIP, doesn't work. Change the initialization of AppsServiceChild.jsm

Comment 111

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Delay processing requests until the registry is ready

This looked mostly green:

https://tbpl.mozilla.org/?tree=Try&rev=89329e289c92

I've relaunched a full try run at https://tbpl.mozilla.org/?tree=Try&rev=0992d0072e90

Comment 112

[Antonio Manuel Amaya Calvo (:amac)]

Also, try run only for the emulator, both opt and debug: https://tbpl.mozilla.org/?tree=Try&rev=07181510eb77 So much green :)

Comment 113

[Kevin Grandon :kgrandon]

Hey Antonio - I noticed in your try run that it breaks a gaia integration test when resuming downloads: https://tbpl.mozilla.org/php/getParsedLog.php?id=43839241&tree=Try#error0

If you get a chance please verify that this use case continues to work. (We are currently trying to get these tests re-enabled on TBPL, and this one is passing for us these days).

Comment 114

[[:fabrice] Fabrice Desré]

Comment on attachment 8456164 [details] [diff] [review]
Delay processing requests until the registry is ready

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

Clearing review until we have all green builds.

Comment 115

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Kevin Grandon :kgrandon from comment #113)
> Hey Antonio - I noticed in your try run that it breaks a gaia integration
> test when resuming downloads:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=43839241&tree=Try#error0
> 
> If you get a chance please verify that this use case continues to work. (We
> are currently trying to get these tests re-enabled on TBPL, and this one is
> passing for us these days).

Hey Kevin... how do get that log from https://tbpl.mozilla.org/?rev=0992d0072e90&tree=Try ? I assume it's the "B2G Desktop Linux x64 Opt" but I don't see any orange or red there?

Comment 116

[Kevin Grandon :kgrandon]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #115)
> (In reply to Kevin Grandon :kgrandon from comment #113)
> > Hey Antonio - I noticed in your try run that it breaks a gaia integration
> > test when resuming downloads:
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=43839241&tree=Try#error0
> > 
> > If you get a chance please verify that this use case continues to work. (We
> > are currently trying to get these tests re-enabled on TBPL, and this one is
> > passing for us these days).
> 
> Hey Kevin... how do get that log from
> https://tbpl.mozilla.org/?rev=0992d0072e90&tree=Try ? I assume it's the "B2G
> Desktop Linux x64 Opt" but I don't see any orange or red there?

It's currently hidden, so to display it we can hack the URL with: https://tbpl.mozilla.org/?rev=0992d0072e90&tree=Try&showall=1

It will be the red `Gij`

Comment 117

[Antonio Manuel Amaya Calvo (:amac)]

Well, that explains at least why I didn't see it :) I was going crazy :)

I'll take a look at that tomorrow, thank you.

Comment 118

[Antonio Manuel Amaya Calvo (:amac)]

I couldn't run the test. I've been trying manually on a flame (configured for 273mb also). And I can't make it fail. I've uploaded what I've been testing at:
http://youtu.be/_ElZ_c5blTM (sorry for the Blair's witch effect :P) Is that what the test is supposed to do? I've also tried canceling/restarting/canceling several times with no failures.

Comment 119

[Kevin Grandon :kgrandon]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #118)
> I couldn't run the test. I've been trying manually on a flame (configured
> for 273mb also). And I can't make it fail. I've uploaded what I've been
> testing at:
> http://youtu.be/_ElZ_c5blTM (sorry for the Blair's witch effect :P) Is that
> what the test is supposed to do? I've also tried
> canceling/restarting/canceling several times with no failures.

Hey Antonio - have you tried running them manually inside of gaia? You can run the following command to do so:

TEST_FILES="apps/verticalhome/test/marionette/app_packaged_resume_test.js" make test-integration

Comment 120

[Kevin Grandon :kgrandon]

Also in the test we pause/resume the download twice from the homescreen. It's similar to your video. I wonder if we could be causing some bad behavior in b2g desktop?

I've also restarted the test run on tbpl a few times to verify.

Comment 121

[Kevin Grandon :kgrandon]

Antonio - please let me know if you aren't able to run the tests locally. The make test-integration command should setup most of the things you need from a gaia checkout. Inside the gaia checkout you can use a custom b2g binary compiled with your patches.

Comment 122

[Antonio Manuel Amaya Calvo (:amac)]

Yeah, I've also tried stopping/restarting several times in a row (two/four times). The problem with the emulator tests is that I can't run the emulator at all on my usual build machine (Ubuntu VM). And I can't compile on my Mac (it's beyond my patience :)). I'll try getting a precompiled binary for macos and just patch manually the jsm/is files and see how it goes. Do the tests fail also when ran against an actual device?

Comment 123

[Kevin Grandon :kgrandon]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #122)
> Yeah, I've also tried stopping/restarting several times in a row (two/four
> times). The problem with the emulator tests is that I can't run the emulator
> at all on my usual build machine (Ubuntu VM). And I can't compile on my Mac
> (it's beyond my patience :)). I'll try getting a precompiled binary for
> macos and just patch manually the jsm/is files and see how it goes. Do the
> tests fail also when ran against an actual device?

These tests currently only run on b2g desktop, I don't think we have everything quite running using our device runner, but we're working on it. I'll also try to get your patches applied and running against a local b2g instance.

Comment 124

[Kevin Grandon :kgrandon]

Also these tests run using b2g desktop, not the emulator. Not sure if that matters in your case though.

Comment 125

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Delay processing DOM API petitions until the application registry is ready

This should fix the failing Gaia integration test (thanks Kevin, BTW!). Try run is at  https://tbpl.mozilla.org/?tree=Try&rev=3da05f1a63f0

Comment 126

[Kevin Grandon :kgrandon]

All of the app tests seemed to pass this time. Thank you! :)

Comment 127

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Delay processing requests until the registry is ready, v4.

This fixes the oth breakage from the last version. Try run is at

https://tbpl.mozilla.org/?tree=Try&rev=a584875f25d5

Comment 128

[Antonio Manuel Amaya Calvo (:amac)]

Try run looks good (lots of orange actually, but nothing that seems related nor new), requesting review for part 3.

Comment 129

[Lucas Adamski [:ladamski]]

This is a dependency of a CAF bug but isn't blocking 2.0.  Should it be?

Comment 130

[Jason Smith [:jsmith]]

(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #129)
> This is a dependency of a CAF bug but isn't blocking 2.0.  Should it be?

I don't think so - we've have had trouble landing this previously on trunk, as it's caused smoketest regressions, so I'm concerned that taking this on 2.0 could cause some critical regressions late in the 2.0 cycle.

Fabrice - What do you think?

Comment 131

[Kevin Grandon :kgrandon]

Assuming we have a green gaia marionette test run, which we do now, I would like to see this in 2.0. Our gaia integration tests provide fairly deep coverage, much more so than the horizontal homescreen. I feel if we don't take this patch, then we will not be able to solve bug 1037706.

Comment 132

[Jason Smith [:jsmith]]

(In reply to Kevin Grandon :kgrandon from comment #131)
> Assuming we have a green gaia marionette test run, which we do now, I would
> like to see this in 2.0. Our gaia integration tests provide fairly deep
> coverage, much more so than the horizontal homescreen. I feel if we don't
> take this patch, then we will not be able to solve bug 1037706.

Let's talk about this offline. I'll ping you in IRC to discuss this.

Comment 133

[[:fabrice] Fabrice Desré]

I already said that I considered that one too risky to uplift. That's a major refactoring.

Why can't the new homescreen use the same tricks as the horizontal one to not be slow at startup?

Comment 134

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #133)
> I already said that I considered that one too risky to uplift. That's a
> major refactoring.

FWIW I agree with Fabrice... and that's a pity since our current code tends to fail randomly. I've seen end user 1.3 device (ebay ZTE Open C latest version) where an app installed from the marketplace could only be launched from the search bar (or the marketplace) because somehow the oninstall event was lost and so the homescreen didn't add the icon. And no icon meant no uninstalling it either, so the user's only option was to do a factory reset.

This patch hopefully will avoid these cases (that was the idea after all).

Comment 135

[[:fabrice] Fabrice Desré]

Comment on attachment 8459556 [details] [diff] [review]
Delay processing requests until the registry is ready, v4.

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

I see some light at the end of the tunnel!

::: dom/apps/src/AppsServiceChild.jsm
@@ +174,5 @@
>          }
>          for (let i = 0; i < apps.length; i++) {
>            let domApp = apps[i].get();
> +          if (!domApp || domApp._window === null) {
> +            apps.splice(i,1);

nit: (i, 1)

@@ +234,5 @@
>      // Get rid of dead weak references.
>      for (let i = 0; i < apps.length; i++) {
> +      let app = apps[i].get();
> +      if (!app || app._window === null) {
> +        apps.splice(i,1);

ditto

::: dom/apps/src/Webapps.jsm
@@ +275,5 @@
>    get registryStarted() {
>      return this._registryStarted.promise;
>    },
>  
> +  // The registry will be safe to clone

nit: add a full stop at the end of the comment.

Comment 136

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 3. Delay processing requests until the registry is ready, nits addressed

r=fabrice

Comment 137

[Antonio Manuel Amaya Calvo (:amac)]

Well, here goes nothing :) Requesting checkin. The patches should be applied in the order specified in the name.

Comment 138

[Carsten Book [:Tomcat]]

the patches failed to apply as example for part 1

applying app-ipc.patch
patching file dom/apps/src/Webapps.js
Hunk #6 FAILED at 811
1 out of 7 hunks FAILED -- saving rejects to file dom/apps/src/Webapps.js.rej
patching file dom/apps/src/Webapps.jsm
Hunk #17 FAILED at 2574
Hunk #19 FAILED at 2785
Hunk #20 FAILED at 2812
Hunk #24 succeeded at 3556 with fuzz 1 (offset 23 lines).
3 out of 27 hunks FAILED -- saving rejects to file dom/apps/src/Webapps.jsm.rej
patching file dom/apps/tests/test_packaged_app_update.html
Hunk #1 succeeded at 75 with fuzz 1 (offset 0 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh app-ipc.patch

could you try to update these patches like rebasing against a current tree ?

Comment 139

[Antonio Manuel Amaya Calvo (:amac)]

It seems there has been some changes on Webapps on the last day... I'm going to relaunch the tests and will re-request checking when/if they pass correctly.

Comment 140

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8453622 [details] [diff] [review]
Part 2. Reject the promise when cancelling a downlaod

Now that bug 997717 landed this doesn't seem to be necessary any more.

Comment 141

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1. Core changes

r=fabrice

Rebased with latest M-C changes

New try run is at: 
https://tbpl.mozilla.org/?tree=Try&rev=3503918f65fa

Comment 142

[Antonio Manuel Amaya Calvo (:amac)]

Try run is green still. Re-requesting checkin. Part 2 is not necessary anymore (I obsoleted it)

Comment 143

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/fd84374f63f1
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/81f96f02263f

Comment 144

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fd84374f63f1
https://hg.mozilla.org/mozilla-central/rev/81f96f02263f

Comment 145

[Bob Clary [:bc] (inactive)]

This appears to have regressed bug 1035867 again. Working on verifying. Fabrice, did you test the steps in bug 1035867 ?

Comment 146

[Bob Clary [:bc] (inactive)]

manually checking m-c i get the regression range to be:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=66cb4d556959&tochange=a91ec42d6a9c

Comment 147

[Antonio Manuel Amaya Calvo (:amac)]

I totally forgot about that other bug (and as I said on bug 1035867 I don't even have an Android phone to test this :)). Anyway I uploaded a possible fix (in case it's caused by the same problem we ran with the B2G debug builds and processNextEvent).

Comment 148

[Ryan VanderMeulen [:RyanVM]]

Backed out for very-likely being the cause of frequent B2G crashtest timeouts (as noted in bug 1015200).

https://hg.mozilla.org/mozilla-central/rev/8cd9e4240f29

Comment 149

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/5ba4a3f85a2e
https://hg.mozilla.org/integration/b2g-inbound/rev/8efaa9f52856

Comment 150

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ba4a3f85a2e
https://hg.mozilla.org/mozilla-central/rev/8efaa9f52856