839810 - Race condition installing apps on linux

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Dale Harvey (:daleharvey)]

When running https://bugzilla.mozilla.org/show_bug.cgi?id=826058 on try we get consistent failures on Linux (only)

The issue is in 

    var request = navigator.mozApps.install(gCachedManifestURL);
    request.onerror = cbError;
    request.onsuccess = continueTest;
    yield;
    var app = request.result;
    ok(app, "App is non-null");
    if (app.installState == "pending") {
      ok(true, "App is pending. Waiting for progress");
      app.onprogress = function() ok(true, "Got download progress");
      app.ondownloadsuccess = continueTest;
      app.ondownloaderror = cbError;
      yield;
    }

On mac the manifests are downloaded, we create an observer for the appcache download progress, execution is returned to content who listens for the ondownloadsuccess event which is delivered

On linux we download the appache contents before returning to content, however the pending status is still reported and the content waits for ondownloadsuccess events which never comes

attaching logs

Comment 1

[Dale Harvey (:daleharvey)]

Attachment: Log: Mac - Working

Comment 2

[Dale Harvey (:daleharvey)]

Attachment: Log: Linux - Broken

Comment 3

[Dale Harvey (:daleharvey)]

So as far as I can tell

On linux the offline cache finishes downloading before we send the install.success event which seems strange at least in its consistency but something we have to account for since the install.success is sent async

https://github.com/mozilla/mozilla-central/blob/master/dom/apps/src/Webapps.jsm#L1858

The offlineCache event cant update the app object that doesnt exist and the app object data sent to install.success is stale. I am assuming we should ensure the 

   this.broadcastMessage("Webapps:Install:Return:OK", aData);

sends the most recent information

Comment 4

[[:fabrice] Fabrice Desré]

The issue is not that Webapps:Install:Return:OK returns stale data. The new app object that we create on the content side at this point registers itself asynchronously to receive app state updates, and it looks like the appcache tries to fire ondownloadsuccess before this registration message (https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#396) is processed on the parent side.

I'm not sure yet how to fix this...

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: patch

Works for me when installing hosted+appcache apps from http://mozqa.com/webapi-permissions-tests/

Comment 6

[Jason Smith [:jsmith]]

Hmm...that patch looks like it using a sleep of some sorts. I don't know how well that will fare when we stick these mochitests in CI. This might result in fixing a subset of instances to get the tests to pass, but there could be the potential for intermittent failures still.

Couldn't you do some sort of wait for condition instead?

Comment 7

[[:fabrice] Fabrice Desré]

For sure this is not the ideal patch, but there's no guarantee that the content will use the app object from install.onsuccess. So we have no straightforward way to solve that. I have an alternate patch in progress, but it's not working yet.

Comment 8

[Dale Harvey (:daleharvey)]

Comment on attachment 712797 [details] [diff] [review]
patch

Tests are passing with this patch, I had tried the same

Its a hack but I think in the interest of getting the tests in there it may be worth landing

(obviously need to remove the accidental whitespace change and would have a pointer to this bug in there)

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Better approach, where we send back and acknowledgment from content to parent to make sure the webpage has a chance to add event handlers and that the app object registers its listeners before we download the appcache.

Comment 10

[Dale Harvey (:daleharvey)]

Comment on attachment 713101 [details] [diff] [review]
patch v2

Approach definitely makes sense and its passing tests here, cheers fabrice

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 713101 [details] [diff] [review]
patch v2

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

::: dom/apps/src/Webapps.jsm
@@ +1770,5 @@
> +  // content side. This let the webpage the opportunity to set event handlers
> +  // on the app before we start firing progress events.
> +  queuedDownload: {},
> +
> +  onInstallSuccessAck : function onInstallSuccessAck(aManifestURL) {

nit: extra space before ':'

Comment 12

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb198943e783

Comment 13

[Ed Morley [:emorley]]

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