Closed Bug 839810 Opened 12 years ago Closed 12 years ago

Race condition installing apps on linux


(Core Graveyard :: DOM: Apps, defect)

Not set


(Not tracked)



(Reporter: daleharvey, Assigned: fabrice)




(3 files, 1 obsolete file)

When running 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;
    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;

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
Attached file Log: Mac - Working
Attached file Log: Linux - Broken
Blocks: 826058
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

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
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 ( is processed on the parent side.

I'm not sure yet how to fix this...
Attached patch patch (obsolete) — Splinter Review
Works for me when installing hosted+appcache apps from
Assignee: nobody → fabrice
Attachment #712797 - Flags: feedback?(dale)
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?
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 on attachment 712797 [details] [diff] [review]

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)
Attachment #712797 - Flags: feedback?(dale) → feedback+
Attached patch patch v2Splinter Review
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.
Attachment #712797 - Attachment is obsolete: true
Attachment #713101 - Flags: feedback?(dale)
Comment on attachment 713101 [details] [diff] [review]
patch v2

Approach definitely makes sense and its passing tests here, cheers fabrice
Attachment #713101 - Flags: feedback?(dale) → feedback+
Attachment #713101 - Flags: review?(ferjmoreno)
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 ':'
Attachment #713101 - Flags: review?(ferjmoreno) → review+
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.